Preserve Annotation to Prevent GC by MIBc · Pull Request #8614 · knative/serving

MIBc · 2020-07-13T15:12:18Z

Proposed Changes

Add preserve annotation to prevent GC from automatically deleting the revision.

knative-prow-robot · 2020-07-13T15:12:30Z

Hi @MIBc. 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.

MIBc · 2020-07-13T15:13:02Z

/assign @whaught

whaught · 2020-07-13T17:03:39Z

/ok-to-test

vagababov

There's probably a problem where we validate revision annotations to be in the specific set.

vagababov · 2020-07-13T17:41:59Z

+	// RevisionPreservedAnnotationKey is the annotation key used for preventing garbage collector
+	// from automatically deleting the revision.
+	RevisionPreservedAnnotationKey = GroupName + "/preserved"
+


I'd go more towards exclude-gc or no-gc.

no-gc is fine.

vagababov · 2020-07-13T17:42:37Z

 	})

 	for _, rev := range revs[gcSkipOffset:] {
+		if _, ok := rev.ObjectMeta.Annotations[serving.RevisionPreservedAnnotationKey]; ok {


Should we check that value is non-empty? I guess the name (as it is) asks for a bool value. So preserve=false, means GC is possible.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

whaught · 2020-07-14T15:51:30Z

There's probably a problem where we validate revision annotations to be in the specific set.

serving/pkg/apis/serving/metadata_validation.go

Line 36 in 3043a13

allowedAnnotations = sets.NewString(

I think we'll want to add this annotation here.

vagababov · 2020-07-14T17:23:49Z

+		preserve := rev.ObjectMeta.Annotations[serving.RevisionPreservedAnnotationKey]
+		if preserve == "true" {
+			continue
+		}


Here an in interesting question is: should we first filter than apply gcSkipOffset limit, since this counts as a revision for minRevision to retain as well?
cc @dprotaso @mattmoor

I think we want it to not count towards the min/max (and treat it as if it were an actively referenced revision even if it is not). The settings for automatic-gc say to the user "how many extra should we keep automatically keep around" for potential rollback, I don't think we'd want it to cut into that if the user is choosing to manage these manually - they could set min to 0 if, for example, they intend to manage everything manually.

I intend for the code layout to change a little bit as we integrate the new v2 settings which might give it a more clear place to live and gets rid of the offset: #8621

We need to make some more meaningful settings like whaught said to solve this question.

Fixes knative#8571 * Add preserve annotation to prevent GC from automatically deleting the revision.

knative-metrics-robot · 2020-07-15T09:53:46Z

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/gc/v2/gc.go	97.4%	97.6%	0.1

whaught · 2020-07-15T16:03:19Z

/lgtm

whaught · 2020-07-15T16:03:40Z

/assign @vagababov @dprotaso

vagababov

/lgtm

dprotaso · 2020-07-15T19:45:45Z

/lgtm
/approve

knative-prow-robot · 2020-07-15T19:45:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, MIBc

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~~ [dprotaso]
~~pkg/reconciler/OWNERS~~ [dprotaso]
~~pkg/testing/OWNERS~~ [dprotaso]

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

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 13, 2020

knative-prow-robot added area/API API objects and controllers needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Jul 13, 2020

knative-prow-robot requested review from dprotaso and mattmoor July 13, 2020 15:12

knative-prow-robot assigned whaught Jul 13, 2020

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

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

vagababov reviewed Jul 13, 2020

View reviewed changes

whaught reviewed Jul 13, 2020

View reviewed changes

Comment thread pkg/reconciler/gc/v2/gc_test.go

whaught mentioned this pull request Jul 14, 2020

Implementation of the new v2 garbage collector #8621

Merged

MIBc force-pushed the annotation branch from 6ece6a3 to 5050dcb Compare July 14, 2020 14:57

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

mattmoor reviewed Jul 14, 2020

View reviewed changes

MIBc force-pushed the annotation branch from 5050dcb to 81e8af5 Compare July 14, 2020 14:59

whaught reviewed Jul 14, 2020

View reviewed changes

Comment thread pkg/reconciler/gc/v2/gc.go Outdated

vagababov reviewed Jul 14, 2020

View reviewed changes

Preserve Annotation to Prevent GC

188c550

Fixes knative#8571 * Add preserve annotation to prevent GC from automatically deleting the revision.

MIBc force-pushed the annotation branch from 81e8af5 to 188c550 Compare July 15, 2020 09:48

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

knative-prow-robot assigned dprotaso and vagababov Jul 15, 2020

vagababov reviewed Jul 15, 2020

View reviewed changes

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

knative-prow-robot merged commit 78c81f8 into knative:master Jul 15, 2020

MIBc mentioned this pull request Aug 28, 2020

REQUEST: New membership for MIBc knative/community#243

Closed

Conversation

MIBc commented Jul 13, 2020

Proposed Changes

Uh oh!

knative-prow-robot commented Jul 13, 2020

Uh oh!

MIBc commented Jul 13, 2020

Uh oh!

whaught commented Jul 13, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

vagababov Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

MIBc Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

vagababov Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

MIBc Jul 14, 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!

Uh oh!

whaught commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vagababov Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

whaught Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

MIBc Jul 15, 2020 • 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!

knative-metrics-robot commented Jul 15, 2020

Uh oh!

whaught commented Jul 15, 2020

Uh oh!

whaught commented Jul 15, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Jul 15, 2020

Uh oh!

knative-prow-robot commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

whaught commented Jul 14, 2020 •

edited

Loading

MIBc Jul 15, 2020 •

edited

Loading