Create v2 GC settings by whaught · Pull Request #8615 · knative/serving

whaught · 2020-07-13T18:29:18Z

Proposed Changes

Creates a new setting for allowing users to specify a maximum number of revisions to retain.
- Currently we aggressively delete all stale revisions after a given amount of time. This will allow users to instead specify a number they would like to keep around and potentially reduce the time-bounds to zero.

knative-prow-robot

@whaught: 0 warnings.

Details

In response to this:

Issue #8208

Proposed Changes

Creates a new setting for allowing users to specify a maximum number of revisions to retain.

Currently we aggressively delete all stale revisions after a given amount of time. This will allow users to instead specify a number they would like to keep around and potentially reduce the time-bounds to zero.

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.

whaught · 2020-07-13T20:45:37Z

/hold

still needs unit test coverage for validation

vagababov · 2020-07-13T20:49:51Z

 			cm.AsDuration("stale-revision-timeout", &c.StaleRevisionTimeout),
 			cm.AsDuration("stale-revision-lastpinned-debounce", &c.StaleRevisionLastpinnedDebounce),

+			cm.AsInt64("stale-revision-maximum-generations", &c.StaleRevisionMaximumGenerations),


some validation below is probably in order:
0 seems invalid value (with current -1 sentinel)
all negative but -1 are invalid as well.
Also max cannot be less than min?

0 is actually a valid value. This is a count of inactive revisions only (not including actively referenced ones). The system will never consider active revisions for deletion and setting 0 would mean "delete everything as soon as it's not serving).

Similarly a min 0 would mean "delete everything that isn't active as soon as the staleness times have elapsed"

vagababov · 2020-07-13T20:51:23Z

ugh :) you've repushed with validation

Co-authored-by: Victor Agababov <vagababov@gmail.com>

knative-test-reporter-robot · 2020-07-13T23:19:09Z

The following jobs failed:

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

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

pkg/gc.TestOurConfig
pkg/gc.TestOurConfig/Example_config
pkg/gc.TestOurConfig/With_value_overrides
pkg/gc.TestOurConfig/Below_minimum_timeout
pkg/gc.TestOurConfigWithResponsiveGC
pkg/gc.TestOurConfigWithResponsiveGC/Example_config
pkg/gc.TestOurConfigWithResponsiveGC/With_value_overrides
pkg/gc.TestOurConfigWithResponsiveGC/Invalid_maximum_generation

and 2 more.

whaught · 2020-07-13T23:31:12Z

/unhold

knative-metrics-robot · 2020-07-13T23:34:33Z

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/gc/config.go	100.0%	94.7%	-5.3

vagababov

/lgtm
/assign @dprotaso @mattmoor

mattmoor

couple of drive-by nits.

Co-authored-by: Matt Moore <mattmoor@vmware.com>

mattmoor

/lgtm
/approve

I'm still puzzled by why the missing quotes didn't blow up our tests, but they look right and it's not your change's fault if they aren't 😅

knative-prow-robot · 2020-07-14T20:02:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~config/OWNERS~~ [mattmoor]
~~pkg/gc/OWNERS~~ [mattmoor]

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

whaught added 3 commits July 13, 2020 11:18

Create a maximum revision GC setting

6cd4f07

update yaml

c3a8e95

alpha note added

8dc2f83

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

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/API API objects and controllers labels Jul 13, 2020

knative-prow-robot reviewed Jul 13, 2020

View reviewed changes

knative-prow-robot requested review from dprotaso and mattmoor July 13, 2020 18:29

whaught added 2 commits July 13, 2020 11:38

update checksum

b02a841

fix comments

815e59e

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 13, 2020

checksum and validation

7fd9a9b

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 13, 2020

vagababov reviewed Jul 13, 2020

View reviewed changes

Comment thread pkg/gc/config.go Outdated

whaught and others added 5 commits July 13, 2020 14:02

Update config/core/configmaps/gc.yaml

c44ec8c

Co-authored-by: Victor Agababov <vagababov@gmail.com>

fix comments

f86fc83

fix unit test

0f7fe32

fix both check

f4b67cc

include unit test

f37c42b

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 Jul 13, 2020

whaught added 3 commits July 13, 2020 15:35

make new settings for v2

917aa15

v2 gc settings

df45a58

fix defaults

81268d8

vagababov reviewed Jul 13, 2020

View reviewed changes

Comment thread config/core/configmaps/gc.yaml

Comment thread pkg/gc/config.go Outdated

Comment thread pkg/gc/config.go Outdated

whaught force-pushed the gc-max branch from bee7194 to 1d82839 Compare July 13, 2020 23:13

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

fix unit test. expand comment

ccdadd5

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 Jul 13, 2020

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 13, 2020

fix validation error

4042084

whaught changed the title ~~Create a Maximum setting for revisions to retain~~ Create v2 GC settings Jul 13, 2020

vagababov reviewed Jul 14, 2020

View reviewed changes

knative-prow-robot assigned dprotaso, mattmoor and vagababov Jul 14, 2020

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

whaught mentioned this pull request Jul 14, 2020

Implementation of the new v2 garbage collector #8621

Merged

mattmoor reviewed Jul 14, 2020

View reviewed changes

Comment thread config/core/configmaps/gc.yaml Outdated

Comment thread config/core/configmaps/gc.yaml Outdated

Comment thread config/core/configmaps/gc.yaml Outdated

Comment thread pkg/gc/config.go Outdated

Comment thread pkg/gc/config.go Outdated

commit suggestions

062ad4d

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

whaught and others added 3 commits July 13, 2020 21:44

Update pkg/gc/config.go

1cd1ab0

Co-authored-by: Matt Moore <mattmoor@vmware.com>

remove prefix

ee250bd

Merge branch 'gc-max' of github.com:whaught/serving into gc-max

faa0167

mattmoor reviewed Jul 14, 2020

View reviewed changes

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

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

knative-prow-robot merged commit fccf86b into knative:master Jul 14, 2020

whaught deleted the gc-max branch July 16, 2020 22:24

Conversation

whaught commented Jul 13, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

whaught commented Jul 13, 2020

Uh oh!

Uh oh!

Uh oh!

vagababov Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

whaught Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

vagababov commented Jul 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-test-reporter-robot commented Jul 13, 2020

Uh oh!

whaught commented Jul 13, 2020

Uh oh!

knative-metrics-robot commented Jul 13, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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 Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants