Skip to content

Conversation

@RaunakShah
Copy link
Contributor

@RaunakShah RaunakShah commented Mar 16, 2022

What type of PR is this?

/kind api-change

What this PR does / why we need it:

This PR updates the following modules:

  • snapshotter client module to v5
  • snapshotted to v6.

This change is needed for the API updates in #665 to be available in external-provisioner code.

Testing:

  1. Create a VolumeSnapshot with older CRDs and controller:

% kubectl get volumesnapshotcontent
NAME                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS         VOLUMESNAPSHOT         VOLUMESNAPSHOTNAMESPACE   AGE
snapcontent-afa5e044-8036-4ded-a3b8-5e3756864727   true         1073741824    Delete           hostpath.csi.k8s.io   csi-hostpath-snapclass-v1   new-snapshot-demo-v1   default                   52m
  1. Updated CRDs and controller and verified that VolumeSnapshot is still ReadyToUse
% kubectl get volumesnapshot       
NAME                    READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT   RESTORESIZE   SNAPSHOTCLASS               SNAPSHOTCONTENT                                    CREATIONTIME   AGE
new-snapshot-demo-v1    true         csi-pvc                             1Gi           csi-hostpath-snapclass-v1   snapcontent-afa5e044-8036-4ded-a3b8-5e3756864727   60m            60m

% kubectl get volumesnapshotcontent
NAME                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS         VOLUMESNAPSHOT          VOLUMESNAPSHOTNAMESPACE   AGE
snapcontent-afa5e044-8036-4ded-a3b8-5e3756864727   true         1073741824    Delete           hostpath.csi.k8s.io   csi-hostpath-snapclass-v1   new-snapshot-demo-v1    default                   60m
  1. Created new VolumeSnapshot from newer CRDs:4.
% kubectl get volumesnapshotcontent
NAME                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS         VOLUMESNAPSHOT          VOLUMESNAPSHOTNAMESPACE   AGE
snapcontent-afa5e044-8036-4ded-a3b8-5e3756864727   true         1073741824    Delete           hostpath.csi.k8s.io   csi-hostpath-snapclass-v1   new-snapshot-demo-v1    default                   60m
snapcontent-fe4dbb1f-2a53-406d-8f7d-07c52379fe54   true         1073741824    Delete           hostpath.csi.k8s.io   csi-hostpath-snapclass-v1   new-snapshot-demo-v11   default                   2m18s
raunakshah@Raunaks-MacBook-Pro-2 kubernetes % kubectl get volumesnapshot       
NAME                    READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT   RESTORESIZE   SNAPSHOTCLASS               SNAPSHOTCONTENT                                    CREATIONTIME   AGE
new-snapshot-demo-v1    true         csi-pvc                             1Gi           csi-hostpath-snapclass-v1   snapcontent-afa5e044-8036-4ded-a3b8-5e3756864727   60m            60m
new-snapshot-demo-v11   true         csi-pvc                             1Gi           csi-hostpath-snapclass-v1   snapcontent-fe4dbb1f-2a53-406d-8f7d-07c52379fe54   2m20s          2m20s
  1. Edited new VolumeSnapshotContent with SourceVolumeMode field. NOTE: Webhook changes are still pending and this field will be made immutable in the future.
Name:         snapcontent-fe4dbb1f-2a53-406d-8f7d-07c52379fe54
Namespace:    
...
Spec:
  Deletion Policy:  Delete
  Driver:           hostpath.csi.k8s.io
  Source:
    Volume Handle:             ad0cda63-a99d-11ec-9d6d-72c856d5dc0b
  Source Volume Mode:          Filesystem
...

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Update snapshotter module to v6 and client module to v5

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 16, 2022
@RaunakShah RaunakShah changed the title [WIP Update snapshotter and client modules [WIP] Update snapshotter and client modules Mar 16, 2022
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 16, 2022
@RaunakShah
Copy link
Contributor Author

/assign @xing-yang

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 17, 2022
@xing-yang
Copy link
Collaborator

Can you add a release note?

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 18, 2022
@xing-yang
Copy link
Collaborator

Can you test installing the old CRDs (without the new field), creating a VolumeSnapshot using the old controller; then updating the CRDs and controller to the new ones, and make sure you can still access the VolumeSnapshot; also creating a VolumeSnapshot using the new CRD and new controller.

@RaunakShah RaunakShah changed the title [WIP] Update snapshotter and client modules Update snapshotter and client modules Mar 22, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2022
@RaunakShah
Copy link
Contributor Author

Can you test installing the old CRDs (without the new field), creating a VolumeSnapshot using the old controller; then updating the CRDs and controller to the new ones, and make sure you can still access the VolumeSnapshot; also creating a VolumeSnapshot using the new CRD and new controller.

@xing-yang Confirmed the above. Added the tests to the PR description. I'll be making changes to the Webhooks validation logic in a separate PR, so for now just tested that everything gets created as expected.

@xing-yang
Copy link
Collaborator

In the testing step 4, can you add a note about the pending webhook change and the new field should be immutable?

@xing-yang
Copy link
Collaborator

Can you also squash the commits?

- Update snapshotter client module to v5
- Update go version in client modules to v1.17
@RaunakShah
Copy link
Contributor Author

Can you also squash the commits?

Done!

@xing-yang
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RaunakShah, 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 Mar 23, 2022
@k8s-ci-robot k8s-ci-robot merged commit a5c387c into kubernetes-csi:master Mar 23, 2022
@@ -1,25 +1,57 @@
module github.com/kubernetes-csi/external-snapshotter/client/v4
module github.com/kubernetes-csi/external-snapshotter/client/v5
Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad. We actually need to update to client/v6. Client/v5 was cut earlier but we didn't update this link.
https://github.com/kubernetes-csi/external-snapshotter/releases/tag/client%2Fv5.0.0

Can you submit another PR to update this to module github.com/kubernetes-csi/external-snapshotter/client/v6?

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.

3 participants