Check route is not ready when underlying revision is not healthy in T…

…estContainerErrorMsg (#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

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)

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)

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) {

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
+}

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 {

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
+}