Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Volume lockname need more context #758

Closed
brunotm opened this issue Nov 21, 2016 · 2 comments
Closed

Volume lockname need more context #758

brunotm opened this issue Nov 21, 2016 · 2 comments

Comments

@brunotm
Copy link
Contributor

brunotm commented Nov 21, 2016

Currently locknames are the volume name without any other context, like path, datastore or tenant.
Which makes possible to have one or more threads unnecessarily waiting for the same lock while trying to perform ops for different volumes with the same name (eg. volumes on different tenants).

This can be easily addressed giving more context to locknames using more unique names:

  • Volume datastore path: "[datastorename] dockvols/tenant?/volname.vmk"
  • Full volume path: "/vmfs/volumes/datastorename/ dockvols/tenant?/volname.vmk"

I think the first approach better for being more compact. (we use the lockname in the thread name which shows up in logs - vmname-lockname).

Any ideas?

@govint
Copy link
Contributor

govint commented Nov 21, 2016

Option 1 is good, but we need these lock APIs to be embedded close to the logic that performs the disk op. And have API around those. Just so multiple code modules don't have to start knowing the locking scheme and use it properly around disk ops. Same goes for authorization as well.

@brunotm
Copy link
Contributor Author

brunotm commented Nov 21, 2016

@govint
The volume locks for create/delete (and even gets) needs to be acquired for the lifetime of that request, or we can easily have 2 request threads racing with the same volume if we just acquire them when touching the volume. Having the general lock in a high level fashion on the top of the thread as we have now is good and can assure thread safety for the code bellow working on that volume without the hassle of fine-grained locking. We of course have exceptions like an request that has to do ops on more the one volume (eg. cloneVMDK) and attention is needed in this case.

I think we're missing documentation about that and the threadutils package - my fault.
I'll try to address those.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants