🐛 Filter cluster-wide objects asserted in ResourceVersion tests to exclude objects of parallel tests #10560
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
chrischdi
force-pushed
the
pr-e2e-fix-resourceversion-tests
branch
from
a3cf70c to
4dbe1f7
Compare
k8s-ci-robot
added
size/L
|
/test help |
|
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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 kubernetes-sigs/prow repository. |
|
/test pull-cluster-api-e2e-main |
This is fine, the changed function signature is explicitly marked not as stable, because of only being used in e2e tests. |
…o exclude objects of parallel tests
chrischdi
force-pushed
the
pr-e2e-fix-resourceversion-tests
branch
from
4dbe1f7 to
1588a2c
Compare
|
/test pull-cluster-api-e2e-main |
chrischdi
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign sbueringer fabriziopandini |
/override pull-cluster-api-apidiff-main |
|
@chrischdi: chrischdi unauthorized: /override is restricted to Repo administrators. 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 kubernetes-sigs/prow repository. |
Let's ignore this, its optional anyway :D |
| // to filter out cluster-wide objects which don't have the clusterName in their | ||
| // object name. This avoids assertions on objects which are part of in-parallel | ||
| // running tests like ExtensionConfig. | ||
| func GetOwnerGraphFilterByClusterNameFunc(clusterName string) func(u unstructured.Unstructured) bool { |
There was a problem hiding this comment.
what about:
SkipClusterObjectsWithoutNamePrefix
There was a problem hiding this comment.
SkipClusterObjectsWithoutNameContains
Because we don't match prefix
There was a problem hiding this comment.
SkipClusterObjectsWithNameFilter
chrischdi
added
the
tide/merge-method-squash
|
/test pull-cluster-api-e2e-main |
|
Note: #10530 was cherry-picked to v1.7 so we should fix at least the resourceVersion test in v1.7 too. I suggest for v1.7 to just filter inside the new function to not change the function signatures there. |
chrischdi
force-pushed
the
pr-e2e-fix-resourceversion-tests
branch
from
fc5e8e6 to
e183cb2
Compare
|
/test pull-cluster-api-e2e-main |
WDYT about checking via cs.k8s.io if someone even uses those funcs? Because I think if they don't, let's just keep it simple and backport this PR (CAPV can easily be adjusted) |
|
Just checked. Only CAPI & CAPV. I would simply cherry-pick. We also have a disclaimer for our test packages regarding changes and it seems very unlikely that anyone is affected anyway. (+ it's easy to just pass in nil to get the previous behavior) |
|
/cherry-pick release-1.7 |
|
@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.7 in a new PR and assign it to you. 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 kubernetes-sigs/prow repository. |
|
/test pull-cluster-api-e2e-main |
|
@chrischdi: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
Thx /lgtm /assign @fabriziopandini |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 998456d3d9cad07d1854d8eea2a5f768465a9a6b
|
|
/lgtm great job in catching this race condition while running E2E tests! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
k8s-ci-robot
added
the
approved
|
@chrischdi: new pull request created: #10570 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 kubernetes-sigs/prow repository. |
sbueringer
changed the title
sbueringer
changed the title
What this PR does / why we need it:
In a rare test case where k8s-upgrade-with-runtime-sdk and the quickstart tests's assertion on
ValidateResourceVersionStableare run in-parallel, we can have a flaky test because:Getting the owner graph included the RuntimeExtension. When
k8s-upgrade-with-runtime-sdkfinishes, it deleted the CR again, so the assertion inValidateResourceVersionStablefailed and the test failed.xref: failure occurency
Also: enables the
ValidateResourceVersionStabletest for thek8s-upgrade-with-runtime-sdkto also test the ExtensionConfig.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
/area e2e-testing