Fix kubectl describe for pods with controllerRef #45467

ddysher · 2017-05-08T04:34:43Z

What this PR does / why we need it:

kubectl describe doesn't take controllerRef into consideration, resulting confusing result. e.g. if we have two replicaset with the same selector, one with 1 replica and the other 2 replicase, then both replicaset will show 3 running pods.

$ kubectl describe rs replicaset-2
Name:           replicaset-2      
Namespace:      default
Selector:       environment=prod
Labels:         environment=prod
Annotations:    <none>
Replicas:       2 current / 2 desired
Pods Status:    3 Running / 0 Waiting / 0 Succeeded / 0 Failed
Pod Template:
  Labels:       environment=prod
  Containers:
   created-from-replicaset:
    Image:              nginx
    Port:               
    Environment:        <none>
    Mounts:             <none>
  Volumes:              <none>
Events:
  FirstSeen     LastSeen        Count   From                    SubObjectPath   Type            Reason                  Message
  ---------     --------        -----   ----                    -------------   --------        ------                  -------
  5m            5m              1       replicaset-controller                   Normal          SuccessfulCreate        Created pod: replicaset-2-39szb
  5m            5m              1       replicaset-controller                   Normal          SuccessfulCreate        Created pod: replicaset-2-470jr

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

xref #24946

Special notes for your reviewer:

Release note:

Fix kubectl describe for pods with controllerRef

k8s-reviewable · 2017-05-08T04:34:50Z

This change is

k8s-ci-robot · 2017-05-08T04:34:52Z

Hi @ddysher. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

spiffxp · 2017-05-08T15:02:16Z

@k8s-bot ok to test

ddysher · 2017-05-10T11:31:52Z

@janetkuo @enisoc @Kargakis

janetkuo · 2017-05-11T00:49:04Z

/assign

janetkuo · 2017-05-11T00:51:34Z

Thanks, we should fix it in kubectl get too.

@ddysher: I'm curious why you created 2 RSes with the same selector. Did you do it by accident or is it a special use case for you?

ddysher · 2017-05-11T01:21:40Z

@janetkuo not really, we want to use this feature to find pod->controller, so I'm simply trying out controllerRef.

Where do I need to fix for kubectl get; get doesn't count pods, maybe there's other options I need to consider?

$ kubectl get rs     
NAME           DESIRED   CURRENT   READY     AGE
replicaset-1   1         1         1         2s
replicaset-2   2         2         2         31s

$ kubectl get pods     
NAME                 READY     STATUS    RESTARTS   AGE
replicaset-1-z3d05   1/1       Running   0          11s
replicaset-2-h5tll   1/1       Running   0          40s
replicaset-2-mmnl3   1/1       Running   0          40s

janetkuo · 2017-05-11T17:31:01Z

@ddysher you're right, get only dump information from controller status, so we only need to fix describe.

janetkuo · 2017-05-11T17:52:00Z

pkg/printers/internalversion/describe.go

@@ -2650,6 +2653,18 @@ func getPodStatusForController(c coreclient.PodInterface, selector labels.Select
 	return
 }

+// isControllerRef returns true if a pod is managed by a controller or if owner
+// reference is not set; otherwise, return false.
+func isControllerRef(pod api.Pod, name string) bool {


nit: name it isControlledBy

janetkuo · 2017-05-11T17:52:38Z

pkg/printers/internalversion/describe.go

+// reference is not set; otherwise, return false.
+func isControllerRef(pod api.Pod, name string) bool {
+	for _, or := range pod.OwnerReferences {
+		fmt.Println(*or.Controller, or.Name, name)


janetkuo · 2017-05-11T17:53:40Z

pkg/printers/internalversion/describe.go

+// isControllerRef returns true if a pod is managed by a controller or if owner
+// reference is not set; otherwise, return false.
+func isControllerRef(pod api.Pod, name string) bool {
+	for _, or := range pod.OwnerReferences {


you can get controllerRef from controller.GetControllerOf(pod)

janetkuo · 2017-05-11T17:55:05Z

pkg/printers/internalversion/describe.go

+func isControllerRef(pod api.Pod, name string) bool {
+	for _, or := range pod.OwnerReferences {
+		fmt.Println(*or.Controller, or.Name, name)
+		if or.Controller != nil && *or.Controller == true && or.Name != name {


compare Name, Kind, and UID

janetkuo · 2017-05-11T17:57:32Z

pkg/printers/internalversion/describe.go

@@ -2650,6 +2653,18 @@ func getPodStatusForController(c coreclient.PodInterface, selector labels.Select
 	return
 }

+// isControllerRef returns true if a pod is managed by a controller or if owner
+// reference is not set; otherwise, return false.


If controllerRef==nil the pod is either an orphan or the apiserver doesn't have controllerRef implemented. If it's an orphan we should return false. @enisoc do we need to return true here for backward compatibility?

I'm returning true here mainly for backward compatibility; didn't realize orphaned pod case. If a pod is orphaned, will we be able to select it? All pods passed in there are selected via controller's selector.

An orphan pod = a pod that doesn't have a controllerRef specified. ControllerRef is a lock when one child is selected by multiple parents via selector. That's to say, a controller can select a pod that it doesn't own, but the controller won't try to manage that pod.

A pod can be orphaned when its controller is deleted non-cascadingly. Other existing controllers with matching selector will try to adopt it, and eventually at most one controller will win.

enisoc · 2017-05-12T21:50:44Z

pkg/printers/internalversion/describe.go

@@ -2654,6 +2657,16 @@ func getPodStatusForController(c coreclient.PodInterface, selector labels.Select
 	return
 }

+// isControlledBy returns true if pod is controlled by given controller or if
+// owner reference is not set; otherwise, return false.
+func isControlledBy(pod api.Pod, name string, uid types.UID) bool {


We try to pass Pod as a pointer whenever possible. You could also just inline this since it's only used in one place and will add only one line to that call site.

enisoc · 2017-05-12T21:51:43Z

pkg/printers/internalversion/describe.go

+// owner reference is not set; otherwise, return false.
+func isControlledBy(pod api.Pod, name string, uid types.UID) bool {
+	controllerRef := controller.GetControllerOf(&pod)
+	if controllerRef != nil && (controllerRef.Name != name || controllerRef.UID != uid) {


Comparing uid alone should be sufficient IMO. @janetkuo Is there a reason you suggested comparing all of name+kind+uid?

You'll need to update your unit test to include non-empty UIDs in the test cases if you take my suggestion.

Yes comparing uid is enough.

enisoc · 2017-05-12T21:58:32Z

pkg/printers/internalversion/describe.go

+	if controllerRef != nil && (controllerRef.Name != name || controllerRef.UID != uid) {
+		return false
+	}
+	return true


Since this is for kubectl 1.7, we only need to support talking to masters that are 1.6-1.8. That means we actually don't need to consider backward compatibility with versions that didn't set controllerRef (pre-1.6).

We do need to consider the orphan case, but we actually want the opposite of this to happen. An orphan should not be considered as owned by the controller in a post-controllerRef (1.6+) world.

To sum up, I suggest logic like the following inlined into getPodStatusForController:

controllerRef := controller.GetControllerOf(&pod) // Skip pods that are orphans or owned by other controllers. if controllerRef == nil || controllerRef.UID != uid { continue }

enisoc · 2017-05-12T22:02:15Z

pkg/printers/internalversion/describe_test.go

+		})
+	d := ReplicationControllerDescriber{f}
+	out, err := d.Describe("foo", "bar", printers.DescriberSettings{ShowEvents: false})
+	fmt.Println(string(out))


It's better to send this to t.Log() so it only shows if the test fails. Also it should be already a string without casting, right?

Thanks for pointing this out. I'll just remove it since we already print it in error case.

ddysher · 2017-05-15T12:11:17Z

@janetkuo @enisoc comment addressed.

janetkuo · 2017-05-15T20:31:04Z

I0515 05:33:49.549]   /go/src/k8s.io/kubernetes/staging/src/k8s.io/client-go is out of date. Please run hack/update-staging-client-go.sh
I0515 05:33:49.549] FAILED   hack/make-rules/../../hack/verify-staging-client-go.sh	101s

enisoc · 2017-05-15T21:20:19Z

pkg/printers/internalversion/describe.go

 	options := metav1.ListOptions{LabelSelector: selector.String()}
 	rcPods, err := c.List(options)
 	if err != nil {
 		return
 	}
 	for _, pod := range rcPods.Items {
+		controllerRef := controller.GetControllerOf(&pod)
+		// Skip pods that are orphans or owned by other controllers.
+		if controllerRef != nil && controllerRef.UID != uid {


This still doesn't skip orphans (controllerRef = nil).

It should be:

if controllerRef == nil || controllerRef.UID != uid { continue }

Please also add an orphan to the unit test to verify this works.

thx! I didn't look closely...

ddysher · 2017-05-17T01:06:48Z

@janetkuo done

janetkuo · 2017-05-17T21:53:56Z

/approve

janetkuo · 2017-05-17T21:54:30Z

/assign @lavalamp for approval

janetkuo · 2017-05-18T00:49:46Z

/assign @lavalamp

ddysher · 2017-06-02T01:00:05Z

rebased

ping @lavalamp

fejta · 2017-06-02T05:19:57Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref: #46827

enisoc · 2017-06-29T18:50:06Z

@ddysher Sorry about the delay in getting someone who can approve this. To unblock the actual fix for kubectl, you could move your BoolPtr() function to be a package-local boolPtr() in the package where you need it. That would limit the scope of this PR to files that @janetkuo can approve.

ddysher · 2017-07-01T10:48:27Z

/retest

ddysher · 2017-07-02T03:01:08Z

@janetkuo @enisoc updated

enisoc · 2017-07-05T17:23:51Z

/lgtm

k8s-github-robot · 2017-07-05T17:24:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ddysher, enisoc, janetkuo

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/printers/OWNERS~~ [janetkuo]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-05T19:25:49Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 8, 2017

k8s-github-robot assigned eparis and ericchiang May 8, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 8, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 8, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 9, 2017

k8s-ci-robot assigned janetkuo May 11, 2017

janetkuo reviewed May 11, 2017

View reviewed changes

ddysher force-pushed the kubectl-describe-controllerRef branch from fe9361f to 145ac17 Compare May 12, 2017 05:37

enisoc reviewed May 12, 2017

View reviewed changes

ddysher force-pushed the kubectl-describe-controllerRef branch from 145ac17 to 7bbe756 Compare May 15, 2017 12:10

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2017

janetkuo approved these changes May 15, 2017

View reviewed changes

enisoc reviewed May 15, 2017

View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 16, 2017

ddysher force-pushed the kubectl-describe-controllerRef branch from 0196c86 to 7508a33 Compare May 17, 2017 01:05

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2017

enisoc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2017

k8s-ci-robot assigned lavalamp May 18, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2017

ddysher force-pushed the kubectl-describe-controllerRef branch from 7508a33 to ac508f5 Compare June 2, 2017 00:53

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 2, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2017

grodrigues3 mentioned this pull request Jun 23, 2017

Deal With Inactive Approvers and Approvers Updates kubernetes/test-infra#3178

Closed

3 tasks

ddysher force-pushed the kubectl-describe-controllerRef branch from ac508f5 to 3a8dfaa Compare July 1, 2017 10:43

Fix kubectl describe for controllerRef

c73b535

ddysher force-pushed the kubectl-describe-controllerRef branch from 3a8dfaa to c73b535 Compare July 1, 2017 10:46

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2017

enisoc unassigned lavalamp Jul 5, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2017

k8s-github-robot merged commit e16b59a into kubernetes:master Jul 5, 2017

Fix kubectl describe for pods with controllerRef #45467

Fix kubectl describe for pods with controllerRef #45467

Conversation

ddysher commented May 8, 2017 • edited by enisoc Loading

k8s-reviewable commented May 8, 2017

k8s-ci-robot commented May 8, 2017

spiffxp commented May 8, 2017

ddysher commented May 10, 2017

janetkuo commented May 11, 2017

janetkuo commented May 11, 2017

ddysher commented May 11, 2017 • edited Loading

janetkuo commented May 11, 2017

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

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

Choose a reason for hiding this comment

ddysher commented May 15, 2017

janetkuo commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher commented May 17, 2017

janetkuo commented May 17, 2017

janetkuo commented May 17, 2017

janetkuo commented May 18, 2017

ddysher commented Jun 2, 2017

fejta commented Jun 2, 2017

enisoc commented Jun 29, 2017

ddysher commented Jul 1, 2017

ddysher commented Jul 2, 2017

enisoc commented Jul 5, 2017

k8s-github-robot commented Jul 5, 2017

k8s-github-robot commented Jul 5, 2017

ddysher commented May 8, 2017 •

edited by enisoc

Loading

ddysher commented May 11, 2017 •

edited

Loading