Conversation
Generated initial VolumeSnapshot controller files using the scaffold tool: go run ./cmd/scaffold-controller -interactive=false -kind=VolumeSnapshot -gophercloud-client=NewBlockStorageV3 -gophercloud-module=github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/snapshots -gophercloud-type=Snapshot -openstack-json-object=snapshot -available-polling-period=15 -deleting-polling-period=15 -required-create-dependency=Volume
Add proper fields, validations, and metadata types for VolumeSnapshotResourceSpec (force, metadata), VolumeSnapshotFilter (status, volumeID), and VolumeSnapshotResourceStatus (size, metadata, progress, projectID, userID, groupSnapshotID, consumesQuota, createdAt, updatedAt) to align with the Cinder Volume Snapshot API. Those are all acoording to https://docs.openstack.org/api-ref/block-storage/v3/#volume-snapshots-snapshots:~:text=Volume%20snapshots%20(snapshots)%C2%B6
Wire the scaffolded VolumeSnapshotClient into the Scope interface, provider implementation, and MockScopeFactory. Add go:generate directives for mock generation of the VolumeSnapshotClient.
13dfd73 to
9a0001a
Compare
winiciusallan
left a comment
There was a problem hiding this comment.
Hi @aldokimi, sorry for the delay in reviewing this PR. Thanks for your contribution!
I made a round of review in the API before getting deeper into controller implementation. Let me know what you think about the comments.
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=255 | ||
| // +optional | ||
| Description *string `json:"description,omitempty"` |
There was a problem hiding this comment.
I did not find any evidence of having Description as a valid field for filtering. Likely a result of the scaffolding-tool?
|
|
||
| // VolumeSnapshotFilter defines an existing resource by its properties | ||
| // +kubebuilder:validation:MinProperties:=1 | ||
| type VolumeSnapshotFilter struct { |
There was a problem hiding this comment.
This is tricky because Cinder manages filters differently from other projects.
They define default filters, but the cloud administrator can also configure them.
https://opendev.org/openstack/cinder/src/branch/master/etc/cinder/resource_filters.json#L7
For an initial implementation, I'm good as it is, but is it worth having a comment talking about this "flexibility"?
| // progress is the percentage of completion of the snapshot creation. | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +optional | ||
| Progress string `json:"progress,omitempty"` |
There was a problem hiding this comment.
Wondering about this field, because it is easy to have a drift here.
A snapshot can be created and has its progress = 0. If no reconciliation was triggered, the user might have the impression that the snapshot wasn't built, even if OpenStack says it is OK.
| // metadata key and value pairs to be associated with the snapshot. | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="metadata is immutable" | ||
| // +kubebuilder:validation:MaxItems:=64 | ||
| // +listType=atomic |
There was a problem hiding this comment.
Shouldn't it be +listType=map with +listKey=name? as we did in
I've noticed that VolumeMetada and ServerMetadata are using listType=atomic. I believe we need to change those, too.
|
hey @aldokimi! are you still planning to continue working on that? if not, we'd be happy to take this further and give the credits for what's been done so far. |
|
Hey @winiciusallan, sorry about this. I got busy with work and studying and missed this one! Yes, you can take over the work from here. Thank you! |
Fixes #635
Summary
Adds a new VolumeSnapshot CRD and controller to manage Cinder volume snapshots.
Reference