Fix e2e rate limiter timeout in namespace permission probe#495
Conversation
The createNS helper passed a shared 120s context to every Create call in the retry loop. When the client rate limiter was exhausted from prior operations, Create blocked in ratelimiter.Wait(ctx) until the context expired, producing "client rate limiter Wait returned an error: context deadline exceeded". Use a per-attempt 10s context so each Create call can fail fast and retry, rather than blocking on the rate limiter for the full timeout.
WalkthroughThe namespace-permissions readiness probe in the test suite now enforces an overall 120-second limit using a computed deadline rather than a single shared timeout context. Each probe attempt uses its own 10-second timeout for the create operation, with timeout detection shifted from checking context errors to comparing against the deadline. Error handling now treats "context deadline exceeded" errors as non-fatal alongside Forbidden errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aliceh, dustman9000 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@dustman9000: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/validation_webhook_tests.go (1)
84-84: Prefer typed timeout checks over error-string matching.Line 84 uses
strings.Contains(createErr.Error(), "context deadline exceeded"), which is brittle and may misclassify unrelated errors. Use standard-libraryerrors.Is(createErr, context.DeadlineExceeded)(with an alias to avoid collision withk8serrorsimport).♻️ Proposed refactor
import ( "context" + goerrors "errors" "strconv" "strings" "time" @@ - if !errors.IsForbidden(createErr) && !strings.Contains(createErr.Error(), "context deadline exceeded") { + if !errors.IsForbidden(createErr) && !goerrors.Is(createErr, context.DeadlineExceeded) { Expect(createErr).ShouldNot(HaveOccurred(), "Unexpected error probing namespace readiness") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/validation_webhook_tests.go` at line 84, Replace the brittle string check for timeouts with a proper typed check: where you currently test createErr with errors.IsForbidden(createErr) and strings.Contains(createErr.Error(), "context deadline exceeded"), stop using the string match and instead use the standard library errors.Is(createErr, context.DeadlineExceeded); to avoid import collision alias the k8s error package (e.g., k8serrors) and ensure the std "errors" and "context" packages are imported, keeping the existing createErr and errors.IsForbidden usage but changing the timeout branch to errors.Is(createErr, context.DeadlineExceeded).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/validation_webhook_tests.go`:
- Line 84: Replace the brittle string check for timeouts with a proper typed
check: where you currently test createErr with errors.IsForbidden(createErr) and
strings.Contains(createErr.Error(), "context deadline exceeded"), stop using the
string match and instead use the standard library errors.Is(createErr,
context.DeadlineExceeded); to avoid import collision alias the k8s error package
(e.g., k8serrors) and ensure the std "errors" and "context" packages are
imported, keeping the existing createErr and errors.IsForbidden usage but
changing the timeout branch to errors.Is(createErr, context.DeadlineExceeded).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f4b8d172-f4aa-4868-bb8b-107dda5c299d
📒 Files selected for processing (1)
test/e2e/validation_webhook_tests.go
2 participants
Summary
sre-regular-user-validatione2e test failing withclient rate limiter Wait returned an error: context deadline exceededcreateNShelper passed a shared 120s context to everyCreatecall in the retry loop. When the client rate limiter was exhausted,Createblocked inratelimiter.Wait(ctx)until the context expiredCreatecall fails fast and retries, instead of blocking on the rate limiter for the full timeoutTest plan
Summary by CodeRabbit