storage, dest: clarify when TOCDigest is used#2218
storage, dest: clarify when TOCDigest is used#2218giuseppe wants to merge 2 commits intocontainers:mainfrom
Conversation
mtrmac
left a comment
There was a problem hiding this comment.
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
mtrmac
left a comment
There was a problem hiding this comment.
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 <gscrivan@redhat.com>
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 <gscrivan@redhat.com>
8d50e4d to
56e3443
Compare
|
@mtrmac Is this ready to go? |
|
LGTM |
mtrmac
left a comment
There was a problem hiding this comment.
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.