Replacing containerd GRPC API with client #113

abhi · 2017-08-05T18:31:36Z

This commit

Replaces the usage of containerd GRPC APIs with the containerd client for all operations related to containerd.
Updated containerd to v1.0alpha4+
Updated runc to v1.0.0

Signed-off-by: Abhinandan Prativadi [email protected]

Random-Liu · 2017-08-07T16:39:21Z

@abhinandanpb Thanks! Ping me when you are ready, and feel free to tell me if you have any questions. :)

Random-Liu

Haven't finished reviewing.
Send out some comments first.

Random-Liu · 2017-08-09T20:16:13Z

pkg/store/sandbox/sandbox_test.go

 	assert := assertlib.New(t)
 	sandboxes := map[string]Sandbox{}
 	for _, id := range ids {
-		sandboxes[id] = Sandbox{metadatas[id]}


Sandbox{Metadata: metadatas[id]}

Random-Liu · 2017-08-09T20:16:38Z

pkg/store/sandbox/sandbox.go

@@ -28,6 +28,8 @@ type Sandbox struct {
 	// Metadata is the metadata of the sandbox, it is immutable after created.
 	Metadata
 	// TODO(random-liu): Add containerd container client.


Remove the TODO.

Random-Liu · 2017-08-09T20:17:27Z

pkg/store/sandbox/sandbox.go

 	Metadata
 	// TODO(random-liu): Add containerd container client.
+	// Containerd sandbox
+	Container containerd.Container


Seems better to call this sandbox container.

// Containerd sandbox container

Random-Liu · 2017-08-09T20:18:52Z

pkg/store/sandbox/sandbox.go

-
+	"github.com/containerd/containerd"
 	"github.com/kubernetes-incubator/cri-containerd/pkg/store"
+	"sync"


nit: Move "sync" up. Our style is usually:

import ( "sync" // golang libraries "github.com/containerd/containerd" // Other libraries "github.com/kubernetes-incubator/cri-containerd/pkg/store" // cri-containerd libraries )

Random-Liu · 2017-08-09T20:19:36Z

pkg/store/image/image.go

 	Config *imagespec.ImageConfig
+	// container image reference
+	Image containerd.Image
 	// TODO(random-liu): Add containerd image client.


Remove the TODO.

And // Containerd image reference seems better.

Random-Liu · 2017-08-09T20:42:34Z

pkg/server/sandbox_run.go

+		containerd.WithNewSnapshotView(id, image.Image)}...)
+
+	var container containerd.Container
+	if container, err = c.client.NewContainer(ctx, id, opts...); err != nil {


nit:

container, err := c.client.NewContainer(ctx, id, opts...) if err != nil { }

Random-Liu · 2017-08-09T20:43:58Z

pkg/server/sandbox_run.go

+		containerd.WithSpec(spec),
+		containerd.WithImage(image.Image),
+		containerd.WithRuntime(defaultRuntime),
+		containerd.WithNewSnapshotView(id, image.Image)}...)


There is a problem with the containerd client. If NewContainer fails, no one will cleanup the snapshot.

In our original implementation, we'll remove the snapshot. But in their client, it's not removed.

This is not your problem, will file an issue to them. But could you add a TODO here to track that?

Looks like I removed the TODO i had added earlier. I can fix it in containerd

Cool. Then let's add TODO on our side, and try to fix it in containerd. :)

Random-Liu · 2017-08-09T20:46:11Z

pkg/server/sandbox_run.go

-	// Start sandbox container in containerd.
-	if _, err := c.taskService.Start(ctx, &tasks.StartTaskRequest{ContainerID: id}); err != nil {
-		return nil, fmt.Errorf("failed to start sandbox container %q: %v",
+	err = task.Start(ctx)


nit to save one line:

if err := task.Start(ctx); err != nil { }

Random-Liu · 2017-08-09T20:52:38Z

pkg/server/sandbox_run.go

 		if retErr != nil {
-			// Cleanup the sandbox container if an error is returned.
-			if err := c.stopSandboxContainer(ctx, id); err != nil {
+			if _, err := task.Delete(ctx); err != nil {


You could not Delete a task once it's created, unless you use the new WithProcessKill option containerd/containerd#1301.

We need the orignal code here, and replace it with Delete WithProcessKill after we bump up containerd version.

For this PR to work we anyways need to bump containerd version. I can incorporate the change. WDYT ?

Sure. Let's update containerd version then.

There may be some change in their api and client, you may need to update your PR correspondingly.

Delete with kill looks useful
containerd/containerd@d8c075a

Random-Liu · 2017-08-09T20:53:26Z

pkg/server/sandbox_run.go

 	if err != nil {
-		return nil, fmt.Errorf("failed to create sandbox container %q: %v",
-			id, err)
+		return nil, fmt.Errorf("failed to create task for sandbox:%q: %v", container.ID(), err)


nit: s/sandbox:%q/sandbox %q.

Random-Liu · 2017-08-09T23:56:53Z

pkg/store/sandbox/sandbox.go

 import (
 	"sync"

+	"github.com/containerd/containerd"


nit: Maybe a little bit too picky, but may help with future PRs, :p

import( "sync" // golang libraries // empty line "github.com/containerd/containerd" // other libraries // empty line "github.com/kubernetes-incubator/cri-containerd/pkg/store" // CRI containerd libraries )

And also the same with the other imports.

I use goimports to format. I will skip using that.

Random-Liu · 2017-08-10T00:00:36Z

pkg/server/status.go

-	resp, err := c.healthService.Check(ctx, &healthapi.HealthCheckRequest{})
-	if err != nil || resp.Status != healthapi.HealthCheckResponse_SERVING {
+
+	if c.client == nil {


Seems not required. Because we've been using c.client.XXX all around, :)
We've made the assumption that once grpc handler runs, client should be ready.

Random-Liu

Not finished yet

Random-Liu · 2017-08-10T00:08:55Z

pkg/server/sandbox_run.go

+		return nil, fmt.Errorf("failed to start sandbox container task %q: %v",
 			id, err)
 	}
+	defer func() {


This is not required. We only need one defer after NewTask to Delete WithProcessKill.

Random-Liu · 2017-08-10T00:15:03Z

pkg/server/sandbox_run.go


-	sandbox.Pid = createResp.Pid
-	sandbox.NetNS = getNetworkNamespace(createResp.Pid)
+	// Create sandbox task in containerd.


Why move this down here?

Random-Liu · 2017-08-10T00:16:52Z

pkg/server/sandbox_run.go

 	if err != nil {
-		return nil, fmt.Errorf("failed to create sandbox container %q: %v",
-			id, err)
+		return nil, fmt.Errorf("failed to create task for sandbox %q: %v", container.ID(), err)


s/container.ID()/id.
id seems more straightforward as sandbox id.

Random-Liu · 2017-08-10T00:27:18Z

pkg/server/sandbox_run.go

+	if err != nil {
 		return nil, fmt.Errorf("failed to create containerd container: %v", err)
 	}
+


nit: unnecessary empty line.

Random-Liu · 2017-08-10T00:40:26Z

pkg/server/sandbox_run.go

-	glog.V(5).Infof("Create sandbox container (id=%q, name=%q) with options %+v.",
-		id, name, createOpts)
-	createResp, err := c.taskService.Create(ctx, createOpts)
+	task, err := container.NewTask(ctx, containerd.NewIOWithFifoSet(os.Stdin, os.Stdout, os.Stderr, fifo, false))


It's fine to use containerd.NewFifos and create fifos in /tmp. However, the problem is that there is no way to pass empty stdin now.

There are 3 options:

Option 1: We create fifos ourselves, and pass them to containerd client. (What you are doing).

Option 2: Still use containerd.NewIO, but add option to disable stdin.

Option 3: Use containerd.NewIO directly, and call task.CloseIO afterwards for containers which don't want stdin.

I feel that option 2 or 3 seems better, because less change is required.

Will move this discussion to your containerd PR.

Another problem with PR. Why using os.Stdin, os.Stdout and os.Stderr.

Should be nil, ioutil.Discard and ioutil.Discard.

This is still possible. The only reason I avoided using NewIO as is -> we need to pass the customer path. The tests were failing because CRI validation checks for logs in /tmp/Podxxxx where containerd by default (NewIO) logs into /tmp/containerd/XXXX.

@abhinandanpb That's not how it works.

Containerd create FIFOs for each container. Currently, we create those fifos and give the path to containerd. In fact, we could use containerd client directly, which creates fifos for us in /tmp/containerd/XXX. We don't really care where the FIFOs are.

We create a Logger for each container, which read log from the FIFO, decorate the log, and write to the CRI log file, which is /var/log/pods/XXX. The test is checking this path, it doesn't care where the FIFOs are.

NewIO accepts streams directly, e.g. for stdout, we just need to open a pipe, pass the writer side to NewIO, pass the reader side to logger.

os.Stdout/os.Stderr/os.Stdin should not be used here.

Random-Liu · 2017-08-10T03:00:53Z

pkg/server/container_create.go

 	if err != nil {
-		return nil, fmt.Errorf("failed to resolve image %q: %v", imageRef, err)
-	}
-	if image == nil {


For localResolve, we still need this.

Random-Liu · 2017-08-10T03:02:54Z

pkg/server/container_create.go

-		},
-		RootFS: id,
-	}); err != nil {
+	opts = append(opts, []containerd.NewContainerOpts{containerd.WithSpec(spec), containerd.WithRuntime(defaultRuntime)}...)


Just:

opts = append(opts, containerd.WithSpec(spec), containerd.WithRuntime(defaultRuntime))

Random-Liu · 2017-08-10T03:06:14Z

pkg/server/container_create.go

 		return nil, fmt.Errorf("failed to create internal container object for %q: %v",
 			id, err)
 	}
+


Why add a new line before defer? I feel like we should group defer with the resource we want it to release?
Or is there any other convention?

i usally let gofmt/goimports take care of formatting and indentation

@abhinandanpb Weird. In my case, gofmt never add unnecessary new lines for me.

Random-Liu · 2017-08-10T03:06:40Z

pkg/server/container_create.go

 	}()

-	container, err := containerstore.NewContainer(meta, containerstore.Status{CreatedAt: time.Now().UnixNano()})
+	container, err := containerstore.NewContainer(meta, containerstore.Status{CreatedAt: time.Now().UnixNano()}, newContainer)


nit: The line is too long, add a new line?

Random-Liu · 2017-08-10T03:08:47Z

pkg/server/container_create.go

+	opts = append(opts, []containerd.NewContainerOpts{containerd.WithSpec(spec), containerd.WithRuntime(defaultRuntime)}...)
+
+	var newContainer containerd.Container
+	if newContainer, err = c.client.NewContainer(ctx, id, opts...); err != nil {


nit:

newContainer, err := c.client.XXXXXXXX if err != nil { return nil, fmt.Errorf("failed to create containerd container: %v", err) }

newContainer/container seems a bit confusing...Let's use cntr or cContainer or something else here?

Random-Liu · 2017-08-10T03:27:59Z

pkg/server/container_start.go

 // startContainer actually starts the container. The function needs to be run in one transaction. Any updates
 // to the status passed in will be applied no matter the function returns error or not.
-func (c *criContainerdService) startContainer(ctx context.Context, id string, meta containerstore.Metadata, status *containerstore.Status) (retErr error) {
+func (c *criContainerdService) startContainer(ctx context.Context, container containerd.Container, meta containerstore.Metadata, status *containerstore.Status) (retErr error) {


nit: The line is too long, add a new line.

Random-Liu · 2017-08-10T03:30:23Z

pkg/server/container_start.go

-			if _, err := c.taskService.Delete(ctx, &tasks.DeleteTaskRequest{ContainerID: id}); err != nil {
-				glog.Errorf("Failed to delete containerd task %q: %v", id, err)
+			if _, err := task.Delete(ctx); err != nil {
+				glog.Errorf("Failed to delete container %q: %v", container.ID(), err)


original error message seems better.

Random-Liu · 2017-08-10T03:31:52Z

pkg/server/container_start.go

-			// Cleanup the containerd task if an error is returned.
-			if _, err := c.taskService.Delete(ctx, &tasks.DeleteTaskRequest{ContainerID: id}); err != nil {
-				glog.Errorf("Failed to delete containerd task %q: %v", id, err)
+			if _, err := task.Delete(ctx); err != nil {


Should add WithProcessKill.
My original code was not correct.

Random-Liu · 2017-08-10T03:34:25Z

pkg/server/container_stop.go

+		}
+
+		err = task.Kill(ctx, stopSignal)
+


? unnecessary empty line? There are several empty lines, seem like something wrong with your editor?

if err := task.Kill(ctx, stopSignal); err != nil { }

Random-Liu · 2017-08-10T03:37:11Z

pkg/server/container_stop.go

-		Signal:      uint32(unix.SIGKILL),
-		All:         true,
-	})
+	err = task.Kill(ctx, unix.SIGKILL)


if err := task.Kill(ctx, unix.SIGKILL); err != nil { }

Random-Liu · 2017-08-10T03:48:56Z

pkg/server/sandbox_run.go

-	glog.V(5).Infof("Create sandbox container (id=%q, name=%q) with options %+v.",
-		id, name, createOpts)
-	createResp, err := c.taskService.Create(ctx, createOpts)
+	task, err := container.NewTask(ctx, containerd.NewIOWithFifoSet(os.Stdin, os.Stdout, os.Stderr, fifo, false))


@abhinandanpb That's not how it works.

Containerd create FIFOs for each container. Currently, we create those fifos and give the path to containerd. In fact, we could use containerd client directly, which creates fifos for us in /tmp/containerd/XXX. We don't really care where the FIFOs are.

We create a Logger for each container, which read log from the FIFO, decorate the log, and write to the CRI log file, which is /var/log/pods/XXX. The test is checking this path, it doesn't care where the FIFOs are.

NewIO accepts streams directly, e.g. for stdout, we just need to open a pipe, pass the writer side to NewIO, pass the reader side to logger.

os.Stdout/os.Stderr/os.Stdin should not be used here.

Random-Liu

First pass review finished.
Will do the second pass after you reply and address comments.

Random-Liu · 2017-08-10T04:29:25Z

pkg/server/container_stop.go

+		if err != nil {
+			if !isContainerdGRPCNotFoundError(err) {
+				return fmt.Errorf("failed to stop container, task not found for container %q: %v", id, err)
+			}


task will be nil if there is an error, we may want to skip the task.Kill when there is an error, even it's not found error.

And we could get task outside of the if clause, so that we don't need to do it twice.

Random-Liu · 2017-08-10T04:30:21Z

pkg/server/container_stop.go


+	task, err := container.Container.Task(ctx, nil)
+	if err != nil {
+		if !isContainerdGRPCNotFoundError(err) {


Skip task.Kill if not found.

Random-Liu · 2017-08-10T04:30:55Z

pkg/server/container_stop_test.go

 			container, err := containerstore.NewContainer(
 				containerstore.Metadata{ID: id},
-				*test.status,
+				*test.status, nil,


Random-Liu · 2017-08-10T04:39:39Z

pkg/server/helpers.go

-		return "", 0, nil, fmt.Errorf("failed to get image %q from containerd image store: %v",
-			normalizedRef, err)
-	}
+


We should add following functions into containerd client.
Image.Config, Image.RootFS, Image.Size etc.

Random-Liu · 2017-08-10T04:42:20Z

pkg/server/image_pull.go

-	// 2) We need desc returned by schema1 converter.
-	// So just put the image metadata after downloading now.
-	// TODO(random-liu): Fix the potential garbage collection race.
-	repoDigest, repoTag := getRepoDigestAndTag(namedRef, desc.Digest, schema1Converter != nil)


We still need to write repoDigest, repoTag and imageID into containerd image metadata store.

Can you elaborate on this .? This has to be written on the containerd side ?

Currently, our internal image store is maintaining the Image ID -> RepoDigests/RepoTags mapping, which is for the requirement of CRI.

However, when user create a container or pulling a image, they usually give us RepoDigest/RepoTag. So we have to do the RepoDigest/RepoTag -> Image ID (and other Information) conversion, which instead of creating another internal store, we are currently leveraging containerd image metadata store to do that. That's why we should put all RepoDigest/RepoTag into the metadata store.

As for why we also need to put image ID: I was told that containerd will do image gc based on reference, an entry in image metadata store is a reference to the corresponding snapshot and content. Other references are not unique enough to identify an image, e.g. repo tag could be reused by another image; schema 1 image doesn't have a repo digest etc.; image id seems to be the only thing we can use to reference the image and keep it from being garbage collected.

Random-Liu · 2017-08-15T16:54:48Z

pkg/server/container_create.go

 	}()

-	container, err := containerstore.NewContainer(meta, containerstore.Status{CreatedAt: time.Now().UnixNano()})
+	container, err := containerstore.NewContainer(meta,


nit: Can we follow this pattern?

That is the containerd/grpc pattern which seems more clean, and we don't need to update unit test everywhere.

Random-Liu · 2017-08-15T16:56:49Z

pkg/server/container_start.go

-		return fmt.Errorf("sandbox container %q is not running", sandboxID)
+	taskStatus, err := s.Status(ctx)
+	if err != nil {
+		return fmt.Errorf("faile to get task status for container %q : %v", id, err)


s/faile/failed

s/for container/for sandbox container.

Random-Liu · 2017-08-15T21:29:56Z

pkg/server/container_start.go

-	glog.V(5).Infof("Create containerd task (id=%q, name=%q) with options %+v.",
-		id, meta.Name, createOpts)
-	createResp, err := c.taskService.Create(ctx, createOpts)
+	task, err := container.NewTask(ctx, containerd.NewIO(os.Stdin, wStdoutPipe, wStderrPipe))


s/os.Stdin/new(bytes.Buffer).

And please add TODO to close container stdin if not needed.

Random-Liu · 2017-08-15T21:37:04Z

pkg/server/container_stop.go

+			if !errdefs.IsNotFound(err) {
+				return fmt.Errorf("failed to stop container, task not found for container %q: %v", id, err)
+			}
+		}


Use else?

Actually it's a bit troublesome that we have to load the task again whenever we want to use it. :p

Random-Liu · 2017-08-15T21:37:56Z

pkg/server/container_stop.go

-	if err != nil {
-		if !isContainerdGRPCNotFoundError(err) && !isRuncProcessAlreadyFinishedError(err) {
-			return fmt.Errorf("failed to kill container %q: %v", id, err)
+	if task != nil {


Random-Liu · 2017-08-15T22:29:47Z

pkg/server/sandbox_stop.go

-	}
+	taskStatus, err := task.Status(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to get task status for sandbox %q : %v", container.ID(), err)


Remove space before :.

Actually, now I prefer not to include id inside error message, unless the caller doesn't know that. Currently, error message is a bit messy. :p We need to gradually clean them up.

Random-Liu · 2017-08-15T22:31:01Z

pkg/server/sandbox_run.go

 		if retErr != nil {
 			// Cleanup the sandbox container if an error is returned.
-			if err := c.stopSandboxContainer(ctx, id); err != nil {
+			if err := c.stopSandboxContainer(ctx, container); err != nil {


Since you've got task here, just call task.Delete(ctx, WithProcessKill).

Random-Liu · 2017-08-15T22:31:39Z

pkg/server/sandbox_stop.go

-func (c *criContainerdService) stopSandboxContainer(ctx context.Context, id string) error {
-	cancellable, cancel := context.WithCancel(ctx)
-	eventstream, err := c.eventService.Subscribe(cancellable, &events.SubscribeRequest{})
+func (c *criContainerdService) stopSandboxContainer(ctx context.Context, container containerd.Container) error {


This function is not that useful now, maybe just embed into StopContainer.

Random-Liu · 2017-08-15T22:34:37Z

pkg/server/sandbox_status.go

+	if task != nil {
+		taskStatus, err := task.Status(ctx)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get task status for sandbox container %q : %v", id, err)


nit: space before :.

Random-Liu · 2017-08-15T22:42:54Z

pkg/server/image_pull.go

 	}
-	glog.V(4).Infof("Pulled image %q with image id %q, repo tag %q, repo digest %q", imageRef, imageID,
+	repoDigest, repoTag := getRepoDigestAndTag(namedRef, image.Target().Digest, image.Target().MediaType == containerdimages.MediaTypeDockerSchema1Manifest)
+	for _, r := range []string{repoTag, repoDigest} {


We also need to put image ID. :)

Thats already taken care of by the client ? https://github.com/containerd/containerd/blob/master/client.go#L252

@abhinandanpb The client only puts the image reference in.

There are several different image references:

Image ID: Image config digest.

Image Pullable Digest: Image manifest digest.

Image ChainID: Calculated from uncompressed image snapshot.

Image Name/Tag: Readable image name, e.g docker.io/library/busybox:latest, docker.io/library/nginx:latest etc.

This issue kubernetes/kubernetes#46255, and the OCI image spec document may be useful here. :)

Got it . Thanks for explaining. Is there anyway we could avoid storing these many-to-one reference copies in the store ? I mean any change we can do on the containerd side to resolve the image ? Not in the scope of this PR. But moving forward.

Random-Liu · 2017-08-15T22:48:27Z

pkg/server/image_pull.go

 		repoTag, repoDigest)
-
 	// Get image information.
 	chainID, size, config, err := c.getImageInfo(ctx, imageRef)


getImageInfo may not be needed, if Image.Size(), Image.RootFS() could provide enough information.

Random-Liu

Several final nits. LGTM overall.

Random-Liu · 2017-08-16T20:39:53Z

pkg/server/container_start.go

-		return fmt.Errorf("sandbox container %q is not running", sandboxID)
+	taskStatus, err := s.Status(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to get task status for sandbox container %q : %v", id, err)


nit: remove space before :.

Random-Liu · 2017-08-16T20:42:07Z

pkg/server/image_pull.go

+			return nil, fmt.Errorf("failed to update image reference %q: %v", r, err)
+		}
+	}
+	glog.V(4).Infof("Pulled image %q with image id %q, repo tag %q, repo digest %q", imageRef, image.Name(),


Log this after you get the image id, so that you could log the image id out.

Random-Liu · 2017-08-16T20:49:04Z

pkg/server/helpers.go

@@ -272,43 +244,45 @@ func normalizeImageRef(ref string) (reference.Named, error) {
 // getImageInfo returns image chainID, compressed size and oci config. Note that getImageInfo


and image id

Random-Liu · 2017-08-16T20:49:32Z

pkg/server/helpers.go

@@ -272,43 +244,45 @@ func normalizeImageRef(ref string) (reference.Named, error) {
 // getImageInfo returns image chainID, compressed size and oci config. Note that getImageInfo
 // assumes that the image has been pulled or it will return an error.
 func (c *criContainerdService) getImageInfo(ctx context.Context, ref string) (


nit: Return image id as imagedigest.Digest.

Random-Liu · 2017-08-16T20:53:28Z

pkg/server/image_pull.go

-	image := imagestore.Image{
+	imageID := desc.Digest.String()
+	if err := c.createImageReference(ctx, imageID, image.Target()); err != nil {
+		return nil, fmt.Errorf("failed to update image reference %q: %v", r, err)


Why r here?
Actually, we could get image id before updating repoDigest, repoTag, so that we could update them all together in a single loop.

abhi · 2017-08-16T21:22:09Z

@Random-Liu updated PR. Thanks for reviewing the changes multiple times :)

Random-Liu · 2017-08-16T21:33:22Z

@abhinandanpb LGTM, let's wait for the test result.

Random-Liu · 2017-08-16T21:35:27Z

pkg/server/helpers.go

 }

-// getImageInfo returns image chainID, compressed size and oci config. Note that getImageInfo
+// getImageInfo returns image chainID, compressed size, imageID and oci config. Note that getImageInfo


oci config and imageID.

To make it the same with the return order.

Random-Liu · 2017-08-16T21:35:42Z

pkg/server/image_pull.go

 		return nil, fmt.Errorf("failed to get image %q information: %v", imageRef, err)
 	}
+	imageID := id.String()
+	glog.V(4).Infof("Pulled image %q with image id %q, repo tag %q, repo digest %q", imageRef, imageID,


Please move this down after the following if clause.

This commit: 1) Replaces the usage of containerd GRPC APIs with the containerd client for all operations related to containerd. 2) Updated containerd to v1.0alpha4+ 3) Updated runc to v1.0.0 Signed-off-by: Abhinandan Prativadi <[email protected]>

abhi · 2017-08-16T21:45:12Z

Done.

Random-Liu · 2017-08-16T21:49:12Z

LGTM
Will merge after test passes.
@abhinandanpb Thanks a lot! :)

Random-Liu · 2017-08-16T21:50:05Z

Fixes #86.
Fixes #49.

Replacing containerd GRPC API with client

k8s-ci-robot added the cncf-cla: no label Aug 5, 2017

abhi force-pushed the client branch 3 times, most recently from 70fd4fb to 4e5f5d1 Compare August 7, 2017 05:08

abhi force-pushed the client branch 4 times, most recently from 3468950 to 0b3953c Compare August 9, 2017 06:22

k8s-ci-robot added cncf-cla: yes and removed cncf-cla: no labels Aug 9, 2017

containerd deleted a comment from k8s-ci-robot Aug 9, 2017

abhi changed the title ~~[WIP ]Replacing containerd GRPC API with client~~ Replacing containerd GRPC API with client Aug 9, 2017

abhi force-pushed the client branch from 0b3953c to eec551f Compare August 9, 2017 18:56

Random-Liu assigned Random-Liu and mikebrow Aug 9, 2017

abhi force-pushed the client branch 2 times, most recently from 542bed6 to d51d23c Compare August 9, 2017 20:16

Random-Liu reviewed Aug 9, 2017

View reviewed changes

abhi force-pushed the client branch 2 times, most recently from 896de47 to 35338e4 Compare August 9, 2017 23:22

Random-Liu reviewed Aug 10, 2017

View reviewed changes

Random-Liu mentioned this pull request Aug 10, 2017

Exposing helper functions to access internal image information containerd/containerd#1311

Merged

abhi force-pushed the client branch 4 times, most recently from 69a3d13 to 763742a Compare August 11, 2017 01:13

abhi force-pushed the client branch 6 times, most recently from ca7eac9 to 0bbdbfe Compare August 15, 2017 00:29

Random-Liu reviewed Aug 15, 2017

View reviewed changes

abhi force-pushed the client branch 3 times, most recently from 054746b to 73ec2fe Compare August 16, 2017 07:13

Random-Liu mentioned this pull request Aug 16, 2017

Add container attach #128

Merged

Random-Liu reviewed Aug 16, 2017

View reviewed changes

abhi force-pushed the client branch from 73ec2fe to 681c13e Compare August 16, 2017 21:20

Random-Liu reviewed Aug 16, 2017

View reviewed changes

Random-Liu added this to the v0.2.0 milestone Aug 16, 2017

Containerd client integration

32e0313

This commit: 1) Replaces the usage of containerd GRPC APIs with the containerd client for all operations related to containerd. 2) Updated containerd to v1.0alpha4+ 3) Updated runc to v1.0.0 Signed-off-by: Abhinandan Prativadi <[email protected]>

abhi force-pushed the client branch from 681c13e to 32e0313 Compare August 16, 2017 21:44

Random-Liu added the lgtm label Aug 16, 2017

Random-Liu mentioned this pull request Aug 16, 2017

Cleanup the image management code to use containerd client #136

Closed

ijc mentioned this pull request Aug 16, 2017

[WIP] kubernetes: use cri-containerd to use containerd as runtime linuxkit/linuxkit#2400

Closed

Random-Liu merged commit 1ae4ee8 into containerd:master Aug 16, 2017

This was referenced Aug 16, 2017

CRI-Containerd Missing Pieces #62

Closed

Don't need to check os: process already finished any more. #125

Closed

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#113 from abhinandanpb/client

6f1c57c

Replacing containerd GRPC API with client

		@@ -272,43 +244,45 @@ func normalizeImageRef(ref string) (reference.Named, error) {
		// getImageInfo returns image chainID, compressed size and oci config. Note that getImageInfo

Replacing containerd GRPC API with client #113

Replacing containerd GRPC API with client #113

Uh oh!

Conversation

abhi commented Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Random-Liu commented Aug 7, 2017

Uh oh!

Random-Liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhi Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhi Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

abhi commented Aug 5, 2017 •

edited

Loading

abhi Aug 9, 2017 •

edited

Loading

Random-Liu Aug 9, 2017 •

edited

Loading

abhi Aug 10, 2017 •

edited

Loading

Random-Liu Aug 10, 2017 •

edited

Loading

Random-Liu Aug 10, 2017 •

edited

Loading

Random-Liu Aug 10, 2017 •

edited

Loading