From 16d55468fd4b17cd0288a819103495d674c12794 Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Mon, 9 Jul 2018 16:24:30 +0000 Subject: [PATCH 1/2] Move flag --- test/conformance/route_test.go | 2 +- test/conformance/service_test.go | 2 +- test/e2e/autoscale_test.go | 2 -- test/e2e/helloworld_test.go | 2 +- test/request.go | 4 ++-- 5 files changed, 5 insertions(+), 7 deletions(-) diff --git a/test/conformance/route_test.go b/test/conformance/route_test.go index 7c1ed83aae0f..e79d05a8c9a3 100644 --- a/test/conformance/route_test.go +++ b/test/conformance/route_test.go @@ -80,7 +80,7 @@ func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.Sugared t.Fatalf("Error fetching Route %s: %v", names.Route, err) } - err = test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, updatedRoute.Status.Domain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText") + err = test.WaitForEndpointState(clients.Kube, logger, updatedRoute.Status.Domain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText") if err != nil { t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, updatedRoute.Status.Domain, expectedText, err) } diff --git a/test/conformance/service_test.go b/test/conformance/service_test.go index 0afa4b75b766..f0d2283516b1 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -61,7 +61,7 @@ func updateServiceWithImage(clients *test.Clients, names test.ResourceNames, ima // Shamelessly cribbed from route_test. We expect the Route and Configuration to be ready if the Service is ready. func assertServiceResourcesUpdated(t *testing.T, logger *zap.SugaredLogger, clients *test.Clients, names test.ResourceNames, routeDomain, expectedGeneration, expectedText string) { // TODO(#1178): Remove "Wait" from all checks below this point. - err := test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, routeDomain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText") + err := test.WaitForEndpointState(clients.Kube, logger, routeDomain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText") if err != nil { t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, routeDomain, expectedText, err) } diff --git a/test/e2e/autoscale_test.go b/test/e2e/autoscale_test.go index 3b690e4788d0..5ebf566f7478 100644 --- a/test/e2e/autoscale_test.go +++ b/test/e2e/autoscale_test.go @@ -57,7 +57,6 @@ func generateTrafficBurst(clients *test.Clients, logger *zap.SugaredLogger, num go func() { test.WaitForEndpointState(clients.Kube, logger, - test.Flags.ResolvableDomain, domain, test.EventuallyMatchesBody(autoscaleExpectedOutput), "MakingConcurrentRequests") @@ -158,7 +157,6 @@ func TestAutoscaleUpDownUp(t *testing.T) { err = test.WaitForEndpointState( clients.Kube, logger, - test.Flags.ResolvableDomain, domain, test.EventuallyMatchesBody(autoscaleExpectedOutput), "CheckingEndpointAfterUpdating") diff --git a/test/e2e/helloworld_test.go b/test/e2e/helloworld_test.go index 6e3b27f6424a..605f6b725f82 100644 --- a/test/e2e/helloworld_test.go +++ b/test/e2e/helloworld_test.go @@ -58,7 +58,7 @@ func TestHelloWorld(t *testing.T) { } domain := route.Status.Domain - err = test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, domain, test.MatchesBody(helloWorldExpectedOutput), "HelloWorldServesText") + err = test.WaitForEndpointState(clients.Kube, logger, domain, test.MatchesBody(helloWorldExpectedOutput), "HelloWorldServesText") if err != nil { t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, domain, helloWorldExpectedOutput, err) } diff --git a/test/request.go b/test/request.go index fc1c28f4ddcf..abe6573b693d 100644 --- a/test/request.go +++ b/test/request.go @@ -65,12 +65,12 @@ func EventuallyMatchesBody(expected string) spoof.ResponseChecker { // the domain in the request headers, otherwise it will make the request directly to domain. // desc will be used to name the metric that is emitted to track how long it took for the // domain to get into the state checked by inState. Commas in `desc` must be escaped. -func WaitForEndpointState(kubeClientset *kubernetes.Clientset, logger *zap.SugaredLogger, resolvableDomain bool, domain string, inState spoof.ResponseChecker, desc string) error { +func WaitForEndpointState(kubeClientset *kubernetes.Clientset, logger *zap.SugaredLogger, domain string, inState spoof.ResponseChecker, desc string) error { metricName := fmt.Sprintf("WaitForEndpointState/%s", desc) _, span := trace.StartSpan(context.Background(), metricName) defer span.End() - client, err := spoof.New(kubeClientset, logger, domain, resolvableDomain) + client, err := spoof.New(kubeClientset, logger, domain, Flags.ResolvableDomain) if err != nil { return err } From 534150390fd919f6922cb05fab222834ea6513c5 Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Mon, 9 Jul 2018 16:47:23 +0000 Subject: [PATCH 2/2] Add Retrying to augment ResponseChecker --- test/conformance/blue_green_test.go | 3 +-- test/conformance/route_test.go | 8 +++++++- test/conformance/service_test.go | 8 +++++++- test/e2e/autoscale_test.go | 7 +++++-- test/e2e/build_test.go | 4 +++- test/e2e/helloworld_test.go | 8 +++++++- test/request.go | 19 ++++++++++++++++--- test/spoof/spoof.go | 13 ------------- 8 files changed, 46 insertions(+), 24 deletions(-) diff --git a/test/conformance/blue_green_test.go b/test/conformance/blue_green_test.go index e16ebbf42d65..b4ae54566581 100644 --- a/test/conformance/blue_green_test.go +++ b/test/conformance/blue_green_test.go @@ -60,8 +60,7 @@ func probeDomain(logger *zap.SugaredLogger, clients *test.Clients, domain string return err } // TODO(tcnghia): Replace this probing with Status check when we have them. - client.RetryCodes = []int{http.StatusNotFound, http.StatusServiceUnavailable} - _, err = client.Poll(req, test.MatchesAny) + _, err = client.Poll(req, test.Retrying(test.MatchesAny, http.StatusNotFound, http.StatusServiceUnavailable)) return err } diff --git a/test/conformance/route_test.go b/test/conformance/route_test.go index e79d05a8c9a3..3b4db5f37d4b 100644 --- a/test/conformance/route_test.go +++ b/test/conformance/route_test.go @@ -19,6 +19,7 @@ limitations under the License. package conformance import ( + "net/http" "strings" "testing" @@ -80,7 +81,12 @@ func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.Sugared t.Fatalf("Error fetching Route %s: %v", names.Route, err) } - err = test.WaitForEndpointState(clients.Kube, logger, updatedRoute.Status.Domain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText") + err = test.WaitForEndpointState( + clients.Kube, + logger, + updatedRoute.Status.Domain, + test.Retrying(test.EventuallyMatchesBody(expectedText), http.StatusServiceUnavailable, http.StatusNotFound), + "WaitForEndpointToServeText") if err != nil { t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, updatedRoute.Status.Domain, expectedText, err) } diff --git a/test/conformance/service_test.go b/test/conformance/service_test.go index f0d2283516b1..ff261cba7082 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -20,6 +20,7 @@ package conformance import ( "encoding/json" + "net/http" "strings" "testing" @@ -61,7 +62,12 @@ func updateServiceWithImage(clients *test.Clients, names test.ResourceNames, ima // Shamelessly cribbed from route_test. We expect the Route and Configuration to be ready if the Service is ready. func assertServiceResourcesUpdated(t *testing.T, logger *zap.SugaredLogger, clients *test.Clients, names test.ResourceNames, routeDomain, expectedGeneration, expectedText string) { // TODO(#1178): Remove "Wait" from all checks below this point. - err := test.WaitForEndpointState(clients.Kube, logger, routeDomain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText") + err := test.WaitForEndpointState( + clients.Kube, + logger, + routeDomain, + test.Retrying(test.EventuallyMatchesBody(expectedText), http.StatusNotFound), + "WaitForEndpointToServeText") if err != nil { t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, routeDomain, expectedText, err) } diff --git a/test/e2e/autoscale_test.go b/test/e2e/autoscale_test.go index 5ebf566f7478..e87388ff31e5 100644 --- a/test/e2e/autoscale_test.go +++ b/test/e2e/autoscale_test.go @@ -19,6 +19,7 @@ limitations under the License. package e2e import ( + "net/http" "strings" "testing" @@ -58,7 +59,7 @@ func generateTrafficBurst(clients *test.Clients, logger *zap.SugaredLogger, num test.WaitForEndpointState(clients.Kube, logger, domain, - test.EventuallyMatchesBody(autoscaleExpectedOutput), + test.Retrying(test.EventuallyMatchesBody(autoscaleExpectedOutput), http.StatusNotFound), "MakingConcurrentRequests") concurrentRequests <- true }() @@ -158,7 +159,9 @@ func TestAutoscaleUpDownUp(t *testing.T) { clients.Kube, logger, domain, - test.EventuallyMatchesBody(autoscaleExpectedOutput), + // Istio doesn't expose a status for us here: https://github.com/istio/istio/issues/6082 + // TODO(tcnghia): Remove this when https://github.com/istio/istio/issues/882 is fixed. + test.Retrying(test.EventuallyMatchesBody(autoscaleExpectedOutput), http.StatusNotFound, http.StatusServiceUnavailable), "CheckingEndpointAfterUpdating") if err != nil { t.Fatalf(`The endpoint for Route %s at domain %s didn't serve diff --git a/test/e2e/build_test.go b/test/e2e/build_test.go index 3b0a255dd2c9..fdaf4fb8ae26 100644 --- a/test/e2e/build_test.go +++ b/test/e2e/build_test.go @@ -18,6 +18,7 @@ limitations under the License. package e2e import ( + "net/http" "strings" "testing" @@ -70,7 +71,8 @@ func TestBuildAndServe(t *testing.T) { } domain := route.Status.Domain - if err := test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, domain, test.MatchesBody(helloWorldExpectedOutput), "HelloWorldServesText"); err != nil { + endState := test.Retrying(test.MatchesBody(helloWorldExpectedOutput), http.StatusNotFound) + if err := test.WaitForEndpointState(clients.Kube, logger, domain, endState, "HelloWorldServesText"); err != nil { t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, domain, helloWorldExpectedOutput, err) } diff --git a/test/e2e/helloworld_test.go b/test/e2e/helloworld_test.go index 605f6b725f82..713281a7ba50 100644 --- a/test/e2e/helloworld_test.go +++ b/test/e2e/helloworld_test.go @@ -19,6 +19,7 @@ limitations under the License. package e2e import ( + "net/http" "strings" "testing" @@ -58,7 +59,12 @@ func TestHelloWorld(t *testing.T) { } domain := route.Status.Domain - err = test.WaitForEndpointState(clients.Kube, logger, domain, test.MatchesBody(helloWorldExpectedOutput), "HelloWorldServesText") + err = test.WaitForEndpointState( + clients.Kube, + logger, + domain, + test.Retrying(test.MatchesBody(helloWorldExpectedOutput), http.StatusNotFound), + "HelloWorldServesText") if err != nil { t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, domain, helloWorldExpectedOutput, err) } diff --git a/test/request.go b/test/request.go index abe6573b693d..aac70b5b3275 100644 --- a/test/request.go +++ b/test/request.go @@ -35,6 +35,22 @@ func MatchesAny(_ *spoof.Response) (bool, error) { return true, nil } +// Retrying modifies a ResponseChecker to retry certain response codes. +func Retrying(rc spoof.ResponseChecker, codes ...int) spoof.ResponseChecker { + return func(resp *spoof.Response) (bool, error) { + for _, code := range codes { + if resp.StatusCode == code { + // Returning (false, nil) causes SpoofingClient.Poll to retry. + // sc.logger.Infof("Retrying for code %v", resp.StatusCode) + return false, nil + } + } + + // If we didn't match any retryable codes, invoke the ResponseChecker that we wrapped. + return rc(resp) + } +} + // MatchesBody checks that the *first* response body matches the "expected" body, otherwise failing. func MatchesBody(expected string) spoof.ResponseChecker { return func(resp *spoof.Response) (bool, error) { @@ -75,9 +91,6 @@ func WaitForEndpointState(kubeClientset *kubernetes.Clientset, logger *zap.Sugar return err } - // TODO(#348): The ingress endpoint tends to return 503's and 404's - client.RetryCodes = []int{http.StatusServiceUnavailable, http.StatusNotFound} - req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s", domain), nil) if err != nil { return err diff --git a/test/spoof/spoof.go b/test/spoof/spoof.go index 0772beee4e2c..b7bc57ea014b 100644 --- a/test/spoof/spoof.go +++ b/test/spoof/spoof.go @@ -68,8 +68,6 @@ type SpoofingClient struct { RequestInterval time.Duration RequestTimeout time.Duration - RetryCodes []int - endpoint string domain string @@ -170,17 +168,6 @@ func (sc *SpoofingClient) Poll(req *http.Request, inState ResponseChecker) (*Res return true, err } - // TODO(jonjohnson): This could just be pulled out into a retrying ResponseChecker middleware thing. - if resp.StatusCode != http.StatusOK { - for _, code := range sc.RetryCodes { - if resp.StatusCode == code { - sc.logger.Infof("Retrying for code %v", resp.StatusCode) - return false, nil - } - } - return true, fmt.Errorf("Status code %d was not a retriable code (%v)", resp.StatusCode, sc.RetryCodes) - } - return inState(resp) })