Fix dynamicresources_test flakiness #133321
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
with this PR, the extended resources tests all pass with zero flake. $ stress ./dynamicresources.test -test.run TestPlugin |
|
/assign @pohly |
|
/retest |
3ce5e25 to
f0c8bad
Compare
|
LGTM label has been added. Git tree hash: c819ef4d149340e7f9599ea340d1f770130ffd25
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: macsko, yliaog 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 |
| pollErr := wait.PollUntilContextTimeout(ctx, 1*time.Second, time.Duration(AssumeExtendedResourceTimeoutDefaultSeconds)*time.Second, true, | ||
| func(ctx context.Context) (bool, error) { | ||
| if err := pl.draManager.ResourceClaims().AssumeClaimAfterAPICall(claim); err != nil { | ||
| logger.V(5).Info("Claim not stored in assume cache", "claim", klog.KObj(claim), "err", err) |
There was a problem hiding this comment.
nit: You could consider moving the log below, i.e.
if errors.Is(err, assumecache.ErrNotFound) {
return false, nil
}
logger.V(5).Info("Claim not stored in assume cache", "claim", klog.KObj(claim), "err", err)not to spam the console with this.
…ot present in the cache (dynamicresources.go) 2/ modified the assume cache verification to not error out as long as the expected claim is in the cache, no matter its latest and api object are different or not. (dynamicresources_test.go). 3/ fixed nil panic as seen from https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/133321/pull-kubernetes-integration/1952472629470302208
|
/lgtm |
|
LGTM label has been added. Git tree hash: 94fa76bdaf00d908919cbe25ecbd555878d4111d
|
|
/retest |
|
@kubernetes/sig-release-leads |
|
Or @kubernetes/release-team-leads |
|
/milestone v1.34 |
|
/hold This PR is still flaking the unit tests in the package it is trying to stabilize: . Please run with -race and stress locally to show this is giving us a flake free test before merge |
|
/retest |
|
The failed test below is about DRADeviceTaints feature, not about DRAExtendedResource feature this PR is focusing on fixing. Similarly, the e2e test failure is also not related to DRAExtendedResource feature. I think it's better for feature owners who are more familar with the feature, and test to look into these flakiness. |
|
Ok, feel free to unhold if you are sure the flaking test is distinct from the issue this PR is fixing, and that the flake isn't caused by the changes in this PR |
|
/hold cancel |
1/ added retries to AssumeClaimAfterAPICall for the object which is not present in the cache (dynamicresources.go)
2/ added wait for informer in postFilter verification test (dynamicresources_test.go).
3/ modified the assume cache verification to not error out as long as
the expected claim is in the cache, no matter its latest and api object
are different or not. (dynamicresources_test.go)
4/ fixed nil panic as seen from https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/133321/pull-kubernetes-integration/1952472629470302208
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR is related to:
#133302
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: