Put pod full name, namespace and uid into docker label #16414

Random-Liu · 2015-10-28T08:14:25Z

This PR mainly did 3 things:

Add file container_labels.go, and move label and filter functions into it, so as to simplify the label related code and prepare for adding more labels in the future.
Add pod full name filter in GetPodStatus() so that ListContainers() will only list containers belong to the target pod.
Add related test case and make fakeDockerContainer support label filter.

Some questions or TODOs:

A lot of manually initialized containers in old test cases of dockertools don't have docker labels, if they use functions using label filter (current now only GetPodStatus() uses label filter), the test case may get unexpected result. I added a simple function to set default labels, but it is still inconvenient to add label for each test case. In this PR I just modified test cases which went wrong. Should I add label for each test case? Or should I think a better way?
In fact, pod full name label was added before https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockertools/manager.go#L701. Can I assume that even old containers have pod name label? If not, I should comment my filter code and uncomment it in the future.

(For issue #15089)

k8s-github-robot · 2015-10-28T08:20:13Z

Labelling this PR as size/L

k8s-bot · 2015-10-28T08:41:08Z

GCE e2e test build/test passed for commit 998413c4ccc83748273b4f5fbfda06e71c30193f.

k8s-bot · 2015-10-28T09:21:08Z

GCE e2e test build/test passed for commit 99b9207798f0876c64b8ba198fad8e3d5a224d8c.

ncdc · 2015-10-28T13:40:44Z

@kubernetes/rh-cluster-infra @mfojtik @smarterclayton

k8s-bot · 2015-10-28T18:30:10Z

GCE e2e test build/test passed for commit 99b9207798f0876c64b8ba198fad8e3d5a224d8c.

ashcrow · 2015-10-28T18:32:51Z

Shippable failure is likely a flake:

FAIL    k8s.io/kubernetes/pkg/storage/etcd  14.651s

Random-Liu · 2015-10-28T18:39:44Z

Yeah, maybe. I passed the test locally.

k8s-bot · 2015-10-28T18:47:22Z

GCE e2e test build/test passed for commit 99b9207798f0876c64b8ba198fad8e3d5a224d8c.

k8s-bot · 2015-10-28T19:02:17Z

GCE e2e test build/test passed for commit 99b9207798f0876c64b8ba198fad8e3d5a224d8c.

Random-Liu · 2015-10-28T20:21:00Z

@ashcrow Thanks! :)

k8s-bot · 2015-10-28T21:08:30Z

GCE e2e test build/test passed for commit f22dbc26286164356e191d48527a9e87ea134112.

k8s-bot · 2015-10-29T07:32:05Z

GCE e2e test build/test passed for commit f22dbc26286164356e191d48527a9e87ea134112.

yujuhong · 2015-10-29T17:44:51Z

pkg/kubelet/dockertools/docker.go

@@ -211,6 +211,7 @@ func (p throttledDockerPuller) IsImagePresent(name string) (bool, error) {
 	return p.puller.IsImagePresent(name)
 }

+// TODO (random-liu) Almost never used, should we remove this?


This is used by GetKubeletDockerContainers. Do you want to rewrite the function?

Ha, what I mean is that we defined the the type "DockerContainers", and defined the function "FindPodContainer". But it seems that we just use "DockerContainers" as slice of APIcontainers (https://github.com/kubernetes/kubernetes/search?utf8=%E2%9C%93&q=GetKubeletDockerContainers&type=Code), never use function like FindPodContainer. So I just wonder whether we should remove this type, :)

Just curious when reading the code, so add a comment here. :)

I believe it was used in other places but we gradually migrated off. Personally I'd be happy to see the type be removed. Feel free to send a PR for this :)

OK. :) Thanks! Haha~

yujuhong · 2015-10-29T17:50:32Z

@Random-Liu, could you remove the "label filtering" code from this PR? I think this PR should focus on writing the labels :-)

vishh · 2015-10-29T17:53:56Z

pkg/kubelet/dockertools/container_labels.go

+	kubernetesPodNameLabel      = "io.kubernetes.pod.name"
+	kubernetesPodNamespaceLabel = "io.kubernetes.pod.namespace"
+
+	kubernetesPodLabel                    = "io.kubernetes.pod.data"


What is this meant for? It is not clear from the variable name. Can you add comments?

@vishh, @Random-Liu didn't add this. This was added for PreStop hook and the label contains the entire serialized pod... I want to remove this. See #/14768 for the details.

edit: we only found out about this label yesterday

@yujuhong I read code about this yesterday, it seems that one of the options is to add some "better-formatted" labels on each container (rather than add the whole pod data), for example add a label like "container_metadata" and write all needed data like PreStop handler into it...

Random-Liu · 2015-10-29T18:12:34Z

OK. I'll remove the filter related code in this PR. :)

k8s-bot · 2015-10-29T21:58:46Z

GCE e2e test build/test passed for commit 68dab7f0b430421f3f4c3a0008ccb67be2932b14.

yujuhong · 2015-10-29T22:27:03Z

pkg/kubelet/dockertools/labels.go

+		// Get restartCount from docker label. If there is no restart count label in an container,
+		// it should be an old container or an invalid container, we just set restart count to 0.
+		// Don't report error, because there must be many old containers without the label now
+		glog.V(3).Infof("Container doesn't have label %s, it may be an old or invalid container", kubernetesContainerRestartCountLabel)


For the logging message to be meaningful in anyway, we need the container id. I'd suggest returning (int, error) and let the caller handle the error.

yujuhong · 2015-10-29T22:31:20Z

pkg/kubelet/dockertools/manager.go

@@ -1643,8 +1620,8 @@ func (dm *DockerManager) createPodInfraContainer(pod *api.Pod) (kubetypes.Docker
 		return "", err
 	}

-	// There is no meaningful labels for infraContainer now, so just pass nil.
-	id, err := dm.runContainerInPod(pod, container, netNamespace, getIPCMode(pod), getPidMode(pod), nil)
+	// Current now we don't care about restart count of infra container, just set it to 0.


Remove "Current now"

FYI, you can use "currently" or not use it at all (implied). "Current now" is not grammatically correct :-)

Oops(corrected)...I don't even know where I learned it. I really need more suggestions like this about my grammar, words etc. Thank you very much! :)

yujuhong · 2015-10-29T22:32:14Z

lgtm with some nits :)

k8s-bot · 2015-10-29T23:56:44Z

GCE e2e test build/test passed for commit 29bb7941200da8923895f5f7bd31afb9400de8c6.

yujuhong · 2015-10-30T00:39:44Z

lgtm. please squash.

Random-Liu · 2015-10-30T00:52:55Z

Sure. Thanks :)

…pod namespace and pod uid into docker label

Random-Liu · 2015-10-30T05:48:52Z

Squashed. :)

k8s-bot · 2015-10-30T06:14:26Z

GCE e2e build/test failed for commit b3585a5.

k8s-bot · 2015-10-30T06:53:49Z

GCE e2e test build/test passed for commit b3585a5.

a-robinson · 2015-10-30T18:39:48Z

@k8s-bot unit test this

k8s-github-robot · 2015-10-30T19:02:00Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

k8s-github-robot · 2015-10-30T19:13:23Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-10-30T19:41:25Z

GCE e2e test build/test passed for commit b3585a5.

k8s-github-robot · 2015-10-30T19:45:25Z

Automatic merge from submit-queue

Auto commit by PR queue bot

googlebot added the cla: yes label Oct 28, 2015

Random-Liu changed the title ~~Put pod full name into dockerlabel~~ Put pod full name into docker label and use label filter to simplify GetPodStatus() Oct 28, 2015

k8s-github-robot assigned vishh Oct 28, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 28, 2015

Random-Liu mentioned this pull request Oct 28, 2015

Leverage docker labels to construct container statuses without consulting pod spec #15089

Closed

yujuhong assigned yujuhong and unassigned vishh Oct 28, 2015

Random-Liu closed this Oct 29, 2015

Random-Liu reopened this Oct 29, 2015

yujuhong reviewed Oct 29, 2015
View reviewed changes

vishh reviewed Oct 29, 2015
View reviewed changes

Random-Liu changed the title ~~Put pod full name into docker label and use label filter to simplify GetPodStatus()~~ Put pod full name, namespace and uid into docker label Oct 29, 2015

yujuhong reviewed Oct 29, 2015
View reviewed changes

Move docker label related functions into labels.go and add pod name, …

b3585a5

…pod namespace and pod uid into docker label

Random-Liu force-pushed the put-podname-into-label branch from 29bb794 to b3585a5 Compare October 30, 2015 05:43

yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2015

k8s-github-robot added the needs-ok-to-merge label Oct 30, 2015

yujuhong added ok-to-merge and removed needs-ok-to-merge labels Oct 30, 2015

k8s-github-robot pushed a commit that referenced this pull request Oct 30, 2015

Merge pull request #16414 from Random-Liu/put-podname-into-label

eb14049

Auto commit by PR queue bot

k8s-github-robot merged commit eb14049 into kubernetes:master Oct 30, 2015

Random-Liu deleted the put-podname-into-label branch October 30, 2015 23:17

Random-Liu mentioned this pull request Nov 2, 2015

Remove unused type DockerContainers. #16653

Merged

yujuhong mentioned this pull request Apr 7, 2016

Improve kubelet's scalability and performance #12540

Closed

zhangxiaoyu-zidif mentioned this pull request Jan 3, 2018

Container spec hash changed. Container will be killed and recreated. #53644

Closed

Put pod full name, namespace and uid into docker label #16414

Put pod full name, namespace and uid into docker label #16414

Conversation

Random-Liu commented Oct 28, 2015

k8s-github-robot commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

ncdc commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

ashcrow commented Oct 28, 2015

Random-Liu commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

Random-Liu commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

k8s-bot commented Oct 29, 2015

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

yujuhong commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Oct 29, 2015

k8s-bot commented Oct 29, 2015

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

yujuhong commented Oct 29, 2015

k8s-bot commented Oct 29, 2015

yujuhong commented Oct 30, 2015

Random-Liu commented Oct 30, 2015

Random-Liu commented Oct 30, 2015

k8s-bot commented Oct 30, 2015

k8s-bot commented Oct 30, 2015

a-robinson commented Oct 30, 2015

k8s-github-robot commented Oct 30, 2015

k8s-github-robot commented Oct 30, 2015

k8s-bot commented Oct 30, 2015

k8s-github-robot commented Oct 30, 2015