Add VolumeSnapshot resource support for Cinder#686
Add VolumeSnapshot resource support for Cinder#686aldokimi wants to merge 9 commits intok-orc:mainfrom
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.
aldokimi
force-pushed
the
dev/volume_snapshot_controller
branch
from
13dfd73 to
9a0001a
Compare
winiciusallan
left a comment
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.
2 participants
Fixes #635
Summary
Adds a new VolumeSnapshot CRD and controller to manage Cinder volume snapshots.
Reference