Implement verification for route readiness for the runLatest. by vagababov · Pull Request #2898 · knative/serving

vagababov · 2019-01-12T00:46:03Z

Proposed Changes

This is the first change to ensure that we mark service as ready only
when all the subresources have successfully reconciled.
In this change runLatest is covered. The service will become ready
only when config.LatestReadyRevision is the one served by the route.
When they mismatch the service will transition into the Unknown
state until route finishes reconciliation.
Unit tests are updated and extended for this case
Integration tests are hardened to make sure the service transitions
into ready state before verifying request/responses.
Also switch to pointer when passing RouteStatus around, since there's no real reason why it should be copied (I will cleanup others independently).

/lint
/cc dgerd
/cc mattmoor

TL;DR: knative#2430 - This is the first change to ensure that we mark service as ready only when all the subresources have successfully reconciled. - In this change runLatest is covered. The service will become ready only when config.LatestReadyRevision is the one served by the route. When they mismatch the service will transition into the `Unknown` state until route finishes reconciliation. - Unit tests are updated and extended for this case - Integration tests are hardened to make sure the service transitions into ready state before verifying request/responses.

vagababov · 2019-01-12T00:46:07Z

/cc dgerd
/cc mattmoor

knative-prow-robot

@vagababov: 0 warnings.

Details

In response to this:

TL;DR: #2430

Proposed Changes

This is the first change to ensure that we mark service as ready only
when all the subresources have successfully reconciled.

In this change runLatest is covered. The service will become ready
only when config.LatestReadyRevision is the one served by the route.
When they mismatch the service will transition into the Unknown
state until route finishes reconciliation.

Unit tests are updated and extended for this case

Integration tests are hardened to make sure the service transitions
into ready state before verifying request/responses.

/lint
/cc dgerd
/cc mattmoor

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.

vagababov · 2019-01-12T00:48:32Z

/test pull-knative-serving-go-coverage

vagababov · 2019-01-12T05:10:26Z

/cc @jonjohnsonjr

vagababov · 2019-01-12T05:10:50Z

/retest

vagababov · 2019-01-13T18:58:05Z

/retest

markusthoemmes

/lgtm

Left a few comments throughout, only nits though.

vagababov

Thanks for the comments.

vagababov · 2019-01-14T18:29:23Z

/test pull-knative-serving-integration-tests

- improve the functional helper name - removed the confusing comment - added a test that validates the proper behaviour when gen 2 config fails, but gen 1 is happy throughout.

vagababov · 2019-01-14T22:20:41Z

/test pull-knative-serving-integration-tests

dgerd · 2019-01-14T23:33:35Z

/lgtm

New test and changes lgtm

vagababov · 2019-01-16T21:26:57Z

/assign vaikas-google

markusthoemmes · 2019-01-17T08:10:29Z

/lgtm

vaikas · 2019-01-17T18:11:20Z

/lgtm
/approve

knative-prow-robot · 2019-01-17T18:11:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vagababov, vaikas-google

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [vaikas-google]
~~pkg/reconciler/OWNERS~~ [vaikas-google]
~~test/OWNERS~~ [vaikas-google]

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

vagababov · 2019-01-17T18:36:25Z

/retest

vagababov · 2019-01-17T18:45:27Z

Interface changed... have to merge and retest.

- reason is a single word - move the validation from callback to the postprocessing.

vagababov · 2019-01-17T19:49:46Z

PTAL.

Co-Authored-By: vagababov <vagababov@users.noreply.github.com>

knative-metrics-robot · 2019-01-17T21:40:03Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/service/service.go	91.3%	91.7%	0.4

dgerd · 2019-01-17T21:46:54Z

/lgtm

knative-prow-robot requested review from dgerd and mattmoor January 12, 2019 00:46

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 12, 2019

knative-prow-robot reviewed Jan 12, 2019

View reviewed changes

Remove the outdated method.

eea76b5

knative-prow-robot requested a review from jonjohnsonjr January 12, 2019 05:10

markusthoemmes reviewed Jan 14, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/service/service.go Outdated

Comment thread pkg/apis/serving/v1alpha1/service_types.go Outdated

Comment thread pkg/reconciler/v1alpha1/service/service.go

knative-prow-robot assigned markusthoemmes Jan 14, 2019

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

Merge branch 'master' into 2430-run-latest

70fd5c7

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

vagababov commented Jan 14, 2019

View reviewed changes

Comment thread pkg/apis/serving/v1alpha1/service_types.go Outdated

Comment thread pkg/reconciler/v1alpha1/service/service.go

Address review comments.

63defe0

dgerd reviewed Jan 14, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/service/service_test.go Outdated

Comment thread pkg/reconciler/v1alpha1/testing/functional.go Outdated

vagababov added 3 commits January 14, 2019 12:21

Address the comments.

52dac4e

- improve the functional helper name - removed the confusing comment - added a test that validates the proper behaviour when gen 2 config fails, but gen 1 is happy throughout.

Comment typo fix.

a0cac3b

Add a comment to clarify what is going on.

18bc2c0

knative-prow-robot assigned dgerd Jan 14, 2019

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

knative-prow-robot assigned vaikas Jan 16, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2019

vagababov added 2 commits January 17, 2019 10:43

Merte branch 'master' into 2430-run-latest

4aa8fe9

Fix the unit test after the merge, since the interface changed.

14c7c38

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

mattmoor reviewed Jan 17, 2019

View reviewed changes

Comment thread pkg/apis/serving/v1alpha1/service_types.go Outdated

mattmoor reviewed Jan 17, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/service/service.go Outdated

Address review comments

bfe3c29

- reason is a single word - move the validation from callback to the postprocessing.

dgerd reviewed Jan 17, 2019

View reviewed changes

Comment thread pkg/apis/serving/v1alpha1/service_types_test.go Outdated

dgerd reviewed Jan 17, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/service/service.go

Dan Gerdesmeier and others added 2 commits January 17, 2019 13:29

Update pkg/apis/serving/v1alpha1/service_types_test.go

fbd8e46

Co-Authored-By: vagababov <vagababov@users.noreply.github.com>

Address comments.

fa0e41b

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

knative-prow-robot merged commit e9fe7dd into knative:master Jan 17, 2019

vagababov deleted the 2430-run-latest branch January 24, 2019 23:33

dgerd mentioned this pull request Feb 14, 2019

Add dgerd to list of serving-api-approvers #3228

Merged

taragu mentioned this pull request Jul 1, 2019

Service's RoutesReady is 'Unknown' when Route's Ready is 'False' #4561

Closed

Conversation

vagababov commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

vagababov commented Jan 12, 2019

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

vagababov commented Jan 12, 2019

Uh oh!

vagababov commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vagababov commented Jan 12, 2019

Uh oh!

vagababov commented Jan 13, 2019

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vagababov commented Jan 14, 2019

Uh oh!

Uh oh!

Uh oh!

vagababov commented Jan 14, 2019

Uh oh!

dgerd commented Jan 14, 2019

Uh oh!

vagababov commented Jan 16, 2019

Uh oh!

markusthoemmes commented Jan 17, 2019

Uh oh!

vaikas commented Jan 17, 2019

Uh oh!

knative-prow-robot commented Jan 17, 2019

Uh oh!

vagababov commented Jan 17, 2019

Uh oh!

vagababov commented Jan 17, 2019

Uh oh!

Uh oh!

Uh oh!

vagababov commented Jan 17, 2019

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Jan 17, 2019

Uh oh!

dgerd commented Jan 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vagababov commented Jan 12, 2019 •

edited

Loading

vagababov commented Jan 12, 2019 •

edited

Loading