From bcd92770b66bbe354b333e84a6860ae1a3e521cb Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Wed, 17 Jul 2019 15:44:27 +0900 Subject: [PATCH] Check route is not ready when underlying revision is not healthy in TestContainerErrorMsg (#4682) * Check route is not ready when underlying revision is not healthy in TestContainerErrorMsg This patch changes TestContainerErrorMsg to: - create new service instead of configuration. - make sure route state is not ready when revision is not healthy. * Add WaitForServiceState to wait for creation of Configuration --- .../api/v1alpha1/errorcondition_test.go | 35 ++++++++++++------ .../api/v1beta1/errorcondition_test.go | 36 +++++++++++++------ test/v1alpha1/route.go | 6 ++++ test/v1alpha1/service.go | 6 ++++ test/v1beta1/route.go | 6 ++++ test/v1beta1/service.go | 6 ++++ 6 files changed, 75 insertions(+), 20 deletions(-) diff --git a/test/conformance/api/v1alpha1/errorcondition_test.go b/test/conformance/api/v1alpha1/errorcondition_test.go index 6f222309b1b7..0103d52e767b 100644 --- a/test/conformance/api/v1alpha1/errorcondition_test.go +++ b/test/conformance/api/v1alpha1/errorcondition_test.go @@ -28,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ptest "knative.dev/pkg/test" "knative.dev/serving/pkg/apis/serving/v1alpha1" + serviceresourcenames "knative.dev/serving/pkg/reconciler/service/resources/names" v1a1opts "knative.dev/serving/pkg/testing/v1alpha1" "knative.dev/serving/test" v1a1test "knative.dev/serving/test/v1alpha1" @@ -48,23 +49,33 @@ func TestContainerErrorMsg(t *testing.T) { clients := test.Setup(t) names := test.ResourceNames{ - Config: test.ObjectNameForTest(t), - Image: test.InvalidHelloWorld, + Service: test.ObjectNameForTest(t), + Image: test.InvalidHelloWorld, } + + defer test.TearDown(clients, names) + test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + // Specify an invalid image path // A valid DockerRepo is still needed, otherwise will get UNAUTHORIZED instead of container missing error - t.Logf("Creating a new Configuration %s", names.Image) - if _, err := v1a1test.CreateConfiguration(t, clients, names); err != nil { - t.Fatalf("Failed to create configuration %s", names.Config) + t.Logf("Creating a new Service %s", names.Service) + svc, err := v1a1test.CreateLatestService(t, clients, names) + if err != nil { + t.Fatalf("Failed to create Service: %v", err) } - defer test.TearDown(clients, names) - test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + names.Config = serviceresourcenames.Configuration(svc) + names.Route = serviceresourcenames.Route(svc) manifestUnknown := string(transport.ManifestUnknownErrorCode) t.Log("When the imagepath is invalid, the Configuration should have error status.") + // Wait for ServiceState becomes NotReady. It also waits for the creation of Configuration. + if err := v1a1test.WaitForServiceState(clients.ServingAlphaClient, names.Service, v1a1test.IsServiceNotReady, "ServiceIsNotReady"); err != nil { + t.Fatalf("The Service %s was unexpected state: %v", names.Service, err) + } + // Checking for "Container image not present in repository" scenario defined in error condition spec - err := v1a1test.WaitForConfigurationState(clients.ServingAlphaClient, names.Config, func(r *v1alpha1.Configuration) (bool, error) { + err = v1a1test.WaitForConfigurationState(clients.ServingAlphaClient, names.Config, func(r *v1alpha1.Configuration) (bool, error) { cond := r.Status.GetCondition(v1alpha1.ConfigurationConditionReady) if cond != nil && !cond.IsUnknown() { if strings.Contains(cond.Message, manifestUnknown) && cond.IsFalse() { @@ -112,7 +123,11 @@ func TestContainerErrorMsg(t *testing.T) { // TODO(jessiezcc): actually validate the logURL, but requires kibana setup t.Logf("LogURL: %s", logURL) - // TODO(jessiezcc): add the check to validate that Route is not marked as ready once https://knative.dev/serving/issues/990 is fixed + t.Log("When the revision has error condition, route should not be ready.") + err = v1a1test.CheckRouteState(clients.ServingAlphaClient, names.Route, v1a1test.IsRouteNotReady) + if err != nil { + t.Fatalf("the Route %s was not desired state: %v", names.Route, err) + } } // TestContainerExitingMsg is to validate the error condition defined at @@ -160,7 +175,7 @@ func TestContainerExitingMsg(t *testing.T) { defer test.TearDown(clients, names) test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) - t.Logf("Creating a new Configuration %s", names.Image) + t.Logf("Creating a new Configuration %s", names.Config) if _, err := v1a1test.CreateConfiguration(t, clients, names, v1a1opts.WithConfigReadinessProbe(tt.ReadinessProbe)); err != nil { t.Fatalf("Failed to create configuration %s: %v", names.Config, err) diff --git a/test/conformance/api/v1beta1/errorcondition_test.go b/test/conformance/api/v1beta1/errorcondition_test.go index 8d1543fae004..02c68f3be0c6 100644 --- a/test/conformance/api/v1beta1/errorcondition_test.go +++ b/test/conformance/api/v1beta1/errorcondition_test.go @@ -28,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ptest "knative.dev/pkg/test" "knative.dev/serving/pkg/apis/serving/v1beta1" + serviceresourcenames "knative.dev/serving/pkg/reconciler/service/resources/names" "knative.dev/serving/test" v1b1test "knative.dev/serving/test/v1beta1" @@ -49,23 +50,34 @@ func TestContainerErrorMsg(t *testing.T) { clients := test.Setup(t) names := test.ResourceNames{ - Config: test.ObjectNameForTest(t), - Image: test.InvalidHelloWorld, + Service: test.ObjectNameForTest(t), + Image: test.InvalidHelloWorld, } + + defer test.TearDown(clients, names) + test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + // Specify an invalid image path // A valid DockerRepo is still needed, otherwise will get UNAUTHORIZED instead of container missing error - t.Logf("Creating a new Configuration %s", names.Image) - if _, err := v1b1test.CreateConfiguration(t, clients, names); err != nil { - t.Fatalf("Failed to create configuration %s", names.Config) + t.Logf("Creating a new Service %s", names.Service) + svc, err := createService(t, clients, names, 2) + if err != nil { + t.Fatalf("Failed to create Service: %v", err) } - defer test.TearDown(clients, names) - test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + + names.Config = serviceresourcenames.Configuration(svc) + names.Route = serviceresourcenames.Route(svc) manifestUnknown := string(transport.ManifestUnknownErrorCode) t.Log("When the imagepath is invalid, the Configuration should have error status.") + // Wait for ServiceState becomes NotReady. It also waits for the creation of Configuration. + if err := v1b1test.WaitForServiceState(clients.ServingBetaClient, names.Service, v1b1test.IsServiceNotReady, "ServiceIsNotReady"); err != nil { + t.Fatalf("The Service %s was unexpected state: %v", names.Service, err) + } + // Checking for "Container image not present in repository" scenario defined in error condition spec - err := v1b1test.WaitForConfigurationState(clients.ServingBetaClient, names.Config, func(r *v1beta1.Configuration) (bool, error) { + err = v1b1test.WaitForConfigurationState(clients.ServingBetaClient, names.Config, func(r *v1beta1.Configuration) (bool, error) { cond := r.Status.GetCondition(v1beta1.ConfigurationConditionReady) if cond != nil && !cond.IsUnknown() { if strings.Contains(cond.Message, manifestUnknown) && cond.IsFalse() { @@ -113,7 +125,11 @@ func TestContainerErrorMsg(t *testing.T) { // TODO(jessiezcc): actually validate the logURL, but requires kibana setup t.Logf("LogURL: %s", logURL) - // TODO(jessiezcc): add the check to validate that Route is not marked as ready once https://knative.dev/serving/issues/990 is fixed + t.Log("Checking to ensure Route is in desired state") + err = v1b1test.CheckRouteState(clients.ServingBetaClient, names.Route, v1b1test.IsRouteNotReady) + if err != nil { + t.Fatalf("the Route %s was not desired state: %v", names.Route, err) + } } // TestContainerExitingMsg is to validate the error condition defined at @@ -161,7 +177,7 @@ func TestContainerExitingMsg(t *testing.T) { defer test.TearDown(clients, names) test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) - t.Logf("Creating a new Configuration %s", names.Image) + t.Logf("Creating a new Configuration %s", names.Config) if _, err := v1b1test.CreateConfiguration(t, clients, names, rtesting.WithConfigReadinessProbe(tt.ReadinessProbe)); err != nil { t.Fatalf("Failed to create configuration %s: %v", names.Config, err) diff --git a/test/v1alpha1/route.go b/test/v1alpha1/route.go index 7a81cfb50753..cab6b3b4fe34 100644 --- a/test/v1alpha1/route.go +++ b/test/v1alpha1/route.go @@ -129,6 +129,12 @@ func IsRouteReady(r *v1alpha1.Route) (bool, error) { return r.Generation == r.Status.ObservedGeneration && r.Status.IsReady(), nil } +// IsRouteNotReady will check the status conditions of the route and return true if the route is +// not ready. +func IsRouteNotReady(r *v1alpha1.Route) (bool, error) { + return !r.Status.IsReady(), nil +} + // AllRouteTrafficAtRevision will check the revision that route r is routing // traffic to and return true if 100% of the traffic is routing to revisionName. func AllRouteTrafficAtRevision(names test.ResourceNames) func(r *v1alpha1.Route) (bool, error) { diff --git a/test/v1alpha1/service.go b/test/v1alpha1/service.go index ea6e101464d0..6752e53bd7e6 100644 --- a/test/v1alpha1/service.go +++ b/test/v1alpha1/service.go @@ -345,3 +345,9 @@ func CheckServiceState(client *test.ServingAlphaClients, name string, inState fu func IsServiceReady(s *v1alpha1.Service) (bool, error) { return s.Generation == s.Status.ObservedGeneration && s.Status.IsReady(), nil } + +// IsServiceNotReady will check the status conditions of the service and return true if the service is +// not ready. +func IsServiceNotReady(s *v1alpha1.Service) (bool, error) { + return s.Generation == s.Status.ObservedGeneration && !s.Status.IsReady(), nil +} diff --git a/test/v1beta1/route.go b/test/v1beta1/route.go index 1df6f76780c6..516092a8afdc 100644 --- a/test/v1beta1/route.go +++ b/test/v1beta1/route.go @@ -110,6 +110,12 @@ func IsRouteReady(r *v1beta1.Route) (bool, error) { return r.Generation == r.Status.ObservedGeneration && r.Status.IsReady(), nil } +// IsRouteNotReady will check the status conditions of the route and return true if the route is +// not ready. +func IsRouteNotReady(r *v1beta1.Route) (bool, error) { + return !r.Status.IsReady(), nil +} + // RetryingRouteInconsistency retries common requests seen when creating a new route // - 404 until the route is propagated to the proxy func RetryingRouteInconsistency(innerCheck spoof.ResponseChecker) spoof.ResponseChecker { diff --git a/test/v1beta1/service.go b/test/v1beta1/service.go index 2d60129778ac..9565e596bfc5 100644 --- a/test/v1beta1/service.go +++ b/test/v1beta1/service.go @@ -244,3 +244,9 @@ func CheckServiceState(client *test.ServingBetaClients, name string, inState fun func IsServiceReady(s *v1beta1.Service) (bool, error) { return s.Generation == s.Status.ObservedGeneration && s.Status.IsReady(), nil } + +// IsServiceNotReady will check the status conditions of the service and return true if the service is +// not ready. +func IsServiceNotReady(s *v1beta1.Service) (bool, error) { + return s.Generation == s.Status.ObservedGeneration && !s.Status.IsReady(), nil +}