From 9e74fd1ed6bc33d1a5e6795cf181c04bc5b187bd Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Fri, 29 Jun 2018 15:59:13 -0700 Subject: [PATCH] Assert against Revision generation annotation instead of config spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In https://github.com/knative/serving/pull/475 I removed assertions against Configuratoin.Spec.Generation b/c it is a hack and not part of the knative spec. In https://github.com/knative/serving/pull/600 I updated the conformance tests to assert against the Revision annotation which contains the generation. BUT THEN in https://github.com/knative/serving/pull/778 when I completely re-wrote the tests to no longer use Ginkgo, I accidentally until both of those changes, so this commit puts them back 😅. BONUS: I hit a case where the length of the loadbalancer ingresses was 0 and got a panic, so if that happens again we'll get an informative error instead. --- test/conformance/route_test.go | 14 +++++--------- test/conformance/service_test.go | 14 +++++--------- test/request.go | 3 +++ test/states.go | 28 +++++++++++++++++++--------- 4 files changed, 32 insertions(+), 27 deletions(-) diff --git a/test/conformance/route_test.go b/test/conformance/route_test.go index 33e32bbaa001..a893a2be56ba 100644 --- a/test/conformance/route_test.go +++ b/test/conformance/route_test.go @@ -18,7 +18,6 @@ limitations under the License. package conformance import ( - "fmt" "strings" "testing" @@ -61,17 +60,14 @@ func updateConfigWithImage(clients *test.Clients, names test.ResourceNames, imag }, } patchBytes, err := json.Marshal(patches) - newConfig, err := clients.Configs.Patch(names.Config, types.JSONPatchType, patchBytes, "") + _, err = clients.Configs.Patch(names.Config, types.JSONPatchType, patchBytes, "") if err != nil { return err } - if newConfig.Spec.Generation != int64(2) { - return fmt.Errorf("The spec was updated so the Generation should be 2 but it was actually %d", newConfig.Spec.Generation) - } return nil } -func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.SugaredLogger, clients *test.Clients, names test.ResourceNames, expectedText string) { +func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.SugaredLogger, clients *test.Clients, names test.ResourceNames, expectedGeneration, expectedText string) { logger.Infof("When the Route reports as Ready, everything should be ready.") err := test.WaitForRouteState(clients.Routes, names.Route, func(r *v1alpha1.Route) (bool, error) { if cond := r.Status.GetCondition(v1alpha1.RouteConditionReady); cond == nil { @@ -99,7 +95,7 @@ func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.Sugared // We want to verify that the endpoint works as soon as Ready: True, but there are a bunch of other pieces of state that we validate for conformance. logger.Infof("The Revision will be marked as Ready when it can serve traffic") - err = test.CheckRevisionState(clients.Revisions, names.Revision, test.IsRevisionReady()) + err = test.CheckRevisionState(clients.Revisions, names.Revision, test.IsRevisionReady(expectedGeneration)) if err != nil { t.Fatalf("Revision %s did not become ready to serve traffic: %v", names.Revision, err) } @@ -174,7 +170,7 @@ func TestRouteCreation(t *testing.T) { } names.Revision = revisionName - assertResourcesUpdatedWhenRevisionIsReady(t, logger, clients, names, "What a spaceport!") + assertResourcesUpdatedWhenRevisionIsReady(t, logger, clients, names, "1", "What a spaceport!") logger.Infof("Updating the Configuration to use a different image") err = updateConfigWithImage(clients, names, imagePaths) @@ -189,5 +185,5 @@ func TestRouteCreation(t *testing.T) { } names.Revision = revisionName - assertResourcesUpdatedWhenRevisionIsReady(t, logger, clients, names, "Re-energize yourself with a slice of pepperoni!") + assertResourcesUpdatedWhenRevisionIsReady(t, logger, clients, names, "2", "Re-energize yourself with a slice of pepperoni!") } diff --git a/test/conformance/service_test.go b/test/conformance/service_test.go index 03daab184e05..9d094936d4b0 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -19,7 +19,6 @@ package conformance import ( "encoding/json" - "fmt" "strings" "testing" @@ -51,18 +50,15 @@ func updateServiceWithImage(clients *test.Clients, names test.ResourceNames, ima if err != nil { return err } - newService, err := clients.Services.Patch(names.Service, types.JSONPatchType, patchBytes, "") + _, err = clients.Services.Patch(names.Service, types.JSONPatchType, patchBytes, "") if err != nil { return err } - if newService.Spec.Generation != int64(2) { - return fmt.Errorf("The spec was updated so the Generation should be 2 but it was actually %d", newService.Spec.Generation) - } return nil } // 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, expectedText string) { +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, namespaceName, names.Route, func(body string) (bool, error) { return body == expectedText, nil @@ -73,7 +69,7 @@ func assertServiceResourcesUpdated(t *testing.T, logger *zap.SugaredLogger, clie // We want to verify that the endpoint works as soon as Ready: True, but there are a bunch of other pieces of state that we validate for conformance. logger.Info("The Revision will be marked as Ready when it can serve traffic") - if err := test.CheckRevisionState(clients.Revisions, names.Revision, test.IsRevisionReady()); err != nil { + if err := test.CheckRevisionState(clients.Revisions, names.Revision, test.IsRevisionReady(expectedGeneration)); err != nil { t.Fatalf("Revision %s did not become ready to serve traffic: %v", names.Revision, err) } @@ -158,7 +154,7 @@ func TestRunLatestService(t *testing.T) { if err := test.WaitForServiceState(clients.Services, names.Service, test.IsServiceReady(), "ServiceIsReady"); err != nil { t.Fatalf("The Service %s was not marked as Ready to serve traffic to Revision %s: %v", names.Service, names.Revision, err) } - assertServiceResourcesUpdated(t, logger, clients, names, routeDomain, "What a spaceport!") + assertServiceResourcesUpdated(t, logger, clients, names, routeDomain, "1", "What a spaceport!") logger.Info("Updating the Service to use a different image") if err := updateServiceWithImage(clients, names, imagePaths[1]); err != nil { @@ -176,7 +172,7 @@ func TestRunLatestService(t *testing.T) { if err := test.WaitForServiceState(clients.Services, names.Service, test.IsServiceReady(), "ServiceIsReady"); err != nil { t.Fatalf("The Service %s was not marked as Ready to serve traffic to Revision %s: %v", names.Service, names.Revision, err) } - assertServiceResourcesUpdated(t, logger, clients, names, routeDomain, "Re-energize yourself with a slice of pepperoni!") + assertServiceResourcesUpdated(t, logger, clients, names, routeDomain, "2", "Re-energize yourself with a slice of pepperoni!") } // TODO(jonjohnsonjr): LatestService roads less traveled. diff --git a/test/request.go b/test/request.go index 0e04307ac862..b6016a48c008 100644 --- a/test/request.go +++ b/test/request.go @@ -93,6 +93,9 @@ func WaitForEndpointState(kubeClientset *kubernetes.Clientset, logger *zap.Sugar if err != nil { return err } + if len(ingress.Status.LoadBalancer.Ingress) != 1 { + return fmt.Errorf("Expected exactly one ingress load balancer, instead had %d: %s", len(ingress.Status.LoadBalancer.Ingress), ingress.Status.LoadBalancer.Ingress) + } if ingress.Status.LoadBalancer.Ingress[0].IP == "" { return fmt.Errorf("Expected ingress loadbalancer IP for %s to be set, instead was empty", ingressName) } diff --git a/test/states.go b/test/states.go index 4a37f37a08dc..8c152a76e8fd 100644 --- a/test/states.go +++ b/test/states.go @@ -18,6 +18,7 @@ package test import ( "fmt" + "github.com/knative/serving/pkg/apis/serving" "github.com/knative/serving/pkg/apis/serving/v1alpha1" corev1 "k8s.io/api/core/v1" ) @@ -48,15 +49,24 @@ func AllRouteTrafficAtRevision(names ResourceNames) func(r *v1alpha1.Route) (boo } // IsRevisionReady will check the status conditions of the revision and return true if the revision is -// ready to serve traffic. It will return false if the status indicates a state other than deploying -// or being ready. It will also return false if the type of the condition is unexpected. -func IsRevisionReady() func(r *v1alpha1.Revision) (bool, error) { +// ready to serve traffic. It will return false until the status becomes true. If the status is true +// and the expected generation annotation is not present an error will be returned. +func IsRevisionReady(generation string) func(r *v1alpha1.Revision) (bool, error) { return func(r *v1alpha1.Revision) (bool, error) { - if cond := r.Status.GetCondition(v1alpha1.RevisionConditionReady); cond == nil { + cond := r.Status.GetCondition(v1alpha1.RevisionConditionReady) + if cond == nil { return false, nil - } else { - return cond.Status == corev1.ConditionTrue, nil } + if cond.Status == corev1.ConditionTrue { + if a, ok := r.Annotations[serving.ConfigurationGenerationAnnotationKey]; ok { + if a != generation { + return true, fmt.Errorf("Expected Revision to be annotated with generation %s but was %s instead", generation, a) + } + return true, nil + } + return true, fmt.Errorf("Expected Revision to be annotated with generation %s but there was no annotation", generation) + } + return false, nil } } @@ -64,10 +74,10 @@ func IsRevisionReady() func(r *v1alpha1.Revision) (bool, error) { // ready. This means that its configurations and routes have all reported ready. func IsServiceReady() func(r *v1alpha1.Service) (bool, error) { return func(s *v1alpha1.Service) (bool, error) { - if cond := s.Status.GetCondition(v1alpha1.ServiceConditionReady); cond == nil { + cond := s.Status.GetCondition(v1alpha1.ServiceConditionReady) + if cond == nil { return false, nil - } else { - return cond.Status == corev1.ConditionTrue, nil } + return cond.Status == corev1.ConditionTrue, nil } }