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

Rearrange order of snapshot APIs #1979

Merged
merged 1 commit into from
Apr 4, 2023
Merged

Conversation

shahpratikr
Copy link
Contributor

@shahpratikr shahpratikr commented Mar 30, 2023

Change Overview

This PR rearranges the order of Snapshot APIs in v1 (stable), v1beta1 and v1alpha1.
This fixes taking snapshot when RKE2 snapshot-validation-hook is enabled.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

Test Plan

  • 💪 Manual

  • ⚡ Unit test

  • 💚 E2E

  • Manually applied this fix in a cluster and was able to take a snapshot of a namespace when RKE2 webhook was enabled.

  • All E2E tests passed.

  • Manually verified backup taken before upgrade is getting restored after upgrade as well

Steps:

  • Deployed cluster with k8s v1.22. (without these changes)
  • Took a snapshot of a volume (attached volume snapshot details below)
  • Upgraded cluster with these changes
  • Restored snapshot

Volume snapshot details:

# kubectl describe volumesnapshot csi-snap-ckt7sqft6ld8jttg
Name:         csi-snap-ckt7sqft6ld8jttg
Namespace:    default
Annotations:  <none>
API Version:  snapshot.storage.k8s.io/v1
Kind:         VolumeSnapshot
Metadata:
  Creation Timestamp:  2023-03-31T06:57:46Z
  Finalizers:
    snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection
    snapshot.storage.kubernetes.io/volumesnapshot-bound-protection
  Generation:  1
  Managed Fields:
    API Version:  snapshot.storage.k8s.io/v1beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
          f:name:
      f:spec:
        .:
        f:source:
          .:
          f:persistentVolumeClaimName:
        f:volumeSnapshotClassName:
    Manager:      executor-server
    Operation:    Update
    Time:         2023-03-31T06:57:46Z
    API Version:  snapshot.storage.k8s.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          v:"snapshot.storage.kubernetes.io/volumesnapshot-bound-protection":
      f:status:
        .:
        f:boundVolumeSnapshotContentName:
    Manager:      snapshot-controller
    Operation:    Update
    Time:         2023-03-31T06:57:47Z
    API Version:  snapshot.storage.k8s.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        f:creationTime:
        f:readyToUse:
        f:restoreSize:
    Manager:         snapshot-controller
    Operation:       Update
    Subresource:     status
    Time:            2023-03-31T06:58:12Z
  Resource Version:  387948
  UID:               8bbeb46b-6609-4b19-9a82-642e663f0afb
Spec:
  Source:
    Persistent Volume Claim Name:  pv-claim
  Volume Snapshot Class Name:      pd-csi-snapshotclass
Status:
  Bound Volume Snapshot Content Name:  snapcontent-8bbeb46b-6609-4b19-9a82-642e663f0afb
  Creation Time:                       2023-03-31T06:57:49Z
  Ready To Use:                        true
  Restore Size:                        1Gi
Events:
  Type    Reason            Age   From                 Message
  ----    ------            ----  ----                 -------
  Normal  CreatingSnapshot  81s   snapshot-controller  Waiting for a snapshot default/csi-snap-ckt7sqft6ld8jttg to be created by the CSI driver.

@github-actions
Copy link
Contributor

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

@infraq infraq added this to In Progress in Kanister Mar 30, 2023
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

Since v1 is the default version in the newer clusters, this change makes sense 👍🏼

@pavannd1 pavannd1 requested review from bathina2, PrasadG193 and viveksinghggits and removed request for viveksinghggits March 30, 2023 23:43
@PrasadG193
Copy link
Contributor

Need to perform upgrade testing. E,g If Snapshot taken from older workflow can be restored by newer workflow. Please add detailed test plans in the PR discription. Thanks

@viveksinghggits
Copy link
Contributor

viveksinghggits commented Mar 31, 2023

@PrasadG193 we should also make sure that the snapshot related api version is different between these two tests/workflows right?

@PrasadG193
Copy link
Contributor

PrasadG193 commented Mar 31, 2023

Yes, we need to test for backward compatibility. If snapshots taken with beta APIs can be restored with stable API

@shahpratikr shahpratikr force-pushed the pratik/rearrange_snapshot_apis branch from 9fb0efb to 131fdaa Compare April 3, 2023 07:10
@shahpratikr
Copy link
Contributor Author

@PrasadG193 we have tried following things to verify the change.

  • create an older k8s cluster (v1.19.16) on minikube, enabled csi-driver and volumesnapshot addons in it. I see that both v1 and v1beta1 snapshot API versions are available in the cluster.
  • when volumesnapshot is created with v1beta1 version, conversion webhook converts it to v1 version while getting volumesnapshot using kubectl get volumesnapshot.
  • we also tried to use older volumesnapshotter version (v4.0.0->minikube default version, v2.1.2->manually tried this), but observed same behavior with them as well

Kanister automation moved this from In Progress to Reviewer approved Apr 4, 2023
@mergify mergify bot merged commit 0459fea into master Apr 4, 2023
Kanister automation moved this from Reviewer approved to Done Apr 4, 2023
@mergify mergify bot deleted the pratik/rearrange_snapshot_apis branch April 4, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants