Add route label to `LatestCreatedRevision` by tanzeeb · Pull Request #7110 · knative/serving

tanzeeb · 2020-03-02T23:34:24Z

Proposed Changes

Add additional scenarios to the MinScale e2e tests to check minScale during the update of a configuration.
Add a route label to the LatestCreatedRevision. This ensures continuity of a reachable revision when a configuration is updated.

Release Note

The `LatestCreatedRevision` of a Configuration will now have a route label. Fixes issue where a Revision's pods are temporarily scaled below `minScale` after the Revision becomes ready.

knative-prow-robot

@tanzeeb: 0 warnings.

Details

In response to this:

Fixes #6733

Proposed Changes

Add additional scenarios to the MinScale e2e tests to check minScale during the update of a configuration.

Add a route label to the LatestCreatedRevision. This ensures continuity of a reachable revision when a configuration is updated.

Release Note
The `LatestCreatedRevision` of a Configuration will now have a route label. Fixes issue where a Revision's pods are temporarily scaled below `minScale` after the Revision becomes ready.

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.

tanzeeb · 2020-03-02T23:38:00Z

/assign @vagababov

cc @duglin @patrickshan

vagababov

Thanks @tanzeeb
/cc @mattmoor @markusthoemmes

vagababov

Overall makes sense to me.
I don't know labeler very much, so I'll leave Matt to approve
/assign @mattmoor
/lgtm

* faster polling (250 ms) * more descriptive error for ensureDesiredScale * inlined latestCreatedRevisionName check

knative-test-reporter-robot · 2020-03-05T22:58:26Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-autotls-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-autotls-tests:

_go_tests.DataRaceAnalysis
test/e2e/autotls.Failure

tanzeeb · 2020-03-07T05:19:15Z

/assign @mattmoor

mattmoor · 2020-03-07T15:17:10Z

+			// If we are tracking the latest revision, we need to label the latest created revision
+			// as well so that there is a smooth transition when the new revision becomes ready.
+			if tt.LatestRevision != nil && *tt.LatestRevision {
+				revisions.Insert(config.Status.LatestCreatedRevisionName)


@vagababov @markusthoemmes it is tempting to also label the LCR with the LRR in this case so that the KPA gets a hint about how big to make the LCR before it is receiving traffic 🤔

(not for this PR)

mattmoor

Thanks, this looks about right. Couple nits, but otherwise 👍

knative-metrics-robot · 2020-03-09T14:24:56Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/labeler/labels.go	89.9%	89.2%	-0.7

mattmoor

/lgtm
/approve

knative-prow-robot · 2020-03-09T15:34:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, tanzeeb

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/reconciler/OWNERS~~ [mattmoor]
~~test/OWNERS~~ [mattmoor]

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

* Test minScale during update to configuration * Add route label to LatestCreatedRevision * Use impl.Enqueue instead of tracker.OnChanged in labeler reconciler * Update minscale readiness e2e test * faster polling (250 ms) * more descriptive error for ensureDesiredScale * inlined latestCreatedRevisionName check * Enqueue route instead of config in labeler reconciler * Only label configurations of RunLatest traffic targets * Label configuration if it or it's revision is in a route * Only pull config for runLatest in labeler

* Add route label to `LatestCreatedRevision` (#7110) * Test minScale during update to configuration * Add route label to LatestCreatedRevision * Use impl.Enqueue instead of tracker.OnChanged in labeler reconciler * Update minscale readiness e2e test * faster polling (250 ms) * more descriptive error for ensureDesiredScale * inlined latestCreatedRevisionName check * Enqueue route instead of config in labeler reconciler * Only label configurations of RunLatest traffic targets * Label configuration if it or it's revision is in a route * Only pull config for runLatest in labeler * Some cleanups in the e2e test (#7193) * Some cleanups in the e2e test * Some cleanups in the e2e test * fix nit * Simplify minscale test. (#7235) The only error returned from the function is when we fail to actually verify the scale. So return boolean instead. * Fix minscale on deploy (#7214) * Test minscale stability when configuration is created * Check both .spec.traffic and .status.traffic for resources to label * Inline condition * Inline owner reference check Co-authored-by: Tanzeeb Khalili <tkhalili@pivotal.io> Co-authored-by: Victor Agababov <vagababov@gmail.com>

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 2, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 2, 2020

knative-prow-robot reviewed Mar 2, 2020

View reviewed changes

knative-prow-robot added area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Mar 2, 2020

knative-prow-robot requested review from chizhg and vagababov March 2, 2020 23:35

knative-prow-robot assigned vagababov Mar 2, 2020

vagababov reviewed Mar 3, 2020

View reviewed changes

knative-prow-robot requested review from markusthoemmes and mattmoor March 3, 2020 00:19

tanzeeb force-pushed the fix-minscale-flappyness branch from 0e75728 to 24ef148 Compare March 4, 2020 19:05

vagababov reviewed Mar 4, 2020

View reviewed changes

knative-prow-robot assigned mattmoor Mar 4, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2020

mattmoor reviewed Mar 5, 2020

View reviewed changes

Comment thread pkg/reconciler/labeler/labels.go Outdated

Comment thread pkg/reconciler/labeler/controller.go Outdated

tanzeeb added 5 commits March 5, 2020 15:06

Test minScale during update to configuration

37fa1e9

Add route label to LatestCreatedRevision

b4a3bf0

Use impl.Enqueue instead of tracker.OnChanged in labeler reconciler

9f347c2

Update minscale readiness e2e test

246494c

* faster polling (250 ms) * more descriptive error for ensureDesiredScale * inlined latestCreatedRevisionName check

Enqueue route instead of config in labeler reconciler

35aa45e

tanzeeb force-pushed the fix-minscale-flappyness branch from 24ef148 to 35aa45e Compare March 5, 2020 22:34

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2020

Only label configurations of RunLatest traffic targets

05ee2c7

mattmoor reviewed Mar 6, 2020

View reviewed changes

Comment thread pkg/reconciler/labeler/controller.go

mattmoor reviewed Mar 6, 2020

View reviewed changes

Comment thread pkg/reconciler/labeler/labels.go Outdated

tanzeeb commented Mar 6, 2020

View reviewed changes

Comment thread pkg/reconciler/labeler/labeler_test.go

Comment thread pkg/reconciler/labeler/labels.go Outdated

Label configuration if it or it's revision is in a route

b7eec55

mattmoor reviewed Mar 7, 2020

View reviewed changes

Comment thread pkg/reconciler/labeler/labels.go Outdated

mattmoor reviewed Mar 7, 2020

View reviewed changes

Comment thread pkg/reconciler/labeler/labels.go Outdated

mattmoor reviewed Mar 7, 2020

View reviewed changes

Only pull config for runLatest in labeler

e4a1182

mattmoor reviewed Mar 9, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2020

knative-prow-robot merged commit b260b2b into knative:master Mar 9, 2020

Conversation

tanzeeb commented Mar 2, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

tanzeeb commented Mar 2, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

knative-test-reporter-robot commented Mar 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tanzeeb commented Mar 7, 2020

Uh oh!

Uh oh!

mattmoor Mar 7, 2020

Choose a reason for hiding this comment

Uh oh!

mattmoor Mar 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Mar 9, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants