GC: add support for GC policies #338
Conversation
|
LGTM |
oci/casext/gc.go
Outdated
| @@ -25,6 +25,9 @@ import ( | |||
| "golang.org/x/net/context" | |||
| ) | |||
|
|
|||
| // GCPolicy is a policy function that returns if a blob can be GC'ed | |||
| type GCPolicy func(desc ispec.Descriptor) (bool, error) | |||
There was a problem hiding this comment.
In line with switching to Paths, this should be a DescriptorPath.
There was a problem hiding this comment.
Want to hide umoci internals from the policy func writers. So just pass in the standard ispec.Descriptor.
There was a problem hiding this comment.
DescriptorPath isn't really an umoci internal, it's fairly publicly used if you want to do ResolveReference (so basically every umoci API user will see DescriptorPath at some point). For a bit of background -- the reason why DescriptorPath exists is that it allows programs to correctly handle arbitrary nesting of descriptors.
However I just re-read the original description of the PR and realised that passing descriptors makes no sense (sorry, it looks like @tych0 and I overlooked this) -- you are only calling policy(desc) if there is no descriptor in the black set (in other words, on blobs that have no descriptor pointing to them), so the descriptor will always be an empty struct. The new tests didn't catch this, so we'll need to update the tests so they can exercise this code properly.
rchincha
force-pushed
the
gc
branch
3 times, most recently
from
d373e90 to
925e169
Compare
There was a problem hiding this comment.
It looks like @tych0 and I both overlooked this -- the original change (using Digest) was actually correct. I misread the original purpose of the PR -- you want to be able to not garbage collect blobs which don't have any descriptors pointing to them. So therefore the policy function won't be able to take a descriptor, because in order for the policy to be checked there must be no descriptor for the blob.
This is unfortunate, since it means the policy function doesn't know the type of the blob, but that is the nature of the problem. I'm a little worried that the new tests didn't catch that the passed ispec.Descriptor was completely empty (even the digest was empty) so those tests should be updated to correctly exercise that code.
Ugh, yeah. That is unfortunate. Makes total sense though. |
rchincha
force-pushed
the
gc
branch
2 times, most recently
from
c05011e to
3382efc
Compare
|
Updated the PR with suggested changes. |
|
https://travis-ci.org/github/opencontainers/umoci/jobs/704404847 |
|
@rchincha That's not the cause of the failure (it's some weird warning we get when running under Travis that I haven't had a chance to debug). |
For our dist-spec implementation [1][2], we have a use case where we use umoci's GC code to clean up orphaned blobs. The issue is that as per dist-spec layer upload and manifest updates are two different API calls and typically in that order, which means layers begin life as orphans. Adding the proposed patch allows us to: a) mitigate the above issue b) have a generic policy framework for future expansion [1] https://github.com/opencontainers/distribution-spec [2] https://github.com/anuvu/zot Signed-off-by: Ramkumar Chinchani <[email protected]>
|
LGTM, thanks and sorry for all the back-and-forth around |
|
LGTM. |
For our dist-spec implementation [1][2],
we have a use case where we use umoci's GC code to clean up orphaned
blobs. The issue is that as per dist-spec layer upload and manifest
updates are two different API calls and typically in that order, which
means layers begin life as orphans.
Adding the proposed patch allows us to:
a) mitigate the above issue
b) have a generic policy framework for future expansion
[1] https://github.com/opencontainers/distribution-spec
[2] https://github.com/anuvu/zot