ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.5 #2725

fuweid · 2019-02-25T09:13:57Z

Ⅰ. Describe what this PR did

The containerd has been improved a lot in v1.2, which makes the service
stable than before. Based on this, we need to do the upgrade.

using following commands to update the vendor

govendor fetch github.com/containerd/containerd/...@v1.2.5
govendor fetch github.com/gogo/protobuf/...@v1.0.0
govendor fetch github.com/gogo/googleapis/...@08a7655d27152912db7aaf4f983275eaf8d128ef
govendor fetch google.golang.org/grpc/...@v1.12.0
govendor fetch github.com/golang/protobuf/...@v1.1.0

Basically, both the contaienrd and shim API interfaces are stable so
that we don't need to worry about the remote API calls. However, the
package level interface has been changed a lot. The commit is used to
align with containerd@v1.2.0.

1. events part:
   using exchange event helper instead of raw grpc conn

2. image part:
  a. import:
     new Import interface makes the import easier without Importer.

  b. commit:
     content helper has been upgraded with less arguments for caller.
     and `diff` package separates the Applier and Comparer interfaces.
     The changes can make the code clear in PouchContainer

3. lease:
  containerd client doesn't provide ListLeases interface directly.
  PouchContainer needs to get LeasesService and make the call by itself.

4. container IO:
  remove codes which comes from containerd. The code was used to align
  with containerd v1.2, which can make upgrade work easier. For now, we
  can remove this.

I also use subshell to run the integration::ping_pouchd command so that the error log can show up in CI.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

no need

NOTE: I change the save and load case because the containerd v1.2.x doesn't contains the related patch for multiple manifest cases. Based on this, I replace helloworld image with busybox:1.25.

Ⅳ. Describe how to verify it

use existing CI testing case

Ⅴ. Special notes for reviews

I separate the change into two commits because the vendor change is too big.
There are somethings need to do after this PR, for example, it is time to remove the ctrd/wrapper_client.go because the grpc has been updated and it can support the defaultMaxStreamsClient

after this patch, the #1897 will be fixed.

➜  /tmp docker pull alpine

➜  /tmp docker save alpine -o alpine.tar

➜  /tmp pouch images
IMAGE ID   IMAGE NAME   SIZE

➜  /tmp pouch load -i alpine.tar docker.io/library/alpine

➜  /tmp pouch images
IMAGE ID       IMAGE NAME                        SIZE
caf27325b298   docker.io/library/alpine:latest   5.53 MB

fix #1897 #1713

codecov · 2019-02-25T09:14:00Z

Codecov Report

Merging #2725 into master will increase coverage by 0.08%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##           master    #2725      +/-   ##
==========================================
+ Coverage   69.32%   69.41%   +0.08%     
==========================================
  Files         278      277       -1     
  Lines       17448    17391      -57     
==========================================
- Hits        12096    12072      -24     
+ Misses       4025     3998      -27     
+ Partials     1327     1321       -6

Flag	Coverage Δ
#criv1alpha2_test	`39.39% <40.32%> (-0.08%)`	⬇️
#integration_test_0	`36.57% <56.45%> (-0.03%)`	⬇️
#integration_test_1	`35.46% <82.25%> (-0.07%)`	⬇️
#integration_test_2	`36.5% <40.32%> (-0.06%)`	⬇️
#integration_test_3	`35.4% <74.19%> (ø)`	⬆️
#node_e2e_test	`35.19% <40.32%> (-0.11%)`	⬇️
#unittest	`28.54% <0%> (+0.1%)`	⬆️

Impacted Files	Coverage Δ
pkg/jsonstream/image_progress.go	`0% <ø> (ø)`	⬆️
daemon/mgr/image.go	`59.45% <0%> (ø)`	⬆️
daemon/mgr/image_utils.go	`85.45% <100%> (ø)`	⬆️
daemon/mgr/image_load.go	`41.17% <100%> (-8.83%)`	⬇️
ctrd/image.go	`69.96% <100%> (+0.7%)`	⬆️
ctrd/utils.go	`83.17% <100%> (+0.82%)`	⬆️
daemon/containerio/cio.go	`70% <100%> (+1.32%)`	⬆️
ctrd/snapshot.go	`56.14% <100%> (+8.77%)`	⬆️
daemon/containerio/io.go	`74.75% <100%> (ø)`	⬆️
ctrd/image_commit.go	`68.02% <66.66%> (ø)`	⬆️
... and 10 more

hack/install/install_containerd.sh

hack/testing/run_daemon_integration.sh

rudyfly · 2019-02-26T13:29:49Z

test/api_image_save_and_load_test.go

@@ -25,20 +25,20 @@ func init() {
 func (suite *APIImageSaveLoadSuite) SetUpTest(c *check.C) {
 	SkipIfFalse(c, environment.IsLinux)

-	PullImage(c, helloworldImage)
+	PullImage(c, busyboxImage125)


can we use image busyboxImage128?

sure. will do

done. I will file other pr to replace all the busyboxImage125 because the 128 tag is stable.

I roll it back because 125 tag has only one manifest, not a list.

I think define busyboxImage is enough, without tag, or we need change lots of places, we have done these times

we cannot use the latest one, because the latest has no stable digest and ID. we don't know what the real ID is.

What I mean is busyboxImage=xxx125 or xxx128, I do not like busyboxImage128 define with tag at all. It can be fix in another pr.

Or can we reset these change in test.

I will change it to busyboxLegacyImage instead of 125 in the following pr.
cc @rudyfly

fuweid · 2019-02-26T16:30:08Z

daemon/mgr/image_utils.go

 	switch cfg.MediaType {
-	case ocispec.MediaTypeImageConfig, images.MediaTypeDockerSchema2Config:
-		data, err := content.ReadBlob(ctx, img.ContentStore(), cfg.Digest)
+	case ocispec.MediaTypeImageConfig, images.MediaTypeDockerSchema2Config,


ping @HusterWan and @Ace-Tang , there is used to support existing downloaded images, for example, redis:2 with the application/octet-stream media type. if we don't support it here, we will miss the image.

cc @rudyfly

ctrd/image.go

ctrd/image_commit.go

Ace-Tang · 2019-02-27T02:09:38Z

ctrd/utils.go

 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 )

+// NewDefaultSpec new a template spec with default.
+func NewDefaultSpec(ctx context.Context, id string) (*specs.Spec, error) {


We do not need default spec from containerd now

Ace-Tang · 2019-02-27T02:22:29Z

daemon/containerio/cio.go

-// NewCioFIFOSet prepares fifo files.
-func NewCioFIFOSet(processID string, withStdin bool, withTerminal bool) (*CioFIFOSet, error) {
+// NewFIFOSet prepares fifo files.
+func NewFIFOSet(processID string, withStdin bool, withTerminal bool) (*cio.FIFOSet, error) {
 	root := "/run/containerd/fifo"


Let make it a variable ?

it was my plan. I will do that in next pr.

HusterWan · 2019-02-27T06:33:51Z

ctrd/client.go

-		e, err := events.Recv()
-		if err != nil {
-			logrus.Errorf("failed to receive event: %v", err)
+		var e *events.Envelope


How about combine var define with below:

var ( action string containerID string attributes = map[string]string{} )

zhuangqh

LGTM

using following commands: govendor fetch github.com/containerd/containerd/...@v1.2.5 govendor fetch github.com/gogo/protobuf/...@v1.0.0 govendor fetch github.com/gogo/googleapis/...@08a7655d27152912db7aaf4f983275eaf8d128ef govendor fetch google.golang.org/grpc/...@v1.12.0 govendor fetch github.com/golang/protobuf/...@v1.1.0 Signed-off-by: Wei Fu <fuweid89@gmail.com>

The containerd has been improved a lot in v1.2, which makes the service stable before. Based on this, we need to do the upgrade. Basically, both the contaienrd and shim API interfaces are stable so that we don't need to worry about the remote API calls. However, the package level interface has been changed a lot. The commit is used to align with containerd@v1.2.5. 1. events part: using exchange event helper instead of raw grpc conn 2. image part: a. import: new Import interface makes the import easier without Importer. b. commit: content helper has been upgraded with less arguments for caller. and `diff` package separates the Applier and Comparer interfaces. The changes can make the code clear in PouchContainer 3. lease: containerd client doesn't provide ListLeases interface directly. PouchContainer needs to get LeasesService and make the call by itself. 4. container IO: remove codes which comes from containerd. The code was used to align with containerd v1.2, which can make upgrade work easier. For now, we can remove this. Signed-off-by: Wei Fu <fuweid89@gmail.com>

pouchrobot added the size/XXL label Feb 25, 2019

fuweid changed the title ~~Me containerd v1.2.x~~ ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.4 Feb 25, 2019

fuweid requested a review from zhuangqh February 25, 2019 11:28

fuweid assigned Ace-Tang Feb 25, 2019

fuweid requested review from rudyfly and HusterWan February 25, 2019 11:28

rudyfly reviewed Feb 26, 2019

View reviewed changes

fuweid commented Feb 26, 2019

View reviewed changes

Ace-Tang reviewed Feb 27, 2019

View reviewed changes

HusterWan reviewed Feb 27, 2019

View reviewed changes

zhuangqh approved these changes Feb 28, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Mar 3, 2019

Weekly Digest (24 February, 2019 - 3 March, 2019) #2732

Closed

fuweid changed the title ~~ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.4~~ [RFC|DONOTMERGE] ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.4 Mar 4, 2019

Ace-Tang approved these changes Mar 5, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Mar 10, 2019

Weekly Digest (3 March, 2019 - 10 March, 2019) #2740

Closed

fuweid added 2 commits March 13, 2019 17:10

fuweid changed the title ~~[RFC|DONOTMERGE] ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.4~~ ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.5 Mar 13, 2019

zhuangqh self-requested a review March 13, 2019 09:29

Ace-Tang merged commit ffeaa43 into AliyunContainerService:master Mar 14, 2019

pouchrobot mentioned this pull request Mar 15, 2019

WeeklyReport of PouchContainer from 2019-03-08 to 2019-03-15 #2752

Closed

fuweid deleted the me-containerd-v1.2.x branch March 25, 2019 01:13

allencloud mentioned this pull request May 10, 2019

[feature request] Consider updating containerd to v1.1.1? #1713

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.5 #2725

ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.5 #2725

fuweid commented Feb 25, 2019 •

edited by allencloud

codecov bot commented Feb 25, 2019 •

edited

rudyfly Feb 26, 2019

fuweid Feb 26, 2019

fuweid Feb 26, 2019

fuweid Feb 27, 2019

Ace-Tang Feb 27, 2019

fuweid Feb 28, 2019

Ace-Tang Mar 5, 2019

Ace-Tang Mar 5, 2019

fuweid Mar 5, 2019 •

edited

fuweid Feb 26, 2019

fuweid Feb 26, 2019

Ace-Tang Feb 27, 2019

Ace-Tang Feb 27, 2019

fuweid Feb 27, 2019

fuweid Feb 27, 2019

Ace-Tang Feb 27, 2019

fuweid Feb 27, 2019

HusterWan Feb 27, 2019

zhuangqh left a comment

ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.5 #2725

ctrd,cli,daemon,pkg: update to adapt with containerd@v1.2.5 #2725

Conversation

fuweid commented Feb 25, 2019 • edited by allencloud

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Feb 25, 2019 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid Mar 5, 2019 • edited

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

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

zhuangqh left a comment

Choose a reason for hiding this comment

fuweid commented Feb 25, 2019 •

edited by allencloud

codecov bot commented Feb 25, 2019 •

edited

fuweid Mar 5, 2019 •

edited