provisioner: Data corruption if PVC Name is used across namespaces

[GrahamDumpleton]

As raised in comment:

• Customizing host path for dynamically provisioned PersistentVolumes #6156 (comment)

in PR:

• Customizing host path for dynamically provisioned PersistentVolumes #6156

a change was made to how directories on disk were named when created by the storage provisioner:

• https://github.com/kubernetes/minikube/pull/6156/files#diff-38e084fc1a153f78c9ebec0cc8eaaff4L60

That is, the change at line 60 from:

path := path.Join(p.pvDir, options.PVName)

to:

path := path.Join(p.pvDir, options.PVC.Name)

This meant that whereas previously the name of the directory on disk was unique, and named after the PV (which used a uuid or similar), the name of the directory on disk was changed to the PVC name.

The problem is that the PVC name isn't unique because the same PVC name can be used in different namespaces.

This change causes a data corruption problem for applications because the PVC in two different namespaces can be assigned the same directory on disk for storage when the PVC names are the same in each namespace.

This means you can't for example run two instances of same database deployment in two different namespaces as they will trample on each others database files and corrupt themselves since they use the same PVC name.

The original method of using the PV name was much safer.

If you really want directory names to be more friendly, you must use the name of the namespace in the path as well. For example, in the form:

/tmp/hostpath-provisioner/<namespace>/<pvc-name>

Without a change to this, will keep having persistent volume data corruption issues.

Steps to reproduce the issue:

1. Create two separate namespaces.

2. Create a PVC in each namespace where the PVC has the same name.

3. Create a deployment in each namespace which mounts the respective PVC in that namespace.

4. Write files to the persistent volume from each pod and will see that is the same storage. You can also look at the PV created for each and see it is the same file system path.

For example:

kind: List
apiVersion: v1
metadata:
  resourceVersion: ""
items:
- apiVersion: v1
  kind: PersistentVolume
  metadata:
    annotations:
      hostPathProvisionerIdentity: 10c2196d-bd40-48fb-b644-c3717626f4c9
      pv.kubernetes.io/provisioned-by: k8s.io/minikube-hostpath
    creationTimestamp: "2020-08-12T12:24:43Z"
    finalizers:
    - kubernetes.io/pv-protection
    name: pvc-5d58d77c-ee4d-4358-9132-9e720518b029
    resourceVersion: "18824"
    selfLink: /api/v1/persistentvolumes/pvc-5d58d77c-ee4d-4358-9132-9e720518b029
    uid: 1c1efd7e-3714-498c-8d90-ecca032b9af6
  spec:
    accessModes:
    - ReadWriteOnce
    capacity:
      storage: 1Gi
    claimRef:
      apiVersion: v1
      kind: PersistentVolumeClaim
      name: eduk8s-portal
      namespace: lab-k8s-fundamentals-ui
      resourceVersion: "18818"
      uid: 5d58d77c-ee4d-4358-9132-9e720518b029
    hostPath:
      path: /tmp/hostpath-provisioner/eduk8s-portal
      type: ""
    persistentVolumeReclaimPolicy: Delete
    storageClassName: standard
    volumeMode: Filesystem
  status:
    phase: Bound
- apiVersion: v1
  kind: PersistentVolume
  metadata:
    annotations:
      hostPathProvisionerIdentity: 10c2196d-bd40-48fb-b644-c3717626f4c9
      pv.kubernetes.io/provisioned-by: k8s.io/minikube-hostpath
    creationTimestamp: "2020-08-12T12:59:19Z"
    finalizers:
    - kubernetes.io/pv-protection
    name: pvc-d6be93f0-b437-4f11-a0a1-f97ff854e51c
    resourceVersion: "20696"
    selfLink: /api/v1/persistentvolumes/pvc-d6be93f0-b437-4f11-a0a1-f97ff854e51c
    uid: 7a33a64c-37eb-4e7c-bb8f-47b0ea750e1f
  spec:
    accessModes:
    - ReadWriteOnce
    capacity:
      storage: 1Gi
    claimRef:
      apiVersion: v1
      kind: PersistentVolumeClaim
      name: eduk8s-portal
      namespace: lab-markdown-sample-ui
      resourceVersion: "20689"
      uid: d6be93f0-b437-4f11-a0a1-f97ff854e51c
    hostPath:
      path: /tmp/hostpath-provisioner/eduk8s-portal
      type: ""
    persistentVolumeReclaimPolicy: Delete
    storageClassName: standard
    volumeMode: Filesystem
  status:
    phase: Bound
  selfLink: ""

Full output of minikube start command used, if not already included:

😄  minikube v1.12.2 on Darwin 10.15.5
✨  Using the hyperkit driver based on existing profile
👍  Starting control plane node minikube in cluster minikube
🔄  Restarting existing hyperkit VM for "minikube" ...
🎉  minikube 1.12.3 is available! Download it: https://github.com/kubernetes/minikube/releases/tag/v1.12.3
💡  To disable this notice, run: 'minikube config set WantUpdateNotification false'

🐳  Preparing Kubernetes v1.18.3 on Docker 19.03.12 ...
🔎  Verifying Kubernetes components...
🔎  Verifying registry addon...
🔎  Verifying ingress addon...
🌟  Enabled addons: default-storageclass, ingress, ingress-dns, registry, storage-provisioner
🏄  Done! kubectl is now configured to use "minikube"

[tstromberg]

Oof. That's not a good behavior for the storage provisioner. Do you mind opening up a PR so that this behaves in a way consistent with your expectations? We can then land it into the next release.

Thank you for raising this issue.

[GrahamDumpleton]

If I were to create a PR, all I would do is give an untested change which set it back to use PVName. I don't know the code base to work out how to retrieve the namespace to change it to incorporate that. Putting it back to PVName though will likely upset people who wanted a more predictable directory path though.

[afbjorklund]

Reverting to the previous code seems like the correct thing to do here, I am not sure that "predictable names" was a good feature anyway (for something that automatic allocates storage for you).

It would also be nice with some regression tests for the storage provisioner, so we can avoid things like storing PV on tmpfs or bugs with multiple namespaces (or multiple hosts) - like this one.

[GrahamDumpleton]

For the benefit of others who may stumble across this issue when investigating apparent data corruption issues, the original behaviour were unique names were used for persistent volumes was not restored. Instead, a path was used that incorporated both the name of the namespace and that of the persistent volume claim. That addressed the reported problem, but is not a full proof solution. The problem that can now arise is where the storage provisioner is too slow to reclaim the persistent volume and delete the physical directory. The consequence is that if you create a persistent volume claim of same name in a namespace, it can grab the original physical directory before it is deleted, with the storage provisioner then being prevented from deleting it since sees it as in use again. If the persistent volume had data in it, the new deployment using the persistent volume claim will see the old data, which can cause a range of application specific problems since it will work off the old data and may not reinitialise a database etc.

In short, if reusing a persistent volume claim name in the same namespace, ensure you wait a period of time for the original persistent volume claim to be released properly and the underlying persistent volume reclaimed and the physical directory deleted. In the worst case you may have to ssh into the minikube VM and watch the directory used by the storage provisioner to ensure the directory is gone before reusing the same persistent volume claim name in a namespace.