Fix gc: delete revision in correct orderer #12752

Taction · 2022-03-18T14:53:29Z

Proposed Changes

First, delete stale revision until only min revision is left or all stale revision has been deleted.
Then, if the count of non-stale revision is greater than max, delete extra older revisions past max.

Release Note

Fix gc: delete revision in correct orderer

Signed-off-by: zhangchao <[email protected]>

knative-prow-robot · 2022-03-18T14:53:45Z

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

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.

psschwei · 2022-03-18T15:05:15Z

/ok-to-test

psschwei

Will wait for the CI to run to give a proper review, but anecdotally I ran the "Steps to Reproduce the Problem" from the linked issue and after GC only the proper revisions remained

Taction · 2022-03-19T07:54:17Z

Make sure the following and this issue will appear every time:
Make sure your gc config is empty before you start, and then follow the "Steps to Reproduce the Problem".
In step 2 you need to create more than 6 revisions to make it easier to appear.(or set non-active to a smaller value)

The reason is the following code will not make the revs in reverse order as expected.
https://github.com/knative/serving/blob/main/pkg/reconciler/gc/gc.go#L92
Revisions in ordered [1,2,3,4,5,6,7] will end up [2,3,4,5,6,7,1], so if more than one revisions need to be deleted, this error will occur.

dprotaso · 2022-03-21T13:42:10Z

We should add a unit test for the erroneous case

pkg/reconciler/gc/gc.go

Taction · 2022-03-21T14:33:59Z

We should add a unit test for the erroneous case

@dprotaso Thanks for your review. Any suggestions about which tests need to be improved or which these tests should be placed?

Signed-off-by: zhangchao <[email protected]>

codecov · 2022-03-22T17:26:35Z

Codecov Report

Merging #12752 (3f45d01) into main (0753bb1) will increase coverage by 0.13%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##             main   #12752      +/-   ##
==========================================
+ Coverage   87.31%   87.44%   +0.13%     
==========================================
  Files         196      196              
  Lines        9750     9760      +10     
==========================================
+ Hits         8513     8535      +22     
+ Misses        952      937      -15     
- Partials      285      288       +3

Impacted Files	Coverage Δ
pkg/reconciler/gc/gc.go	`94.59% <90.47%> (-2.47%)`	⬇️
pkg/reconciler/revision/background.go	`90.00% <0.00%> (-1.82%)`	⬇️
pkg/reconciler/configuration/configuration.go	`84.61% <0.00%> (-1.54%)`	⬇️
pkg/activator/net/revision_backends.go	`91.73% <0.00%> (-0.87%)`	⬇️
pkg/apis/serving/fieldmask.go	`95.17% <0.00%> (+0.03%)`	⬆️
pkg/apis/config/features.go	`96.00% <0.00%> (+0.16%)`	⬆️
pkg/reconciler/revision/rate_limiter.go	`100.00% <0.00%> (+83.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0753bb1...3f45d01. Read the comment docs.

pkg/reconciler/gc/reconciler_test.go

dprotaso · 2022-03-22T18:52:44Z

pkg/reconciler/gc/gc.go

+ // This situation should not appear in most cases, so performance here is not a big concern.
+ if i > staleCount {
+ for j := i; j > staleCount; j-- {
+ revs[j] = revs[j-1]


FYI - there's no test coverage of this movement/shift

https://app.codecov.io/gh/knative/serving/compare/12752/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr%20comments&utm_term=knative#D1L91

I wonder if all this shifting logic could be simpler if we just let the array be sparse

ie. null out the entry and skip over them further down when pruning max revisions

Yes! This makes sense. Done.

Signed-off-by: zhangchao <[email protected]>

dprotaso · 2022-03-23T14:04:49Z

/lgtm
/approve

thanks for the PR 🎉

knative-prow-robot · 2022-03-23T14:05:59Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/reconciler/OWNERS~~ [dprotaso]

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

fix gc delete revision orderer

a5f8639

Signed-off-by: zhangchao <[email protected]>

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 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. labels Mar 18, 2022

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 Mar 18, 2022

psschwei reviewed Mar 18, 2022

View reviewed changes

dprotaso reviewed Mar 21, 2022

View reviewed changes

pkg/reconciler/gc/gc.go Outdated Show resolved Hide resolved

Taction added 2 commits March 22, 2022 23:10

fix review: remove array

7b6de82

Signed-off-by: zhangchao <[email protected]>

add unit test

5e9740c

Signed-off-by: zhangchao <[email protected]>

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 Mar 22, 2022

dprotaso reviewed Mar 22, 2022

View reviewed changes

fix review

3f45d01

Signed-off-by: zhangchao <[email protected]>

knative-prow-robot assigned dprotaso Mar 23, 2022

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

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

knative-prow-robot merged commit d6f0dbb into knative:main Mar 23, 2022

Taction deleted the fix_gc branch March 24, 2022 01:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gc: delete revision in correct orderer #12752

Fix gc: delete revision in correct orderer #12752

Taction commented Mar 18, 2022

knative-prow-robot commented Mar 18, 2022

psschwei commented Mar 18, 2022

psschwei left a comment

Taction commented Mar 19, 2022 •

edited

Loading

dprotaso commented Mar 21, 2022

Taction commented Mar 21, 2022

codecov bot commented Mar 22, 2022 •

edited

Loading

dprotaso Mar 22, 2022

dprotaso Mar 22, 2022

Taction Mar 23, 2022

dprotaso commented Mar 23, 2022

knative-prow-robot commented Mar 23, 2022

Fix gc: delete revision in correct orderer #12752

Fix gc: delete revision in correct orderer #12752

Conversation

Taction commented Mar 18, 2022

Proposed Changes

knative-prow-robot commented Mar 18, 2022

psschwei commented Mar 18, 2022

psschwei left a comment

Choose a reason for hiding this comment

Taction commented Mar 19, 2022 • edited Loading

dprotaso commented Mar 21, 2022

Taction commented Mar 21, 2022

codecov bot commented Mar 22, 2022 • edited Loading

Codecov Report

dprotaso Mar 22, 2022

Choose a reason for hiding this comment

dprotaso Mar 22, 2022

Choose a reason for hiding this comment

Taction Mar 23, 2022

Choose a reason for hiding this comment

dprotaso commented Mar 23, 2022

knative-prow-robot commented Mar 23, 2022

Taction commented Mar 19, 2022 •

edited

Loading

codecov bot commented Mar 22, 2022 •

edited

Loading