-
Notifications
You must be signed in to change notification settings - Fork 788
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
Compute diffID for mapped-layer at creating image source #2801
Conversation
If UID and GID mappings are specified, the container has a mapped-layer, whose diffID is not computed when created. Committing the image fails due to lack of diffID. This fix computes diffID at creating an image source if a layer doesn't have a diffID (UncompressedDigest). This fix also tests if a container with UID and GID mappings can be committed. Signed-off-by: Hironori Shiina <[email protected]>
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hshiina, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
LGTM, thanks!
Want a final head nod from @nalind and/or @TomSweeneyRedHat
if layer.UncompressedDigest == "" { | ||
return nil, errors.Errorf("unable to look up size of layer %q", layerID) | ||
} | ||
if !i.exporting && !i.squash && layerID != i.layerID && layer.UncompressedDigest != "" { |
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 going back and forth. I'm just wondering if we should keep the if statement still, but replace the return with a debug. I'm leaning towards no, but not fully @nalind thoughts?
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.
Well right now, we don't set this information on the mapped layer in the storage library, and this forces us to reconstruct the layer in such a way that its diffID might be different than that of the layer that it was based off of. We either do that, or we try to redirect ourselves to the corresponding not-mapped layer, and I don't think the storage library gives us quite enough information to do that.
LGTM |
/lgtm |
Thanks @hshiina |
Just noting that this also fixes a BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1758999 |
Signed-off-by: Hironori Shiina [email protected]
What type of PR is this?
/kind bug
What this PR does / why we need it:
If UID and GID mappings are specified, the container has a
mapped-layer, whose diffID is not computed when created.
Committing the image fails due to lack of diffID. This fix
computes diffID at creating an image source if a layer
doesn't have a diffID (UncompressedDigest).
This fix also tests if a container with UID and GID mappings
can be committed.
How to verify it
This PR adds a test to verify if a container with UID and GID
mappings can be committed to
idmapping
innamespaces.bats
.Which issue(s) this PR fixes:
Fixes #2800
Special notes for your reviewer:
Does this PR introduce a user-facing change?