Support for in-tree PV Deletion protection finalizer

[deepakkinni]

What type of PR is this?

/kind feature
/sig storage

What this PR does / why we need it:

Support for in-tree volume deletion protection as a part of kubernetes/enhancements#2644

Which issue(s) this PR fixes:

Fixes # kubernetes/enhancements#2644

Special notes for your reviewer:

Testing done:

1. Verified that the new finalizer kubernetes.io/pv-controller is present on the PV

apiVersion: v1
kind: PersistentVolume
metadata:
  name: pvc-2f355c16-1c7d-445c-8620-38d2b002118a
  uid: 6ba5d724-7e4d-4bf7-99f1-2cd9cbc3512f
  resourceVersion: '169754'
  creationTimestamp: '2022-03-03T23:04:37Z'
  annotations:
    kubernetes.io/createdby: vsphere-volume-dynamic-provisioner
    pv.kubernetes.io/bound-by-controller: 'yes'
    pv.kubernetes.io/provisioned-by: kubernetes.io/vsphere-volume
  finalizers:
    - kubernetes.io/pv-controller
    - kubernetes.io/pv-protection
status:
  phase: Bound
spec:
  capacity:
    storage: 1Gi
  vsphereVolume:
    volumePath: >-
      [vsanDatastore]
      6a492162-5521-98f4-6007-0200406001ba/kubernetes-dynamic-pvc-2f355c16-1c7d-445c-8620-38d2b002118a.vmdk
    fsType: ext4
    storagePolicyName: vSAN Default Storage Policy
    storagePolicyID: aa6d5a82-1c88-45da-85d3-3d74b91a5bad
  accessModes:
    - ReadWriteOnce
  claimRef:
    kind: PersistentVolumeClaim
    namespace: default
    name: vcp-pvc-1
    uid: 2f355c16-1c7d-445c-8620-38d2b002118a
    apiVersion: v1
    resourceVersion: '169688'
  persistentVolumeReclaimPolicy: Delete
  storageClassName: vcp-sc
  volumeMode: Filesystem

2. Verified that deleting the pv first followed by deleting the PVC results in the disk being removed from the underlying storage.

Does this PR introduce a user-facing change?

Support in-tree PV deletion protection finalizer.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/3181

Signed-off-by: Deepak Kinni dkinni@vmware.com

[k8s-ci-robot]

@deepakkinni: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[deepakkinni]

/retest

[deepakkinni]

/retest

[deepakkinni]

/assign @jsafrane @xing-yang

[jsafrane]

I also miss an e2e test for this feature - can it be tested with a hostpath volume?

[jsafrane]

Now that I think about an e2e test, maybe iSCSI or NFS might be a better choice. We don't support deletion of these volumes in-tree. Hence, create a PV that looks like iSCSI, but it does not need to represent any actual volume (and don't use it in a Pod). Set its ReclaimPolicy to Delete or Retain, check it gets the finalizer, delete it, check it's not deleted (Delete policy) / it is deleted (Retain) and that could be it for the "easy" cases.

The hard case is to ensure that if user deletes a PV, the finalizer is removed after PV controller / external-provisioner deletes the corresponding volume. Hence you need a way how to check the volume is really deleted. We don't have a good way how to do it. For CSI, HostPath CSI driver + deletion hooks could work, for in-tree I don't see an easy way.

It can be in a separate PR.

[deepakkinni]

It can be in a separate PR.

Thanks, it's a bit more complicated than I anticipated, will have the tests in a separate PR. This PR will have only the unit tests.

[jsafrane]

/lgtn
/approve

[jsafrane]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deepakkinni, jsafrane

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:

• pkg/controller/volume/persistentvolume/OWNERS [jsafrane]
• pkg/features/OWNERS [jsafrane]

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

[xing-yang]

@jsafrane This is not enabled by default? I thought only an API object is subject to this new change that Beta means disabled by default.

[xing-yang]

This is from the email announcing Beta API off by default: https://groups.google.com/a/kubernetes.io/g/dev/c/tkzbBcS0JI8
Beta feature gates remain on by default and beta fields on stable APIs remain on by default.

[jsafrane]

you're right, it should be enabled by default. @deepakkinni, can you please fix it?

[deepakkinni]

Sure, I'll work on it.