PV reused for multiple users

[hwuethrich]

First of all, thanks for this great project! :)

We noticed that im some case, the PV userdata configmap ends up containing the same PV names for multiple users. This results in multiple PVCs being created for the same PV when two users with the same PV in the configmap try to start a session at the same time.

Without digging too deep in the code, this might be a result of how Kubernetes handles PV: It looks like Kubernetes is reusing an existing (currenty unused) PVs whenever a new user is starting a session for the first time.

I already created a dedicated storage class with reclaimPolicy: Retain. Isn't this behaviour expected, as the PVs are released after each session and Kubernetes will just reuse them for any new PVC?

We are using Kubernetes v1.21.4 with the vsphere-csi plugin and the following config (deployed using the helm chart):

...
userdataSpec:
  accessModes: ["ReadWriteOnce"]
  storageClassName: vsphere-block-retain
  resources:
    requests:
      storage: 20Gi
...

Thanks for your help!

Update: I forgot to mention we are using OIDC for user auth

[tinyzimmer]

It was only a matter of time until an issue like this came up with the user data stuff 😛 . Surprised it took this long.

There are a ton of variables at play so it'll be difficult to pin down the exact issue.

Without digging too deep in the code, this might be a result of how Kubernetes handles PV: It looks like Kubernetes is reusing an existing (currenty unused) PVs whenever a new user is starting a session for the first time.

This seems fun. If I remember the current logic correctly, when a user needs a new PV, A PVC is created to get the storage class to allocate one, and then whatever it allocates is used for future sessions (and stored in that ConfigMap you mention). I don't recall putting in a check to see if it's a dupe, because at the time I think I just sort of trusted the CSI or whatever to know that I want a new PV.

It almost sounds like the storage driver is saying, "well here I have this PV already setup that no one is using currently, so just take this". But that feels like it could cause loads of downstream problems not just for kvdi. The volume is never empty (unless you've tweaked the desktop init behavior) so I see no reason why a storage class would think there isn't any data there and it is safe to reuse.

Unfortunately I don't have access to a vsphere environment to try to replicate this exact issue. I noticed the thumbs up from someone else and I'd be curious if they are also using vsphere. If you are up for it, we may have to work this problem together 😄 .

[hwuethrich]

Thanks for your quick reponse! 👍🏻

Also, why is it necessary to delete the PVC after the session is terminated? Couldn't the PVC be left alone and then reused for the next session? This way, you wouldn't have to maintain a mapping between users and PVs and volumes would be retained automatically for the lifetime of the PVC.

[tinyzimmer]

It's a very good question. I'll need to find some time to go through it all once again to remind myself of some of the decisions made. Two thoughts that come to mind immediately are:

• Keep things tidy. Probably not as big an issue anymore but back in the day there were lower resource count limits.
• PVCs are namespace scoped, while the PV isn't. It's possible a user will launch their sessions across different namespaces. And if a PVC is already holding the PV in a different namespace, it will fail.

If this is part of the issue, it would be relatively easy to create a configuration to disable that behavior. But in doing so the user would have to know they are constrained to a single namespace.

[tinyzimmer]

The biggest question I'm asking myself right now is why did I bother with PVCs at all. Knowing myself, it was probably laziness (i.e. much easier to get a PV created for me via the PVC API then to do so directly). I'll have a better idea when I dig into it more.

[hwuethrich]

• PVCs are namespace scoped, while the PV isn't. It's possible a user will launch their sessions across different namespaces. And if a PVC is already holding the PV in a different namespace, it will fail.

This is of course a valid point. In our case this wouldn't be a problem as all sessions are created in the same namespace.

If this is part of the issue, it would be relatively easy to create a configuration to disable that behavior. But in doing so the user would have to know they are constrained to a single namespace.

I solution like this would be very welcome! As mentioned above, we only use a single namespace. I was trying to figure out who to blame for reusing the existing PV (and if it is valid to do so) but I couldn't find anything in the logs so far.

The biggest question I'm asking myself right now is why did I bother with PVCs at all.

Yes, you could probably also create the PV manually before creating the PVC and binding it to the PV, but I think as long as you don't need multiple namespaces, leaving the PVC intact is easier.

Another use case for this: I wanted to resize the volume (which only works in vsphere if it is not mounted by a pod) but because the PVC is deleted, I had to manually create a PVC to resize the volume.

[tinyzimmer]

I'll take a stab at it sometime this weekend. Maybe see if there are any other low hanging issues I can take care of while I'm in there.

[tinyzimmer]

Feel free to give https://github.com/kvdi/kvdi/releases/tag/v0.3.6 a go. And make sure you point at the new helm repo in the README.

TLDR: There is now a retainPVCs: true you can set inside the userdataSpec. So in the case of your example above

userdataSpec:
  retainPVCs: true
  accessModes: ["ReadWriteOnce"]
  storageClassName: vsphere-block-retain
  resources:
    requests:
      storage: 20Gi

I guess I'll add that using this field, knowing that your users are all in one namespace, the special StorageClass for retaining is really not necessary anymore. Nothing should try to delete the volumes in the first place. Though you may still want it just to keep something else from messing it up.

[hwuethrich]

@tinyzimmer Awesome! Thank you very much for your quick response! FYI, I noticed only one minor issue (which might not be important): The label desktopName on the PVC is not updated when I start a new session. The label value still contains the name of the previous session.

[tinyzimmer]

Ah good catch. I should probably do something about that, but you are right that it's benign. Some of the labels are used internally as selectors, but I'm pretty sure that's an example of one that's just there for debugging purposes.