Helper methods dealing with ControllerRef

[nilebox]

What this PR does / why we need it:
Adds helper methods for working with controllerRef (controller's OwnerReference).

It is based on the existing code from Kubernetes plus extracting some common logic:

• NewControllerRef is based on examples from daemon controller, deployment controller, job controller
• GetControllerOf is copied from controller_ref_manager.go
• IsControlledBy is a common logic extracted from resource controllers: deployment_util.go#L568, history.go#L276 and many others.

It will also be useful for writing custom resource controllers, for example service-catalog#979

[k8s-ci-robot]

Hi @nilebox. 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 /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.

I understand the commands that are listed here.

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.

[nilebox]

/cc @caesarxuchao @pmorie

[ash2k]

I understand that this is a copy of existing code. I'm wondering why it is written that way? I'd simplify it:

func GetControllerOf(controllee meta_v1.Object) *meta_v1.OwnerReference {
	for _, ref := range controllee.GetOwnerReferences() {
		if ref.Controller != nil && *ref.Controller {
			return &ref
		}
	}
	return nil
}

[caesarxuchao]

That's a difficult to debug bug in golang. &ref is different from &ownerRefs[i]. ref is a local variable. It took me quite a while to figure it out when i wrote the code initially.

[ash2k]

Yes, I understand what you mean :)
The difference is that my version returns a pointer to a copy (as you correctly pointed out) and the original version returns a pointer to the value stored in the slice. However in this case it does not matter in practice - there are only four implementations of GetOwnerReferences() - all of them return copies of the internal owner references slice. So the code that called GetControllerOf() cannot mutate the in-object owner references in any case because it will gat a pointer to a copy.

[caesarxuchao]

Yes, both should work. Maybe at first i intended to return the original ref, but its not the intention now.

I think your version is better, it's always returning a pointer to a copy, no matter how GetOwnerReferences is implemented.

Maybe we can update the comment to "GetControllerOf returns a pointer to a copy of the controllerRef if controllee has a controller"?

[nilebox]

@caesarxuchao done

[ash2k]

/sig api-machinery

[ash2k]

Are you going to refactor all places where the original methods used in separate PR(s)/commits?

[nilebox]

@ash2k definitely as a separate PR(s), as it will touch a lot of files, and will be harder to get reviewed and merged.

[spiffxp]

/ok-to-test
@kubernetes/sig-api-machinery-pr-reviews PTAL (@ash2k I'm using the github team to notify instead of just applying the label)

[ash2k]

@spiffxp I don't have any special permissions on this repo so I think I cannot use mentions. Tried once - it didn't work.

[spiffxp]

@ash2k ah, cool... FWIW, I believe the bot now repeats sig mentions if it notices you're not a member, so they'll notify... if that's still not working for you, it's a bug to be filed against kubernetes/test-infra

[nilebox]

Looks like there is some flakiness in Kube tests:

1. pull-kubernetes-unit - reports on kubefed TLS test failing (completely unrelated to the change), passes locally:

❯ go test -v k8s.io/kubernetes/federation/pkg/kubefed/init -run TestCertsTLS$

=== RUN   TestCertsTLS
--- PASS: TestCertsTLS (1.56s)
PASS
ok  	k8s.io/kubernetes/federation/pkg/kubefed/init	1.646s

2. pull-kubernetes-e2e-kops-aws - Building Docker image times out (90 minutes), I have the same issue locally.

The same tests currently fail in other pull requests I looked at, for example #48689

[sttts]

@nilebox 1 is fixed in master

[sttts]

@nilebox which docker image times out and how can I reproduce this locally?

[nilebox]

@sttts rebased onto latest master, let's see if anything gets fixed.

which docker image times out and how can I reproduce this locally?

make release (as per https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/48319/pull-kubernetes-e2e-kops-aws/33930/)

❯ make release
+++ [0710 13:55:21] Verifying Prerequisites....
+++ [0710 13:55:21] Using Docker for MacOS
+++ [0710 13:55:21] Building Docker image kube-build:build-0cbf19789f-5-v1.8.3-1

[nilebox]

One more failure https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/48319/pull-kubernetes-federation-e2e-gce/13803/

IOError: [Errno 28] No space left on device

nice...

[nilebox]

/retest

[nilebox]

@sttts the Docker image build is still timing out, as well as in other pull requests (for example, #48726 (comment) and #48750 (comment)).

[fejta]

/test pull-kubernetes-e2e-kops-aws

[sttts]

/lgtm

Please squash.

@caesarxuchao approved?

[nilebox]

Squashed, ready to be merged.

[sttts]

/lgtm

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nilebox, sttts

Associated issue: 979

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

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

• staging/src/k8s.io/apimachinery/pkg/OWNERS [sttts]

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]

Automatic merge from submit-queue (batch tested with PRs 48279, 48566, 48319, 48794, 47952)