Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update VolumeGroupSnapshot to v1beta1 #1150

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

leonardoce
Copy link
Contributor

@leonardoce leonardoce commented Aug 30, 2024

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 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 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
@nixpanic
Copy link
Member

nixpanic commented Oct 1, 2024

/cc xing-yang

@leonardoce
Copy link
Contributor Author

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

Copy link
Contributor

@yati1998 yati1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xing-yang
Copy link
Collaborator

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

@leonardoce
Copy link
Contributor Author

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

@yati1998
Copy link
Contributor

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
Copy link
Contributor Author

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

@leonardoce
Copy link
Contributor Author

/draft

@leonardoce leonardoce marked this pull request as draft October 23, 2024 15:34
@k8s-ci-robot 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
@mulbc
Copy link

mulbc commented Oct 28, 2024

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

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

yati1998 commented Nov 6, 2024

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

@leonardoce
Copy link
Contributor Author

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.

@k8s-ci-robot 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 leonardoce marked this pull request as ready for review November 8, 2024 16:37
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2024
@Madhu-1
Copy link
Contributor

Madhu-1 commented Nov 25, 2024

@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?

@leonardoce leonardoce force-pushed the update-vgs-beta branch 4 times, most recently from 529df83 to 24e5a53 Compare November 27, 2024 15:25
@iPraveenParihar
Copy link

iPraveenParihar commented Dec 2, 2024

Hey, I tested this using the CSI HostPath Driver in a GitHub Action. The test, which can be viewed here, includes the following steps:

  1. Create 3 PVCs and mount them to a Pod.
  2. Create a VolumeGroupSnapshot.
  3. Restore PVCs from VolumeSnapshots, mount them to a Pod.
  4. Delete the VolumeGroupSnapshot.
  5. Delete the Pods and PVCs.

This sequence is repeated 20 times.

cc @Madhu-1

@manishym
Copy link
Contributor

manishym commented Dec 2, 2024

I have run the k8s e2e test on vgs. It is passing.

Conformance test: not doing test setup.
  I1202 21:02:28.820217 1196005 e2e.go:109] Starting e2e run "e05caadd-9770-4603-828c-799227b70b3f" on Ginkgo node 1
Running Suite: Kubernetes e2e suite - /home/myathnalli/data/work/k8s/kubernetes/_output/bin
===========================================================================================
Random Seed: 1733153547 - will randomize all specs

Will run 21 of 6622 specs
••Checking for custom logdump instances, if any
----------------------------------------------------------------------------------------------------
k/k version of the log-dump.sh script is deprecated!
Please migrate your test job to use test-infra's repo version of log-dump.sh!
Migration steps can be found in the readme file.
----------------------------------------------------------------------------------------------------
Sourcing kube-util.sh
Detecting project
Skeleton Provider: detect-project not implemented
Dumping logs from master locally to '/home/myathnalli/data/work/k8s/kubernetes/_artifacts'
KUBE_MASTER_IP:
KUBE_MASTER:
/home/myathnalli/data/work/k8s/kubernetes/cluster/log-dump/log-dump.sh: line 385: MASTER_NAME: unbound variable


Ran 2 of 6622 Specs in 428.863 seconds
SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 6620 Skipped
PASS

Ginkgo ran 1 suite in 7m10.968249776s
Test Suite Passed
+ cleanup
+ '[' false = true ']'
+ '[' true = true ']'
+ kind export logs /home/myathnalli/data/work/k8s/kubernetes/_artifacts
Exporting logs for cluster "kind" to:
/home/myathnalli/data/work/k8s/kubernetes/_artifacts
+ kind delete cluster
Deleting cluster "kind" ...
Deleted nodes: ["kind-control-plane" "kind-worker2" "kind-worker"]
+ rm -f _output/bin/e2e.test
+ '[' -n /tmp/tmp.CQSRZTuJUg ']'
+ rm -rf /tmp/tmp.CQSRZTuJUg
+ CLEANED_UP=true
+ exit 0

@Madhu-1
Copy link
Contributor

Madhu-1 commented Dec 2, 2024

@gnufied The above is the results for e2e tests

@leonardoce
Copy link
Contributor Author

I rebased it over the new master and tested it. My smoke tests look good.

@gnufied
Copy link
Contributor

gnufied commented Dec 2, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2024
@xing-yang
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leonardoce, xing-yang

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2024
@k8s-ci-robot k8s-ci-robot merged commit a0cef53 into kubernetes-csi:master Dec 2, 2024
8 checks passed
@gbartolini
Copy link

This is great news!!! Amazing work, well done! Congratulations!

@msau42
Copy link
Collaborator

msau42 commented Dec 9, 2024

API review lgtm

@jonathon2nd
Copy link

I was wondering how I can install the update when I am getting the following error
CustomResourceDefinition.apiextensions.k8s.io "volumegroupsnapshots.groupsnapshot.storage.k8s.io" is invalid: status.storedVersions[0]: Invalid value: "v1alpha1": must appear in spec.versions

See piraeusdatastore/helm-charts#54

@pl4nty
Copy link

pl4nty commented Dec 21, 2024

@jonathon2nd kubectl patch crd x.groupsnapshot.storage.k8s.io --subresource='status' --type='merge' -p '{"status":{"storedVersions": ["v1beta1"]}}' where x is each of volumegroupsnapshots, volumegroupsnapshotclasses, and volumegroupsnapshotcontents

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update VolumeGroupSnapshot CRDs to v1beta1