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

nak3 · 2019-07-10T11:34:50Z

This PR fixes following TODO comment

serving/test/conformance/api/v1alpha1/errorcondition_test.go

Line 114 in aac25fb

    
           // TODO(jessiezcc): add the check to validate that Route is not marked as ready once https://github.com/knative/serving/issues/990 is fixed

Proposed Changes

This patch changes TestContainerErrorMsg to:

create new service instead of configuration.
make sure route state is not ready when revision is not healthy.

/lint

Release Note

NONE

knative-prow-robot · 2019-07-10T11:35:14Z

Hi @nak3. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot

@nak3: 0 warnings.

In response to this:

This PR fixes following TODO comment

serving/test/conformance/api/v1alpha1/errorcondition_test.go

Line 114 in aac25fb

// TODO(jessiezcc): add the check to validate that Route is not marked as ready once https://github.com/knative/serving/issues/990 is fixed

Proposed Changes

This patch changes TestContainerErrorMsg to:

create new service instead of configuration.

make sure route state is not ready when revision is not healthy.

/lint

Release Note
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dprotaso · 2019-07-10T13:37:10Z

/ok-to-test

dprotaso · 2019-07-10T13:58:03Z

test/v1alpha1/route.go

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


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

r.Generation == r.Status.ObservedGeneration

@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

dprotaso · 2019-07-10T13:58:17Z

test/v1beta1/route.go

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


Same comment applies re: generation check

dprotaso · 2019-07-10T14:32:59Z

/assign @dprotaso

nak3 · 2019-07-10T15:06:27Z

/test pull-knative-serving-integration-tests

Hmm.. log says "container-error-msg-rxgqnihf" not found but the Configuration container-error-msg-rxgqnihf does exist in the logs.

    errorcondition_test.go:86: Failed to validate configuration state: configuration "container-error-msg-rxgqnihf" is not in desired state, got: &{TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name: GenerateName: Namespace: SelfLink: UID: ResourceVersion: Generation:0 CreationTimestamp:0001-01-01 00:00:00 +0000 UTC DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[] OwnerReferences:[] Initializers:nil Finalizers:[] ClusterName:} Spec:{DeprecatedGeneration:0 DeprecatedBuild:nil DeprecatedRevisionTemplate:<nil> Template:<nil>} Status:{Status:{ObservedGeneration:0 Conditions:[]} ConfigurationStatusFields:{LatestReadyRevisionName: LatestCreatedRevisionName:}}}: configurations.serving.knative.dev "container-error-msg-rxgqnihf" not found

- apiVersion: serving.knative.dev/v1beta1
  kind: Configuration
  metadata:
    annotations:
      serving.knative.dev/creator: prow-job@knative-tests.iam.gserviceaccount.com
      serving.knative.dev/lastModifier: prow-job@knative-tests.iam.gserviceaccount.com
    creationTimestamp: "2019-07-10T13:49:24Z"
    deletionGracePeriodSeconds: 0
    deletionTimestamp: "2019-07-10T13:49:30Z"
    finalizers:
    - foregroundDeletion
    generation: 2
    labels:
      serving.knative.dev/service: container-error-msg-rxgqnihf
    name: container-error-msg-rxgqnihf
    ....

And my env always passes the test.

dprotaso · 2019-07-10T15:17:14Z

Yeah looks like it's being addressed on here: #4594

/lgtm
/approve

nak3 · 2019-07-10T16:12:52Z

/hold

It looks like test does not pass. I will look into it.

nak3 · 2019-07-11T04:05:36Z

/hold cancel

dprotaso · 2019-07-12T20:47:41Z

/lgtm

dprotaso · 2019-07-12T20:48:19Z

@nak3 you'll need to rebase

nak3 · 2019-07-13T01:33:59Z

@dprotaso Thank you! Yes, I rebased now.

markusthoemmes

/lgtm
/approve

nak3 · 2019-07-15T15:26:15Z

/test pull-knative-serving-integration-tests

…estContainerErrorMsg This patch changes TestContainerErrorMsg to: - create new service instead of configuration. - make sure route state is not ready when revision is not healthy.

markusthoemmes

/lgtm
/approve

knative-prow-robot · 2019-07-17T06:22:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, markusthoemmes, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [dprotaso,markusthoemmes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 10, 2019

knative-prow-robot requested a review from dprotaso July 10, 2019 11:34

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 10, 2019

knative-prow-robot requested a review from tcnghia July 10, 2019 11:34

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Jul 10, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2019

knative-prow-robot reviewed Jul 10, 2019

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2019

dprotaso reviewed Jul 10, 2019

View reviewed changes

knative-prow-robot assigned dprotaso Jul 10, 2019

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 10, 2019

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2019

nak3 force-pushed the route-check branch from ae09840 to 62c1663 Compare July 11, 2019 03:36

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2019

nak3 force-pushed the route-check branch from 62c1663 to fff1745 Compare July 13, 2019 01:01

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2019

markusthoemmes approved these changes Jul 15, 2019

View reviewed changes

knative-prow-robot assigned markusthoemmes Jul 15, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019

nak3 added 2 commits July 17, 2019 13:21

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

9871ce1

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

0c65578

nak3 force-pushed the route-check branch from fff1745 to 0c65578 Compare July 17, 2019 04:21

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019

markusthoemmes approved these changes Jul 17, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019

knative-prow-robot merged commit bcd9277 into knative:master Jul 17, 2019

nak3 deleted the route-check branch July 17, 2019 06:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #4682

nak3 commented Jul 10, 2019

knative-prow-robot commented Jul 10, 2019

knative-prow-robot left a comment

dprotaso commented Jul 10, 2019

dprotaso Jul 10, 2019

nak3 Jul 10, 2019

dprotaso Jul 10, 2019

dprotaso commented Jul 10, 2019

nak3 commented Jul 10, 2019

dprotaso commented Jul 10, 2019

nak3 commented Jul 10, 2019

nak3 commented Jul 11, 2019

dprotaso commented Jul 12, 2019

dprotaso commented Jul 12, 2019

nak3 commented Jul 13, 2019

markusthoemmes left a comment

nak3 commented Jul 15, 2019

markusthoemmes left a comment

knative-prow-robot commented Jul 17, 2019

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 #4682

Conversation

nak3 commented Jul 10, 2019

Proposed Changes

knative-prow-robot commented Jul 10, 2019

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

dprotaso commented Jul 10, 2019

dprotaso Jul 10, 2019

Choose a reason for hiding this comment

nak3 Jul 10, 2019

Choose a reason for hiding this comment

dprotaso Jul 10, 2019

Choose a reason for hiding this comment

dprotaso commented Jul 10, 2019

nak3 commented Jul 10, 2019

dprotaso commented Jul 10, 2019

nak3 commented Jul 10, 2019

nak3 commented Jul 11, 2019

dprotaso commented Jul 12, 2019

dprotaso commented Jul 12, 2019

nak3 commented Jul 13, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

nak3 commented Jul 15, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jul 17, 2019