[Federation] Add a worker queue to the generic sync controller.

[perotinus]

This is in preparation for converting the ReplicaSet controller to be a generic sync controller.

This doesn't include support for multiple workers yet: it's not immediately obvious how to support the command-line flags for ReplicaSet (or, I suppose in general, how do TypeAdapters support external configuration via whatever flag mechanism we're using).

cc @marun

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.

[marun]

@k8s-bot ok to test

[nikhiljindal]

/approve

LGTM. Will add lgtm label once you verify that e2e tests pass.

The unit test failure seems to be for deleting a federated service. cc @shashidharatd hope we didnt introduce any flakiness with recent service controller changes.

[shashidharatd]

@nikhiljindal, yeah it is a flake in deleting a federated service. it has been showing up more frequently in recent builds. I am surprised on what causes this flakiness, and why it was not showing up before service controller refactoring. the delete finalizers part is almost untouched by the refactoring.
Also i am unable to reproduce this failed test when i run locally. Appreciate any pointers to debug further to find the root cause. Thanks.

[shashidharatd]

@k8s-bot unit test this

[shashidharatd]

The flakiness in unit test of federated service deletion is fixed by #45265. Now there is another flake in pkg/kubelet/kuberuntime for which i have raised an issue #45281

[perotinus]

@nikhiljindal @shashidharatd @marun Can you someone PTAL and run the Federation e2e tests? This has been updated to have the worker method do redelivery, which makes the reconcile method a bit simpler.

[shashidharatd]

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

[nikhiljindal]

statusError will deliver this as s.deliver(namespacedName, 0, true) which is a change in behavior. Is that intended?

[perotinus]

That is correct: I'm inclined to believe that returning true before was a typo, since this is an error condition and diverging like this would have warranted at least a comment as to why.

[marun]

I think this PR is not controversial and modulo avoiding returning errors from reconcile it should merge before #45374.

---

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

---

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

type reconciliationStatus int

const (

(No action required) Would it make sense to centralize status rather than defining for each remaining controller?

---

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

		item := obj.(*util.DelayingDelivererItem)
		namespacedName := item.Value.(*types.NamespacedName)
		status, err := s.reconcile(*namespacedName)

I think that @shashidharatd's recent work with the services PR suggests having the reconcile handle errors internally to ensure traceability.

---

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

			s.deliver(*namespacedName, s.clusterAvailableDelay, false)
		default:
			glog.Errorf("Unhandled reconciliation status: %s", status)

Is this just defensive? Seems like something better handled via testing.

---

Comments from Reviewable

[perotinus]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

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

Previously, marun (Maru Newby) wrote…

(No action required) Would it make sense to centralize status rather than defining for each remaining controller?

Absolutely! Not sure where it should live, though: it seems like it could live in the sync package since it will eventually, ideally, be an implementation detail of the sync controller and not used elsewhere.

---

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

Previously, marun (Maru Newby) wrote…

I think that @shashidharatd's recent work with the services PR suggests having the reconcile handle errors internally to ensure traceability.

That seems reasonable.

---

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

Previously, marun (Maru Newby) wrote…

Is this just defensive? Seems like something better handled via testing.

Agreed. I copied this from the reconcile loop in the replicaset controller without thinking too much about it. There doesn't seem to be harm in leaving it, though it scares me a bit to think how this codepath would be triggered–it would require casting–so I think I'll remove it.

---

Comments from Reviewable

[marun]

/lgtm

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

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

Previously, perotinus (Jonathan MacMillan) wrote…

Absolutely! Not sure where it should live, though: it seems like it could live in the sync package since it will eventually, ideally, be an implementation detail of the sync controller and not used elsewhere.

Makes sense to me.

---

Comments from Reviewable

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

• federation/pkg/federation-controller/OWNERS [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 45453, 45307, 44987)

[marun]

I didn't notice until I went to rebase on this, but shouldn't this be s.ClusterAvailableDelay?