Skip to content

bugfix: avoid to re-calculate blob state for schema1#2595

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
fuweid:bugfix_avoid_re_calculate_blob_state
Sep 6, 2018
Merged

bugfix: avoid to re-calculate blob state for schema1#2595
crosbymichael merged 1 commit intocontainerd:masterfrom
fuweid:bugfix_avoid_re_calculate_blob_state

Conversation

@fuweid
Copy link
Copy Markdown
Member

@fuweid fuweid commented Aug 30, 2018

Use containerd.io/uncompressed label to avoid to re-calculate blob
diffID.

Signed-off-by: Wei Fu fuweid89@gmail.com

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 30, 2018

Codecov Report

Merging #2595 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2595   +/-   ##
=======================================
  Coverage   44.03%   44.03%           
=======================================
  Files          94       94           
  Lines       10235    10235           
=======================================
  Hits         4507     4507           
  Misses       5008     5008           
  Partials      720      720
Flag Coverage Δ
#linux 47.5% <ø> (+0.02%) ⬆️
#windows 40.66% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96986c0...9db21de. Read the comment docs.

@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Sep 2, 2018

ping @AkihiroSuda @dmcgowan and @stevvooe

Please take a look at this, thanks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-use "containerd.io/uncompressed" here instead of using a structured value. Maybe we can have containerd.io/legacy.schema1.empty to determine if a tar layer is empty. In that past we have hardcoded the uncompressed empty tar digest but not sure if that is reliable enough.

For media type, could just use this logic... if uncompressed == blob images.MediaTypeDockerSchema2Layer else images.MediaTypeDockerSchema2LayerGzip. Nothing else is supported for schema1, no need to store it

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually after reading back through this code, I don't believe empty is relevant at this part of the code. The empty attribute is determined by manifest, which is always loaded first. If the blob was empty, fetch will never be attempted and this code would not be called. So this code will only be called on non-empty layers. For empty layers, they are only included as part of the history and not part of the hash id calculation for the layer, so they never have a relevant diffid.

https://github.com/containerd/containerd/blob/master/remotes/docker/schema1/converter.go#L95

@fuweid fuweid force-pushed the bugfix_avoid_re_calculate_blob_state branch from a6e0c48 to fa15652 Compare September 5, 2018 10:56
@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Sep 5, 2018

ping @dmcgowan updated it and please take a look.

Use containerd.io/uncompressed label to avoid to re-calculate blob
diffID.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid force-pushed the bugfix_avoid_re_calculate_blob_state branch from fa15652 to 9db21de Compare September 6, 2018 01:21
@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Sep 6, 2018

LGTM

1 similar comment
@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@crosbymichael crosbymichael merged commit 901b2ea into containerd:master Sep 6, 2018
@fuweid fuweid deleted the bugfix_avoid_re_calculate_blob_state branch September 6, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants