reconstruct the content for newly committed image if compressed layers are missing #2297
Conversation
cardyok
changed the title
cardyok
changed the title
cardyok
force-pushed
the
reconstruct_content_for_missing_content
branch
from
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 <zuniorone@gmail.com>
cardyok
force-pushed
the
reconstruct_content_for_missing_content
branch
from
2a543a5
to
02fe347
Compare
if content is missing, compress and reconstruct it from unpacked data again Signed-off-by: Cardy.Tang <zuniorone@gmail.com>
cardyok
changed the title
|
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.
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.
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.
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.
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