[Federation] Add simple upgrade test #43500

marun · 2017-03-22T05:45:02Z

This PR adds a simple upgrade test that targets all registered federated types.

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

k8s-reviewable · 2017-03-22T05:45:08Z

This change is

marun · 2017-03-22T06:34:29Z

This is admittedly too big even excluding #42025's commits, and it might be easier to review each commit separately. What's the timeline on upgrade testing, and will it be able to merge before master is opened back up?

perotinus · 2017-03-22T17:25:05Z

@marun Do you want me to start reviewing this yet? Are you planning to do something to make it easier to review, or should I review the three commits you mentioned?

marun · 2017-03-22T19:39:02Z

@perotinus I'm happy to break this up into separate PRs if that makes sense to you, but if you're willing to tackle it as is please do.

marun · 2017-03-22T19:40:32Z

@perotinus I think the separate commits are at least coherent. The 2 big ones are just refactor, nothing new is being implemented.

marun · 2017-03-22T20:55:53Z

/assign @shashidharatd

perotinus · 2017-03-22T21:47:22Z

federation/BUILD

@@ -26,6 +26,7 @@ filegroup(
        "//federation/cmd/genfeddocs:all-srcs",
        "//federation/cmd/kubefed:all-srcs",
        "//federation/develop:all-srcs",
+        "//federation/pkg/crud:all-srcs",


Naming nit: crudmachinery? crudtestmachinery? I worry that crud isn't a descriptive enough name, and could be mistaken for something that deals with CRUD operations in federation production code rather than as a test harness.

Or, is part of this change reconceptualizing the CRUD machinery as something to use in production code?

perotinus · 2017-03-22T21:49:00Z

federation/pkg/crud/resource.go

 // type for which an implementation of this interface exists.
-//
-// TODO reuse resource adapters defined for use with a generic controller as per
-// https://github.com/kubernetes/kubernetes/pull/41050
 type ResourceAdapter interface {


As long as you're moving this, perhaps it's worth documenting the methods in the interface?

perotinus · 2017-03-22T21:49:38Z

federation/pkg/crud/secret.go

@@ -0,0 +1,93 @@
+/*


Didn't this code exist somewhere else already? I thought I reviewed this previously. Is there something you can base this off of?

The goal of this refactor commit is separating the integration-specific stuff from the stuff that can be reused in integration/e2e/controllers. This is the stuff that can be reused.

perotinus · 2017-03-22T21:50:24Z

federation/pkg/crud/util/crudhelper.go

+// federated types via the Federation API and validates that the
+// results of those operations are propagated to clusters that are
+// members of a federation.
+type CRUDHelper struct {


CRUDOperationExerciser? CRUDOperationValidator?

perotinus · 2017-03-22T21:55:39Z

federation/pkg/crud/util/crudhelper.go

@@ -0,0 +1,215 @@
+/*


This too. Can you base this off of something else?

ditto. This was moved from test/integration/federation/framework. Are you not seeing the code being moved from there to here?

No, I'm not. I'm seeing a new file rather than a diff from the old file. Lots of green, and St. Patrick's Day is over now.

perotinus · 2017-03-22T21:59:04Z

test/e2e_federation/framework/cluster.go

+
+type ClusterMap map[string]*Cluster
+
+// cluster keeps track of the assorted objects and state related to each cluster


perotinus · 2017-03-22T22:00:51Z

test/e2e_federation/framework/cluster.go

@@ -0,0 +1,164 @@
+/*


Is it worth basing this off of the file that has much of its source? It would be nice not to lose the history.

As per our discussion online, git doesn't support 'forking' a file with history.

perotinus · 2017-03-22T22:01:09Z

test/e2e_federation/framework/cluster.go

+	}
+}
+
+func ClusterIsReadyOrFail(f *Framework, context *E2EContext) {


Can you document this while you're here?

perotinus · 2017-03-22T22:02:17Z

hack/.linted_packages

@@ -388,7 +388,6 @@ staging/src/k8s.io/kube-aggregator/pkg/client/listers/apiregistration/v1alpha1
 staging/src/k8s.io/sample-apiserver


Do you need to rebase your commit? Looks like this should follow through from where it was added.

I don't follow. All I'm doing here is disabling linting for test/e2e_federation/upgrade so it's possible to dot import ginkgo for the upgrade test.

Ah, OK. I assumed this some sort of automagically-generated updated that didn't get propagated.

perotinus · 2017-03-22T22:02:44Z

test/e2e_federation/framework/crud.go

@@ -0,0 +1,44 @@
+/*


Does this belong in this commit?

This allows the crud helper to be used for the upgrade test.

perotinus · 2017-03-22T22:03:14Z

test/e2e_federation/upgrades/generic.go

+)
+
+// GenericUpgradeTest validates that a federated resource remains
+// propagated before and after a controlplane upgrade


control plane

perotinus · 2017-03-22T22:03:24Z

test/e2e_federation/upgrades/generic.go

+}
+
+// Setup creates a resource and validates its propagation to member clusters
+func (t *GenericUpgradeTest) Setup(f *fedframework.Framework) {


While your logic regarding SetUp vs Setup may be sound, I'm just being consistent with the interface I'm implementing:

https://github.com/kubernetes/kubernetes/blob/master/test/e2e_federation/upgrades/upgrade.go#L43

The federation upgrade interface is based on the kubernetes one:

https://github.com/kubernetes/kubernetes/blob/master/test/e2e/upgrades/upgrade.go#L46

perotinus · 2017-03-22T22:04:44Z

test/e2e_federation/upgrades/generic.go

+}
+
+// Test validates that a resource remains propagated post-upgrade
+func (t *GenericUpgradeTest) Test(f *fedframework.Framework, done <-chan struct{}, upgrade FederationUpgradeType) {


Why isn't the client responsible for waiting on the channel?

I'm not sure what you mean. As per the documentation for the kube upgrade framework, the Test method is intended to support validating behavior during the upgrade and afterwards, with the done channel providing the signal for when the upgrade has been completed. In the case of federation, there goal is currently only to test after the upgrade has been completed, as indicated by the closing of the channel.

perotinus · 2017-03-22T22:04:52Z

test/e2e_federation/upgrades/generic.go

+}
+
+// Teardown cleans up remaining resources
+func (t *GenericUpgradeTest) Teardown(f *fedframework.Framework) {


ditto - implementing an interface.

marun · 2017-03-24T18:20:37Z

Review status: 0 of 44 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

test/e2e_federation/upgrades/generic.go, line 31 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

control plane

Done.

test/e2e_federation/upgrades/generic.go, line 39 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

SetUp

While your logic regarding SetUp vs Setup may be sound, I'm just being consistent with the interface I'm implementing:

https://github.com/kubernetes/kubernetes/blob/master/test/e2e_federation/upgrades/upgrade.go#L43

The federation upgrade interface is based on the kubernetes one:

https://github.com/kubernetes/kubernetes/blob/master/test/e2e/upgrades/upgrade.go#L46

test/e2e_federation/upgrades/generic.go, line 50 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Why isn't the client responsible for waiting on the channel?

I'm not sure what you mean. As per the documentation for the kube upgrade framework, the Test method is intended to support validating behavior during the upgrade and afterwards, with the done channel providing the signal for when the upgrade has been completed. In the case of federation, there goal is currently only to test after the upgrade has been completed, as indicated by the closing of the channel.

test/e2e_federation/upgrades/generic.go, line 57 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

TearDown

ditto - implementing an interface.

Comments from Reviewable

marun · 2017-03-24T18:20:53Z

ugh, reviewable fail

marun · 2017-04-13T21:20:13Z

rebased

marun · 2017-04-13T23:20:34Z

@k8s-bot bazel test this

marun · 2017-04-17T16:13:52Z

rebased

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

marun · 2017-04-17T17:24:38Z

Updated bazel

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

marun · 2017-04-17T20:32:06Z

rebased

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

marun · 2017-04-17T20:33:01Z

@shashidharatd Thoughts?

shashidharatd · 2017-04-18T04:27:08Z

test/e2e_federation/upgrade.go

@@ -25,7 +25,7 @@ import (
 	. "github.com/onsi/ginkgo"
 )

-var upgradeTests = []upgrades.Test{}
+var upgradeTests = upgrades.SimpleUpgradeTests()


Since we will be adding upgrade tests for more than simple types, please add SimpleUpgradeTests to the upgradeTests slice instead assigning.

YAGNI - whoever ends up adding more ~~types~~ tests can figure out how best to add to the slice.

Also, I'm calling it 'simple' for now, but I anticipate being able to perform upgrade tests generically on all resources by the end of 1.7.

shashidharatd · 2017-04-18T04:29:30Z

test/e2e_federation/upgrades/simple.go

+
+// SecretUpgradeTest validates that a secret remains propagated before
+// and after a controlplane upgrade
+func SimpleUpgradeTests() []Test {


comment not matching function

shashidharatd · 2017-04-18T04:30:29Z

@marun, a couple of minor issues, otherwise LGTM

shashidharatd · 2017-04-18T16:28:41Z

test/e2e_federation/upgrades/simple.go

+}
+
+// SecretUpgradeTests collects simple upgrade tests for registered federated types
+func SimpleUpgradeTests() []Test {


SecretUpgradeTests should have been SimpleUpgradeTests in comment

Oops. Done.

shashidharatd · 2017-04-19T03:02:26Z

/lgtm

k8s-github-robot · 2017-04-19T04:37:01Z

[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:

~~hack/OWNERS~~ [shashidharatd]
~~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

k8s-github-robot · 2017-04-19T05:20:25Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 22, 2017

k8s-github-robot assigned csbell Mar 22, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Mar 22, 2017

marun force-pushed the fed-secret-upgrade-test branch from fbc75b8 to b1f6bc9 Compare March 22, 2017 06:15

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2017

marun force-pushed the fed-secret-upgrade-test branch from b1f6bc9 to 3665749 Compare March 22, 2017 06:17

marun changed the title ~~Fed secret upgrade test~~ WIP [Federation] Add upgrade test for secrets Mar 22, 2017

marun force-pushed the fed-secret-upgrade-test branch 2 times, most recently from 19c10a7 to 6282955 Compare March 22, 2017 06:26

marun force-pushed the fed-secret-upgrade-test branch 2 times, most recently from 76121b6 to 0418044 Compare March 22, 2017 15:13

marun force-pushed the fed-secret-upgrade-test branch from 0418044 to 16af4f5 Compare March 22, 2017 18:15

marun mentioned this pull request Mar 22, 2017

[Federation] refactor controller for secrets #41050

Merged

marun changed the title ~~WIP [Federation] Add upgrade test for secrets~~ [Federation] Add upgrade test for secrets Mar 22, 2017

marun mentioned this pull request Mar 22, 2017

[Federation] Add type-agnostic e2e crud test #43194

Merged

k8s-ci-robot assigned shashidharatd Mar 22, 2017

perotinus reviewed Mar 22, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2017

csbell added the sig/federation label Apr 13, 2017

marun force-pushed the fed-secret-upgrade-test branch from c1e763d to ea103db Compare April 13, 2017 21:19

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2017

marun force-pushed the fed-secret-upgrade-test branch from ea103db to d2ae2b2 Compare April 13, 2017 21:20

marun force-pushed the fed-secret-upgrade-test branch from d2ae2b2 to a32936b Compare April 17, 2017 16:13

marun force-pushed the fed-secret-upgrade-test branch from a32936b to d146cb5 Compare April 17, 2017 17:24

marun force-pushed the fed-secret-upgrade-test branch from d146cb5 to 73abdb4 Compare April 17, 2017 20:31

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2017

shashidharatd reviewed Apr 18, 2017

View reviewed changes

marun force-pushed the fed-secret-upgrade-test branch from 73abdb4 to 8d0386f Compare April 18, 2017 06:41

shashidharatd reviewed Apr 18, 2017

View reviewed changes

fed: Add simple upgrade test

9dc74d6

marun force-pushed the fed-secret-upgrade-test branch from 8d0386f to 9dc74d6 Compare April 18, 2017 22:46

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2017

k8s-github-robot merged commit bdc2b20 into kubernetes:master Apr 19, 2017

marun deleted the fed-secret-upgrade-test branch April 19, 2017 17:59


		type ClusterMap map[string]*Cluster

		// cluster keeps track of the assorted objects and state related to each cluster

		@@ -388,7 +388,6 @@ staging/src/k8s.io/kube-aggregator/pkg/client/listers/apiregistration/v1alpha1
		staging/src/k8s.io/sample-apiserver

[Federation] Add simple upgrade test #43500

[Federation] Add simple upgrade test #43500

Conversation

marun commented Mar 22, 2017 • edited

k8s-reviewable commented Mar 22, 2017

marun commented Mar 22, 2017

perotinus commented Mar 22, 2017

marun commented Mar 22, 2017

marun commented Mar 22, 2017

marun commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun Mar 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun commented Mar 24, 2017

marun commented Mar 24, 2017

marun commented Apr 13, 2017

marun commented Apr 13, 2017

marun commented Apr 17, 2017

marun commented Apr 17, 2017

marun commented Apr 17, 2017

marun commented Apr 17, 2017

Choose a reason for hiding this comment

marun Apr 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashidharatd commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashidharatd commented Apr 19, 2017

k8s-github-robot commented Apr 19, 2017

k8s-github-robot commented Apr 19, 2017

marun commented Mar 22, 2017 •

edited

marun Mar 24, 2017 •

edited

marun Apr 18, 2017 •

edited