[Federation] Cleanup e2e framework

[marun]

This PR is intended to simplify maintenance of the federation e2e tests:

• refactor cluster helpers into framework so they can be reused by upgrade tests
• remove unused functions
• move backend pod management to the tests that require it rather than relying on the common cluster object
• use a slice instead of a map to manage cluster objects to make it simpler for tests that need it to find the primary cluster themselves

Commit fed: Refactor e2e cluster functions into framework for reuse was originally included in #43500, but is included here because it formed the basis for this cleanup.

cc: @kubernetes/sig-federation-pr-reviews @perotinus

[k8s-reviewable]

This change is 

[marun]

rebased

[marun]

@k8s-bot gce etcd3 e2e test this

[perotinus]

Review status: 0 of 15 files reviewed at latest revision, 4 unresolved discussions.

---

test/e2e_federation/service.go, line 249 at r1 (raw file):

					// Delete the backend pod from the shard which is local to the discovery pod.
					primaryCluster := clusters[0]

Hmm. I'm not sure how I feel about this: it doesn't seem like something that we can guarantee or check except by careful code review. How do you guarantee that the first object is the primary cluster, and that we never get it wrong when modifying related code later?

---

test/e2e_federation/framework/cluster.go, line 83 at r1 (raw file):

// waitForAllRegisteredClusters waits for all clusters defined in e2e context to be created
// return ClusterList until the listed cluster items equals clusterCount

What does this bit of the comment mean? How is it returning "until"?

---

test/e2e_federation/framework/cluster.go, line 109 at r1 (raw file):

	cfgOverride := &clientcmd.ConfigOverrides{
		ClusterInfo: clientcmdapi.Cluster{
			Server: c.Spec.ServerAddressByClientCIDRs[0].ServerAddress,

Do we know for sure that the first item in this list is the one we want? Could this be abstracted into a method on the cluster?

---

test/e2e_federation/framework/framework.go, line 236 at r1 (raw file):

func (f *Framework) GetRegisteredClusters() ClusterSlice {
	return getRegisteredClusters(f)

Why not just inline that code here?

---

Comments from Reviewable

[marun]

Review status: 0 of 15 files reviewed at latest revision, 4 unresolved discussions.

---

test/e2e_federation/service.go, line 249 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Hmm. I'm not sure how I feel about this: it doesn't seem like something that we can guarantee or check except by careful code review. How do you guarantee that the first object is the primary cluster, and that we never get it wrong when modifying related code later?

Picking the first cluster in the list is exactly what the previous code was doing, and I'm assuming it works because the order of retrieval is consistent across the test and the controller. I'm just moving responsibility to the only test that cares about that property.

---

test/e2e_federation/framework/cluster.go, line 83 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

What does this bit of the comment mean? How is it returning "until"?

I haven't changed this function or its comment, just moved it, and this code goes away in #44073.

---

test/e2e_federation/framework/cluster.go, line 109 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Do we know for sure that the first item in this list is the one we want? Could this be abstracted into a method on the cluster?

I'm not changing behavior here, and this code goes away in #44073.

---

test/e2e_federation/framework/framework.go, line 236 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Why not just inline that code here?

I wanted to put the cluster-related code in one place to simplify maintenance. I don't think there's an advantage to bulking up the framework type, which is already a catch-all.

---

Comments from Reviewable

[marun]

rebased

[shashidharatd]

Overall good job. LGTM
/lgtm

---

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

---

Comments from Reviewable

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun, shashidharatd

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

• test/e2e_federation/OWNERS [marun]

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

[shashidharatd]

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

[k8s-ci-robot]

@marun: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	ea82508	link	@k8s-bot pull-kubernetes-federation-e2e-gce 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.

[marun]

@shashidharatd that job is broken (and not by this PR)

[shashidharatd]

@marun, should not be a problem to merge this pr. was just hoping the pre-merge tests started working. :)

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 44191, 44117, 44072)