From 07bf1f8fb519cddf5b5a9002822270cb0d13741c Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 28 Jan 2019 11:58:59 -0800 Subject: [PATCH] Simplify upgrade_test - only loop during k8s master changes. --- cmd/e2e-test/upgrade_test.go | 131 ++++++++++++++++++----------------- pkg/e2e/status.go | 19 +++-- 2 files changed, 79 insertions(+), 71 deletions(-) diff --git a/cmd/e2e-test/upgrade_test.go b/cmd/e2e-test/upgrade_test.go index 05d7495d96..3b49a603c6 100644 --- a/cmd/e2e-test/upgrade_test.go +++ b/cmd/e2e-test/upgrade_test.go @@ -18,7 +18,6 @@ package main import ( "context" - "strings" "testing" "github.com/kr/pretty" @@ -66,11 +65,20 @@ func TestUpgrade(t *testing.T) { t.Logf("Ingress created (%s/%s)", s.Namespace, tc.ing.Name) s.PutStatus(e2e.Unstable) - runs := 0 - // needUpdate indicates that the Ingress sync has NOT yet been triggered - needUpdate := true + ing := waitForStableIngress(true, tc.ing, s, t) + t.Logf("GCLB resources created (%s/%s)", s.Namespace, tc.ing.Name) + + whiteboxTest(ing, s, tc.numForwardingRules, tc.numBackendServices, t) + for { - if s.MasterUpgraded() && needUpdate { + // While k8s master is upgrading, it will return a connection refused + // error for any k8s resource we try to hit. We loop until the + // master upgrade has finished. + if s.MasterUpgrading() { + continue + } + + if s.MasterUpgraded() { t.Logf("Detected master upgrade, adding a path to Ingress to force Ingress update") // force ingress update. only add path once newIng := fuzz.NewIngressBuilderFromExisting(tc.ing). @@ -84,74 +92,69 @@ func TestUpgrade(t *testing.T) { // to Unstable. It is set back to Stable after WaitForIngress below // finishes successfully. s.PutStatus(e2e.Unstable) - needUpdate = false } - } - // While k8s master is upgrading, it will return a connection refused - // error for any k8s resource we try to hit. We loop until the - // master upgrade has finished. - if s.MasterUpgrading() { - continue - } - options := &e2e.WaitForIngressOptions{ - ExpectUnreachable: runs == 0, - } - ing, err := e2e.WaitForIngress(s, tc.ing, options) - if err != nil { - if strings.Contains(err.Error(), "connection refused") { - // We ignore any connection refused errors because there is an - // unavoidable race condition between when a master upgrade is - // triggered and when WaitForIngress() is called. - continue - } - t.Fatalf("error waiting for Ingress to stabilize: %v", err) + break } + } - s.PutStatus(e2e.Stable) + // Verify the Ingress has stabilized after the master upgrade and we + // trigger an Ingress update + ing = waitForStableIngress(false, ing, s, t) + t.Logf("GCLB is stable (%s/%s)", s.Namespace, tc.ing.Name) + whiteboxTest(ing, s, tc.numForwardingRules, tc.numBackendServices, t) + + // If the Master has upgraded and the Ingress is stable, + // we delete the Ingress and exit out of the loop to indicate that + // the test is done. + deleteOptions := &fuzz.GCLBDeleteOptions{ + SkipDefaultBackend: true, + } - if runs == 0 { - t.Logf("GCLB resources created (%s/%s)", s.Namespace, tc.ing.Name) - } else { - t.Logf("GCLB is stable (%s/%s)", s.Namespace, tc.ing.Name) - } + vip := ing.Status.LoadBalancer.Ingress[0].IP + gclb, err := fuzz.GCLBForVIP(context.Background(), Framework.Cloud, vip, fuzz.FeatureValidators(features.All)) + if err != nil { + t.Fatalf("Error getting GCP resources for LB with IP = %q: %v", vip, err) + } - // Perform whitebox testing. - if len(ing.Status.LoadBalancer.Ingress) < 1 { - t.Fatalf("Ingress does not have an IP: %+v", ing.Status) - } + if err := e2e.WaitForIngressDeletion(context.Background(), gclb, s, ing, deleteOptions); err != nil { // Sometimes times out waiting + t.Errorf("e2e.WaitForIngressDeletion(..., %q, nil) = %v, want nil", ing.Name, err) + } + }) + } +} - vip := ing.Status.LoadBalancer.Ingress[0].IP - t.Logf("Ingress %s/%s VIP = %s", s.Namespace, tc.ing.Name, vip) - gclb, err := fuzz.GCLBForVIP(context.Background(), Framework.Cloud, vip, fuzz.FeatureValidators(features.All)) - if err != nil { - t.Fatalf("Error getting GCP resources for LB with IP = %q: %v", vip, err) - } +func waitForStableIngress(expectUnreachable bool, ing *v1beta1.Ingress, s *e2e.Sandbox, t *testing.T) *v1beta1.Ingress { + options := &e2e.WaitForIngressOptions{ + ExpectUnreachable: expectUnreachable, + } - // Do some cursory checks on the GCP objects. - if len(gclb.ForwardingRule) != tc.numForwardingRules { - t.Errorf("got %d fowarding rules, want %d;\n%s", len(gclb.ForwardingRule), tc.numForwardingRules, pretty.Sprint(gclb.ForwardingRule)) - } - if len(gclb.BackendService) != tc.numBackendServices { - t.Errorf("got %d backend services, want %d;\n%s", len(gclb.BackendService), tc.numBackendServices, pretty.Sprint(gclb.BackendService)) - } + ing, err := e2e.WaitForIngress(s, ing, options) + if err != nil { + t.Fatalf("error waiting for Ingress to stabilize: %v", err) + } + + s.PutStatus(e2e.Stable) + return ing +} - runs++ +func whiteboxTest(ing *v1beta1.Ingress, s *e2e.Sandbox, numForwardingRules, numBackendServices int, t *testing.T) { + if len(ing.Status.LoadBalancer.Ingress) < 1 { + t.Fatalf("Ingress does not have an IP: %+v", ing.Status) + } - // If the Master has upgraded and the Ingress is stable, - // we delete the Ingress and exit out of the loop to indicate that - // the test is done. - if s.MasterUpgraded() && !needUpdate { - deleteOptions := &fuzz.GCLBDeleteOptions{ - SkipDefaultBackend: true, - } - if err := e2e.WaitForIngressDeletion(context.Background(), gclb, s, ing, deleteOptions); err != nil { - t.Errorf("e2e.WaitForIngressDeletion(..., %q, nil) = %v, want nil", ing.Name, err) - } else { - break - } - } - } - }) + vip := ing.Status.LoadBalancer.Ingress[0].IP + t.Logf("Ingress %s/%s VIP = %s", s.Namespace, ing.Name, vip) + gclb, err := fuzz.GCLBForVIP(context.Background(), Framework.Cloud, vip, fuzz.FeatureValidators(features.All)) + if err != nil { + t.Fatalf("Error getting GCP resources for LB with IP = %q: %v", vip, err) + } + + // Do some cursory checks on the GCP objects. + if len(gclb.ForwardingRule) != numForwardingRules { + t.Errorf("got %d fowarding rules, want %d;\n%s", len(gclb.ForwardingRule), numForwardingRules, pretty.Sprint(gclb.ForwardingRule)) + } + if len(gclb.BackendService) != numBackendServices { + t.Errorf("got %d backend services, want %d;\n%s", len(gclb.BackendService), numBackendServices, pretty.Sprint(gclb.BackendService)) } } diff --git a/pkg/e2e/status.go b/pkg/e2e/status.go index f9ecaf4e26..7e9d58f98c 100644 --- a/pkg/e2e/status.go +++ b/pkg/e2e/status.go @@ -172,10 +172,16 @@ func (sm *StatusManager) setMasterUpgrading(ts string) { } func (sm *StatusManager) masterUpgrading() bool { + sm.f.lock.Lock() + defer sm.f.lock.Unlock() + return len(sm.cm.Data[masterUpgradingKey]) > 0 } func (sm *StatusManager) masterUpgraded() bool { + sm.f.lock.Lock() + defer sm.f.lock.Unlock() + return len(sm.cm.Data[masterUpgradedKey]) > 0 } @@ -185,14 +191,13 @@ func (sm *StatusManager) flush() { glog.V(3).Infof("Attempting to flush %v", sm.cm.Data) - // If master is in the process of upgrading, we exit early and turn off the - // ConfigMap informer. - if sm.masterUpgrading() && sm.informerRunning { + // If master is in the process of upgrading, we stop the informer. + if sm.informerRunning && len(sm.cm.Data[masterUpgradingKey]) > 0 { sm.stopInformer() } // Restart ConfigMap informer if it was previously shut down - if !sm.informerRunning && sm.masterUpgraded() { + if !sm.informerRunning && len(sm.cm.Data[masterUpgradedKey]) > 0 { glog.V(2).Infof("Master has successfully upgraded at %s", sm.cm.Data[masterUpgradedKey]) sm.startInformer() } @@ -204,9 +209,9 @@ func (sm *StatusManager) flush() { // order to not overwrite our ConfigMap data. if err != nil { // if the k8s master is upgrading, we suppress the error message because - // the error is expected. - if !sm.masterUpgrading() { - glog.Warningf("Error getting ConfigMap: %v", err) + // we expect a "connection refused" error in this situation. + if len(sm.cm.Data[masterUpgradingKey]) > 0 { + return } glog.Warningf("Error getting ConfigMap: %v", err)