NO-JIRA: detect unused annotation rules #2117
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@danwinship: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
danwinship
force-pushed
the
detect-unused-rules
branch
from
4918215 to
815d932
Compare
|
@danwinship: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
danwinship
force-pushed
the
detect-unused-rules
branch
from
815d932 to
7d1f521
Compare
|
@danwinship: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
| for _, item := range items { | ||
| if _, exists := matches[item]; exists { |
There was a problem hiding this comment.
Don't we index matches by the label and not the item/pattern?
Can you please introduce simple tests for the new logic?
There was a problem hiding this comment.
@danwinship can you please address this point?
There was a problem hiding this comment.
yes, that check was incorrect, and I don't remember why I'd originally added it so I just removed it.
There was a problem hiding this comment.
Ack, thanks for the update. The changes look cleaner now.
| @@ -85,7 +70,6 @@ var ( | |||
|
|
|||
| // TODO(node): configure the cri handler for the runtime class to make this work | |||
| "should run a Pod requesting a RuntimeClass with a configured handler", | |||
| "should reject a Pod requesting a RuntimeClass with conflicting node selector", | |||
There was a problem hiding this comment.
I see that some of the tests from this commit are still present in the codebase.
- Why were they not matched?
- For the ones that are present, are we confident that they can be enabled? I suppose we should run payload tests to be sure.
There was a problem hiding this comment.
For the ones that are present, are we confident that they can be enabled?
No tests are enabled by this PR; you can see that openshift-hack/e2e/annotate/generated/zz_generated.annotations.go itself is unmodified. The PR only removes rules that had no effect on the output.
Why were they not matched?
In this case, it appears that the test's full name ends up being "[sig-node] RuntimeClass should reject a Pod requesting a RuntimeClass with conflicting node selector", so it is already matched by the pattern "RuntimeClass should reject" above (old line 80 / new line 65).
The rule being removed here was added as part of the kube 1.16 rebase in openshift/origin#23811, without noticing that it was redundant with the other rule which was added 4 months earlier.
The rule on line 65 was added on 2019-05-18 in openshift/origin#22857 as a temporary hack until we had an updated RHCOS build:
+ // requires a 1.14 kubelet
+ "when the NodeLease feature is enabled",
+ "RuntimeClass should reject",It was reverted one month later in openshift/origin#23198 but then un-reverted two days later in openshift/origin#23227. Then people forgot about it, until 2 years later Clayton did some cleanup and removed the comment and the first line in openshift/origin#24835, but by that point nobody remembered that the comment applied to the second line too, so it didn't get removed.
Removing "RuntimeClass should reject" does change the generated output though, enabling 3 tests that are currently disabled.
But anyway, this sort of thing is not unique to the redundant rules; we have all sorts of rules that were added "temporarily" 12 releases ago and then forgotten about forever. 🙁
There was a problem hiding this comment.
Thanks for the thorough analysis
I suppose for the tests that have been flaking in the past, the current CI runs/analysis should be more relevant than the stale annotation skips.
No tests are enabled by this PR; you can see that openshift-hack/e2e/annotate/generated/zz_generated.annotations.go itself is unmodified. The PR only removes rules that had no effect on the output
right, payload runs won't help here...
| @@ -194,25 +155,13 @@ var ( | |||
| // tests that are known flaky | |||
| "[Flaky]": { | |||
| `Job should run a job to completion when tasks sometimes fail and are not locally restarted`, // seems flaky, also may require too many resources | |||
| // TODO(node): test works when run alone, but not in the suite in CI | |||
| `\[Feature:HPA\] Horizontal pod autoscaling \(scale resource: CPU\) \[sig-autoscaling\] ReplicationController light Should scale from 1 pod to 2 pods`, | |||
There was a problem hiding this comment.
I went only through a couple of tests, but for example I see this one is still present.
Or does it make sense to enable group by group in multiple PRs and with a time delay?
There was a problem hiding this comment.
It appears that this test got renamed, with the [sig-autoscaling] moved from the middle to the front, such that this pattern no longer matches.
So we had a rule that matched the old name of a test that used to fail, and then the test got fixed, but we still kept skipping it, and then later the test got renamed, so now the rule no longer matches, and we don't skip it, which is fine, because it passes now anyway.
There was a problem hiding this comment.
Or does it make sense to enable group by group in multiple PRs and with a time delay?
I can change it to just print warnings rather than erroring out, and then people can do whatever they want with it... ?
There was a problem hiding this comment.
I guess, you are right. It should be fine if the tests pass anyway nowadays.
There was a problem hiding this comment.
I think it is okay to hard fail and remove the old skips. Let's see @bertinatto opinion on this.
There was a problem hiding this comment.
Let's keep it failing for now
openshift-merge-robot
added
the
needs-rebase
danwinship
force-pushed
the
detect-unused-rules
branch
from
7d1f521 to
2c3f544
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@danwinship: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: atiratree, bertinatto, danwinship 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/remove-label backports/unvalidated-commits |
openshift-ci
bot
added
backports/validated-commits
bertinatto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@danwinship: This pull request explicitly references no jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@danwinship: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Tests get removed and renamed but we never update our annotation rules...
This makes it so that
update-test-annotations.shwill actually fail if there are unused rules, though it could be made a warning instead.This also removes all currently-unused rules. In some cases it's clear that a test just got renamed, and in some cases it seems pretty likely that weird tests got removed or recategorized upstream. But I didn't do any investigation of any of them.
Presumably you'd want this in origin too...
/assign @atiratree
(Previously: #830)