feature: add pouch history functionality #2066

xiechengsheng · 2018-08-08T06:56:52Z

Signed-off-by: xiechengsheng [email protected]

Ⅰ. Describe what this PR did

feature: add pouch history functionality

Ⅱ. Does this pull request fix one issue?

Fix #1578

Ⅲ. Describe how you did it

The image history info always exists in function containerdImageToOciImage, I just get this info and send to client side.
Thanks @fuweid in comments. We indeed could get layer size.

Ⅳ. Describe how to verify it

# pouch history ruby
IMAGE          CREATED       CREATED BY                                      SIZE        COMMENT
1d8640b852eb   2 weeks ago   /bin/sh -c #(nop)  CMD ["irb"]                  0.00 B      
<missing>      2 weeks ago   /bin/sh -c mkdir -p "$GEM_HOME" && chmod 7...   148.00 B    
<missing>      2 weeks ago   /bin/sh -c #(nop)  ENV PATH=/usr/local/bun...   0.00 B      
<missing>      2 weeks ago   /bin/sh -c #(nop)  ENV BUNDLE_PATH=/usr/lo...   0.00 B      
<missing>      2 weeks ago   /bin/sh -c #(nop)  ENV GEM_HOME=/usr/local...   0.00 B      
<missing>      2 weeks ago   /bin/sh -c set -ex   && buildDeps='   biso...   20.53 MB    
<missing>      2 weeks ago   /bin/sh -c #(nop)  ENV BUNDLER_VERSION=1.16.3   0.00 B      
<missing>      3 weeks ago   /bin/sh -c #(nop)  ENV RUBYGEMS_VERSION=2.7.7   0.00 B      
<missing>      3 weeks ago   /bin/sh -c #(nop)  ENV RUBY_DOWNLOAD_SHA25...   0.00 B      
<missing>      3 weeks ago   /bin/sh -c #(nop)  ENV RUBY_VERSION=2.5.1       0.00 B      
<missing>      3 weeks ago   /bin/sh -c #(nop)  ENV RUBY_MAJOR=2.5           0.00 B      
<missing>      3 weeks ago   /bin/sh -c mkdir -p /usr/local/etc  && {  ...   206.00 B    
<missing>      3 weeks ago   /bin/sh -c set -ex;  apt-get update;  apt-...   203.31 MB   
<missing>      3 weeks ago   /bin/sh -c apt-get update && apt-get insta...   47.75 MB    
<missing>      3 weeks ago   /bin/sh -c set -ex;  if ! command -v gpg >...   4.14 MB     
<missing>      3 weeks ago   /bin/sh -c apt-get update && apt-get insta...   10.24 MB    
<missing>      3 weeks ago   /bin/sh -c #(nop)  CMD ["bash"]                 0.00 B      
<missing>      3 weeks ago   /bin/sh -c #(nop) ADD file:370028dca6e8ca9...   43.21 MB

Ⅴ. Special notes for reviews

Here we just set imageID of top image layer, we do nothing with the lower image ID, after pouch enables build/commit functionality, we should get local lower image(parent image) layer ID. 😆

codecov-io · 2018-08-08T07:58:10Z

Codecov Report

Merging #2066 into master will decrease coverage by 0.22%.
The diff coverage is 17.54%.

@@            Coverage Diff             @@
##           master    #2066      +/-   ##
==========================================
- Coverage   65.35%   65.12%   -0.23%     
==========================================
  Files         207      207              
  Lines       16226    16283      +57     
==========================================
  Hits        10605    10605              
- Misses       4311     4362      +51     
- Partials     1310     1316       +6

Flag	Coverage Δ
#criv1alpha1test	`33.31% <1.75%> (-0.18%)`	⬇️
#criv1alpha2test	`33.9% <1.75%> (-0.19%)`	⬇️
#integrationtest	`39.93% <17.54%> (-0.15%)`	⬇️
#unittest	`23.78% <0%> (-0.1%)`	⬇️

Impacted Files	Coverage Δ
apis/server/router.go	`91.72% <100%> (+0.05%)`	⬆️
daemon/mgr/image.go	`57.76% <8%> (-10.97%)`	⬇️
apis/server/image_bridge.go	`70% <83.33%> (+0.85%)`	⬆️
cri/stream/httpstream/spdy/upgrade.go	`54.28% <0%> (-5.72%)`	⬇️
ctrd/container.go	`42.06% <0%> (-1.45%)`	⬇️
daemon/mgr/container.go	`55.36% <0%> (-0.41%)`	⬇️
cri/v1alpha1/cri.go	`64.04% <0%> (-0.18%)`	⬇️
cri/v1alpha2/cri.go	`64.89% <0%> (-0.18%)`	⬇️
daemon/logger/jsonfile/utils.go	`73.17% <0%> (+1.62%)`	⬆️
daemon/mgr/system.go	`79.33% <0%> (+1.65%)`	⬆️

allencloud · 2018-08-09T05:02:47Z

I tried to use this pull request:

root@ubuntu:~/go/src/github.com/alibaba/pouch# pouch history registry.hub.docker.com/library/ruby:latest
IMAGE          CREATED        CREATED BY                                      SIZE        COMMENT
857bc7ff918f   2 months ago   /bin/sh -c #(nop)  CMD ["irb"]                  329.20 MB
<missing>      2 months ago   /bin/sh -c mkdir -p "$GEM_HOME" && chmod 7...   0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV PATH=/usr/local/bun...   0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV BUNDLE_PATH=/usr/lo...   0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV GEM_HOME=/usr/local...   0.00 B
<missing>      2 months ago   /bin/sh -c set -ex   && buildDeps='   biso...   0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV BUNDLER_VERSION=1.16.2   0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV RUBYGEMS_VERSION=2.7.7   0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV RUBY_DOWNLOAD_SHA25...   0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV RUBY_VERSION=2.5.1       0.00 B
<missing>      2 months ago   /bin/sh -c #(nop)  ENV RUBY_MAJOR=2.5           0.00 B
<missing>      2 months ago   /bin/sh -c mkdir -p /usr/local/etc  && {  ...   0.00 B
<missing>      2 months ago   /bin/sh -c set -ex;  apt-get update;  apt-...   0.00 B
<missing>      2 months ago   /bin/sh -c apt-get update && apt-get insta...   0.00 B
<missing>      2 months ago   /bin/sh -c set -ex;  if ! command -v gpg >...   0.00 B
<missing>      2 months ago   /bin/sh -c apt-get update && apt-get insta...   0.00 B
<missing>      3 months ago   /bin/sh -c #(nop)  CMD ["bash"]                 0.00 B
<missing>      3 months ago   /bin/sh -c #(nop) ADD file:9572fdb59dfbb9b...   0.00 B

I found that except the top layer, the others are all size 0? Does it make sense? @xiechengsheng @fuweid

allencloud · 2018-08-09T06:43:10Z

apis/server/router.go

@@ -64,6 +64,7 @@ func initRoute(s *Server) http.Handler {
 	s.addRoute(r, http.MethodPost, "/images/{name:.*}/tag", s.postImageTag)
 	s.addRoute(r, http.MethodPost, "/images/load", withCancelHandler(s.loadImage))
 	s.addRoute(r, http.MethodGet, "/images/save", withCancelHandler(s.saveImage))
+	s.addRoute(r, http.MethodGet, "/images/{name:.*}/history", s.imageHistory)


s/imageHistory/getImageHistory to keep the naming consistent?

ok, will fix this.

allencloud · 2018-08-09T06:44:53Z

apis/swagger.yml

+          schema:
+            type: "array"
+            items:
+              type: "object"


Could you encapsulate the whole object into an ImageHistoryRespone or HistoryResultItem ？
Maybe this would be much nicer and this will be a separate file in apis/types.

xiechengsheng · 2018-08-09T10:19:34Z

@allencloud , According to @fuweid , the whole image layers exist in pouch root direction io.containerd.content.v1.content, but after discussing with @fuweid offline, it seems to be impossible(or too hard?) to find the relevance between ociImage history messages and those sha256 numbers.

fuweid · 2018-08-09T16:40:32Z

hi @xiechengsheng, I checked the definition of config in image and found that the history information is for each layer. It means that the layer blob data represents the diff size. I wrote a demo about this.

func main() {
        client, err := containerd.New("/run/containerd/containerd.sock", containerd.WithDefaultNamespace(namespaces.Default))
        if err != nil {
                panic(err)
        }
        defer client.Close()

        // pull an image
        ctx := context.Background()
        img, err := client.GetImage(ctx, "docker.io/library/ubuntu:latest")
        if err != nil {
                panic(err)
        }

        // layers info
        cs := img.ContentStore()
        manifest, err := images.Manifest(ctx, cs, img.Target(), platforms.Default())
        if err != nil {
                panic(err)
        }

        // diffID info
        diffIDs, err := img.RootFS(ctx)
        if err != nil {
                panic(err)
        }

        if len(manifest.Layers) != len(diffIDs) {
                panic("number of layers should equal to the diffIDs")
        }

        // history size
        total := int64(0)
        for _, l := range manifest.Layers {
                info, err := cs.Info(ctx, l.Digest)
                if err != nil {
                        panic(err)
                }
                total += info.Size
                fmt.Println(progress.Bytes(info.Size))
        }
        fmt.Println("total: ", progress.Bytes(total))
}

$ sudo env PATH=$PATH go run main.go
30.2 MiB
847.0 B
468.0 B
849.0 B
163.0 B
total:  30.2 MiB

However, we still missing ID because we don't have build the thing. How do you think? 😄

xiechengsheng · 2018-08-10T05:38:57Z

Agree. I think the diffID couldn't represent the lower image layer's ID.

xiechengsheng · 2018-08-10T09:52:29Z

@fuweid @allencloud Please re-check, thanks a lot~

fuweid · 2018-08-10T10:03:07Z

apis/swagger.yml

+  /images/{imageid}/history:
+    get:
+      summary: "Get an image's history"
+      description: "Return the parent layers about image"


return the history of each layer of image

fuweid · 2018-08-10T10:06:15Z

cli/history.go

+)
+
+// historyDescription is used to describe history command in detail and auto generate command doc.
+var historyDescription = "Return the history information on Pouch image"


Return the history information on Pouch image I think we use Return the history information about image, how do you think?

fuweid · 2018-08-11T01:26:18Z

daemon/mgr/image.go

+
+	// layers info
+	cs := img.ContentStore()
+	manifest, err := ctrdmetaimages.Manifest(ctx, cs, img.Target(), platforms.Default())


could we extract the function for this part? call the getLayer, how do you think?

fuweid · 2018-08-11T01:33:07Z

daemon/mgr/image.go

+
+	// Note: ociImage History layers info and manifest layers info are all in order from bottom-most to top-most, but the
+	// user-interactive history is in order from top-most to top-bottom, so we need to reverse ociImage History traverse order.
+	for i := range ociImageHistory {


could we combine this with the following part which is about to get the size of blobs?

fuweid · 2018-08-11T01:33:45Z

daemon/mgr/image.go

+
+	ociImageHistory := ociImage.History
+	lenOciImageHistory := len(ociImageHistory)
+	history := make([]types.HistoryResultItem, lenOciImageHistory)


I think we need to check the size of history is valid.

allencloud · 2018-08-13T02:25:36Z

I think this pull request is really close to make it. While there is still something to finish:

rebase
some minor change.

@xiechengsheng

xiechengsheng · 2018-08-13T05:06:28Z

ok, will finish this asap.

xiechengsheng · 2018-08-13T07:18:46Z

daemon/mgr/image.go

+		}
+	}
+
+	if countNonEmptyLayer != len(manifest.Layers) {


@fuweid if you think this check is useless, then we can merge these two for loop into one.

fuweid · 2018-08-13T16:16:46Z

daemon/mgr/image.go

+
+		// Note: number of manifest layers should be less than ociImage History messages due to the existence of empty layers.
+		// The size of these empty layers should be set to 0.
+		if !history[i].EmptyLayer {


we can do the valid check here, like

if j < 0 { return nil, fmt.Errorf("the layer data don't match the layers") }

Out of the loop, we need to check the j is -1 or not. I think we can make the logic clean. How do you think?

I think you are right.

Signed-off-by: xiechengsheng <[email protected]>

fuweid · 2018-08-14T14:57:51Z

LGTM

pouchrobot added kind/feature size/XL labels Aug 8, 2018

xiechengsheng force-pushed the add-history branch 3 times, most recently from 70a3e41 to 4649598 Compare August 8, 2018 07:14

xiechengsheng force-pushed the add-history branch 3 times, most recently from c3398c5 to b66bbee Compare August 9, 2018 03:57

allencloud reviewed Aug 9, 2018

View reviewed changes

xiechengsheng force-pushed the add-history branch from b66bbee to 75d0d75 Compare August 9, 2018 10:48

allencloud assigned fuweid Aug 9, 2018

xiechengsheng force-pushed the add-history branch 2 times, most recently from 5946f11 to d99f47e Compare August 10, 2018 08:03

pouchrobot added the conflict/needs-rebase label Aug 11, 2018

fuweid suggested changes Aug 11, 2018

View reviewed changes

xiechengsheng force-pushed the add-history branch from d99f47e to 09acabb Compare August 13, 2018 07:14

xiechengsheng commented Aug 13, 2018

View reviewed changes

xiechengsheng force-pushed the add-history branch from 09acabb to b88b2d2 Compare August 13, 2018 07:41

fuweid reviewed Aug 13, 2018

View reviewed changes

pouchrobot removed the conflict/needs-rebase label Aug 14, 2018

xiechengsheng force-pushed the add-history branch from b88b2d2 to 6717b7b Compare August 14, 2018 09:02

pouchrobot added size/XXL and removed size/XL labels Aug 14, 2018

feature: add pouch history functionality

86c6cd2

Signed-off-by: xiechengsheng <[email protected]>

xiechengsheng force-pushed the add-history branch from 6717b7b to 86c6cd2 Compare August 14, 2018 12:35

pouchrobot added size/XL and removed size/XXL labels Aug 14, 2018

fuweid approved these changes Aug 14, 2018

View reviewed changes

fuweid merged commit 5aee31c into AliyunContainerService:master Aug 14, 2018

pouchrobot mentioned this pull request Aug 18, 2018

Weekly Report of PouchContainer from 2018-08-11 to 2018-08-18 #2114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add pouch history functionality #2066

feature: add pouch history functionality #2066

xiechengsheng commented Aug 8, 2018 •

edited

Loading

codecov-io commented Aug 8, 2018 •

edited

Loading

allencloud commented Aug 9, 2018

allencloud Aug 9, 2018

xiechengsheng Aug 9, 2018

allencloud Aug 9, 2018 •

edited

Loading

xiechengsheng Aug 9, 2018

xiechengsheng commented Aug 9, 2018

fuweid commented Aug 9, 2018 •

edited

Loading

xiechengsheng commented Aug 10, 2018

xiechengsheng commented Aug 10, 2018

fuweid Aug 10, 2018

fuweid Aug 10, 2018

fuweid Aug 11, 2018

fuweid Aug 11, 2018

fuweid Aug 11, 2018

allencloud commented Aug 13, 2018

xiechengsheng commented Aug 13, 2018

xiechengsheng Aug 13, 2018

fuweid Aug 13, 2018

xiechengsheng Aug 14, 2018

fuweid commented Aug 14, 2018

feature: add pouch history functionality #2066

feature: add pouch history functionality #2066

Conversation

xiechengsheng commented Aug 8, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Aug 8, 2018 • edited Loading

Codecov Report

allencloud commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiechengsheng commented Aug 9, 2018

fuweid commented Aug 9, 2018 • edited Loading

xiechengsheng commented Aug 10, 2018

xiechengsheng commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Aug 13, 2018

xiechengsheng commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Aug 14, 2018

xiechengsheng commented Aug 8, 2018 •

edited

Loading

codecov-io commented Aug 8, 2018 •

edited

Loading

allencloud Aug 9, 2018 •

edited

Loading

fuweid commented Aug 9, 2018 •

edited

Loading