Volumes provisioned for migrated CSI drivers have wrong provisioned-by annotation

[jsafrane]

When CSI migration is enabled and the external-provisioner provisions an in-tree PV, it files wrong pv.kubernetes.io/provisioned-by annotation. As result, when the CSI migration is disabled later, such PVs can be deleted only by the external-provisioner. They should be deleted by the in-tree one.

1. Enable CSI migration (OpenStack Cinder used below).

2. Create StorageClass for in-tree volume plugin

   provisioner: kubernetes.io/cinder
   

3. Create PVC with this StorageClass

4. Check that the external-provisioner created in-tree PV (with cinder), but with a wrong provisioned-by:

   apiVersion: v1
   kind: PersistentVolume
   metadata:
     annotations:
       pv.kubernetes.io/provisioned-by: cinder.csi.openstack.org
     name: pvc-6d2b1fb9-324c-4350-926f-639ff087eaaa
   spec:
     cinder:
       fsType: ext4
       volumeID: 49921926-4dd7-4d17-b32d-742cff332a2f
   

5. Disable the CSI migration.

6. Delete the PVC.

Actual result: The PV is deleted by the CSI driver.
Expected result: The PV is deleted by in-tree volume plugin. The CSI driver may not be installed at that time.

[Jiawei0227]

/assign

[Jiawei0227]

Some finding just list here for reference:

1. Original PD case:

• PVC annotation:
  • pv.kubernetes.io/bind-completed: yes
  • pv.kubernetes.io/bound-by-controller: yes
  • volume.beta.kubernetes.io/storage-provisioner: kubernetes.io/gce-pd
• PV annotation:
  • kubernetes.io/createdby: gce-pd-dynamic-provisioner
  • pv.kubernetes.io/bound-by-controller: "yes"
  • pv.kubernetes.io/provisioned-by: kubernetes.io/gce-pd

After migration, PV and PVC will both add new annotation:

• pv.kubernetes.io/migrated-to: pd.csi.storage.gke.io

After rollback, this annotation is removed from both PV and PVC

2. New PD after migration is turned on case:

• PVC annotation:
  • pv.kubernetes.io/bind-completed: yes
  • pv.kubernetes.io/bound-by-controller: yes
  • volume.beta.kubernetes.io/storage-provisioner: pd.csi.storage.gke.io
• PV annotation:
  • pv.kubernetes.io/provisioned-by: pd.csi.storage.gke.io

3. Directly create CSI PD:

• PVC annotation:
  • pv.kubernetes.io/bind-completed: yes
  • pv.kubernetes.io/bound-by-controller: yes
  • volume.beta.kubernetes.io/storage-provisioner: pd.csi.storage.gke.io
  • volume.kubernetes.io/selected-node: kubernetes-minion-group-5l72
• PV annotation
  • pv.kubernetes.io/provisioned-by: pd.csi.storage.gke.io

[Jiawei0227]

So if we do not fix the bug mentioned in this issue, the problem is when user rollback the cluster to non-csi migration enabled state, and delete the PVC that is created when using in-tree storageclass when CSI migration status is enabled will result in a state that the PVC get deleted successfully. But the PV cannot be deleted and will result in a release status because of the logic here: https://github.com/kubernetes/kubernetes/blob/0df8c69731a90b2dfbe40e8cda2406a00663e9b0/pkg/controller/volume/persistentvolume/pv_controller.go#L637

[Jiawei0227]

/cc @msau42

[Jiawei0227]

So in this issue is basically saying the case 2 I mentioned above is wrong, specifically the pv annotation should not be set to pv.kubernetes.io/provisioned-by: pd.csi.storage.gke.io

[Jiawei0227]

From discussion with @msau42 offline, we may have the following options:

1. Change case 2's PV to pv.kubernetes.io/provisioned-by: intree and add a pv.kubernetes.io/migrated-to: csi. And keep the PVC as it is.

The downside is when we rollback, there is no way to tell whether the PV is actually get provisioned by intree/CSI except by looking at the PVC object storage-provisioner.
Also, this somehow makes the provisioned-by field loss its original meaning

2. To supply option 1, we could introduce a new annotation for PV that actually get created by CSI but is represented as in-tree... Although it might seem a little messy and confusing because we already have the provisonBy field. Some like:

PV annotation:

• pv.kubernetes.io/provisioned-by: kubernetes.io/gce-pd
• pv.kubernetes.io/migrated-to: pd.csi.storage.gke.io
• pv.kubernetes.io/csi-migration: true

Note that the pv.kubernetes.io/migrated-to: pd.csi.storage.gke.io will be removed after CSIMigration rollback, so the pv.kubernetes.io/csi-migration: true will just be a signal for us to distinguish if the volume is actually created by intree or csi.

3. Leave it as it is. Not fix this issue because it is intend behavior. Since the volume is provisioned by CSI driver, it should also be deleted by CSI driver so we dont want to actually fix it?

[jsafrane]

Note that the pv.kubernetes.io/migrated-to: pd.csi.storage.gke.io will be removed after CSIMigration rollback, so the pv.kubernetes.io/csi-migration: true will just be a signal for us to distinguish if the volume is actually created by intree or csi.

Do we need such a signal "to distinguish if the volume is actually created by intree or csi"? Who uses the annotation?

I am wondering if we can just use StorageClass.Provisioner as value of pv.kubernetes.io/provisioned-by: when creating PV around these parts:
https://github.com/kubernetes-sigs/sig-storage-lib-external-provisioner/blob/5fd51cfade8c066f42fc33300c3fe700b0f67cae/controller/controller.go#L1449

Or translate the driver name to in-tree there when pvc.kubernetes.io/migrated-to is present in the PVC, if we want it super correctly.

PV controller will then add/clear pv.kubernetes.io/migrated-to as needed.

[Jiawei0227]

I am wondering if we can just use StorageClass.Provisioner as value of pv.kubernetes.io/provisioned-by: when creating PV around these parts:

I tried this approach. There is one problem with it which is the external-attacher finalizer. When the PVC is attached to a node, the external attacher will add a finalizer: "external-attacher/pd-csi-storage-gke-io" to the PV object. And it only gets removed by external-attacher when the PV is in deleted status.

Related code: https://github.com/kubernetes-csi/external-attacher/blob/3d49e838d8ecc491b0a7ce423f32b7e43aaa51af/pkg/controller/csi_handler.go#L635

So the problem I am facing is that once I rollback to non-csi migration cluster, and remove the pd csi deployment, there is no external-attacher to remove the finalizer for this PV so this PV is not able to be deleted...

So one solution I can think of is to remove this finalizer when we remove the migrated-to field here https://github.com/kubernetes/kubernetes/blob/2195718940282cc4e1b553760c1e9e020b9fb992/pkg/controller/volume/persistentvolume/pv_controller_base.go#L401. What do you think?

[Jiawei0227]

I am checking how this is designed by David in the first place, it seems the migrated-to annotation is designed only for PV/PVC that is originally created by in-tree. For CSI created PV/PVC, it should only be handled by CSI even for rollback scenario . So I think do we think this design is not desirable?

I still think we should consider keep the current behavior which let the csi-provisioner to delete this PV since it is the one that creates it. The sequence does not sounds reasonable for a user to turn on migration, create a PVC use the in-tree plugin and then turn off migration and delete the PVC and hope the in-tree to kick in and delete it.

If somehow the migration does not work as expected, user should delete the PVC before turn off migration. Otherwise why would they delete it after turn migration off?

[jsafrane]

Good point about the external-attacher. We should fix either both the attacher + the provisioner or require users to run with CSI driver installed even after downgrade. Exploring the second option, why the external-provision creates in-tree PVs then? We could save us a lot of headache and provision CSI PVs, if we expect that CSI driver is installed even after CSI migration is disabled.

[jsafrane]

So one solution I can think of is to remove this finalizer when we remove the migrated-to field here https://github.com/kubernetes/kubernetes/blob/2195718940282cc4e1b553760c1e9e020b9fb992/pkg/controller/volume/persistentvolume/pv_controller_base.go#L401. What do you think?

I would prefer if a component that added a finalizer would also remove it. The component may be changed later. I.e. the external-attacher could remove the finalizer from PVs that are not CSI && don't have the migrated-to annotation - that's simple. If we can assume that the attacher is running for a while after the CSI migration is disabled to clean them up.

[msau42]

If the pv still had the attacher finalizer on it, doe that mean we didn't drain the node first?

…

On Wed, Mar 10, 2021, 00:50 Jan Šafránek ***@***.***> wrote: So one solution I can think of is to remove this finalizer when we remove the migrated-to field here https://github.com/kubernetes/kubernetes/blob/2195718940282cc4e1b553760c1e9e020b9fb992/pkg/controller/volume/persistentvolume/pv_controller_base.go#L401. What do you think? I would prefer if a component that added a finalizer would also remove it. The component may be changed later. I.e. the external-attacher could remove the finalizer from PVs that are not CSI && don't have the migrated-to annotation - that's simple. If we can assume that the attacher is running for a while after the CSI migration is disabled to clean them up. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#576 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF2QYPOEMNQKRVZ6ZWY7RLTTC4XERANCNFSM4YZJ2IKQ> .

[jsafrane]

Today, the finalizer is removed after PV gets DeletionTimestamp. That guarantees that new Pods don't start at the time the attacher removes the finalizer. So once a PV is used by external-attacher, the finalizer is there for whole lifetime of the PV.

What I am suggesting is to remove the finalizer when migrated-to annotation is removed. Because at that moment CSI migration is disabled in A/D controller, nodes were drained and nothing can't be attached via CSI at this point (in theory 😄 ).

[Jiawei0227]

That guarantees that new Pods don't start at the time the attacher removes the finalizer. So once a PV is used by external-attacher, the finalizer is there for whole lifetime of the PV.

Yeah, I was quite surprised when I found this. Can you elaborate why do we keep the finalizer for the whole time?