From ba0b3ad31d7b934b7ce6f840d5a2992770a19bb9 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 Configuration.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 undid 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 | 17 +++++++++-------- test/conformance/service_test.go | 18 +++++++++--------- test/spoof/spoof.go | 4 ++++ test/states.go | 16 ++++++++++++++++ 4 files changed, 38 insertions(+), 17 deletions(-) diff --git a/test/conformance/route_test.go b/test/conformance/route_test.go index ab83839085c6..bc16ea7650f2 100644 --- a/test/conformance/route_test.go +++ b/test/conformance/route_test.go @@ -19,7 +19,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.") if err := test.WaitForRouteState(clients.Routes, names.Route, test.IsRouteReady, "RouteIsReady"); err != nil { t.Fatalf("The Route %s was not marked as Ready to serve traffic to Revision %s: %v", names.Route, names.Revision, err) @@ -95,6 +91,11 @@ func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.Sugared if err != nil { t.Fatalf("Revision %s did not become ready to serve traffic: %v", names.Revision, err) } + logger.Infof("The Revision will be annotated with the generation") + err = test.CheckRevisionState(clients.Revisions, names.Revision, test.IsRevisionAtExpectedGeneration(expectedGeneration)) + if err != nil { + t.Fatalf("Revision %s did not have an expected annotation with generation %s: %v", names.Revision, expectedGeneration, err) + } logger.Infof("Updates the Configuration that the Revision is ready") err = test.CheckConfigurationState(clients.Configs, names.Config, func(c *v1alpha1.Configuration) (bool, error) { return c.Status.LatestReadyRevisionName == names.Revision, nil @@ -175,7 +176,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) @@ -190,5 +191,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 7f34e1c6c53f..3eb7b43d453b 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -20,7 +20,6 @@ package conformance import ( "encoding/json" - "fmt" "strings" "testing" @@ -52,18 +51,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, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText") if err != nil { @@ -75,7 +71,11 @@ func assertServiceResourcesUpdated(t *testing.T, logger *zap.SugaredLogger, clie if err := test.CheckRevisionState(clients.Revisions, names.Revision, test.IsRevisionReady); err != nil { t.Fatalf("Revision %s did not become ready to serve traffic: %v", names.Revision, err) } - + logger.Infof("The Revision will be annotated with the generation") + err = test.CheckRevisionState(clients.Revisions, names.Revision, test.IsRevisionAtExpectedGeneration(expectedGeneration)) + if err != nil { + t.Fatalf("Revision %s did not have an expected annotation with generation %s: %v", names.Revision, expectedGeneration, err) + } logger.Info("The Service's latestReadyRevisionName should match the Configuration's") err = test.CheckConfigurationState(clients.Configs, names.Config, func(c *v1alpha1.Configuration) (bool, error) { return c.Status.LatestReadyRevisionName == names.Revision, nil @@ -163,7 +163,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 { @@ -181,7 +181,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/spoof/spoof.go b/test/spoof/spoof.go index 1a1cfccbea8a..0772beee4e2c 100644 --- a/test/spoof/spoof.go +++ b/test/spoof/spoof.go @@ -103,6 +103,10 @@ func New(kubeClientset *kubernetes.Clientset, logger *zap.SugaredLogger, domain return nil, err } + if len(ingress.Status.LoadBalancer.Ingress) != 1 { + return nil, 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 nil, 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 56821905a2c4..8f9b9d4b3265 100644 --- a/test/states.go +++ b/test/states.go @@ -21,6 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" + "github.com/knative/serving/pkg/apis/serving" "github.com/knative/serving/pkg/apis/serving/v1alpha1" ) @@ -113,3 +114,18 @@ func IsConfigRevisionCreationFailed(c *v1alpha1.Configuration) (bool, error) { } return false, nil } + +// IsRevisionAtExpectedGeneration returns a function that will check if the annotations +// on the revision include an annotation for the generation and that the annotation is +// set to the expected value. +func IsRevisionAtExpectedGeneration(expectedGeneration string) func(r *v1alpha1.Revision) (bool, error) { + return func(r *v1alpha1.Revision) (bool, error) { + if a, ok := r.Annotations[serving.ConfigurationGenerationAnnotationKey]; ok { + if a != expectedGeneration { + return true, fmt.Errorf("Expected Revision %s to be annotated with generation %s but was %s instead", r.Name, expectedGeneration, a) + } + return true, nil + } + return true, fmt.Errorf("Expected Revision %s to be annotated with generation %s but there was no annotation", r.Name, expectedGeneration) + } +}