Skip GC proces when revision is still used by route by nak3 · Pull Request #4235 · knative/serving

nak3 · 2019-06-04T06:12:22Z

Currently GC process only skips LatestReadyRevision. Hence, even if
route uses non-LatestReadyRevision, GC removes the revision. It causes
service outage with RevisionMissing error suddenly.

This patch changes to stop GC when revision is still used by route.

/lint

Fixes #4234

(And I am wondering if #4208 (comment) original report was caused by this.)

Proposed Changes

Skip GC proces when revision is still used by route

Release Note

Revision is not GC'd when it is used by route

Currently GC process only skips `LatestReadyRevision`. Hence, even if route uses non-LatestReadyRevision, GC removes the revision. It causes service outage with `RevisionMissing` error suddenly. This patch changes to stop GC when revision is still used by route.

knative-prow-robot

@nak3: 0 warnings.

Details

In response to this:

Currently GC process only skips LatestReadyRevision. Hence, even if
route uses non-LatestReadyRevision, GC removes the revision. It causes
service outage with RevisionMissing error suddenly.

This patch changes to stop GC when revision is still used by route.

/lint

Fixes #4234

(And I am wondering if #4208 (comment) original report was caused by this.)

Proposed Changes

Skip GC proces when revision is still used by route

Release Note
Revision is not GC'd when it is used by route

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 · 2019-06-04T06:12:37Z

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.

Details

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 · 2019-06-04T06:12:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nak3
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

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:

cmd/controller/OWNERS
pkg/reconciler/OWNERS

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

markusthoemmes · 2019-06-04T08:40:01Z

 		return nil
 	}

+	route, err := c.routeLister.Routes(config.Namespace).Get(config.Name)


This is based on the assumption that the route name equals the configuration name, right? I think that might only coincidently be true for KnServices but is certainly not something we can depend on generally.

Yes, you are correct. I assumed that config and route are always same. But now, updated to get the route by label's (serving.knative.dev/route) value.

markusthoemmes · 2019-06-04T08:40:28Z

/ok-to-test

knative-metrics-robot · 2019-06-04T08:44:49Z

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/configuration/configuration.go	88.7%	88.5%	-0.1

knative-metrics-robot · 2019-06-04T09:25:58Z

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/configuration/configuration.go	88.7%	87.6%	-1.1

duglin · 2019-06-04T11:33:04Z

I think this might address #4208 as well, but I'm pretty sure I saw the issue even with the longer/default GC times, but I'm not able to reliably reproduce it. So let's be optimistic and hope this got it all.

mattmoor · 2019-06-04T14:16:42Z

/hold

Currently GC process only skips LatestReadyRevision

cc @greghaynes

This is inaccurate. The GC process skips this, yes, but it also skips:

The last N generations of Revision
Any revision that has received a "heartbeat" in the last M seconds/minutes/hours

The contract with the route controller is that any routable revision receives heartbeats every resync period (currently 10h).

We should (and probably don't) reject GC settings that drop the period below the resync period, which won't end well.

vagababov · 2019-06-04T16:34:05Z

Any revision that has received a "heartbeat" in the last M seconds/minutes/hours is that code elsewhere?

greghaynes · 2019-06-04T16:35:47Z

Correct, as is any route to a revision (even with 0% traffic) should prevent a revision from being GC'd, if we've seen behavior otherwise I'd really like to know the scenario to cause this bug.

re: GC time - we've talked about decoupling the GC controller from config and IMO this is one good reason to do that: Right now our min GC time is a function of config controller resync period which seems like a odd coupling for a user.

greghaynes · 2019-06-04T16:36:40Z

@vagababov https://github.com/knative/serving/blob/master/pkg/reconciler/route/reconcile_resources.go#L256 is the heavy lifting bit of code. The gc revisions code in the config controller uses annotation to determine what to GC

greghaynes · 2019-06-04T16:43:26Z

oh I see the issue now - It's what @mattmoor mentions about setting the GC period low. Since the heartbeat can only be guaranteed to happen as frequent as the route resync period (12h) if you lower your gc period below this then you'll GC revisions before the heartbeat is updated. An easy fix of sanity checking GC period seems fine to me

greghaynes · 2019-06-04T17:30:50Z

Pushed #4245

nak3 · 2019-06-05T01:08:06Z

I agree and I am thinking #4245 is better.

Anyway, thank you all taking look at this PR.

mattmoor · 2019-06-05T02:25:06Z

Thanks @nak3

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

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

knative-prow-robot requested review from dgerd and mattmoor June 4, 2019 06:12

knative-prow-robot reviewed Jun 4, 2019

View reviewed changes

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 Jun 4, 2019

knative-prow-robot added the area/API API objects and controllers label Jun 4, 2019

markusthoemmes reviewed Jun 4, 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 Jun 4, 2019

Get route name based on label value instead of same name

f82bd9b

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

nak3 mentioned this pull request Jun 5, 2019

Sanity check GC revision timeout #4245

Merged

mattmoor closed this Jun 5, 2019

nak3 deleted the keep-revision branch June 23, 2019 06:52

Conversation

nak3 commented Jun 4, 2019

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

knative-prow-robot commented Jun 4, 2019

Uh oh!

knative-prow-robot commented Jun 4, 2019

Uh oh!

markusthoemmes Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

nak3 Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Jun 4, 2019

Uh oh!

knative-metrics-robot commented Jun 4, 2019

Uh oh!

knative-metrics-robot commented Jun 4, 2019

Uh oh!

duglin commented Jun 4, 2019

Uh oh!

mattmoor commented Jun 4, 2019

Uh oh!

vagababov commented Jun 4, 2019

Uh oh!

greghaynes commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greghaynes commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greghaynes commented Jun 4, 2019

Uh oh!

greghaynes commented Jun 4, 2019

Uh oh!

nak3 commented Jun 5, 2019

Uh oh!

mattmoor commented Jun 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nak3 Jun 4, 2019 •

edited

Loading

greghaynes commented Jun 4, 2019 •

edited

Loading

greghaynes commented Jun 4, 2019 •

edited

Loading