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

fix(webhook): reject PVC deletion request when dependent snapshots exists #1669

Merged
merged 6 commits into from
Apr 16, 2020

Conversation

mittachaitu
Copy link

@mittachaitu mittachaitu commented Apr 15, 2020

What this PR does / why we need it:
This PR rejects the PVC deletion request when the snapshot exists for the corresponding volume.
When VolumeSnapshot/VolumeSnapshotData exist for particular volume then the deletion of PVC will be errored with the following message

sai@sai:~/Desktop/learn/aws_external_disk$ kubectl delete -f pvc.yml 
Error from server: error when deleting "pvc.yml": admission webhook "admission-webhook.openebs.io" denied the request: snapshots verification failed pvc demo-vol1-claim has '1' snapshot(s)

Note:

  • The request will be rejected only if the admission server pod is in running state.
  • Irrespective of the ReclaimPolicy of PV, the PVC deletion request will be rejected.

Sample SnapshotExample which will help for reviewers:
volumesnapshot example:

apiVersion: volumesnapshot.external-storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"volumesnapshot.external-storage.k8s.io/v1","kind":"VolumeSnapshot","metadata":{"annotations":{},"labels":{"name":"snapshot-demo-csto"},"name":"snapshot-demo-cstor","namespace":"default"},"spec":{"persistentVolumeClaimName":"demo-vol1-claim"}}
  creationTimestamp: "2020-04-15T14:07:06Z"
  generation: 3
  labels:
    SnapshotMetadata-PVName: pvc-c6a09b80-4d3c-4302-ab91-dd6b35dc990d
    SnapshotMetadata-Timestamp: "1586959626455945327"
    name: snapshot-demo-csto
  name: snapshot-demo-cstor
  namespace: default
  resourceVersion: "1199"
  selfLink: /apis/volumesnapshot.external-storage.k8s.io/v1/namespaces/default/volumesnapshots/snapshot-demo-cstor
  uid: 148cf3fb-a999-4298-8541-1c8e3501b318
spec:
  persistentVolumeClaimName: demo-vol1-claim
  snapshotDataName: k8s-volume-snapshot-6459c902-7f22-11ea-ac93-0242ac110004
status:
  conditions:
  - lastTransitionTime: "2020-04-15T14:07:07Z"
    message: Snapshot created successfully
    reason: ""
    status: "True"
    type: Ready
  creationTimestamp: null

VolumeSnapshotData example:

apiVersion: volumesnapshot.external-storage.k8s.io/v1
kind: VolumeSnapshotData
metadata:
  creationTimestamp: "2020-04-15T14:07:07Z"
  generation: 1
  name: k8s-volume-snapshot-6459c902-7f22-11ea-ac93-0242ac110004
  resourceVersion: "1198"
  selfLink: /apis/volumesnapshot.external-storage.k8s.io/v1/volumesnapshotdatas/k8s-volume-snapshot-6459c902-7f22-11ea-ac93-0242ac110004
  uid: a1086239-97a4-4cb2-963f-f8953c1fa4a4
spec:
  openebsVolume:
    capacity: 5G
    snapshotId: pvc-c6a09b80-4d3c-4302-ab91-dd6b35dc990d_snapshot-demo-cstor_1586959626466213150
  persistentVolumeRef:
    kind: PersistentVolume
    name: pvc-c6a09b80-4d3c-4302-ab91-dd6b35dc990d
  volumeSnapshotRef:
    kind: VolumeSnapshot
    name: default/snapshot-demo-cstor-148cf3fb-a999-4298-8541-1c8e3501b318
status:
  conditions:
  - lastTransitionTime: "2020-04-15T14:07:07Z"
    message: Snapshot created successfully
    reason: ""
    status: "True"
    type: Ready
  creationTimestamp: null

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #openebs/openebs#2994

Release Note:

  • PVC deletion request will be error out if the volume has dependent snapshots(VolumeSnapshot (or) VolumeSnapshotData).

Special notes for your reviewer:

Checklist:

  • Fixes #
  • Labelled this PR & related issue with documentation tag
  • PR messages has document related information
  • Labelled this PR & related issue with breaking-changes tag
  • PR messages has breaking changes related information
  • Labelled this PR & related issue with requires-upgrade tag
  • PR messages has upgrade related information
  • Commit has unit tests
  • Commit has integration tests

mittachaitu added 4 commits April 15, 2020 19:21
…ists

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
…esnapshot.external-storage.k8s.io

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
@mittachaitu mittachaitu self-assigned this Apr 15, 2020
@mittachaitu mittachaitu added this to the 1.10 milestone Apr 15, 2020
@mittachaitu mittachaitu added the pr/release-note PR should be included in release notes label Apr 15, 2020
snapshotDataList, err := wh.snapClientSet.
VolumesnapshotV1().
VolumeSnapshotDatas().
List(metav1.ListOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Not remember exactly , does VolumeSnapshotData not have any PV based label ?

Copy link
Author

Choose a reason for hiding this comment

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

The PR description already has an example to make easy reviews

Copy link
Contributor

@prateekpandey14 prateekpandey14 Apr 16, 2020

Choose a reason for hiding this comment

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

ok i can see that you have added the full SnapshotData resources ( it was not there before when i reviewed)

Copy link
Author

Choose a reason for hiding this comment

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

metadata added later.

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
@mittachaitu
Copy link
Author

sai@sai:~/gocode/src/github.com/openebs/maya/tests/cstor/snapshot$ ginkgo -v -- -kubeconfig=/var/run/kubernetes/admin.kubeconfig -cstor-maxpools=1 -cstor-replicas=1
Running Suite: Test cstor volume snapshot provisioning
======================================================
Random Seed: 1586967695
Will run 1 of 1 specs

STEP: waiting for maya-apiserver pod to come into running state
STEP: waiting for openebs-provisioner pod to come into running state
STEP: Verifying 'admission-server' pod status as running
STEP: building a namespace
STEP: creating a namespace
[cstor] TEST SNAPSHOT PROVISIONING when cstor pvc with replicacount n is created 
  should create cstor volume target pod
  /home/sai/gocode/src/github.com/openebs/maya/tests/cstor/snapshot/snapshot_test.go:96
STEP: building spc object
STEP: creating storagepoolclaim
STEP: verifying healthy csp count
STEP: building a CAS Config with generated SPC name
STEP: building storageclass object
STEP: creating storageclass
STEP: building a persistentvolumeclaim
STEP: creating cstor persistentvolumeclaim
STEP: verifying volume target pod count as 1
STEP: verifying cstorvolume replica count
STEP: verifying pvc status as bound
STEP: building a cstor volume snapshot
STEP: creating cstor volume snapshot
STEP: verifying snapshot status as ready
STEP: Deleting PersistentVolumeClaim Should Reject the Request Because Of Snapshot Existence
STEP: deleting cstor volume snapshot
STEP: verifying deleted snapshot
STEP: Verifying deletion of snapshotdata
STEP: deleting above pvc
STEP: verifying target pod count as 0
STEP: verifying deleted pvc
STEP: verifying if cstorvolume is deleted
STEP: deleting resources created for cstor volume snapshot provisioning
STEP: deleting storagepoolclaim
STEP: deleting storageclass

• [SLOW TEST:111.625 seconds]
[cstor] TEST SNAPSHOT PROVISIONING
/home/sai/gocode/src/github.com/openebs/maya/tests/cstor/snapshot/snapshot_test.go:36
  when cstor pvc with replicacount n is created
  /home/sai/gocode/src/github.com/openebs/maya/tests/cstor/snapshot/snapshot_test.go:95
    should create cstor volume target pod
    /home/sai/gocode/src/github.com/openebs/maya/tests/cstor/snapshot/snapshot_test.go:96
------------------------------
STEP: deleting namespace

Ran 1 of 1 Specs in 111.682 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

Ginkgo ran 1 suite in 1m57.727257024s
Test Suite Passed

@mittachaitu mittachaitu requested review from shubham14bajpai and removed request for mynktl April 16, 2020 05:29
Copy link
Contributor

@mynktl mynktl left a comment

Choose a reason for hiding this comment

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

looks good to me

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Copy link
Contributor

@shubham14bajpai shubham14bajpai left a comment

Choose a reason for hiding this comment

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

LGTM

@prateekpandey14 prateekpandey14 merged commit 952c4c3 into openebs-archive:master Apr 16, 2020
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