Skip to content

Check route is not ready when underlying revision is not healthy in TestContainerErrorMsg #4682

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knative-prow-robot merged 2 commits into from
Jul 17, 2019
Merged

Check route is not ready when underlying revision is not healthy in TestContainerErrorMsg #4682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9871ce1
Check route is not ready when underlying revision is not healthy in T…
nak3 Jul 10, 2019
0c65578
Add WaitForServiceState to wait for creation of Configuration
nak3 Jul 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions test/conformance/api/v1alpha1/errorcondition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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() {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
36 changes: 26 additions & 10 deletions test/conformance/api/v1beta1/errorcondition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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() {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 6 additions & 0 deletions test/v1alpha1/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Member

@dprotaso dprotaso Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IsRouteReady check has a generation check that we probably want here

r.Generation == r.Status.ObservedGeneration

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dprotaso Thank you!

But it seems that routes.status.ObservedGeneration is not assigned until the Route becomes ready. So, the test fails due to Generation=1 vs ObservedGeneration=0 mismatch.
For example, when we deployed invalid image below,

cat <<EOF | kubectl apply -f -
apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: helloworld-go
spec:
  runLatest:
    configuration:
      revisionTemplate:
        spec:
          container:
            image: image-registry.openshift-image-registry.svc:5000/knative-serving/invalidhelloworld
            resources:
              limits:
                cpu: 100m
                memory: 200M
              requests:
                cpu: 100u
                memory: 100M
            env:
              - name: TARGET
                value: "Go Sample v1"
EOF

observedGeneration is not assigned and does have 0 for observedGeneration until it becomes Ready.

$ kubectl get rt -o yaml helloworld-go |grep -E "generation|observedGeneration"
  generation: 1

}

// 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) {
Expand Down
6 changes: 6 additions & 0 deletions test/v1alpha1/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
6 changes: 6 additions & 0 deletions test/v1beta1/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Member

@dprotaso dprotaso Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment applies re: generation check

}

// 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 {
Expand Down
6 changes: 6 additions & 0 deletions test/v1beta1/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}