Merge pull request #22481 from ihmccreery/better-upgrade-tests

Auto commit by PR queue bot

hack/jenkins/job-configs/kubernetes-jenkins/kubernetes-upgrades.yaml

@@ -152,7 +152,7 @@
                 export E2E_OPT="--check_version_skew=false"
                 export E2E_UP="false"
                 export E2E_DOWN="false"
-                export GINKGO_TEST_ARGS="--ginkgo.focus=\[Feature:Upgrade\].*upgrade-master"
+                export GINKGO_TEST_ARGS="--ginkgo.focus=\[Feature:MasterUpgrade\]"
         - 'kubernetes-upgrade-{provider}-{version-old}-{version-new}-{step}':
             step: 'step4-e2e-old'
             runner: '{runner-old}'
@@ -169,7 +169,7 @@
                 export E2E_OPT="--check_version_skew=false"
                 export E2E_UP="false"
                 export E2E_DOWN="false"
-                export GINKGO_TEST_ARGS="--ginkgo.focus=\[Feature:Upgrade\].*upgrade-cluster"
+                export GINKGO_TEST_ARGS="--ginkgo.focus=\[Feature:NodeUpgrade\]"
         - 'kubernetes-upgrade-{provider}-{version-old}-{version-new}-{step}':
             step: 'step6-e2e-old'
             runner: '{runner-old}'

test/e2e/cluster_upgrade.go

@@ -36,11 +36,6 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-const (
-	// version applies to upgrades; kube-push always pushes local binaries.
-	versionURLFmt = "https://storage.googleapis.com/kubernetes-release/%s/%s.txt"
-)
-
 // realVersion turns a version constant s into a version string deployable on
 // GKE.  See hack/get-build.sh for more information.
 func realVersion(s string) (string, error) {
@@ -84,12 +79,6 @@ func masterUpgradeGKE(v string) error {
 	return err
 }
 
-var masterPush = func(_ string) error {
-	// TODO(mikedanese): Make master push use the provided version.
-	_, _, err := runCmd(path.Join(testContext.RepoRoot, "hack/e2e-internal/e2e-push.sh"), "-m")
-	return err
-}
-
 var nodeUpgrade = func(f *Framework, replicas int, v string) error {
 	// Perform the upgrade.
 	var err error
@@ -106,6 +95,9 @@ var nodeUpgrade = func(f *Framework, replicas int, v string) error {
 	}
 
 	// Wait for it to complete and validate nodes and pods are healthy.
+	//
+	// TODO(ihmccreery) We shouldn't have to wait for nodes to be ready in
+	// GKE; the operation shouldn't return until they all are.
 	Logf("Waiting up to %v for all nodes to be ready after the upgrade", restartNodeReadyAgainTimeout)
 	if _, err := checkNodesReady(f.Client, restartNodeReadyAgainTimeout, testContext.CloudConfig.NumNodes); err != nil {
 		return err
@@ -115,8 +107,10 @@ var nodeUpgrade = func(f *Framework, replicas int, v string) error {
 }
 
 func nodeUpgradeGCE(rawV string) error {
+	// TODO(ihmccreery) This code path should be identical to how a user
+	// would trigger a node update; right now it's very different.
 	v := "v" + rawV
-	Logf("Preparing node upgarde by creating new instance template for %q", v)
+	Logf("Preparing node upgrade by creating new instance template for %q", v)
 	stdout, _, err := runCmd(path.Join(testContext.RepoRoot, "hack/e2e-internal/e2e-upgrade.sh"), "-P", v)
 	if err != nil {
 		return err
@@ -191,27 +185,17 @@ var _ = Describe("Upgrade [Feature:Upgrade]", func() {
 		//  - volumes
 		//  - persistent volumes
 	})
+
 	AfterEach(func() {
 		w.Cleanup()
 	})
 
-	Describe("kube-push", func() {
+	Describe("master upgrade", func() {
-		It("of master should maintain responsive services", func() {
+		It("should maintain responsive services [Feature:MasterUpgrade]", func() {
 			By("Validating cluster before master upgrade")
 			expectNoError(validate(f, svcName, rcName, ingress, replicas))
 			By("Performing a master upgrade")
-			testMasterUpgrade(ip, v, masterPush)
+			testUpgrade(ip, v, masterUpgrade)
-			By("Validating cluster after master upgrade")
-			expectNoError(validate(f, svcName, rcName, ingress, replicas))
-		})
-	})
-
-	Describe("upgrade-master", func() {
-		It("should maintain responsive services", func() {
-			By("Validating cluster before master upgrade")
-			expectNoError(validate(f, svcName, rcName, ingress, replicas))
-			By("Performing a master upgrade")
-			testMasterUpgrade(ip, v, masterUpgrade)
 			By("Checking master version")
 			expectNoError(checkMasterVersion(f.Client, v))
 			By("Validating cluster after master upgrade")
@@ -219,7 +203,7 @@ var _ = Describe("Upgrade [Feature:Upgrade]", func() {
 		})
 	})
 
-	Describe("upgrade-cluster", func() {
+	Describe("node upgrade", func() {
 		var tmplBefore, tmplAfter string
 		BeforeEach(func() {
 			if providerIs("gce") {
@@ -256,18 +240,36 @@ var _ = Describe("Upgrade [Feature:Upgrade]", func() {
 			}
 		})
 
-		It("should maintain a functioning cluster", func() {
+		It("should maintain a functioning cluster [Feature:NodeUpgrade]", func() {
 			By("Validating cluster before node upgrade")
 			expectNoError(validate(f, svcName, rcName, ingress, replicas))
 			By("Performing a node upgrade")
-			testNodeUpgrade(f, nodeUpgrade, replicas, v)
+			// Circumnavigate testUpgrade, since services don't necessarily stay up.
+			Logf("Starting upgrade")
+			expectNoError(nodeUpgrade(f, replicas, v))
+			Logf("Upgrade complete")
+			By("Checking node versions")
+			expectNoError(checkNodesVersions(f.Client, v))
+			By("Validating cluster after node upgrade")
+			expectNoError(validate(f, svcName, rcName, ingress, replicas))
+		})
+
+		It("should maintain responsive services [Feature:ExperimentalNodeUpgrade]", func() {
+			By("Validating cluster before node upgrade")
+			expectNoError(validate(f, svcName, rcName, ingress, replicas))
+			By("Performing a node upgrade")
+			testUpgrade(ip, v, func(v string) error {
+				return nodeUpgrade(f, replicas, v)
+			})
+			By("Checking node versions")
+			expectNoError(checkNodesVersions(f.Client, v))
 			By("Validating cluster after node upgrade")
 			expectNoError(validate(f, svcName, rcName, ingress, replicas))
 		})
 	})
 })
 
-func testMasterUpgrade(ip, v string, mUp func(v string) error) {
+func testUpgrade(ip, v string, upF func(v string) error) {
 	Logf("Starting async validation")
 	httpClient := http.Client{Timeout: 2 * time.Second}
 	done := make(chan struct{}, 1)
@@ -294,18 +296,18 @@ func testMasterUpgrade(ip, v string, mUp func(v string) error) {
 			// because this validation runs in another goroutine. Without this,
 			// a failure is very confusing to track down because from the logs
 			// everything looks fine.
-			msg := fmt.Sprintf("Failed to contact service during master upgrade: %v", err)
+			msg := fmt.Sprintf("Failed to contact service during upgrade: %v", err)
 			Logf(msg)
 			Failf(msg)
 		}
 	}, 200*time.Millisecond, done)
 
-	Logf("Starting master upgrade")
+	Logf("Starting upgrade")
-	expectNoError(mUp(v))
+	expectNoError(upF(v))
 	done <- struct{}{}
 	Logf("Stopping async validation")
 	wg.Wait()
-	Logf("Master upgrade complete")
+	Logf("Upgrade complete")
 }
 
 func checkMasterVersion(c *client.Client, want string) error {
@@ -325,15 +327,6 @@ func checkMasterVersion(c *client.Client, want string) error {
 	return nil
 }
 
-func testNodeUpgrade(f *Framework, nUp func(f *Framework, n int, v string) error, replicas int, v string) {
-	Logf("Starting node upgrade")
-	expectNoError(nUp(f, replicas, v))
-	Logf("Node upgrade complete")
-	By("Checking node versions")
-	expectNoError(checkNodesVersions(f.Client, v))
-	Logf("All nodes are at version %s", v)
-}
-
 func checkNodesVersions(c *client.Client, want string) error {
 	l := ListSchedulableNodesOrDie(c)
 	for _, n := range l.Items {