metadata: image removal triggers GC#1960
Conversation
dmcgowan
changed the title
dmcgowan
changed the title
dmcgowan
force-pushed
the
images-removal-dirty
branch
from
62a473f to
4a8c983
Compare
Codecov Report
@@ Coverage Diff @@
## master #1960 +/- ##
==========================================
+ Coverage 45.7% 45.73% +0.02%
==========================================
Files 94 94
Lines 9307 9341 +34
==========================================
+ Hits 4254 4272 +18
- Misses 4350 4364 +14
- Partials 703 705 +2
Continue to review full report at Codecov.
|
|
LGTM |
There was a problem hiding this comment.
This if is now unnecessary:
return createBucketIfNotExists(tx, imagesBucketPath(namespace)...)There was a problem hiding this comment.
Since this is a delete, shouldn't this use getImageBukcet() and ignore any "ErrBucketNotFound" errors? (or maybe not ignore them, not sure if delete is supposed to be idempotent at this level).
I believe right now it would create the the bucket if it doesn't exist, which is a bit unexpected.
There was a problem hiding this comment.
I agree, that is not correct behavior. The transaction ends up getting rolled back anyway but it is unnecessary to create.
There was a problem hiding this comment.
nit: these three lines are identical in the container store so could be moved into a DB.markDirtyCSWithLock() or something like that
There was a problem hiding this comment.
I am going to keep it like this for now, it is only the second use of this particular pattern and adding a function does not necessarily improve readability.
dmcgowan
force-pushed
the
images-removal-dirty
branch
from
4a8c983 to
cee77c5
Compare
Directly get and check whether a bucket is empty. This prevents unnecessarily loading all records of the buckets into memory just to check existence. Also added checks for content and snapshots. Signed-off-by: Derek McGowan <[email protected]>
The boltdb image store now manages its own transactions when one is not provided, but allows the caller to pass in a transaction through the context. This makes the image store more similar to the content and snapshot stores. Additionally, use the reference to the metadata database to mark the content store as dirty after an image has been deleted. The deletion of an image means a reference to a piece of content is gone and therefore garbage collection should be run to check if any resources can be cleaned up as a result. Signed-off-by: Derek McGowan <[email protected]>
dmcgowan
force-pushed
the
images-removal-dirty
branch
from
cee77c5 to
89fa154
Compare
|
LGTM Merging this now so we can test it for 1.0.2. Please backport since we've released 1.0.1. |
5 participants
Currently an image removal with the sync flag will not by itself cause a GC to run the same way removal of content or snapshots would. This fixes this by refactoring the image store to have access to the metadata store to mark the content dirty flag.
To do this...
The boltdb image store now manages its own transactions when one is not provided, but allows the caller to pass in a transaction through the context. This makes the image store more similar to the content and snapshot stores. Additionally, use the reference to the metadata database to mark the content store as dirty after an image has been deleted. The deletion of an image means a reference to a piece of content is gone and therefore garbage collection should be run to check if any resources can be cleaned up as a result.
The namespace empty check logic also needed to be updated since it was relying on using a transaction to initialize the container and image stores. Now the transaction is just directly used to check whether the buckets are empty, saving unnecessarily loading all the content into memory just to check whether or not a key exists under the buckets.
Credit to @tonistiigi for finding image removal not triggering gc without sync