rkt: rewrote GetPods to use rkt's api service #17969

cgonyeo · 2015-11-30T18:45:18Z

This commit modifies the kubelet to use rkt's new API service when the kubelet wants to list all current pods.

This involved adding annotations to the rkt pod's manifest that contain
information about the kubernetes pod, which is later read by the
kubelet.

k8s-bot · 2015-11-30T18:46:13Z

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

k8s-bot · 2015-11-30T18:47:12Z

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

yifan-gu · 2015-11-30T18:47:40Z

cc @dchen1107 @jonboulle

k8s-github-robot · 2015-11-30T18:48:41Z

Labelling this PR as size/L

jonboulle · 2015-11-30T19:10:36Z

pkg/kubelet/rkt/rkt.go

+		}
+	}
+
+	manifest.Annotations = append(manifest.Annotations, []appctypes.Annotation{


To be pedantic - you should check for any existing annotations by these names, since the spec says they must be unique:

Annotation names must be unique within the list.

@jonboulle We should validate this when marshaling the manifest

Sure but the behaviour is not going to be very user friendly in that case.

yifan-gu · 2015-11-30T19:42:06Z

@dgonyeo I believe there should be other files in this PR right?

cgonyeo · 2015-11-30T22:32:27Z

Not sure what files I'm missing here.

yifan-gu · 2015-11-30T22:36:01Z

@dgonyeo I was thinking you have modified rkt_info stuff. Never mind.
Would be better we add a test for GetPods().

thockin · 2015-12-01T05:55:40Z

pkg/kubelet/rkt/rkt.go

@@ -71,6 +72,16 @@ const (
 	unitRktID             = "RktID"
 	unitRestartCount      = "RestartCount"

+	k8sRktKubeletAnno      = "k8s.io/rkt/managed-by-kubelet"
+	k8sRktKubeletAnnoValue = "true"
+	k8sRktUIDAnno          = "k8s.io/rkt/uid"


I don't think these are valid annotations. you want rkt.kubernetes.io/uid, etc

yifan-gu · 2015-12-03T00:37:57Z

pkg/kubelet/rkt/rkt.go

+		if countString, ok := manifest.Annotations.Get(k8sRktRestartCountAnno); ok {
+			num, err := strconv.Atoi(countString)
+			if err != nil {
+				continue


print the error here?

besides, why continue here, but return error in other places?

I had a feeling that if I errored on that, that one could prevent the kubelet from restarting a given pod by inserting a pod with a matching uid annotation and a garbage restart count annotation.

@dgonyeo OK, but that can also happen by inserting random bytes in the pod manifest to let json.Unmarshal fail?
I am ok we all continue in these places, with a warning message printed.

yifan-gu · 2015-12-03T02:01:09Z

pkg/kubelet/rkt/rkt.go

+		}
+
+		manifest := appcschema.PodManifest{}
+		err = json.Unmarshal(inspectResp.Pod.Manifest, manifest)


unmarshal needs a pointer

yujuhong · 2015-12-04T20:56:24Z

Please run gofmt.

hack/verify-gofmt.sh
!!! 'gofmt -s' needs to be run on the following files: 
./pkg/kubelet/rkt/rkt.go
./pkg/kubelet/rkt/rkt_test.go
make: *** [verify] Error 1

yifan-gu · 2015-12-05T01:44:22Z

@k8s-bot ok to test

cgonyeo · 2015-12-09T20:07:39Z

@yifan-gu should be ready for another pass.

k8s-github-robot · 2015-12-09T20:08:54Z

Labelling this PR as size/XL

yifan-gu · 2015-12-09T21:43:45Z

pkg/kubelet/rkt/rkt.go

+	}, opts.PortMappings, nil
+}
+
+func getMatchingPodsRequest(pod *api.Pod) *rktapi.ListPodsRequest {


probably not the best name, how about just return the filter, and call it kubernetesPodFilter or something?

yifan-gu · 2015-12-09T22:02:02Z

commented, just some small testing stuff. @dgonyeo

This involved adding annotations to the rkt pod's manifest that contain information about the kubernetes pod, which is later read by the kubelet.

cgonyeo · 2015-12-09T22:44:11Z

Done. How's that look?

yifan-gu · 2015-12-09T23:20:17Z

LGTM. I think we need to do some further reorganizing on the code later.
@yujuhong do you want to take a look at this before I add lgtm label?

k8s-github-robot · 2015-12-10T01:07:21Z

@k8s-bot ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

k8s-bot · 2015-12-10T02:20:53Z

GCE e2e test build/test passed for commit 5a16b47.

k8s-github-robot · 2015-12-10T02:31:28Z

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-12-10T10:34:13Z

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

k8s-bot · 2015-12-10T11:02:45Z

GCE e2e test build/test passed for commit 5a16b47.

yujuhong · 2015-12-10T20:03:57Z

@k8s-bot unit test this

k8s-github-robot · 2015-12-11T06:42:30Z

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

k8s-bot · 2015-12-11T07:13:13Z

GCE e2e test build/test passed for commit 5a16b47.

k8s-github-robot · 2015-12-11T07:13:37Z

Automatic merge from submit-queue

Auto commit by PR queue bot

googlebot added the cla: yes label Nov 30, 2015

k8s-github-robot assigned thockin Nov 30, 2015

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

cgonyeo force-pushed the rkt_api_get_pods branch from a6b9225 to 64ac183 Compare November 30, 2015 18:51

jonboulle reviewed Nov 30, 2015
View reviewed changes

cgonyeo force-pushed the rkt_api_get_pods branch from 64ac183 to 4bf4079 Compare November 30, 2015 22:33

thockin assigned yujuhong and unassigned thockin Dec 1, 2015

thockin reviewed Dec 1, 2015
View reviewed changes

cgonyeo force-pushed the rkt_api_get_pods branch 2 times, most recently from b2410fe to 8361f45 Compare December 1, 2015 21:26

yifan-gu reviewed Dec 3, 2015
View reviewed changes

cgonyeo force-pushed the rkt_api_get_pods branch from 8361f45 to 2446c81 Compare December 3, 2015 00:57

yifan-gu reviewed Dec 3, 2015
View reviewed changes

cgonyeo force-pushed the rkt_api_get_pods branch 2 times, most recently from 259e69a to 6e39cd3 Compare December 4, 2015 19:16

cgonyeo force-pushed the rkt_api_get_pods branch from 6e39cd3 to 4bb45da Compare December 4, 2015 22:20

cgonyeo force-pushed the rkt_api_get_pods branch from 9b5f87b to a80f280 Compare December 9, 2015 20:07

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2015

yifan-gu reviewed Dec 9, 2015
View reviewed changes

rkt: rewrote GetPods to use rkt's api service

5a16b47

This involved adding annotations to the rkt pod's manifest that contain information about the kubernetes pod, which is later read by the kubelet.

cgonyeo force-pushed the rkt_api_get_pods branch from a80f280 to 5a16b47 Compare December 9, 2015 22:43

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

k8s-github-robot added the needs-ok-to-merge label Dec 10, 2015

yifan-gu added ok-to-merge and removed needs-ok-to-merge labels Dec 10, 2015

k8s-github-robot pushed a commit that referenced this pull request Dec 11, 2015

Merge pull request #17969 from dgonyeo/rkt_api_get_pods

6b8eb90

Auto commit by PR queue bot

k8s-github-robot merged commit 6b8eb90 into kubernetes:master Dec 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rkt: rewrote GetPods to use rkt's api service #17969

rkt: rewrote GetPods to use rkt's api service #17969

cgonyeo commented Nov 30, 2015

k8s-bot commented Nov 30, 2015

k8s-bot commented Nov 30, 2015

yifan-gu commented Nov 30, 2015

k8s-github-robot commented Nov 30, 2015

jonboulle Nov 30, 2015

yifan-gu Nov 30, 2015

jonboulle Nov 30, 2015

yifan-gu commented Nov 30, 2015

cgonyeo commented Nov 30, 2015

yifan-gu commented Nov 30, 2015

thockin Dec 1, 2015

yifan-gu Dec 3, 2015

yifan-gu Dec 3, 2015

cgonyeo Dec 3, 2015

yifan-gu Dec 3, 2015

yifan-gu Dec 3, 2015

yujuhong commented Dec 4, 2015

yifan-gu commented Dec 5, 2015

cgonyeo commented Dec 9, 2015

k8s-github-robot commented Dec 9, 2015

yifan-gu Dec 9, 2015

yifan-gu commented Dec 9, 2015

cgonyeo commented Dec 9, 2015

yifan-gu commented Dec 9, 2015

k8s-github-robot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-github-robot commented Dec 10, 2015

k8s-github-robot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

yujuhong commented Dec 10, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015

rkt: rewrote GetPods to use rkt's api service #17969

rkt: rewrote GetPods to use rkt's api service #17969

Conversation

cgonyeo commented Nov 30, 2015

k8s-bot commented Nov 30, 2015

k8s-bot commented Nov 30, 2015

yifan-gu commented Nov 30, 2015

k8s-github-robot commented Nov 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifan-gu commented Nov 30, 2015

cgonyeo commented Nov 30, 2015

yifan-gu commented Nov 30, 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

Choose a reason for hiding this comment

yujuhong commented Dec 4, 2015

yifan-gu commented Dec 5, 2015

cgonyeo commented Dec 9, 2015

k8s-github-robot commented Dec 9, 2015

Choose a reason for hiding this comment

yifan-gu commented Dec 9, 2015

cgonyeo commented Dec 9, 2015

yifan-gu commented Dec 9, 2015

k8s-github-robot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-github-robot commented Dec 10, 2015

k8s-github-robot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

yujuhong commented Dec 10, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015