Skip to content

bugfix: cache empty layer for docker schema1 image#2691

Merged
crosbymichael merged 2 commits intocontainerd:masterfrom
fuweid:bugfix_cache_empty_label
Oct 15, 2018
Merged

bugfix: cache empty layer for docker schema1 image#2691
crosbymichael merged 2 commits intocontainerd:masterfrom
fuweid:bugfix_cache_empty_label

Conversation

@fuweid
Copy link
Copy Markdown
Member

@fuweid fuweid commented Sep 28, 2018

containerd should cache empty label for docker schema1 image.

if not, the original empty layer will be non-empty layer and the image
config will be changed too. in this case, the image ID will be changed.

check the blob empty label to avoid changing image ID when repull docker
schema1 image.

Signed-off-by: Wei Fu [email protected]

#2678

containerd should cache empty label for docker schema1 image.

if not, the original empty layer will be non-empty layer and the image
config will be changed too. in this case, the image ID will be changed.

check the blob empty label to avoid changing image ID when repull docker
schema1 image.

Signed-off-by: Wei Fu <[email protected]>
@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Sep 28, 2018

I use the following script to test it.

#!/usr/bin/env bash

set -euo pipefail

get_image_id() {
  local image_name

  image_name=$1
  ctr image list | grep "${image_name}" | awk '{ print $3 }'
}

pull_image() { 
  local image_name image_id1 image_id2

  image_name=$1

  set +e; ctr image rm "${image_name}"; set -e

  # first round
  ctr --debug image pull "${image_name}"
  image_id1="$(get_image_id ${image_name})"

  # second round to make sure the image id should not be changed
  ctr image pull "${image_name}"
  image_id2="$(get_image_id ${image_name})"

  [[ "'${image_id1}'" != "'${image_id2}'" ]] \
    && echo "image id changed after repull image" \
    && exit 1

  # last round...
  ctr image pull "${image_name}"
  image_id2="$(get_image_id ${image_name})"

  [[ "'${image_id1}'" != "'${image_id2}'" ]] \
    && echo "image id changed after repull image" \
    && exit 1

  ctr image rm --sync "${image_name}"
}

main() {
  local image_list

  image_list=(
    gcr.io/google_containers/pause:3.0
    gcr.io/google_containers/busybox:1.24
    gcr.io/google_containers/nginx-slim:0.7
    k8s.gcr.io/busybox:1.24
  )

  for image in "${image_list[@]}"; do
    pull_image "${image}"
  done

}

main

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 28, 2018

Codecov Report

Merging #2691 into master will decrease coverage by 1.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2691      +/-   ##
==========================================
- Coverage   45.09%   43.54%   -1.56%     
==========================================
  Files          92      101       +9     
  Lines       10132    10734     +602     
==========================================
+ Hits         4569     4674     +105     
- Misses       4842     5326     +484     
- Partials      721      734      +13
Flag Coverage Δ
#linux 47.22% <ø> (-1.63%) ⬇️
#windows 40.67% <ø> (-1.21%) ⬇️
Impacted Files Coverage Δ
remotes/docker/resolver.go 58.36% <0%> (-2.97%) ⬇️
platforms/cpuinfo.go 4.61% <0%> (-1.05%) ⬇️
metadata/content.go 41.71% <0%> (-0.22%) ⬇️
oci/spec_opts.go 20.77% <0%> (-0.12%) ⬇️
runtime/v2/shim/util_windows.go 0% <0%> (ø)
runtime/v2/shim/util_unix.go 0% <0%> (ø)
runtime/v2/shim/shim.go 0% <0%> (ø)
runtime/v2/shim/shim_unix.go 0% <0%> (ø)
runtime/v2/shim/shim_windows.go 40.85% <0%> (ø)
runtime/v2/shim/util.go 0% <0%> (ø)
... and 5 more

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 df60d32...26506e9. Read the comment docs.

@estesp estesp requested a review from dmcgowan September 28, 2018 14:59
@dmcgowan dmcgowan added this to the 1.2 milestone Oct 2, 2018
@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Oct 3, 2018

Change looks good, trying to figure out if the key name is ok or needs to follow a different pattern. I will make sure this gets into rc.2 or 1.2 final though.

Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan
Copy link
Copy Markdown
Member

I pushed a change with a small label update empty.layer -> empty-layer

LGTM on green

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@crosbymichael crosbymichael merged commit 5993d09 into containerd:master Oct 15, 2018
@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Oct 16, 2018

Thank you! @dmcgowan

@fuweid fuweid deleted the bugfix_cache_empty_label branch October 18, 2018 01:46
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