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

New functions to pre-provision VolumeSnapshot and VolumeSnapshotContent #1282

Merged
merged 13 commits into from
Mar 23, 2022

Conversation

ihcsim
Copy link
Contributor

@ihcsim ihcsim commented Mar 14, 2022

Change Overview

This PR adds two new functions to manage CSI volume snapshots and snapshot contents:

  • CreateCSISnapshotStatic - create a pre-provisioned VolumeSnapshot/VolumeSnapshotContent resource pairs
  • DeleteCSISnapshotContent - delete VolumeSnapshotContent resources.

For more information on how the pre-provisioned snapshots are different from those created with PVCs, see the Kubernetes documentation here.

This PR is a follow-up on #1246, where we had decided not to extend the existing KubeOps function to delete cluster-scoped resources like VolumeSnapshotContent, to avoid misuses.

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

See #1246 (comment).

Test Plan

These test steps require the following 3rd party components to be installed on the Kubernetes cluster:

Create the following blueprint:

$ cat <<EOF | kubectl apply -f -
apiVersion: cr.kanister.io/v1alpha1
kind: Blueprint
metadata:
  name: manage-static-snapshots
  namespace: kanister
actions:
  # new function - create a pair of static VolumeSnapshot and VolumeSnapshotContent
  createStaticSnapshots:
    phases:
    - func: CreateCSISnapshotStatic
      name: create
      args:
        name: "{{ .ArtifactsIn.params.KeyValue.name }}"
        namespace: "{{ .ArtifactsIn.params.KeyValue.namespace }}"   # namespace must exist
        snapshotClass: "{{ .ArtifactsIn.params.KeyValue.class }}"
        driver: "{{ .ArtifactsIn.params.KeyValue.csiDriver }}"
        snapshotHandle: "{{ .ArtifactsIn.params.KeyValue.handle }}"
  # existing function - delete VolumeSnapshot
  deleteVolumeSnapshot:
    inputArtifactNames:
    - volumeSnapshotName
    - volumeSnapshotNameSpace
    phases:
    - func: DeleteCSISnapshot
      name: delete
      args:
        name: "{{ .ArtifactsIn.params.KeyValue.volumeSnapshotName }}"
        namespace: "{{ .ArtifactsIn.params.KeyValue.volumeSnapshotNamespace }}"
  # new function - delete VolumeSnapshotContent
  deleteVolumeSnapshotContent:
    inputArtifactNames:
    - volumeSnapshotContentName
    phases:
    - func: DeleteCSISnapshotContent
      name: delete
      args:
        name: "{{ .ArtifactsIn.params.KeyValue.volumeSnapshotContentName }}"
EOF

Use this ActionSet resource to create the static resources:

$ cat <<EOF | kubectl create -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: create-static-snapshot-
  namespace: kanister
spec:
  actions:
  - name: createStaticSnapshots
    blueprint: manage-static-snapshots
    object:
       kind: Namespace
       name: kanister
    artifacts:
      params:
        keyValue:
          name: test-snapshot-content
          namespace: default
          class: csi-hostpath-snapclass
          csiDriver: hostpath.csi.k8s.io
          handle: 7bdd0de3-aaeb-11e8-9aae-0242ac110002
EOF

Confirm that the resources are created:

$ kubectl get vs,vsc
NAME                                                           READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT                                                RESTORESIZE   SNAPSHOTCLASS                   SNAPSHOTCONTENT                                                      CREATIONTIME   AGE
volumesnapshot.snapshot.storage.k8s.io/test-snapshot-content   false                    test-snapshot-content-content-9503c7b0-32d2-46e2-ad6e-ee8f3fa949b6                 csi-hostpath-snapclass-retain   test-snapshot-content-content-9503c7b0-32d2-46e2-ad6e-ee8f3fa949b6                  11s

NAME                                                                                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS             VOLUMESNAPSHOT          VOLUMESNAPSHOTNAMESPACE   AGE
volumesnapshotcontent.snapshot.storage.k8s.io/test-snapshot-content-content-9503c7b0-32d2-46e2-ad6e-ee8f3fa949b6   false                      Retain           hostpath.csi.k8s.io   csi-hostpath-snapclass-retain   test-snapshot-content   default                   11s

The resource deletion behaviour is defined by the deletionPolicy of the snapshotClass of the snapshot.

Since by default, the csi-hostpath-snapclass snapshot class uses the Delete policy, deleting the VolumeSnapshot will also delete the VolumeSnapshotContent:

$ cat <<EOF | kubectl create -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: delete-vs-
  namespace: kanister
spec:
  actions:
  - name: deleteVolumeSnapshot
    blueprint: manage-static-snapshots
    object:
      apiVersion: v1
      group: snapshot.storage.k8s.io
      resource: volumesnapshots
      name: test-snapshot-content
      namespace: default
    artifacts:
      params:
        keyValue:
          volumeSnapshotName: test-snapshot-content
          volumeSnapshotNamespace: default
EOF

$ kubectl get vs,vsc
No resources found

Change the deletionPolicy of the snapshot class to Retain:

kubectl patch volumesnapshotclass csi-hostpath-snapclass --type='json' -p '[{"op":"replace","path":"/deletionPolicy","value":"Retain"}]'

Recreate the VolumeSnapshot and VolumeSnapshotContent resources.

With the Retain policy, deleting the VolumeSnapshot will not delete the VolumeSnapshotContent:

$ cat <<EOF | kubectl create -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: delete-vs-
  namespace: kanister
spec:
  actions:
  - name: deleteVolumeSnapshot
    blueprint: manage-static-snapshots
    object:
      apiVersion: v1
      group: snapshot.storage.k8s.io
      resource: volumesnapshots
      name: test-snapshot-content
      namespace: default
    artifacts:
      params:
        keyValue:
          volumeSnapshotName: test-snapshot-content
          volumeSnapshotNamespace: default
EOF

$ kubectl get vs,vsc
NAME                                                                                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS             VOLUMESNAPSHOT          VOLUMESNAPSHOTNAMESPACE   AGE
volumesnapshotcontent.snapshot.storage.k8s.io/test-snapshot-content-content-9503c7b0-32d2-46e2-ad6e-ee8f3fa949b6   false                      Retain           hostpath.csi.k8s.io   csi-hostpath-snapclass-retain   test-snapshot-content   default                   4m52s

To delete the unbounded VolumeSnapshotContent, create an ActionSet to invoke the new DeleteCSISnapshotContent function:

$ cat <<EOF | kubectl create -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: delete-vsc-
  namespace: kanister
spec:
  actions:
  - name: deleteVolumeSnapshotContent
    blueprint: manage-static-snapshots
    object:
      apiVersion: v1
      group: snapshot.storage.k8s.io
      resource: volumesnapshotcontents
      name: test-snapshot-content-content-9503c7b0-32d2-46e2-ad6e-ee8f3fa949b6
    artifacts:
      params:
        keyValue:
          volumeSnapshotContentName: test-snapshot-content-content-9503c7b0-32d2-46e2-ad6e-ee8f3fa949b6
EOF

$ kubectl get vs,vsc
No resources found

Deleting a bounded VolumeSnapshotContent has no effects, as the resource would be protected by the CSI controller:

$ kubectl get vs,vsc
NAME                                                           READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT                                                RESTORESIZE   SNAPSHOTCLASS            SNAPSHOTCONTENT                                                      CREATIONTIME   AGE
volumesnapshot.snapshot.storage.k8s.io/test-snapshot-content   false                    test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c                 csi-hostpath-snapclass   test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c                  3s

NAME                                                                                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS      VOLUMESNAPSHOT          VOLUMESNAPSHOTNAMESPACE   AGE
volumesnapshotcontent.snapshot.storage.k8s.io/test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c   false                      Delete           hostpath.csi.k8s.io   csi-hostpath-snapclass   test-snapshot-content   default                   3s

$ cat <<EOF | kubectl create -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: delete-vsc-
  namespace: kanister
spec:
  actions:
  - name: deleteVolumeSnapshotContent
    blueprint: manage-static-snapshots
    object:
      apiVersion: v1
      group: snapshot.storage.k8s.io
      resource: volumesnapshotcontents
      name: test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c
    artifacts:
      params:
        keyValue:
          volumeSnapshotContentName: test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c
EOF

$ kubectl get vs,vsc
NAME                                                           READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT                                                RESTORESIZE   SNAPSHOTCLASS
     SNAPSHOTCONTENT                                                      CREATIONTIME   AGE
volumesnapshot.snapshot.storage.k8s.io/test-snapshot-content   false                    test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c                 csi-hostpath-snapcla
ss   test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c                  2m17s

NAME                                                                                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS      VOLUMESNAPSHOT          VOLUMESNAPSHOTNAMESPACE   AGE
volumesnapshotcontent.snapshot.storage.k8s.io/test-snapshot-content-content-ff80129c-23dd-437d-b02e-2bb886db325c   false                      Delete           hostpath.csi.k8s.io   csi-hostpath-snapclass   test-snapshot-content   default                   2m17s

@ihcsim ihcsim changed the title New Functions To Pre-provisioned VolumeSnapshot and VolumeSnapshotContent New Functions to pre-provision VolumeSnapshot and VolumeSnapshotContent Mar 14, 2022
@ihcsim ihcsim changed the title New Functions to pre-provision VolumeSnapshot and VolumeSnapshotContent New functions to pre-provision VolumeSnapshot and VolumeSnapshotContent Mar 14, 2022
@ihcsim
Copy link
Contributor Author

ihcsim commented Mar 14, 2022

@pavannd1 @viveksinghggits While writing my test ActionSet, I am a bit confused by the action's required object field. It looks like the original intention was to use this field to identify the resource that the function should operate on. There are some validation logic to ensure these resources exist, which doesn't make sense for Create functions.

Also, some existing functions defined their own required arguments that duplicate these resource metadata, and their code don't read from the object template parameter. I think for this PR, I'm going to follow the existing code pattern in the {Create|Delete}CSISnapshot functions which use their own function arguments, instead of the object template parameter.

@viveksinghggits
Copy link
Contributor

@ihcsim does that mean the object field that you have specified while creating the actionset is of no use in this case? Also do you think we should add an example of how to use this in examples directory or docs are enough.

docs/functions.rst Outdated Show resolved Hide resolved
docs/functions.rst Outdated Show resolved Hide resolved
docs/functions.rst Outdated Show resolved Hide resolved
docs/functions.rst Outdated Show resolved Hide resolved
docs/functions.rst Outdated Show resolved Hide resolved
pkg/function/create_csi_snapshot_static.go Outdated Show resolved Hide resolved
@pavannd1 pavannd1 requested a review from shlokc9 March 15, 2022 19:09
@ihcsim ihcsim force-pushed the preprovision-snapshot branch 2 times, most recently from 94d1fe5 to a71e075 Compare March 16, 2022 18:17
@muffl0n
Copy link
Contributor

muffl0n commented Mar 17, 2022

snapshotClass should be non-mandantory. Kubernetes picks the default if nothing is specified.

Apart from that, the functions work like a charm! Thank you so much for implementing them so fast! ❤️

@ihcsim
Copy link
Contributor Author

ihcsim commented Mar 17, 2022

snapshotClass should be non-mandantory.

Yeah, we have some existing code that check for the existence of the snapshotClass. Since they go all the way back to v1alpha1 of the CRD, I am a bit hesitant to refactor them in this PR. AIUI, a default snapshot class is only set if one of the classes is annotated with snapshot.storage.kubernetes.io/is-default-class: "true", implying that on some clusters, there may not be a default class. WDYT?

docs/functions.rst Show resolved Hide resolved
docs/functions.rst Show resolved Hide resolved
pkg/function/create_csi_snapshot_static.go Outdated Show resolved Hide resolved
pkg/function/create_csi_snapshot_static.go Outdated Show resolved Hide resolved
pkg/function/create_csi_snapshot_static.go Outdated Show resolved Hide resolved
pkg/function/create_csi_snapshot_static_test.go Outdated Show resolved Hide resolved
pkg/function/create_csi_snapshot_static_test.go Outdated Show resolved Hide resolved
pkg/function/create_csi_snapshot_static_test.go Outdated Show resolved Hide resolved
pkg/function/create_csi_snapshot_static_test.go Outdated Show resolved Hide resolved
pkg/function/delete_csi_snapshot_content_test.go Outdated Show resolved Hide resolved
ihcsim and others added 9 commits March 22, 2022 14:35
…tions

The new 'create' function allows user to pre-provision a pair of
VolumeSnapshot and VolumeSnapshotContent resources, which aren't associated
with any existing PersistentVolumeClaim resource.

See https://kubernetes.io/docs/concepts/storage/volume-snapshots/ for
the difference between dynamic and pre-provisioned snapshots.

Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Co-authored-by: Pavan Navarathna <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
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.

LGTM

@ihcsim ihcsim added the kueue label Mar 23, 2022
@mergify mergify bot merged commit e3e762e into master Mar 23, 2022
@mergify mergify bot deleted the preprovision-snapshot branch March 23, 2022 23:44
@ihcsim ihcsim mentioned this pull request Mar 24, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants