-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage, dest: clarify when TOCDigest is used #2218
storage, dest: clarify when TOCDigest is used #2218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m afraid this review is probably way too repetitive, I just wanted to get some first impression out immediately.
The primary concern is that if if we create or reuse a layer by DiffID, vs. create/reuse a layer by TOC, those two layers should have a different ID.
That doesn’t depend on whether the manifest entry has a TOC digest, but whether we used the TOC digest for creating or looking up that particular layer.
43a6803
to
f8127e9
Compare
opened a PR for c/storage to not allow two TOCs to be specified: containers/storage#1778 |
Container storage PR merged, can we move this forward? |
|
@rhatdan If you want to help move things forward, consider working on #1980 (comment) . If nothing else, that on is a showstopper and if I run out of time, I’m going to do something drastic. |
this PR is not addressing the comment #1980 (comment) but only to make clearer when the TOC digest is used. I'll take a look at that separately. |
d5eeca7
to
dc31ecd
Compare
opened a PR for c/storage for the outstanding issue with converted images (#1980 (comment)): |
dc31ecd
to
7e8a196
Compare
can we move this forward? |
7e8a196
to
73632a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most importantly, the design issue around indexing by (compressed) blob digest when individual layers might actually have been pulled/matched by TOC ( #2218 (comment) ) remains outstanding.
Resolving that will almost certainly affect many other areas with review comments.
53f4916
to
8d50e4d
Compare
Signed-off-by: Giuseppe Scrivano <[email protected]>
This update introduces an enhancement in the blob handling mechanism, specifically by separating the TOC digest from the uncompressed/compressed digest. Follow-up for: containers#1080. Signed-off-by: Giuseppe Scrivano <[email protected]>
8d50e4d
to
56e3443
Compare
@mtrmac Is this ready to go? |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I didn’t yet look at the full picture of layer finding/preparation vs. commit, submitting this ~early to point out some of the concerns.
(Also note the probably-outstanding comments from earlier reviews.) |
if it makes things easier, would you mind taking this over since you are already working in the same area in c/image? |
@giuseppe Let’s try that, I’ll take this over for now. Please note the latter part of #2218 (comment) (related to an image having “the same” chunked layer twice, I think). Either that’s an acceptable limitation for now, or it might need a change in how c/storage creates/uses/processes layers. |
This update introduces an enhancement in the blob handling mechanism, specifically by separating the TOC digest from the uncompressed/compressed digest.
Follow-up for: #1080.