Annotations: Provide container metadata for VM based runtimes #546

jcvenegas · 2018-01-16T17:35:02Z

For hypervisor-based container runtimes (like Kata Containers, Clear Containers
or runv) a pod will be created in a VM and then create containers within the VM.

When a runtime is requested for container commands like create and start, both
the instal "pause" container and next containers need to be added to the pod
namespace (same VM).

A runtime does not know if it needs to create/start a VM or if it needs to add a
container to an already running VM pod.

This patch adds a way to provide this information through container annotations.
When starting a container or a sandbox, 2 annotations are added:

type (Container or Sandbox)
sandbox name

This allow to a VM based runtime to decide if they need to create a pod VM or
container within the VM pod.

Signed-off-by: Jose Carlos Venegas Munoz [email protected]

abhi · 2018-01-16T17:54:28Z

@jcvenegas thank you for submitting the PR. Aren't these already available as labels and metadata ? Any reason to be passed as annotations ?

abhi · 2018-01-16T17:55:05Z

for e.g.:

"Labels": {
        "io.cri-containerd.kind": "container",
        "io.kubernetes.container.name": "kube-controller-manager",
        "io.kubernetes.pod.name": "kube-controller-manager-abhi-k8s-ubuntu-0",
        "io.kubernetes.pod.namespace": "kube-system",
        "io.kubernetes.pod.uid": "1985db349c24e16ece3f84cce82a7183"
    },

Random-Liu · 2018-01-16T18:01:33Z

/ok-to-test

Random-Liu · 2018-01-16T18:02:11Z

@abhi I think they need runc annotation instead of containerd label.

mikebrow

Looking good.. just a nit on the label names.

mikebrow · 2018-01-16T18:05:03Z

pkg/annotations/annotations.go

+	ContainerTypeContainer = "container"
+
+	// ContainerType is the container type (sandbox or container) annotation
+	ContainerType = "io.kubernetes.cri-containerd.ContainerType"


How about cri.containerType, cri.sandboxID, cri.sandboxName.
Might want to substitute sandbox for pod, since these labels might get printed out and most folks won't know that sandbox is the internal name for pod...

or may be io.kubernetes.containerd.ContainerType and so on below ?

@jcvenegas Same question here. Are we going to have different annotation key for cri-o, cri-containerd or dockershim? Shouldn't that be a standard name? Will katacontainers be looking at different annotations from different container runtimes?

@mikebrow @abhi : "cri.containerType" sounds good to me. It is a more agnostic cri implementation.
About podID or sanboxID. What about PodSandbox tight to cri api definitions and includes both pod and sandbox to avoid confusions.

Today kata is looking for different annotations. I like the idea to go beyond and take a first step towards standardization.

sameo · 2018-01-16T18:18:50Z

@abhi We need to get those as OCI annotations.

sameo · 2018-01-16T18:22:05Z

pkg/annotations/annotations.go

@@ -0,0 +1,19 @@
+package annotations


@jcvenegas You need to add the Kubernetes boilerplate to this file.

/* Copyright 2018 The Containerd Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */

Thank you, added.

Random-Liu · 2018-01-16T18:35:04Z

pkg/annotations/annotations.go

+	ContainerTypeContainer = "container"
+
+	// ContainerType is the container type (sandbox or container) annotation
+	ContainerType = "io.kubernetes.cri-containerd.ContainerType"


@jcvenegas Same question here. Are we going to have different annotation key for cri-o, cri-containerd or dockershim? Shouldn't that be a standard name? Will katacontainers be looking at different annotations from different container runtimes?

Random-Liu · 2018-01-16T18:58:34Z

pkg/server/container_create.go

@@ -147,6 +148,9 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate container %q spec: %v", id, err)
 	}
+	spec.Annotations[annotations.SandboxID] = sandboxID
+	spec.Annotations[annotations.SandboxName] = name


Can we do this in generateContainerSpec?

name is the name of the container, right? Not the sandbox name. What name do you need here? From the annotation you applied on sandbox, it seems that you require the pod name?

That is true this is pod name, I will provide the sandbox struct to generateContainerSpec and populate with the right value.

Random-Liu · 2018-01-16T19:00:22Z

pkg/server/sandbox_run.go

@@ -331,6 +332,10 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r
 	g.SetLinuxResourcesCPUShares(uint64(defaultSandboxCPUshares))
 	g.SetProcessOOMScoreAdj(int(defaultSandboxOOMAdj))

+	g.AddAnnotation(annotations.ContainerType, annotations.ContainerTypeSandbox)
+	g.AddAnnotation(annotations.SandboxID, id)
+	g.AddAnnotation(annotations.SandboxName, config.Metadata.Name)


ditto. It's not very clear to me what kind of sandbox name do you need.

jcvenegas · 2018-01-16T21:27:00Z

pkg/annotations/annotations.go

+	ContainerTypeContainer = "container"
+
+	// ContainerType is the container type (sandbox or container) annotation
+	ContainerType = "io.kubernetes.cri.ContainerType"


@mikebrow changed namespace annotations to io.kubernetes.cri

Can we use io.kubernetes.cri.container-type or io.kubernetes.cri.containertype.

The Kubernetes naming convention https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/identifiers.md

Basically for label key, we want it to be DNS subdomain.

suggest to correct this.

jcvenegas · 2018-01-16T21:28:24Z

pkg/server/container_create.go

@@ -365,6 +367,9 @@ func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint3
 		g.AddProcessAdditionalGid(uint32(group))
 	}

+	g.AddAnnotation(annotations.ContainerType, annotations.ContainerTypeContainer)
+	g.AddAnnotation(annotations.SandboxID, sandboxID)


@Random-Liu moved annotation to generateContainerSpec.

abhi · 2018-01-16T21:42:07Z

@jcvenegas there is lint failure.

I0116 21:34:34.878] pkg/server/container_create_test.go:302:120:warning: unused variable or constant too few arguments in call to c.generateContainerSpec (varcheck)
I0116 21:34:34.879] pkg/server/container_create_test.go:705:46:warning: unused variable or constant cannot use testPid (variable of type uint32) as string value in argument to c.generateContainerSpec (varcheck)
I0116 21:34:34.880] pkg/server/container_create_test.go:705:55:warning: unused variable or constant cannot use config (variable of type *github.com/containerd/cri-containerd/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime.ContainerConfig) as uint32 value in argument to c.generateContainerSpec (varcheck)
I0116 21:34:34.882] pkg/server/container_create_test.go:705:63:warning: unused variable or constant cannot use sandboxConfig (variable of type *github.com/containerd/cri-containerd/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime.PodSandboxConfig) as *github.com/containerd/cri-containerd/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime.ContainerConfig value in argument to c.generateContainerSpec (varcheck)
I0116 21:34:34.883] pkg/server/container_create_test.go:705:78:warning: unused variable or constant cannot use imageConfig (variable of type *github.com/containerd/cri-containerd/vendor/github.com/opencontainers/image-spec/specs-go/v1.ImageConfig) as *github.com/containerd/cri-containerd/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime.PodSandboxConfig value in argument to c.generateContainerSpec (varcheck)
I0116 21:34:34.884] pkg/server/container_create_test.go:705:94:warning: unused variable or constant too few arguments in call to c.generateContainerSpec (varcheck)
I0116 21:34:37.397] Makefile:67: recipe for target 'lint' failed

you can run make verify locally

abhi

LGTM

mikebrow

/LGTM

Random-Liu · 2018-01-17T09:50:01Z

pkg/annotations/annotations.go

+	ContainerTypeContainer = "container"
+
+	// ContainerType is the container type (sandbox or container) annotation
+	ContainerType = "io.kubernetes.cri.ContainerType"


Can we use io.kubernetes.cri.container-type or io.kubernetes.cri.containertype.

The Kubernetes naming convention https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/identifiers.md

Basically for label key, we want it to be DNS subdomain.

Random-Liu · 2018-01-17T09:50:10Z

pkg/annotations/annotations.go

+	ContainerType = "io.kubernetes.cri.ContainerType"
+
+	// SandboxID is the sandbox ID annotation
+	SandboxID = "io.kubernetes.cri.SandboxID"


Random-Liu · 2018-01-17T09:50:19Z

pkg/server/container_create.go

@@ -365,6 +367,9 @@ func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint3
 		g.AddProcessAdditionalGid(uint32(group))
 	}

+	g.AddAnnotation(annotations.ContainerType, annotations.ContainerTypeContainer)
+	g.AddAnnotation(annotations.SandboxID, sandboxID)


Random-Liu · 2018-01-17T09:51:10Z

pkg/server/container_create_test.go

@@ -177,13 +177,15 @@ func TestGeneralContainerSpec(t *testing.T) {
 	testPid := uint32(1234)
 	config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
 	c := newTestCRIContainerdService()
-	spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig, nil)
+	testSandboxID := "SandboxID"


You may want to check whether the annotations are set properly in default specCheck. However, if you don't have to do that, I can send a following up PR to do that. :)

yeah agree - not worries, adding proper checks to specCheck

sameo · 2018-01-17T13:37:27Z

cc @miaoyq @guangxuli @xiangpengzhao @yanxuean

guangxuli · 2018-01-17T14:45:27Z

@sameo SGTM, cool work. BTW, is this a first step cri-containerd support cc/kata runtime?

For hypervisor-based container runtimes (like Kata Containers, Clear Containers or runv) a pod will be created in a VM and then create containers within the VM. When a runtime is requested for container commands like create and start, both the instal "pause" container and next containers need to be added to the pod namespace (same VM). A runtime does not know if it needs to create/start a VM or if it needs to add a container to an already running VM pod. This patch adds a way to provide this information through container annotations. When starting a container or a sandbox, 2 annotations are added: - type (Container or Sandbox) - sandbox name This allow to a VM based runtime to decide if they need to create a pod VM or container within the VM pod. Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>

jcvenegas · 2018-01-17T16:00:52Z

@guangxuli that is right this is the brings initial support to allow kata and other hypervisor based runtime containers work with cri-containerd :)

jcvenegas · 2018-01-17T16:03:54Z

pkg/annotations/annotations.go

+	ContainerTypeContainer = "container"
+
+	// ContainerType is the container type (sandbox or container) annotation
+	ContainerType = "io.kubernetes.cri.container-type"


@Random-Liu @yanxuean updated annotations values according to the Kubernetes naming convention.

Random-Liu · 2018-01-17T18:24:47Z

@jcvenegas Thanks for contributing this! :D

k8s-ci-robot added size/M needs-ok-to-test labels Jan 16, 2018

k8s-ci-robot removed the needs-ok-to-test label Jan 16, 2018

mikebrow reviewed Jan 16, 2018

View reviewed changes

sameo reviewed Jan 16, 2018

View reviewed changes

jcvenegas force-pushed the pod-annotations branch from 5061be3 to bff9131 Compare January 16, 2018 18:35

Random-Liu reviewed Jan 16, 2018

View reviewed changes

jcvenegas force-pushed the pod-annotations branch from bff9131 to 072b1ed Compare January 16, 2018 21:25

jcvenegas commented Jan 16, 2018

View reviewed changes

jcvenegas force-pushed the pod-annotations branch from 072b1ed to dc52d81 Compare January 16, 2018 21:33

jcvenegas force-pushed the pod-annotations branch from dc52d81 to f051cb5 Compare January 16, 2018 22:19

abhi approved these changes Jan 16, 2018

View reviewed changes

k8s-ci-robot assigned mikebrow Jan 17, 2018

mikebrow reviewed Jan 17, 2018

View reviewed changes

k8s-ci-robot added the lgtm label Jan 17, 2018

Random-Liu reviewed Jan 17, 2018

View reviewed changes

Random-Liu self-assigned this Jan 17, 2018

jcvenegas force-pushed the pod-annotations branch from f051cb5 to b383b02 Compare January 17, 2018 15:57

jcvenegas commented Jan 17, 2018

View reviewed changes

Random-Liu merged commit 57323b2 into containerd:master Jan 17, 2018

Random-Liu mentioned this pull request Jan 18, 2018

Minor code cleanup. #554

Merged

YaoZengzeng mentioned this pull request Jun 25, 2018

[feature request] make runtime advantages support in CRI managers for Kubernetes AliyunContainerService/pouch#1580

Closed

Annotations: Provide container metadata for VM based runtimes #546

Annotations: Provide container metadata for VM based runtimes #546

Conversation

jcvenegas commented Jan 16, 2018

abhi commented Jan 16, 2018 • edited Loading

abhi commented Jan 16, 2018

Random-Liu commented Jan 16, 2018

Random-Liu commented Jan 16, 2018

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameo commented Jan 16, 2018

Choose a reason for hiding this comment

mikebrow Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Jan 16, 2018 • edited Loading

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

Random-Liu Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcvenegas Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhi commented Jan 16, 2018

abhi left a comment

Choose a reason for hiding this comment

mikebrow left a comment

Choose a reason for hiding this comment

Random-Liu Jan 17, 2018 • edited Loading

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

sameo commented Jan 17, 2018

guangxuli commented Jan 17, 2018

jcvenegas commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Random-Liu commented Jan 17, 2018

abhi commented Jan 16, 2018 •

edited

Loading

Random-Liu Jan 16, 2018 •

edited

Loading

mikebrow Jan 16, 2018 •

edited

Loading

Random-Liu Jan 16, 2018 •

edited

Loading

Random-Liu Jan 17, 2018 •

edited

Loading

jcvenegas Jan 16, 2018 •

edited

Loading

Random-Liu Jan 17, 2018 •

edited

Loading

jcvenegas commented Jan 17, 2018 •

edited

Loading