[Federation] Convert the deployment controller to a sync controller.

[perotinus]

This is based off of the work done for the ReplicaSet controller. It extracts out a schedulingAdapter that handles the shared logic between the two controllers.

Targets #40989

Release note:

NONE

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

[perotinus]

/assign @marun

[perotinus]

@k8s-bot pull-kubernetes-federation-e2e-gce test this

[marun]

Change seems straightforward to me. Tests should be passing before lgtm.

[marun]

Is s.ConcurrentReplicaSetSyncs no longer relevant?

[perotinus]

It is no longer relevant: it could be reintroduced in a future PR, but I haven't seen any evidence that it's useful.

Removing it as a flag could be interesting, since it's a breaking change to remove it. It's probably not a huge deal, since I don't think most people are running the controller manager themselves, but it's something we do need to think about. It's functionally harmless to leave it, though it's misleading.

[marun]

Should this file become scheduling_test.go?

[perotinus]

That makes sense. Done.

[fejta]

@k8s-bot pull-kubernetes-e2e-kops-aws test this
ref: kubernetes/test-infra#2932

[marun]

@perotinus ReplicaSet has merged! Happy to review this once you rebase.

[perotinus]

@marun Hold off on reviewing this for now: there are some issues with testing I need to work out still. I'll ping the PR when it's ready.

Thanks!

[perotinus]

@marun OK, the tests are fixed. This should be ready for review now.

[perotinus]

/retest

[perotinus]

/test pull-kubernetes-e2e-gce-etcd3

[k8s-reviewable]

This change is 

[marun]

As per inline comment, I'm not sure reflection is the best strategy to provide type-agnostic scheduling, but otherwise this looks reasonable.

---

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

federation/pkg/federatedtypes/scheduling.go, line 94 at r1 (raw file):

			return nil, err
		}
		selectorObj := reflect.ValueOf(obj).Elem().FieldByName("Spec").FieldByName("Selector").Interface().(*metav1.LabelSelector)

Why is reflection preferable to using adapter methods to retrieve the relevant fields?

---

federation/pkg/federatedtypes/scheduling.go, line 99 at r1 (raw file):

			return nil, fmt.Errorf("invalid selector: %v", err)
		}
		objectMeta := reflect.ValueOf(obj).Elem().FieldByName("ObjectMeta").Interface().(metav1.ObjectMeta)

I think it would be preferable to use the meta accessor to access ObjectMeta.

---

federation/pkg/federatedtypes/scheduling.go, line 102 at r1 (raw file):

		return clientset.Core().Pods(objectMeta.Namespace).List(metav1.ListOptions{LabelSelector: selector.String()})
	}
	current, estimatedCapacity, err := clustersReplicaState(clusterNames, key, objectGetter, podsGetter)

current what?

---

federation/pkg/federatedtypes/scheduling.go, line 107 at r1 (raw file):

	}

	plnr := planner.NewPlanner(&fedapi.ReplicaAllocationPreferences{

Consider simplifying this logic e.g.

• try to retrieve preferences
• if preferences can't be retrieved, use default
• create planner with preferences

---

Comments from Reviewable

[perotinus]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

federation/pkg/federatedtypes/scheduling.go, line 94 at r1 (raw file):

Previously, marun (Maru Newby) wrote…

Why is reflection preferable to using adapter methods to retrieve the relevant fields?

I played around with this for a little while. Right now, my impression is that the reflection is not that pretty but that having the code to get the spec/status/etc in different places is harder to follow and reason about. Also, some of the pure functions not on the adapter currently use reflection to get info from the objects; they would either have to have an adapter passed in, or have more functions passed in to get what they need from an object, which makes their interfaces more complicated without much benefit IMO.

I can play with this some more–the option that I would choose in place of reflection would be several functions passed into the scheduling adapter and stored on it–but I don't think that's a cleaner way to do it than the reflection that's here.

---

federation/pkg/federatedtypes/scheduling.go, line 99 at r1 (raw file):

Previously, marun (Maru Newby) wrote…

I think it would be preferable to use the meta accessor to access ObjectMeta.

Ah, yes. I'd seen that before but had forgotten about it. Changed.

---

federation/pkg/federatedtypes/scheduling.go, line 102 at r1 (raw file):

Previously, marun (Maru Newby) wrote…

current what?

Done.

---

federation/pkg/federatedtypes/scheduling.go, line 107 at r1 (raw file):

Previously, marun (Maru Newby) wrote…

Consider simplifying this logic e.g.

• try to retrieve preferences
• if preferences can't be retrieved, use default
• create planner with preferences

Done.

---

Comments from Reviewable

[perotinus]

/retest

[marun]

/lgtm

---

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[marun]

/retest

[k8s-ci-robot]

@perotinus: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-cross	34a7703485e12c63e44dfe76f3518deaee981179	link	@k8s-bot pull-kubernetes-cross test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[perotinus]

/retest

[nikhiljindal]

/approve

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: csbell, marun, nikhiljindal, perotinus

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:

• federation/OWNERS [csbell,nikhiljindal]

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 47619, 47951, 46260, 48277)