Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Switch to new containerd api #78

Merged
Random-Liu merged 7 commits into from
Jun 16, 2017
Merged

Switch to new containerd api #78

Random-Liu merged 7 commits into from
Jun 16, 2017

Conversation

@Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Jun 16, 2017 •
edited
Loading

Based on #68.

Update containerd to 4ae34cccc5b496c6547ff28dbeed1bde4773fa7a, and integrate with new containerd API.

Note that this PR doesn't use containerd client. We'll switch to containerd client later.

@Random-Liu Random-Liu added this to the v0.1.0-alpha.1 milestone Jun 16, 2017
@Random-Liu
Copy link
Member Author

Random-Liu commented Jun 16, 2017

I've tested this with #72, #77. The test result is not changed:

Summarizing 6 Failures:

[Fail] [k8s.io] Networking runtime should support networking [It] runtime should support port mapping with host port and container port [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/networking.go:206

[Fail] [k8s.io] Security Context runtime should support container with security context [It] runtime should support RunAsUser [security context] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/container.go:326

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support attach [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:183

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support exec [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:127

[Fail] [k8s.io] Security Context runtime should support container with security context [It] runtime should support RunAsUserName [security context] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/container.go:326

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:231

Ran 36 of 36 Specs in 91.309 seconds
FAIL! -- 30 Passed | 6 Failed | 0 Pending | 0 Skipped --- FAIL: TestE2ECRI (91.31s)
FAIL

Ginkgo ran 1 suite in 1m31.382036903s
Test Suite Failed
exit status 1

@kubernetes-incubator/maintainers-cri-containerd

@Random-Liu
Copy link
Member Author

Random-Liu commented Jun 16, 2017 •
edited
Loading

@mikebrow Let's try to get this merge soon, so as to unblock your client change.

I'll rebase other PRs after this one is merged.

@Random-Liu Random-Liu force-pushed the switch-to-new-containerd-client branch from 2e25466 to 6ee7269 Compare June 16, 2017 02:34
mikebrow
mikebrow reviewed Jun 16, 2017
View reviewed changes
pkg/server/container_start.go
sandboxID := meta.SandboxID
// Make sure sandbox is running.
sandboxInfo, err := c.containerService.Info(ctx, &execution.InfoRequest{ContainerID: sandboxID})
sandboxInfo, err := c.taskService.Info(ctx, &execution.InfoRequest{ContainerID: sandboxID})
Copy link
Member

@mikebrow mikebrow Jun 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah wasn't sure how far we wanted to push the use of the term "task" as a replacement for "container" given that task isn't in CRI/Kube and really is just a new naming convention for docker/containerd. So I only changed it where it "had" to change.

Copy link
Member Author

@Random-Liu Random-Liu Jun 16, 2017

Choose a reason for hiding this comment

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

Because containerd also has container notion, which is the metadata + task of the container, it may cause some naming conflict or confusion if we don't follow the same pattern. :)

That's fine, we'll have a separate PR to do the cleanup.

mikebrow
mikebrow reviewed Jun 16, 2017
View reviewed changes
pkg/server/container_create.go Outdated
}

// Generate container runtime spec.
mounts := c.generateContainerMounts(getSandboxRootDir(c.rootDir, sandbox.ID), config)
Copy link
Member

@mikebrow mikebrow Jun 16, 2017

Choose a reason for hiding this comment

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

Not sure why the cached sandboxID was created.. but it's not being used consistently here.

Copy link
Member Author

@Random-Liu Random-Liu Jun 16, 2017

Choose a reason for hiding this comment

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

Good catch, will fix.

Copy link
Member Author

@Random-Liu Random-Liu Jun 16, 2017

Choose a reason for hiding this comment

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

Done.

mikebrow
mikebrow reviewed Jun 16, 2017
View reviewed changes
pkg/server/container_create.go
return &runtime.CreateContainerResponse{ContainerId: id}, nil
}

func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint32, config *runtime.ContainerConfig,
Copy link
Member

@mikebrow mikebrow Jun 16, 2017

Choose a reason for hiding this comment

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

Makes sense that this code needed to move from start to create given the containerd changes... but it makes it hard to review :-)

Copy link
Member Author

@Random-Liu Random-Liu Jun 16, 2017

Choose a reason for hiding this comment

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

Yeah, pure code move. No change except the unit test.

mikebrow
mikebrow approved these changes Jun 16, 2017
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

No issues found. Well done! See one nit... but otherwise let's go ahead and merge this so we can move forward.

LGTM

@Random-Liu Random-Liu force-pushed the switch-to-new-containerd-client branch from 6ee7269 to d643599 Compare June 16, 2017 16:43
@Random-Liu Random-Liu changed the title Switch to new containerd client Switch to new containerd api Jun 16, 2017
@Random-Liu Random-Liu mentioned this pull request Jun 16, 2017
Closed
@Random-Liu
Copy link
Member Author

Random-Liu commented Jun 16, 2017

Apply LGTM based on #78 (review).

Thanks for reviewing and also thanks for the initial commits!

@Random-Liu Random-Liu merged commit cc43c86 into containerd:master Jun 16, 2017
@Random-Liu Random-Liu deleted the switch-to-new-containerd-client branch June 16, 2017 16:56
@mikebrow mikebrow mentioned this pull request Jun 16, 2017
Closed
lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017
@Random-Liu
Merge pull request containerd#78 from Random-Liu/switch-to-new-contai…
8937abd 
…nerd-client

Switch to new containerd api
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mikebrow mikebrow mikebrow approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@yujuhong yujuhong

@mikebrow mikebrow

Labels

cncf-cla: yes lgtm

Projects

None yet

Milestone

v0.1.0-alpha.1

Development

Successfully merging this pull request may close these issues.

4 participants

@Random-Liu @mikebrow @yujuhong @k8s-ci-robot