c8d/push: Push lazy blobs with distribution source labels#74
Conversation
9142d60 to
81bd076
Compare
9541dc3 to
6282eaa
Compare
|
Undrafting this as it allows the build+push case to work. |
|
Added some comments and cleaned up the source label handling a bit. |
thaJeztah
left a comment
There was a problem hiding this comment.
ah, have a call, so need to continue later; submitted some nits (no blockers so far)
| // Do a small peek of the content to check if content is definitely not a json. | ||
| // Returns true when content is definitely not a json. | ||
| // If it returns false, then the content may still not be a json. |
There was a problem hiding this comment.
| // Do a small peek of the content to check if content is definitely not a json. | |
| // Returns true when content is definitely not a json. | |
| // If it returns false, then the content may still not be a json. | |
| // peekNotJson does a small peek of the content to check if content is definitely not JSON. | |
| // It returns true if content is definitely not JSON, or false if it was unable to detect if it's | |
| // JSON or not. |
The error doesn't not appear to be described (on how it should be used in combination with the bool)
There was a problem hiding this comment.
Thanks. I didn't bother much describing the error case, as it's not really something that should happen under normal circumstances. This is called only when traversing the content store, so we expect the content to be there.
The only case of error here would be if the content is silently deleted in background between the beginning of the Walk and running handler for this particular content.
BTW, I wonder if there is some way to "lock" the store, so we can be 100% sure that this won't happen.
There was a problem hiding this comment.
I think you can use a leased context maybe?
There was a problem hiding this comment.
But how do I put a lease on everything in the Content store? :P
And if I do - will lease be enough to pause something like ctr content remove <hash> until the Walk is finished?
There was a problem hiding this comment.
Yeah nah it doesn't work, the lease you add to your context is added to objects you create during that context.
thaJeztah
left a comment
There was a problem hiding this comment.
Left some comments, but mostly looks sane 😄
This makes it possible to push multi-platform images which contents are missing from the local content store, but can be fetched from a source repository. Co-authored-by: Djordje Lukic <djordje.lukic@docker.com> Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
0720a48 to
cf0b3d6
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Let's make sure we have a tracking ticket to improve and upstream a more official approach, and discuss with the containerd maintainers.
|
When upstreaming:
combine with one or more of; |
This makes it possible to push multi-platform images if some contents are missing from the local content store, but can be fetched from a source repository.
The trick here is wrapping the ContentStore to return a fake
content.Infowith acontainerd.io/distribution.source.label which specifies the source registry and repository for the blob. It's then used by the repository to cross-repo mount the blobs.Caveats:
buildx purge -aor restarting the daemon)Test:
Test result with a fresh repo