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

store: add locking around cas diskv #460

Closed
sgotti opened this issue Feb 3, 2015 · 6 comments
Closed

store: add locking around cas diskv #460

sgotti opened this issue Feb 3, 2015 · 6 comments
Labels
component/store help wanted
Milestone
vfuture

Comments

@sgotti
Copy link
Contributor

sgotti commented Feb 3, 2015

As a reminder (I'll post patches when the various #392, #393 lands) there's the need for some locking around cas diskv stores. Now they are needed to avoid multiple processes writing the same image and image manifest to the store and also when (in future) a delete functions will be added to the store.

This is my proposal:

  • Do not lock on basic diskv funcs like Read, Import etc... as there's the need to write on multiple diskv stores inside the same lock. For example in WriteACI (after cas: Add Imagemanifeststore, save ImageManifest on WriteACI and ad GetImageManifest function. #392) there's the need to write the ACI and the image manifest inside an unique lock.
  • Fine grained locking on the single key of the diskv store with single writer/multiple reader pattern.
  • To avoid deadlocks do not lock the diskv key inside the db locks or vice versa. Peraphs choose a lock order (always first lock the diskv key before the db)
  • As image and imagemanifest stores are tied together the image and imagemanifest for the same key should use an unique lock.

Some places where locking is needed:

Functions like GetACI (#395) can call multiple times the DB functions and the diskv functions (like GetImageManifest) so if we want to avoid any problem we should take a global lock. Not doing this means that during GetACI loop an image or image manifest could be added or removed. I don't think this is a problem as this is checked and the error is handled.
@sgotti sgotti mentioned this issue Feb 3, 2015
Closed
@sgotti
Copy link
Contributor Author

sgotti commented Feb 5, 2015

@jonboulle I'm willing to work on this after #362, #392, #393. Honestly I already have some patches but, opening a PR will mean a lot of rebasing between this and the other PRs. Just let me know what you'll prefer. Tnx!

@sgotti sgotti mentioned this issue Feb 27, 2015
Closed
3 tasks
sgotti added a commit to sgotti/rkt that referenced this issue Mar 4, 2015
@sgotti
cas: add locking around diskv functions.
d42860b 
This patch adds locking like explained in rkt#460.
To avoid deadlocks the diskv locks shouldn't be taken inside the db access (if
needed).
sgotti added a commit to sgotti/rkt that referenced this issue Mar 4, 2015
@sgotti
cas: add locking around diskv functions.
50588e8 
This patch adds diskv stores locking like explained in rkt#460.
To avoid deadlocks the diskv locks shouldn't be taken inside the db access
code.
@sgotti sgotti mentioned this issue Mar 4, 2015
Merged
sgotti added a commit to sgotti/rkt that referenced this issue Mar 5, 2015
@sgotti
cas: add locking around diskv functions.
5df0893 
This patch adds diskv stores locking like explained in rkt#460.
To avoid deadlocks the diskv locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 5, 2015
@sgotti
cas: add locking around diskv functions.
3c6c348 
This patch adds diskv stores locking like explained in rkt#460.
To avoid deadlocks the diskv locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 6, 2015
@sgotti
cas: add image locking.
4618dea 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 6, 2015
@sgotti
cas: add image locking.
f282de0 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 6, 2015
@sgotti
cas: add image locking.
fd5d536 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 9, 2015
@sgotti
cas: add image locking.
f564308 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 10, 2015
@sgotti
cas: add image locking.
0bf7a3a 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 10, 2015
@sgotti
cas: add image locking.
ecad281 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 11, 2015
@sgotti
cas: add image locking.
18ce34e 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 11, 2015
@sgotti
cas: add image locking.
999c776 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 17, 2015
@sgotti
cas: add image locking.
4c34eb8 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 23, 2015
@sgotti
cas: add image locking.
1be727a 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 23, 2015
@sgotti
cas: add image locking.
1b0fd5a 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 23, 2015
@sgotti
cas: add image locking.
2a9335b 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 24, 2015
@sgotti
cas: add image locking.
8bbbde4 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 25, 2015
@sgotti
cas: add image locking.
056bd9c 
This patch adds image locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
In future also the tree store will be part of an image.

This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 25, 2015
@sgotti
cas: add image locking.
286935e 
This patch adds image and tree store locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.

The tree store is kept separate from the image for various reasons:

* It can be composed by multiple images if the upper image has dependencies
* It's rendered and removed separately from image.

For this reason also the tree store locks are different from the image locks.
The tree store locks should be taken before the image locks.
sgotti added a commit to sgotti/rkt that referenced this issue Mar 26, 2015
@sgotti
cas: add image locking.
85412e0 
This patch adds image and tree store locking like explained in rkt#460.
An image is composed by the ACI file in the blob store and it's imageManifest
in the imageManifest store.
This means that a lock on an image covers multiple elements.
For this reason the keyLock functions are used to take a lock.

To avoid deadlocks the image locks shouldn't be taken inside the db access
code.

The tree store is kept separate from the image for various reasons:

* It can be composed by multiple images if the upper image has dependencies
* It's rendered and removed separately from image.

For this reason also the tree store locks are different from the image locks.
The tree store locks should be taken before the image locks.
@sgotti
Copy link
Contributor Author

sgotti commented Apr 2, 2015

The proposed implementation (and also the tree store locking) has landed in #571.
There're additional patches that uses the locking like #603 that contains a possible solution for #550 (comment)

Testing all the possible locking combinations between multiple processes is quite difficult but should be useful to find a way to do reproducible tests and maybe add it to a test suite like #600.

@jonboulle jonboulle removed the core label Apr 29, 2015
@jonboulle jonboulle changed the title Add locking around cas diskv stores store: add locking around cas diskv May 1, 2015
@iaguis
Copy link
Member

iaguis commented Jan 18, 2016

I believe this is implemented in #571 and #603.

Is that so? @sgotti @jonboulle

@iaguis iaguis added this to the vfuture milestone Jan 18, 2016
@jonboulle
Copy link
Contributor

I think so but would be good to get an ack from sgotti.

@sgotti
Copy link
Contributor Author

sgotti commented Jan 18, 2016

@jonboulle @iaguis Correct. Sorry but I forgot to close this!

@iaguis
Copy link
Member

iaguis commented Jan 18, 2016

Thanks! Closing.

@iaguis iaguis closed this as completed Jan 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
component/store help wanted
Projects
None yet
Milestone
vfuture
Development

No branches or pull requests
3 participants
@iaguis @sgotti @jonboulle