-
Notifications
You must be signed in to change notification settings - Fork 597
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
reconstruct the content for newly committed image if compressed layers are missing #2297
base: main
Are you sure you want to change the base?
reconstruct the content for newly committed image if compressed layers are missing #2297
Conversation
1aa96b4
to
2a543a5
Compare
Is this still WIP? |
yes, this pr includes 2 commits, current commit resumes container once upper layer finish committing to reduce the unavailable time for container |
Signed-off-by: Cardy.Tang <[email protected]>
2a543a5
to
02fe347
Compare
if content is missing, compress and reconstruct it from unpacked data again Signed-off-by: Cardy.Tang <[email protected]>
kindly ping @AkihiroSuda |
if !errdefs.IsNotFound(err) { | ||
return fmt.Errorf("get layer %v failed: %w", layer.Digest.String(), err) | ||
} | ||
snapshotID := identity.ChainID(config.RootFS.DiffIDs[:i+1]).String() |
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.
Can we get the snapshot ID from the metadata store (e.g. containers.Container
) rather than manually computing it here?
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.
yea that works too. My concern is that since we are reconstructing layers (digest recorded in manifest is not found), we better rely on manifest/config which is image relevant instead of snapshotter plugin..
return fmt.Errorf("failed to compress layer for %v: %w", layer.Digest.String(), err) | ||
} | ||
if diffID != config.RootFS.DiffIDs[i] { | ||
logrus.Infof("newly created Diff id %v does not match with diff id in config %v: %v", diffID, config.RootFS.DiffIDs[i], err) |
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.
This should be a warning. Also, it should be warned that the contents of the image has been changed by this diff computation.
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.
This func does not change content of the original image, it reconstructs the content for new image if original content is missing. So nothing is changed for original image consumers.
Could you add tests? |
Sure thing! thx |
implements #2295