Update VolumeGroupSnapshot to v1beta1 #1150

leonardoce · 2024-08-30T10:00:13Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Update the definition of CRDs to move VolumeGroupSnapshot, VolumeGroupSnapshotContent, and VolumeGroupSnapshotClass from v1alpha1 to v1beta1.

cc: @xing-yang

Which issue(s) this PR fixes:

Fixes #1119

Special notes for your reviewer:

This PR entirely removes the capability to serve the v1alpha version. Existing clients will be broken.
This is consistent with the relative PR comment and the related Kubernetes documentation.

The underlying structure of v1alpha and v1beta1 is consistent.

Does this PR introduce a user-facing change?:

`VolumeGroupSnapshot`, `VolumeGroupSnapshotContent`, and `VolumeGroupSnapshotClass`
are now available in `v1beta1` version. The support for the `v1alpha1` version have been removed.

k8s-ci-robot · 2024-08-30T10:00:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: leonardoce
Once this PR has been reviewed and has the lgtm label, please assign msau42 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nixpanic · 2024-10-01T14:49:00Z

/cc xing-yang

leonardoce · 2024-10-18T06:46:40Z

I rebased this PR and added a commit to add and set the new field VolumeSnapshotHandlePairList of the VolumeGroupSnapshotContent resource, as described in the latest iteration of the relative KEP.

Thank you!

cc: @xing-yang

client/apis/volumegroupsnapshot/v1beta1/types.go

yati1998

LGTM

xing-yang · 2024-10-21T17:48:10Z

@leonardoce We discussed this PR during today's CSI Implementation meeting. @jsafrane suggested that we break this PR into two PRs.

PR 1: Add new field and populate the new field but keeping the APIs at alpha.
PR 2: Move the APIs to Beta.
There are other things we need to do between PR 1 and PR2. See detailed plan here: https://docs.google.com/document/d/1jjliw0tFhCPnT9ixlYpj61k5u8_-awf1qSCWrkunshQ/edit?tab=t.0

leonardoce · 2024-10-21T18:13:07Z

Ok. I'll close it and file the one to add the new field

yati1998 · 2024-10-23T10:57:12Z

Ok. I'll close it and file the one to add the new field
@leonardoce can we please have any timeline by when can we get this done?

leonardoce · 2024-10-23T15:33:52Z

Hi @yati1998. I filed PR #1169 to add the new field.

leonardoce · 2024-10-23T15:34:05Z

/draft

mulbc · 2024-10-28T09:24:51Z

#1169 has been merged - can we merge this PR now?

yati1998 · 2024-11-06T11:50:18Z

@leonardoce can I know why this PR is in draft? I think we were good on this part?

leonardoce · 2024-11-06T12:19:55Z

Hi @yati1998.
According to this plan: https://docs.google.com/document/d/1jjliw0tFhCPnT9ixlYpj61k5u8_-awf1qSCWrkunshQ/edit?tab=t.0#heading=h.fizenae5evi0
We need at least #1177 to be merged before this one.

xing-yang · 2024-11-21T03:13:08Z

Can you please rebase?

client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yaml

yati1998 · 2024-11-22T02:18:33Z

@gnufied can you please review this as well?

leonardoce · 2024-11-22T18:05:51Z

examples/kubernetes/groupsnapshot-v1beta1.yaml

@@ -1,5 +1,5 @@
 ---
-apiVersion: groupsnapshot.storage.k8s.io/v1alpha1
+apiVersion: groupsnapshot.storage.k8s.io/v1beta1


Hi! If you are referring to app.kubernetes.io/name: postgresql, that's just the selector this example uses to get the PVC list. I don't think we need to change it 👍

xing-yang · 2024-11-22T18:07:12Z

@leonardoce Please ignore my comments:)

gnufied · 2024-11-22T18:24:52Z

We should compile this branch rebased with master and then run e2e tests in a loop for sometime. I have often done this for testing pre-release code which needs more testing. This will give us greater confidence about this feature. This does not require making a release.

gnufied · 2024-11-22T22:31:37Z

client/apis/volumegroupsnapshot/v1beta1/types.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 // +kubebuilder:object:generate=true
-package v1alpha1


Why is VolumeGroupSnapshot.Status.CreationTime --> metav1.Time and why is VolumeGroupSnapshotContentStatus.CreationTime --> int64 ?

I know that change was not introduced in this PR, but since we are moving this to beta, we have to be careful about all the decisions we make.

The same is happening with independent VolumeSnapshots:

external-snapshotter/client/apis/volumesnapshot/v1/types.go

Line 158 in 07425bd

CreationTime *metav1.Time `json:"creationTime,omitempty" protobuf:"bytes,2,opt,name=creationTime"`

vs:

external-snapshotter/client/apis/volumesnapshot/v1/types.go

Line 408 in 07425bd

CreationTime *int64 `json:"creationTime,omitempty" protobuf:"varint,2,opt,name=creationTime"`

The content object looks more directly related to the GRPC CSI call.

This is how it is displayed in CLI (via kubectl ) then:

creationTime: "2024-11-22T22:14:35Z" creationTime: 1732313675689050917

@xing-yang can you confirm if this intentional? This doesn't seem right.

We can always convert int64 to metav1.Time so as this is nicely handled by k8s tooling. I am not sure if this has to be int64...

In CSI spec, the timestamp is .google.protobuf.Timestamp which represents seconds of UTC time since Unix epoch 1970-01-01T00:00:00Z. So in VolumeSnapshotContent and VolumeGroupSnapshotContent, it is a direct mapping to what is returned from the CSI driver.
VolumeSnapshot and VolumeGroupSnapshot are in user namespace, we try to show them nicely in the readable format of metav1.Time.

VolumeSnapshotContent and VolumeGroupSnapshotContent are admin API objects. We don't need to show them in user readable format. I think they should stay true to the original values returned by the CSI driver.
Since we've already made a decision for this in VolumeSnapshotContent, it also looks inconsistent now if we are showing them differently in VolumeGroupSnapshotContent.

Having time represented as int64 imo causes unnecessary indirection for person reading the output of kubectl.

status: creationTime: 1732377314333256450 readyToUse: true volumeGroupSnapshotHandle: 53e1f6ce-a9b3-11ef-adf7-88c9b3b05f70 volumeSnapshotHandlePairList: - snapshotHandle: 53e1f6e0-a9b3-11ef-adf7-88c9b3b05f70 volumeHandle: 4711d50d-a9b3-11ef-adf7-88c9b3b05f70

I understand we made this decision for volumesnapshotcontent already, but this seems like making one bad choice to cover another. I could be wrong of course, @jsafrane @msau42 what do you think?

Since VolumeSnapshot and VolumeGroupSnapshot already have timestamps shown in user readable format, my preference is to keep the timestamps in VolumeSnapshotContent and VolumeGroupSnapshotContent the same as what is returned by the CSI driver as they are admin facing, but let's see what @jsafrane and @msau42 think.

@leonardoce can you please update the comments with the highlighted text?

type VolumeGroupSnapshotStatus struct {
..........
// CreationTime is the timestamp when the point-in-time group snapshot is taken
// by the underlying storage system.
// If not specified, it may indicate that the creation time of the group snapshot
// is unknown.
// The format of this field is a Unix nanoseconds time encoded as an int64.
// On Unix, the command date +%s%N returns the current time in nanoseconds
// since 1970-01-01 00:00:00 UTC.
// This field is updated based on the CreationTime field in VolumeGroupSnapshotContentStatus
// +optional
CreationTime *metav1.Time json:"creationTime,omitempty" protobuf:"bytes,2,opt,name=creationTime"

type VolumeGroupSnapshotContentStatus struct {
..........
// CreationTime is the timestamp when the point-in-time group snapshot is taken
// by the underlying storage system.
// If not specified, it indicates the creation time is unknown.
// If not specified, it means the readiness of a group snapshot is unknown.
// The format of this field is a Unix nanoseconds time encoded as an int64.
// On Unix, the command date +%s%N returns the current time in nanoseconds
// since 1970-01-01 00:00:00 UTC.
// This field is the source for the CreationTime field in VolumeGroupSnapshotStatus
// +optional
CreationTime *int64 json:"creationTime,omitempty" protobuf:"varint,2,opt,name=creationTime"

I improved the comments @xing-yang.

client/apis/volumegroupsnapshot/v1beta1/types.go

gnufied · 2024-11-23T03:16:16Z

I noticed that following field in volumeGroupSnapshot.Status is user editable after setting it:

BoundVolumeGroupSnapshotContentName *string `json:"boundVolumeGroupSnapshotContentName,omitempty" protobuf:"bytes,1,opt,name=boundVolumeGroupSnapshotContentName"`

Should this be immutable after creation?

xing-yang · 2024-11-23T15:13:02Z

I noticed that following field in volumeGroupSnapshot.Status is user editable after setting it:

BoundVolumeGroupSnapshotContentName *string json:"boundVolumeGroupSnapshotContentName,omitempty" protobuf:"bytes,1,opt,name=boundVolumeGroupSnapshotContentName"
Should this be immutable after creation?

This field is in the Status. Are we supposed to make fields in Status immutable? We have not done that for VolumeSnapshot APIs.

gnufied · 2024-11-24T14:20:23Z

This field is in the Status. Are we supposed to make fields in Status immutable? We have not done that for VolumeSnapshot APIs.

It would have been nice to have. But not a blocker I think.

gnufied · 2024-11-25T02:24:08Z

client/apis/volumegroupsnapshot/v1beta1/types.go

-	// VolumeSnapshotContentRef is a reference to the volume snapshot content resource
-	VolumeSnapshotContentRef core_v1.LocalObjectReference `json:"volumeSnapshotContentRef,omitempty" protobuf:"bytes,2,opt,name=volumeSnapshotContentRef"`
-}
-
 // VolumeGroupSnapshotContentSource represents the CSI source of a group snapshot.


Not visible in PR - Can VolumeHandle and SnapshotHandle inside VolumeSnapshotHandlePair be empty? Or they both have to be specified. I didn't see these fields marked as required and neither see any validation kinda logic preventing saving of these fields without any values. Correct me if I missed something.

VolumeSnapshotHandlePair itself is not required, but if a VolumeSnapshotHandlePair is specified, then both VolumeHandle and SnapshotHandle should be required. Otherwise, it is not very useful.
We can add CEL for them.

We can add CEL for them.

Yeah we should IMO.

@leonardoce Can you make changes to make VolumeHandle and SnapshotHandle required in VolumeSnapshotHandlePair?

Sure! I think it's better to file a different PR for that and keep this as a giant substitution. If we use this PR to review the API, it will become really difficult to rebase.

Is that fine with you @gnufied ?

The main reason I wanted all API changes in same PR is, so as we don't have to do follow up API fixes.

@gnufied I see both fields are already marked as required (meaning they are not optional) in the CRD.
See:

external-snapshotter/client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yaml

Lines 290 to 312 in b79aef9

volumeSnapshotHandlePairList:

description: |-

VolumeSnapshotHandlePairList is a list of CSI "volume_id" and "snapshot_id"

pair returned by the CSI driver to identify snapshots and their source volumes

on the storage system.

items:

description: VolumeSnapshotHandlePair defines a pair of a source

volume handle and a snapshot handle

properties:

snapshotHandle:

description: |-

SnapshotHandle is a unique id returned by the CSI driver to identify a volume

snapshot on the storage system

type: string

volumeHandle:

description: |-

VolumeHandle is a unique id returned by the CSI driver to identify a volume

on the storage system

type: string

required:

- snapshotHandle

- volumeHandle

type: object

Do you still need me to add some CEL rules for them? Thanks!

That should be sufficient. The API server will reject it if VolumeHandle or SnapshotHandle is not set in VolumeSnapshotHandlePair.

Can you add "// Required." as comments for both VolumeHandle and SnapshotHandle in types.go?

I did it and rebased the PR too.

Madhu-1 · 2024-11-25T12:37:54Z

@gnufied @xing @leonardoce i have tested this PR + #1219 https://jenkins-ceph-csi.apps.ocp.cloud.ci.centos.org/blue/organizations/jenkins/mini-e2e_k8s-1.31/detail/mini-e2e_k8s-1.31/201/pipeline/ ceph/ceph-csi#4974 (we already have framework for volumegroupsnapshot)

This is what it does

Create 3 PVC
Create a groupsnapshot
Create a clones from the 3 snapshots (as part of group)
Create pod using the clones
Delete the pod
Delete the clones
Delete the group
Delete the PVC

Repeat the above in a loop of 20 , let me know if you guys wants me to test anything else?

Signed-off-by: Leonardo Cecchi <[email protected]>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Aug 30, 2024

k8s-ci-robot requested review from jingxu97 and yuxiangqian August 30, 2024 10:00

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 30, 2024

k8s-ci-robot requested a review from xing-yang October 1, 2024 14:49

leonardoce force-pushed the update-vgs-beta branch from c3d17b0 to 6592941 Compare October 17, 2024 15:43

nixpanic reviewed Oct 18, 2024

View reviewed changes

client/apis/volumegroupsnapshot/v1beta1/types.go Outdated Show resolved Hide resolved

nixpanic mentioned this pull request Oct 18, 2024

add VolumeGroupSnapshotClass for CephFS and RBD red-hat-storage/ocs-operator#2859

Open

leonardoce force-pushed the update-vgs-beta branch from 4f5aa74 to e72f89f Compare October 19, 2024 09:05

yati1998 reviewed Oct 21, 2024

View reviewed changes

leonardoce mentioned this pull request Oct 23, 2024

Add and set the new VolumeSnapshotHandlePairList field #1169

Merged

leonardoce marked this pull request as draft October 23, 2024 15:34

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024

leonardoce force-pushed the update-vgs-beta branch from e72f89f to a920081 Compare November 6, 2024 12:54

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2024

leonardoce force-pushed the update-vgs-beta branch from a920081 to 2849922 Compare November 8, 2024 16:22

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2024

leonardoce force-pushed the update-vgs-beta branch from d0b046e to 06003b4 Compare November 21, 2024 16:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2024

xing-yang reviewed Nov 21, 2024

View reviewed changes

client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yaml Outdated Show resolved Hide resolved

leonardoce commented Nov 22, 2024

View reviewed changes

leonardoce force-pushed the update-vgs-beta branch from 977bca6 to a834ee5 Compare November 22, 2024 18:44

gnufied reviewed Nov 22, 2024

View reviewed changes

gnufied reviewed Nov 23, 2024

View reviewed changes

client/apis/volumegroupsnapshot/v1beta1/types.go Show resolved Hide resolved

leonardoce force-pushed the update-vgs-beta branch from 011ceb7 to a475473 Compare November 24, 2024 07:55

gnufied reviewed Nov 25, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2024

leonardoce force-pushed the update-vgs-beta branch from a475473 to 7ceb7bb Compare November 25, 2024 08:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2024

leonardoce force-pushed the update-vgs-beta branch 3 times, most recently from f866a51 to 529df83 Compare November 26, 2024 21:12

leonardoce added 5 commits November 27, 2024 10:43

Update VolumeGroupSnapshot to v1beta1

5f39804

Signed-off-by: Leonardo Cecchi <[email protected]>

Remove v1alpha1 API

f55f507

Remove unused structures

d7d24ef

Improve VolumeGroupSnapshotStatus comments

c501cae

Improve VolumeSnapshotPairList comments

24e5a53

leonardoce force-pushed the update-vgs-beta branch from 529df83 to 24e5a53 Compare November 27, 2024 15:25

	volumeSnapshotHandlePairList:
	description: \|-
	VolumeSnapshotHandlePairList is a list of CSI "volume_id" and "snapshot_id"
	pair returned by the CSI driver to identify snapshots and their source volumes
	on the storage system.
	items:
	description: VolumeSnapshotHandlePair defines a pair of a source
	volume handle and a snapshot handle
	properties:
	snapshotHandle:
	description: \|-
	SnapshotHandle is a unique id returned by the CSI driver to identify a volume
	snapshot on the storage system
	type: string
	volumeHandle:
	description: \|-
	VolumeHandle is a unique id returned by the CSI driver to identify a volume
	on the storage system
	type: string
	required:
	- snapshotHandle
	- volumeHandle
	type: object

Update VolumeGroupSnapshot to v1beta1 #1150

Are you sure you want to change the base?

Update VolumeGroupSnapshot to v1beta1 #1150

Conversation

leonardoce commented Aug 30, 2024 • edited Loading

k8s-ci-robot commented Aug 30, 2024

nixpanic commented Oct 1, 2024

leonardoce commented Oct 18, 2024

yati1998 left a comment

Choose a reason for hiding this comment

xing-yang commented Oct 21, 2024

leonardoce commented Oct 21, 2024

yati1998 commented Oct 23, 2024

leonardoce commented Oct 23, 2024

leonardoce commented Oct 23, 2024

mulbc commented Oct 28, 2024

yati1998 commented Nov 6, 2024

leonardoce commented Nov 6, 2024

xing-yang commented Nov 21, 2024

yati1998 commented Nov 22, 2024

Choose a reason for hiding this comment

xing-yang commented Nov 22, 2024

gnufied commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Nov 23, 2024

xing-yang commented Nov 23, 2024

gnufied commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoce Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Nov 25, 2024

leonardoce commented Aug 30, 2024 •

edited

Loading

gnufied Nov 22, 2024 •

edited

Loading

gnufied Nov 25, 2024 •

edited

Loading

leonardoce Nov 25, 2024 •

edited

Loading