Sanity check GC revision timeout by greghaynes · Pull Request #4245 · knative/serving

greghaynes · 2019-06-04T17:27:09Z

If revision timeout is set below route resync period our gc controller
will GC revisions before they are able to receive a heartbeat.

Fixes: #4234

Release Note

NONE

knative-prow-robot

@greghaynes: 1 warning.

Details

In response to this:

If revision timeout is set below route resync period our gc controller
will GC revisions before they are able to receive a heartbeat.

Fixes: #4234

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.

nak3 · 2019-06-05T00:50:47Z

+func SanityCheckedConfigMapLoader(minRevisionTimeout time.Duration) func(configMap *corev1.ConfigMap) (*Config, error) {
+	return func(configMap *corev1.ConfigMap) (*Config, error) {
+		cfg, err := NewConfigFromConfigMap(configMap)
+		if cfg != nil && cfg.StaleRevisionTimeout < minRevisionTimeout {


We do not need to check cfg.StaleRevisionTimeout - cfg.StaleRevisionLastpinnedDebounce < minRevisionTimeout?

It looks like lastPinned annotation is skipped to update until StaleRevisionLastpinnedDebounce here:

serving/pkg/reconciler/route/reconcile_resources.go

Lines 281 to 284 in f50908d

// Enforce a delay before performing an update on lastPinned to avoid excess churn.

if lastPin.Add(lpDebounce).After(c.clock.Now()) {

return nil

}

It looks like this comment already warns about it.

serving/config/config-gc.yaml

Lines 44 to 45 in f50908d

# This minus lastpinned-debounce be longer than the controller resync period (10 hours)

stale-revision-timeout: "15h"

nak3 · 2019-06-05T01:01:20Z

I am prefer to this PR rather than mine (#4235). But I have one concern.
We will not able to test GC easily once this PR was merged(?) Whenever we would like to test GC and observe the behavior, we have to wait for 10 hours every time 😰

mattmoor · 2019-06-05T02:24:45Z

 	return &c, nil
 }
+
+// SanityCheckedConfigMapLoader provides sanity checking for minRevisionTimeout and returns an error if this value is invalid


Can we just change NewConfigFromConfigMap to add this validation? This is generally where we do this sort of validation.

The issue is ResyncPeriod is a variable and we need to supply a constructor to the ConfigStore which is of type func(*k8s.io/api/core/v1.ConfigMap) (*Config , error). The alternative way I can see Is to make NewConfigFromConfigMap a method on a struct which stores the ResyncPeriod.

I am curious why NewConfigFromConfigMap needs this signature if the return value of this function does? Can't this just be curried? e.g. time.Duration -> *corev1.ConfigMap -> (*Config, error)?

Curry made. :)

Can't wait for the haskell rewrite :)

greghaynes · 2019-06-05T20:21:27Z

@nak3 #3910 should help with your testing issue - we should be able to adjust the resync period of that reconciler in that case.

greghaynes · 2019-06-05T22:34:12Z

/test pull-knative-serving-upgrade-tests

If revision timeout is set below route resync period our gc controller will GC revisions before they are able to receive a heartbeat. Fixes: knative#4234

mattmoor

/lgtm
/approve

knative-prow-robot · 2019-06-07T02:43:11Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [mattmoor]

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

From ROLES.md 1/ Reviewer of the codebase for at least 3 months. knative#4245 is 9 months old. 2/ Primary reviewer for at least 10 substantial PRs to the codebase. [4 XLs](https://github.com/knative/serving/pulls?q=is%3Apr++is%3Aclosed+reviewed-by%3Anak3+label%3Asize%2FXL+) [13 Ls](https://github.com/knative/serving/pulls?q=is%3Apr++is%3Aclosed+reviewed-by%3Anak3+label%3Asize%2FL) 3/ [56 Reviewed PRs](https://github.com/knative/serving/pulls?q=is%3Apr++is%3Aclosed+reviewed-by%3Anak3+) 4/ [116 Merged PRs]( https://github.com/knative/serving/pulls?q=is%3Apr+is%3Aclosed+author%3Anak3) 4/ Nominated by nghia@ with no objections from other leads

* Add nak3 to networking approvers. From ROLES.md 1/ Reviewer of the codebase for at least 3 months. #4245 is 9 months old. 2/ Primary reviewer for at least 10 substantial PRs to the codebase. [4 XLs](https://github.com/knative/serving/pulls?q=is%3Apr++is%3Aclosed+reviewed-by%3Anak3+label%3Asize%2FXL+) [13 Ls](https://github.com/knative/serving/pulls?q=is%3Apr++is%3Aclosed+reviewed-by%3Anak3+label%3Asize%2FL) 3/ [56 Reviewed PRs](https://github.com/knative/serving/pulls?q=is%3Apr++is%3Aclosed+reviewed-by%3Anak3+) 4/ [116 Merged PRs]( https://github.com/knative/serving/pulls?q=is%3Apr+is%3Aclosed+author%3Anak3) 4/ Nominated by nghia@ with no objections from other leads * Sort the lines. * Sort lines alphabetically * Remove merge conflict markers.

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

View reviewed changes

Comment thread pkg/gc/config.go Outdated

knative-prow-robot requested review from markusthoemmes and tcnghia June 4, 2019 17:27

knative-prow-robot added area/API API objects and controllers area/networking labels Jun 4, 2019

greghaynes force-pushed the sanity-check-gc-timeout branch from 3518665 to 21c7e2b Compare June 4, 2019 17:30

greghaynes mentioned this pull request Jun 4, 2019

Skip GC proces when revision is still used by route #4235

Closed

greghaynes force-pushed the sanity-check-gc-timeout branch 2 times, most recently from 85f6868 to e51f97d Compare June 4, 2019 17:37

nak3 reviewed Jun 5, 2019

View reviewed changes

mattmoor reviewed Jun 5, 2019

View reviewed changes

greghaynes force-pushed the sanity-check-gc-timeout branch 3 times, most recently from 3734048 to b5ee977 Compare June 5, 2019 21:01

nak3 reviewed Jun 6, 2019

View reviewed changes

Comment thread pkg/gc/config.go Outdated

nak3 reviewed Jun 6, 2019

View reviewed changes

Comment thread pkg/gc/config.go Outdated

nak3 reviewed Jun 6, 2019

View reviewed changes

Comment thread pkg/gc/config.go Outdated

Sanity check GC revision timeout

4ab8a18

If revision timeout is set below route resync period our gc controller will GC revisions before they are able to receive a heartbeat. Fixes: knative#4234

greghaynes force-pushed the sanity-check-gc-timeout branch from b5ee977 to 4ab8a18 Compare June 6, 2019 17:30

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2019

mattmoor reviewed Jun 7, 2019

View reviewed changes

knative-prow-robot assigned mattmoor Jun 7, 2019

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

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

knative-prow-robot merged commit 73c274b into knative:master Jun 7, 2019

tcnghia mentioned this pull request Mar 11, 2020

Add nak3 to networking-approvers. #7216

Merged

	// Enforce a delay before performing an update on lastPinned to avoid excess churn.
	if lastPin.Add(lpDebounce).After(c.clock.Now()) {
	return nil
	}

	# This minus lastpinned-debounce be longer than the controller resync period (10 hours)
	stale-revision-timeout: "15h"

Conversation

greghaynes commented Jun 4, 2019

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nak3 Jun 5, 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!

nak3 Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

nak3 commented Jun 5, 2019

Uh oh!

mattmoor Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

greghaynes Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

mattmoor Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

greghaynes Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

greghaynes commented Jun 5, 2019

Uh oh!

greghaynes commented Jun 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jun 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nak3 Jun 5, 2019 •

edited

Loading