Implement containerd shim v2 API for Kata Containers by lifupan · Pull Request #572 · kata-containers/runtime

lifupan · 2018-08-10T05:32:49Z

This is the init code of containerd shim v2 for kata containers, and I had done the general
test with the containerd's client command ctr and cri's crictl, both of them worked well.

Fixes #485

katacontainersbot · 2018-08-10T05:48:49Z

PSS Measurement:
Qemu: 167881 KB
Proxy: 4183 KB
Shim: 8854 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2004008 KB

opendev-zuul · 2018-08-10T06:28:54Z

Build succeeded (third-party-check pipeline).

kata-runsh : SUCCESS in 55m 42s

katacontainersbot · 2018-08-10T07:09:15Z

PSS Measurement:
Qemu: 170027 KB
Proxy: 3984 KB
Shim: 8859 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003572 KB

There are maybe more than one containers in a sandbox, thus it's needed to cleanup all of those containers' mount and id mapping files when do cleanup a sandbox. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

opendev-zuul · 2018-08-10T07:54:10Z

Build succeeded (third-party-check pipeline).

kata-runsh : SUCCESS in 58m 07s

katacontainersbot · 2018-08-10T08:10:11Z

PSS Measurement:
Qemu: 167678 KB
Proxy: 4082 KB
Shim: 8758 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2002944 KB

There are maybe more than one containers in a sandbox, thus it's needed to cleanup all of those containers' mount and id mapping files when do cleanup a sandbox. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

katacontainersbot · 2018-08-10T08:40:30Z

PSS Measurement:
Qemu: 170260 KB
Proxy: 4120 KB
Shim: 8906 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003992 KB

opendev-zuul · 2018-08-10T09:02:33Z

Build succeeded (third-party-check pipeline).

kata-runsh : SUCCESS in 1h 00m 26s

Removed some unused variables and retuned some if/else code style. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

katacontainersbot · 2018-08-10T09:33:41Z

PSS Measurement:
Qemu: 169873 KB
Proxy: 4094 KB
Shim: 8905 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003860 KB

opendev-zuul · 2018-08-10T10:13:48Z

Build succeeded (third-party-check pipeline).

kata-runsh : SUCCESS in 57m 12s

jodh-intel · 2018-08-10T11:39:35Z

Hi @lifupan - thanks for raising! This is clearly a very large PR. I'm not sure I'll be able to review it today but will review on Monday if not.

sboeuf · 2018-08-10T14:13:01Z

@lifupan Thanks for submitting the implementation of Kata for containerd-shim-v2!
One thing that comes to mind, we need to introduce a bunch of integration tests for this.
You mentioned

I had done the general test with the containerd's client command ctr and cri's crictl, both of them worked well.

Could you please work with @chavafg and @GabyCT in order to introduce more testing to our CI, because I want to make sure we get no regression regarding your implementation once it gets merged.

lifupan · 2018-08-11T12:46:22Z

@sboeuf Sure, no problem.

Removed some unused variables and retuned some if/else code style, at the same time, do not export those functions and variables which will not used out this module. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

katacontainersbot · 2018-08-14T02:48:21Z

PSS Measurement:
Qemu: 167640 KB
Proxy: 4126 KB
Shim: 8784 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003620 KB

Removed some unused variables and retuned some if/else code style, at the same time, do not export those functions and variables which will not used out this module. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

katacontainersbot · 2018-08-14T03:31:46Z

PSS Measurement:
Qemu: 167620 KB
Proxy: 4010 KB
Shim: 9027 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003612 KB

Removed some unused variables and retuned some if/else code style, at the same time, do not export those functions and variables which will not used out this module. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

katacontainersbot · 2018-08-14T03:54:23Z

PSS Measurement:
Qemu: 167888 KB
Proxy: 4064 KB
Shim: 8840 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003876 KB

Removed some unused variables and retuned some if/else code style, at the same time, do not export those functions and variables which will not used out this module. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

opendev-zuul · 2018-08-14T04:27:11Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : RETRY_LIMIT in 5m 28s

lifupan · 2018-11-27T09:33:12Z

/test

sboeuf

@lifupan last round of review :)
I left a bunch of comments, and I would appreciate if you could address them. It shouldn't take long.
Thanks again for the hard work, and I'm looking forward to get this PR merged by the end of the week!!

sboeuf · 2018-11-27T21:31:53Z

containerd-shim-v2/create.go

+	// Since there is a bug in kata for sharedPidNs, here to
+	// remove the pidns to disable the sharePidNs temporarily,
+	// once kata fixed this issue, we can remove this line.
+	removeNameSpace(&ociSpec, specs.PIDNamespace)


Have you opened an issue for that, or is there an issue already opened?
Please refer the issue link and number from the comment here.

Nit: s/removeNameSpace/removeNamespace

sboeuf · 2018-11-27T21:34:11Z

containerd-shim-v2/service.go

+	s.Lock()
+	defer s.Unlock()
+
+	ns, err := namespaces.NamespaceRequired(ctx)


Thanks for the pointer!

sboeuf · 2018-11-27T21:42:19Z

containerd-shim-v2/service.go

+		}
+	}
+
+	container, err := create(ctx, s, r, ns, s.config)


So you explained to me that ns was a concept brought by containerd and is completely different from regular Linux namespaces. Why are you reusing this namespace string to be the network namespace path?

Sorry for the wrong explanation above. I had just dived into the containerd and found that here the "namespace" is the network namespace created by the containerd's CNI plugin, and that's why I passed it to kata's network ns path. Sorry for the confusion.

Oh okay then that's nice because CNI is handled in a very straightforward way by doing so, I like it!
However, could you change the naming of the variable from ns to netns to make sure it's very obvious that we retrieved the network namespace from containerd.

I had renamed it to netns and a line of comment.

sboeuf · 2018-11-27T21:49:35Z

containerd-shim-v2/create.go

+		}
+
+		if n.Path == "" {
+			n.Path = netns


Why are we replacing the network namespace path with the containerd namespace name?
When the path is empty, this means the caller expect the network namespace to be created by the container runtime, and this is done later by CreateSandbox().

The reason I had explained above for why replacing the network namespace with netns.

Since the "netns" is created by containerd's plugin, thus if containerd doesn't enabled any network plugin, then here the "netns" will be empty and yes it will be done later by CreateSandbox().

sboeuf · 2018-11-27T22:00:51Z

containerd-shim-v2/stream.go

+var (
+	bufPool = sync.Pool{
+		New: func() interface{} {
+			buffer := make([]byte, 32<<10)


Please makes this a constant, and explain why it is needed.

sboeuf · 2018-11-27T22:25:30Z

containerd-shim-v2/service.go

+	return &taskAPI.StateResponse{
+		ID:         execs.id,
+		Bundle:     c.bundle,
+		Pid:        PID,


sboeuf · 2018-11-27T22:33:35Z

containerd-shim-v2/service.go

+
+	//wait for container
+	if r.ExecID == "" {
+		ret = <-c.exitCh


I think here it would be very nice to add s.sandbox.StopContainer(). We want to make sure the container is completely stopped inside the VM. The container process might terminate because it properly returned, and we want to ensure there's nothing left inside the VM.

Actually it doesn't need. Once a container process exited, here it will public a TaskExitEventTopic to containerd, When containerd received this event, it will request a
Delete() service to this shimv2. The shimv2 will do the container stop and clean in this service.

Ah yes that's cool! Compared to the madness that we have to handle with our OCI CLI implementation, this is a real improvement here.
I was just worried that if the call from containerd was not coming right away, we would want to stop our container as we know the process exited.
I guess if this is needed in the future, we could add it later, right?

Yes, if it's needed in the future I'll add it.

Sounds good, thanks.

sboeuf · 2018-11-27T22:40:03Z

containerd-shim-v2/service.go

@@ -350,7 +350,46 @@ func (s *service) ResizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*

 // State returns runtime state information for a process
 func (s *service) State(ctx context.Context, r *taskAPI.StateRequest) (*taskAPI.StateResponse, error) {


Just noticing the way this function is implemented brings duplication additionally to the states of the Kata API. Instead, it maintains its own states.
@bergwolf @lifupan WDYT?

I thinks this states is needed. First the containerd requested states isn't entirely match to kata's states. there must be a transfer between them and this states does it; Second, there is some states value kata doesn't keep thus shimv2 needs to store it in this states.

Yes I realized that, and I saw that you tried to translate the Kata states when possible, but I understand containerd expects more states than what we have with Kata. I'm not asking to change anything, it's just sad that we have to redefine another layer of states again...

sboeuf · 2018-11-27T22:44:10Z

containerd-shim-v2/service.go

+	}
+
+	if signum == syscall.SIGTERM {
+		err = s.sandbox.SignalProcess(c.id, processID, syscall.SIGKILL, r.All)


Why are we sending a SIGKILL here? The signal is a SIGTERM and has already been sent.
If there's a good explanation for this, please add a comment in the code explaining this.

Since kubernetes will use SIGTERM signal to stop a container by default, but some container process such as "shell" will not response to this signal, thus container stop will always get timeout response, in order to terminate the container successfully, here send another SIGKILL to make sure the process will be terminated successfully. I'll add it to comments.

Well I understand but I would expect the CRI layer (CRI-containerd or CRI-O) to actually send a SIGKILL themselves if they realize that SIGTERM didn't work. Is it something that we want to handle at this level?
/cc @bergwolf ^

I think we should follow runc's practice and only send SIGTERM. If the container process chooses to ignore SIGTERM, the upper layer should send other signals after timeout. @lifupan do you see any test cases that runc passes but shimv2 fails due to SIGTERM timeout?

@bergwolf exactly, the upper layer knows about the workload and should be the one that knows if SIGTERM is appropriate or not.

@lifupan let's not duplicate some fix that we had to use with kata CLI since they might not be needed in this case. Do you have a concrete test case that will actually fail if you don't send a SIGKILL after a SIGTERM ?

@sboeuf Yes, once I started a container running a cmd in shell, the stop container request always timeout and wait for a lone time to stop it.

Yes, IIRC the interactive shell ignores SIGTERM, I think it would respond to the SIGTERM if you run without -it.

Fair enough, it's not ideal, but we'll fix that later once we understand better the issue. I don't want to delay this PR because of this detail.

Thanks @sboeuf @jodh-intel @WeiZhang555 @bergwolf @egernst. Thanks for your patient and careful help and review.

sboeuf · 2018-11-27T22:45:30Z

containerd-shim-v2/metrics.go

+)
+
+func marshalMetrics(s *service, containerID string) (*google_protobuf.Any, error) {
+	stats, err := s.sandbox.StatsContainer(containerID)


That's not the only reason why containerd-shim-v2 is very cool, but one of the main ones :)

Add the "Create" api support for creating a pod or container. Signed-off-by: fupan <lifupan@gmail.com>

Add the Start api support of start a pod or container created before. Signed-off-by: fupan <lifupan@gmail.com>

Add the Exec api support for exec an process in a running container. Signed-off-by: fupan <lifupan@gmail.com>

Add the Wait api to wait on a started container or exec process. Signed-off-by: fupan <lifupan@gmail.com>

Add the State api support to get a container or exec process's states. Signed-off-by: fupan <lifupan@gmail.com>

Add the Delete api support to delete a stopped container or pod. Signed-off-by: fupan <lifupan@gmail.com>

Add the Cleanup api support to cleanup the pod and containers running in it when the pod's corresponding shim died. Signed-off-by: fupan <lifupan@gmail.com>

Add the Pids api support to get the processes pids running in the pod. Signed-off-by: fupan <lifupan@gmail.com>

Add the CloseIO api support to close a process's input stream. Signed-off-by: fupan <lifupan@gmail.com>

Add the Connect api to get the shim's info. Signed-off-by: fupan <lifupan@gmail.com>

Add the Shutdown api support to shutdown the shim. Signed-off-by: fupan <lifupan@gmail.com>

Add the Update api support to update a running process's resouce. Signed-off-by: fupan <lifupan@gmail.com>

Add the ResizePty api support to resize the console. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

Add the Pause api support to pause a container running in the pod. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

Add the Resume api support to resume a paused container. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

Add the Kill api support to send signal to a given container process. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn> Signed-off-by: fupan.li <lifupan@gmail.com>

Add the Stats api support to get the container's resouces statistic. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

Add unit test cases. Signed-off-by: fupan <lifupan@gmail.com>

Add the Makefile target of building shimv2. Fixes: kata-containers#485 Signed-off-by: fupan <lifupan@gmail.com>

lifupan · 2018-11-28T09:04:53Z

/test

sboeuf

Here we go!!! Let merge this PR :)

gnawux · 2018-11-28T16:54:22Z

Finally merged,
Thanks all the reviewers.

Time to have a beer @lifupan

lifupan force-pushed the shimv2 branch from 29aa8bf to 76ffe80 Compare August 10, 2018 06:55

lifupan force-pushed the shimv2 branch from 8ac4f5f to 7bb73a9 Compare August 10, 2018 07:54

lifupan force-pushed the shimv2 branch from 7bb73a9 to 8fdf25e Compare August 10, 2018 08:24

lifupan added a commit to hyperhq/kata-runtime that referenced this pull request Aug 10, 2018

containerd-shim-kata-v2: retune the code style

3e86062

Removed some unused variables and retuned some if/else code style. Fixes: kata-containers#572 Signed-off-by: fupan <lifupan@gmail.com>

lifupan force-pushed the shimv2 branch from 3e86062 to d97d32f Compare August 14, 2018 02:36

lifupan force-pushed the shimv2 branch from d97d32f to c637940 Compare August 14, 2018 03:19

lifupan force-pushed the shimv2 branch from c637940 to 9a92620 Compare August 14, 2018 03:42

lifupan force-pushed the shimv2 branch from 9a92620 to 9e597ee Compare August 14, 2018 04:25

sboeuf reviewed Nov 27, 2018

View reviewed changes

lifupan and others added 19 commits November 28, 2018 14:29

containerd-shim-kata-v2: add the create service support

72fd6e0

Add the "Create" api support for creating a pod or container. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the start service support

4c5b296

Add the Start api support of start a pod or container created before. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the exec service support

269c940

Add the Exec api support for exec an process in a running container. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service wait support

fbaefc9

Add the Wait api to wait on a started container or exec process. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service State support

fd18b22

Add the State api support to get a container or exec process's states. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service Delete support

a0e6456

Add the Delete api support to delete a stopped container or pod. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service Cleanup support

709bc9a

Add the Cleanup api support to cleanup the pod and containers running in it when the pod's corresponding shim died. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service Pids support

8c95b75

Add the Pids api support to get the processes pids running in the pod. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service CloseIO support

ec4f27b

Add the CloseIO api support to close a process's input stream. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service Connect support

87f591a

Add the Connect api to get the shim's info. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service Shutdown support

642231b

Add the Shutdown api support to shutdown the shim. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service Update support

47326f5

Add the Update api support to update a running process's resouce. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add the service ResizePty support

cd321a3

Add the ResizePty api support to resize the console. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

containerd-shim-kata-v2: add the service Pause support

8df33d3

Add the Pause api support to pause a container running in the pod. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

containerd-shim-kata-v2: add the service Resume support

9ee53be

Add the Resume api support to resume a paused container. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

containerd-shim-kata-v2: add the service Kill support

5cc016c

Add the Kill api support to send signal to a given container process. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn> Signed-off-by: fupan.li <lifupan@gmail.com>

containerd-shim-kata-v2: add the service Stats support

7951041

Add the Stats api support to get the container's resouces statistic. Signed-off-by: ZeroMagic <anthonyliu@zju.edu.cn>

containerd-shim-kata: add unit test cases

8199d10

Add unit test cases. Signed-off-by: fupan <lifupan@gmail.com>

containerd-shim-kata-v2: add building of shimv2 into Makefile

02f8b29

Add the Makefile target of building shimv2. Fixes: kata-containers#485 Signed-off-by: fupan <lifupan@gmail.com>

sboeuf approved these changes Nov 28, 2018

View reviewed changes

gnawux mentioned this pull request Dec 3, 2018

deprecation of devicemapper: need path forward for block devices + Kata #943

Closed

gnawux mentioned this pull request Dec 20, 2018

Document the usage of containerd-shimv2-kata kata-containers/documentation#322

Closed

liangxianlong mentioned this pull request Jan 10, 2019

some problems when use "docker stats" with kata-runtime moby/moby#38329

Closed

2 tasks

BetaXOi mentioned this pull request Jan 14, 2019

Any plans to support containerd shim v2? moby/moby#38559

Closed

BetaXOi mentioned this pull request Jan 24, 2019

Creating container DO NOT need runtime anymore in kata-shim-v2? #1170

Closed

gnawux mentioned this pull request Nov 17, 2021

Propose a Shim in Rust (and one more thing) kata-containers/kata-containers#3052

Closed

		@@ -350,7 +350,46 @@ func (s service) ResizePty(ctx context.Context, r taskAPI.ResizePtyRequest) (*

		// State returns runtime state information for a process
		func (s service) State(ctx context.Context, r taskAPI.StateRequest) (*taskAPI.StateResponse, error) {

Conversation

lifupan commented Aug 10, 2018 • edited by gnawux Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katacontainersbot commented Aug 10, 2018

Uh oh!

opendev-zuul bot commented Aug 10, 2018

Uh oh!

katacontainersbot commented Aug 10, 2018

Uh oh!

opendev-zuul bot commented Aug 10, 2018

Uh oh!

katacontainersbot commented Aug 10, 2018

Uh oh!

katacontainersbot commented Aug 10, 2018

Uh oh!

opendev-zuul bot commented Aug 10, 2018

Uh oh!

katacontainersbot commented Aug 10, 2018

Uh oh!

opendev-zuul bot commented Aug 10, 2018

Uh oh!

jodh-intel commented Aug 10, 2018

Uh oh!

sboeuf commented Aug 10, 2018

Uh oh!

lifupan commented Aug 11, 2018

Uh oh!

katacontainersbot commented Aug 14, 2018

Uh oh!

katacontainersbot commented Aug 14, 2018

Uh oh!

katacontainersbot commented Aug 14, 2018

Uh oh!

opendev-zuul bot commented Aug 14, 2018

Uh oh!

lifupan commented Nov 27, 2018

Uh oh!

sboeuf 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!

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!

Choose a reason for hiding this comment

Uh oh!

sboeuf Nov 28, 2018 • 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

lifupan commented Aug 10, 2018 •

edited by gnawux

Loading

sboeuf Nov 28, 2018 •

edited

Loading