[Federation] Add a SchedulingAdapter that can extend the FederatedTypeAdapter and that provides hooks for scheduling objects into clusters.

[perotinus]

Release note:

NONE

[k8s-reviewable]

This change is 

[k8s-ci-robot]

Hi @perotinus. 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.

[perotinus]

cc @marun @irfanurrehman

[marun]

@k8s-bot ok to test

[marun]

Good progress! I think I'd like to see this work follow #45374's example of keeping functions small enough to be unit tested. It may be worth considering rebasing on that work to ensure that reconcile with the proposed hook methods remains testable.

---

Reviewed 12 of 13 files at r1, 11 of 11 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

federation/pkg/federatedtypes/adapter.go, line 58 at r2 (raw file):

	ClusterWatch(client kubeclientset.Interface, namespace string, options metav1.ListOptions) (watch.Interface, error)

	// The following operations allow the adapter to modify objects before they are propogated

Consider following the approach suggested in #45497 (comment) to minimize unnecessary no-ops in adapters that don't need these functions.

---

federation/pkg/federatedtypes/replicaset.go, line 95 at r2 (raw file):

}

//		lrs := &extensionsv1.ReplicaSet{

Remove?

---

federation/pkg/federatedtypes/replicaset.go, line 172 at r2 (raw file):

}

func (a *ReplicaSetAdapter) PrepareForUpdateFunc() func(obj pkgruntime.Object, key string, clusters []*fedv1beta1.Cluster, informer fedutil.FederatedInformer) (interface{}, error) {

I reviewed @irfanurrehman's PR first, which used more specific hook methods instead of the generic events you propose here. Consider coordinating with him to come to agreement on a mutually acceptable path forward.

---

federation/pkg/federatedtypes/replicaset.go, line 173 at r2 (raw file):

func (a *ReplicaSetAdapter) PrepareForUpdateFunc() func(obj pkgruntime.Object, key string, clusters []*fedv1beta1.Cluster, informer fedutil.FederatedInformer) (interface{}, error) {
	return func(obj pkgruntime.Object, key string, clusters []*fedv1beta1.Cluster, informer fedutil.FederatedInformer) (interface{}, error) {

Why is it desirable to return the function vs having the client call invoke it directly? Same comment for the other hook implementations.

Are you intending to ensure unit test coverage for the hook methods?

---

federation/pkg/federation-controller/sync/controller.go, line 335 at r2 (raw file):

func (s *FederationSyncController) reconcile(namespacedName types.NamespacedName) (reconciliationStatus, error) {
	if !s.isSynced() {
		return statusNotSynced, nil

As per our offline discussion, I think it is preferable to minimize the scope of any one function such that logging can be done inline and testing can still reliably validate failure modes without having to look at error text.

---

federation/pkg/federation-controller/sync/controller.go, line 473 at r2 (raw file):

	}

	// Evertyhing is in order but let's be double sure

I switched to returning 'ok' in #45374. Unless I'm missing something, 'double sure' won't do anything but mask potential errors.

---

federation/pkg/federation-controller/sync/replicasetcontroller_test.go, line 47 at r2 (raw file):

)

//func TestParseFederationReplicaSetReference(t *testing.T) {

Should this be removed? I'm guessing #44525 ensures this anyway, consider rebasing on top of that.

---

federation/pkg/federation-controller/util/podanalyzer/pod_helper.go, line 20 at r2 (raw file):

import (
	"fmt"

It looks like the changes to this package impact the deployment controller, but there are no changes to that controller included in this PR.

Consider submitting the pod changes in a separate PR to simplify life for your reviewers.

---

Comments from Reviewable

[perotinus]

Review status: 5 of 22 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

federation/pkg/federatedtypes/adapter.go, line 58 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

Consider following the approach suggested in #45497 (comment) to minimize unnecessary no-ops in adapters that don't need these functions.

Done.

---

federation/pkg/federatedtypes/replicaset.go, line 95 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

Remove?

Done.

---

federation/pkg/federatedtypes/replicaset.go, line 173 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

Why is it desirable to return the function vs having the client call invoke it directly? Same comment for the other hook implementations.

Are you intending to ensure unit test coverage for the hook methods?

I've modified this

I have to think a bit more about test coverage for the hook methods: I agree that it makes sense and should be done, but passing in the federated informer will make these tests a bit more ugly. Perhaps the solution here is to have the hooks be lightweight wrappers that call into other methods.

---

federation/pkg/federation-controller/sync/controller.go, line 335 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

As per our offline discussion, I think it is preferable to minimize the scope of any one function such that logging can be done inline and testing can still reliably validate failure modes without having to look at error text.

Done.

---

federation/pkg/federation-controller/sync/controller.go, line 473 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

I switched to returning 'ok' in #45374. Unless I'm missing something, 'double sure' won't do anything but mask potential errors.

I rebased on top of your PR, so this should be good now.

---

federation/pkg/federation-controller/sync/replicasetcontroller_test.go, line 47 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

Should this be removed? I'm guessing #44525 ensures this anyway, consider rebasing on top of that.

Done.

---

federation/pkg/federation-controller/util/podanalyzer/pod_helper.go, line 20 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

It looks like the changes to this package impact the deployment controller, but there are no changes to that controller included in this PR.

Consider submitting the pod changes in a separate PR to simplify life for your reviewers.

These changes should be isolated to the first commit which is being reviewed in #45252. However, that PR may be dropped because of testing issues, in which case the code to modify the pod analysis method will potentially be moved here.

---

Comments from Reviewable

[perotinus]

@k8s-bot test this

[irfanurrehman]

Hope u can take care of these.
thanks in advance!

[irfanurrehman]

Please update these to generic types (or make them interfaces, where they are passed/used).

[irfanurrehman]

as above, hpa has different set of properties to pass around!

[irfanurrehman]

Please ensure that, the cluster objects are queried and passed as one set here (probably as a map), indicating which cluster has the object, and which does not (one way is use a map of same size as current available clusters with nil values identifying absence of object in that cluster).
Other option I could think of is; don't query the objects here at all; just pass the informer here (as you are doing), and let this interface or any other one in the flow later return the existing set vs scheduled set of objects (or only scheduled set of objects indicating, in which cluster to update, into which cluster to delete and which cluster to create ... (Or else I can't think of how exactly to do it ).
Probably u can think of on these lines:
say in the rs controller schedule, as a result of schedule, the local cluster rs can be deleted from some clusters also
so there has to be some way for this reconcile to be able to finalise the result comparing the original set of object vs new set of objects including all three conditions:

• if not exist in a cluster in existing set, schedule says create it.
• if exist in a cluster in existing set, schedule says update it.
• if exist in a cluster in existing set, schedule says delete it.

Also the above to be done without race conditions (means objects or set of cluster objects queried at one location to come up with the schedule result); and probably then the originally queried set and the newly scheduled set compared to arrive at cluster operations.

[marun]

While I think further refinement is possible, code freeze is looming and I think supporting replicasets with the sync controller represents is a necessary evolution towards a more maintainable codebase. Modulo the minor concerns inline, and the need for a test-specific equivalency check on the adapter interface, I'll be ready to lgtm.

---

Reviewed 7 of 20 files at r4, 7 of 22 files at r6, 7 of 7 files at r7.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

federation/pkg/federatedtypes/adapter.go, line 63 at r7 (raw file):

}

// SchedulingStatus contains the status of the objects that are being

(No action required) Would it make sense to put the scheduling-specific stuff into its own file?

---

federation/pkg/federatedtypes/adapter.go, line 65 at r7 (raw file):

Previously, irfanurrehman wrote…

Please update these to generic types (or make them interfaces, where they are passed/used).

I think it would be preferable to merge this very specific proposal and have your follow-on make it generic.

---

federation/pkg/federation-controller/sync/controller.go, line 449 at r7 (raw file):

	var schedulingInfo *federatedtypes.SchedulingInfo
	if adapter.IsSchedulingAdapter() {

(No action required) Consider simplifying as follows:

schedulingInfo, err := getSchedulingInfo(adapter, obj, key, selectedClusters, informer)
if err {
  return statusError
}

---

federation/pkg/federation-controller/sync/controller.go, line 453 at r7 (raw file):

		if !ok {
			glog.Fatalf("Adapter for kind %q does not properly implement SchedulingAdapter.", kind)
			return statusError

Is this return statement necessary given that glog.Fatalf calls os.Exit?

---

federation/pkg/federation-controller/sync/controller.go, line 470 at r7 (raw file):

		schedulingAdapter, ok := adapter.(federatedtypes.SchedulingAdapter)
		if !ok {
			glog.Fatalf("Adapter for kind %q does not properly implement SchedulingAdapter.", kind)

ditto

---

federation/pkg/federation-controller/sync/controller.go, line 522 at r7 (raw file):

		// The data should not be modified.
		desiredObj := adapter.Copy(obj)
		kind := adapter.Kind()

Saving a copy of the kind could be done outside of the loop.

---

federation/pkg/federation-controller/sync/controller.go, line 537 at r7 (raw file):

				err = fmt.Errorf("adapter for kind %s does not properly implement SchedulingAdapter.", kind)
				glog.Fatalf("Error: %v", err)
				return nil, err

Again, not sure what the point is of returning past glog.Fatalf.

---

federation/pkg/federation-controller/sync/controller.go, line 570 at r7 (raw file):

	}

	for _, cluster := range unselectedClusters {

Why is it desirable to separating clusters into a slice for each of selected and unselected? Did you consider using a mapping of cluster name to a boolean indicating selection status?

If separate iteration is preferable, consider breaking this functiondecomposing this function to simplify testing:

operations := make([]util.FederatedOperation, 0)
getOperationsForSelectedClusters(operations, ...)
getOperationsForUnselectedClusters(operations, ...)

---

federation/pkg/federation-controller/sync/controller.go, line 573 at r7 (raw file):

		clusterObj, found, err := accessor(cluster.Name)
		if err != nil {
			wrappedErr := fmt.Errorf("Failed to get %s %q from cluster %q: %v", adapter.Kind(), key, cluster.Name, err)

s/adapter.Kind()/kind/

---

federation/pkg/federation-controller/sync/controller_test.go, line 163 at r7 (raw file):

}

func TestClusterOperations(t *testing.T) {

Are there more test changes pending? The changes to clusterOperations suggest a need for more coverage than is added here.

---

Comments from Reviewable

[perotinus]

Review status: 6 of 7 files reviewed at latest revision, 15 unresolved discussions.

---

federation/pkg/federatedtypes/adapter.go, line 63 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

(No action required) Would it make sense to put the scheduling-specific stuff into its own file?

Done.

---

federation/pkg/federatedtypes/adapter.go, line 65 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

I think it would be preferable to merge this very specific proposal and have your follow-on make it generic.

I'm inclined to agree: I don't understand the HPA use case as fully as you do, so my efforts to make this generic would probably not be as effective and clean as yours.

---

federation/pkg/federatedtypes/adapter.go, line 74 at r7 (raw file):

Previously, irfanurrehman wrote…

as above, hpa has different set of properties to pass around!

That makes sense. In light of Maru's comment above, I think I'd prefer to defer this work to you, since you understand your use case better.

---

federation/pkg/federation-controller/sync/controller.go, line 449 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

(No action required) Consider simplifying as follows:

schedulingInfo, err := getSchedulingInfo(adapter, obj, key, selectedClusters, informer)
if err {
  return statusError
}

Do you mean extract a method that gets the scheduling info and hides the logic for casting and checking for isScheduleObject? I can do that, but it seems a bit odd to extract just this one scheduling method call and leave the other two inline below.

---

federation/pkg/federation-controller/sync/controller.go, line 453 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

Is this return statement necessary given that glog.Fatalf calls os.Exit?

Done.

---

federation/pkg/federation-controller/sync/controller.go, line 455 at r7 (raw file):

Previously, irfanurrehman wrote…

Please ensure that, the cluster objects are queried and passed as one set here (probably as a map), indicating which cluster has the object, and which does not (one way is use a map of same size as current available clusters with nil values identifying absence of object in that cluster).
Other option I could think of is; don't query the objects here at all; just pass the informer here (as you are doing), and let this interface or any other one in the flow later return the existing set vs scheduled set of objects (or only scheduled set of objects indicating, in which cluster to update, into which cluster to delete and which cluster to create ... (Or else I can't think of how exactly to do it ).
Probably u can think of on these lines:
say in the rs controller schedule, as a result of schedule, the local cluster rs can be deleted from some clusters also
so there has to be some way for this reconcile to be able to finalise the result comparing the original set of object vs new set of objects including all three conditions:

• if not exist in a cluster in existing set, schedule says create it.
• if exist in a cluster in existing set, schedule says update it.
• if exist in a cluster in existing set, schedule says delete it.

Also the above to be done without race conditions (means objects or set of cluster objects queried at one location to come up with the schedule result); and probably then the originally queried set and the newly scheduled set compared to arrive at cluster operations.

I agree that this is a good strategy. In the interest of getting this PR into 1.7 and not blocking, I'd prefer to defer this work.

---

federation/pkg/federation-controller/sync/controller.go, line 470 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

ditto

Done.

---

federation/pkg/federation-controller/sync/controller.go, line 522 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

Saving a copy of the kind could be done outside of the loop.

Done.

---

federation/pkg/federation-controller/sync/controller.go, line 537 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

Again, not sure what the point is of returning past glog.Fatalf.

This is probably an old habit from my Objective-C days, where C assertions were disabled in release-mode code so the pattern was to assert and then return. Removed.

---

federation/pkg/federation-controller/sync/controller.go, line 570 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

Why is it desirable to separating clusters into a slice for each of selected and unselected? Did you consider using a mapping of cluster name to a boolean indicating selection status?

If separate iteration is preferable, consider breaking this functiondecomposing this function to simplify testing:

operations := make([]util.FederatedOperation, 0)
getOperationsForSelectedClusters(operations, ...)
getOperationsForUnselectedClusters(operations, ...)

This seemed simpler in my head. I think it would have been nice to express this with set operations, but in the absence of that, the two slices seemed like a good solution.

Also, not that this code went in in a different PR.

Would you be OK deferring changing this until a later PR? I don't really want to bolt testing onto this PR, I'd rather get this in and then think about how to factor it cleanly for good tests without a looming deadline.

---

federation/pkg/federation-controller/sync/controller.go, line 573 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

s/adapter.Kind()/kind/

Done.

---

federation/pkg/federation-controller/sync/controller_test.go, line 163 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

Are there more test changes pending? The changes to clusterOperations suggest a need for more coverage than is added here.

Yes, I agree. Since the code freeze is looming, are you OK doing more extensive test changes in a follow up?

---

federation/pkg/federatedtypes/replicaset.go, line 172 at r2 (raw file):

Previously, marun (Maru Newby) wrote…

I reviewed @irfanurrehman's PR first, which used more specific hook methods instead of the generic events you propose here. Consider coordinating with him to come to agreement on a mutually acceptable path forward.

Done.

---

Comments from Reviewable

[perotinus]

Thanks for the review! I'm working on the test equivalency change now.

---

Review status: 6 of 7 files reviewed at latest revision, 15 unresolved discussions.

---

Comments from Reviewable

[k8s-ci-robot]

@perotinus: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins unit/integration	d8129ed	link	@k8s-bot unit test this
Jenkins GCE etcd3 e2e	d8129ed	link	@k8s-bot gce etcd3 e2e 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]

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

[perotinus]

OK, the test equivalency changes are pushed, and will soon be added to #46527.

[marun]

/lgtm

---

Reviewed 4 of 4 files at r9.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

federation/pkg/federation-controller/sync/controller.go, line 449 at r7 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Do you mean extract a method that gets the scheduling info and hides the logic for casting and checking for isScheduleObject? I can do that, but it seems a bit odd to extract just this one scheduling method call and leave the other two inline below.

(No action required) I meant to suggest that you do the same for the other 2 methods as well. I think it would be cleaner, but it can be done in the future if at all.

---

federation/pkg/federation-controller/sync/controller.go, line 570 at r7 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

This seemed simpler in my head. I think it would have been nice to express this with set operations, but in the absence of that, the two slices seemed like a good solution.

Also, not that this code went in in a different PR.

Would you be OK deferring changing this until a later PR? I don't really want to bolt testing onto this PR, I'd rather get this in and then think about how to factor it cleanly for good tests without a looming deadline.

Yeah, it's fine for now.

---

federation/pkg/federation-controller/sync/controller_test.go, line 163 at r7 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Yes, I agree. Since the code freeze is looming, are you OK doing more extensive test changes in a follow up?

I'm ok with that given that the proposed change shouldn't result in a change in coverage, just that the coverage could stand to be improved.

---

Comments from Reviewable

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marun, perotinus
We suggest the following additional approver: madhusudancs

Assign the PR to them by writing /assign @madhusudancs in a comment when ready.

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

• federation/OWNERS

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

[perotinus]

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

---

federation/pkg/federation-controller/sync/controller.go, line 449 at r7 (raw file):

Previously, marun (Maru Newby) wrote…

(No action required) I meant to suggest that you do the same for the other 2 methods as well. I think it would be cleaner, but it can be done in the future if at all.

OK, SGTM.

---

Comments from Reviewable

[csbell]

/approved

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 46801, 45184, 45930, 46192, 45563)