test: retry operations in e2e tests#1071
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: simonpasquier The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAlert rule E2E tests now use shared polling and retry helpers for creation, deletion, and PrometheusRule verification. A Makefile target builds the E2E test binary, and ChangesAlert rule E2E reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/helpers_test.go (1)
23-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
pollnever honors a caller-supplied context; it hardcodescontext.Background().
polldoesn't accept acontext.Context, so it can't be cancelled by the outer test context (e.g. a test-level deadline). All three call sites pass their ownctxinto the retried operation but the polling loop itself runs on an unrelated background context. As per path instructions, Go code should usecontext.Context for cancellation and timeouts.♻️ Suggested fix: thread ctx through
-func poll(interval, timeout time.Duration, f func() error) error { +func poll(ctx context.Context, interval, timeout time.Duration, f func() error) error { var lastErr error - err := wait.PollUntilContextTimeout(context.Background(), interval, timeout, true, func(context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(context.Context) (bool, error) { if lastErr = f(); lastErr != nil { return false, nil }(call sites would then pass their existing
ctx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/helpers_test.go` around lines 23 - 37, Update poll to accept a context.Context parameter and pass it to wait.PollUntilContextTimeout instead of context.Background(). Update all three poll call sites to provide their existing ctx values, preserving the current retry and error-handling behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/delete_alert_rule_test.go`:
- Line 95: Handle the error returned by resp.Body.Close() in the defer near the
affected test, rather than discarding it. Update the deferred cleanup to check
the close result and propagate or report any error using the test’s existing
error-handling mechanism.
In `@test/e2e/helpers_test.go`:
- Around line 39-50: The retries around non-idempotent alert-rule operations are
unsafe. In test/e2e/helpers_test.go:39-50, update createRuleViaAPIWithRetry to
verify existing state or otherwise avoid creating a duplicate when the server
may have already processed the request; in
test/e2e/delete_alert_rule_test.go:80-121, make the bulk-delete retry treat
already-deleted/not-found rules as success and avoid repeatedly failing on IDs
removed by an earlier attempt.
---
Nitpick comments:
In `@test/e2e/helpers_test.go`:
- Around line 23-37: Update poll to accept a context.Context parameter and pass
it to wait.PollUntilContextTimeout instead of context.Background(). Update all
three poll call sites to provide their existing ctx values, preserving the
current retry and error-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: db90c7b5-bec6-45bc-81a7-c48e7cf18598
📒 Files selected for processing (3)
test/e2e/create_alert_rule_test.gotest/e2e/delete_alert_rule_test.gotest/e2e/helpers_test.go
| if err != nil { | ||
| return fmt.Errorf("failed to make delete request: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked resp.Body.Close() error return.
Flagged by errcheck; per path instructions, Go code should never ignore error returns.
🔧 Suggested fix
- defer resp.Body.Close()
+ defer func() {
+ if cerr := resp.Body.Close(); cerr != nil {
+ t.Logf("failed to close response body: %v", cerr)
+ }
+ }()🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 95-95: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/delete_alert_rule_test.go` at line 95, Handle the error returned by
resp.Body.Close() in the defer near the affected test, rather than discarding
it. Update the deferred cleanup to check the close result and propagate or
report any error using the test’s existing error-handling mechanism.
Sources: Path instructions, Linters/SAST tools
| func createRuleViaAPIWithRetry(ctx context.Context, f *framework.Framework, createAlertRuleRequest managementrouter.CreateAlertRuleRequest) (string, error) { | ||
| var id string | ||
| err := poll(time.Second, 20*time.Second, func() error { | ||
| var err error | ||
| id, err = createRuleViaAPI(ctx, f, createAlertRuleRequest) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create alert rule: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
| return id, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Retry-wrapping non-idempotent create/delete calls without idempotency safeguards. Both sites use poll to retry an HTTP write whose server-side effect may already have taken hold even though the client observed an error, so retries can create duplicates or fail permanently against already-mutated state — the opposite of the intended flakiness reduction.
test/e2e/helpers_test.go#L39-L50:createRuleViaAPIWithRetrymay create a duplicate alert rule (different ID, same spec) if the create actually succeeded server-side but the client failed to observe it before retrying.test/e2e/delete_alert_rule_test.go#L80-L121: the bulk-deletepollresends the same fixedruleIDs[0]/ruleIDs[1]request; if one ID was already deleted by a prior attempt, the retry will keep failing on that ID until the 1-minute timeout, causing a false test failure.
Consider making retries idempotency-aware, e.g., treat "already deleted"/"not found" as success on delete, or query current state before retrying on create.
📍 Affects 2 files
test/e2e/helpers_test.go#L39-L50(this comment)test/e2e/delete_alert_rule_test.go#L80-L121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/helpers_test.go` around lines 39 - 50, The retries around
non-idempotent alert-rule operations are unsafe. In
test/e2e/helpers_test.go:39-50, update createRuleViaAPIWithRetry to verify
existing state or otherwise avoid creating a duplicate when the server may have
already processed the request; in test/e2e/delete_alert_rule_test.go:80-121,
make the bulk-delete retry treat already-deleted/not-found rules as success and
avoid repeatedly failing on IDs removed by an earlier attempt.
There was a problem hiding this comment.
This is indeed a good remark but it also deserves a discussion with @sradco @PeterYurkovich and @jgbernalp about the semantics of the bulk delete API:
- What's the API response if an id doesn't exist?
- What's the API response if the delete operation partially succeeded?
Right now the API returns HTTP 200 OK on partial success which is a bit misleading IMHO
There was a problem hiding this comment.
There isn't an HTTP code for partial success/failures, which is why the request returns a per-item response result in the body. I'd assume the response if the id doesn't exist should just be 200 OK on the request and then on the individual item in the body a "no ID found"
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
There is a "new" http code for multi status. Which internally assigns status to individual operations.
There was a problem hiding this comment.
I saw that yesterday, but if you look at the description it states it is used exclusively for a specific usecase
This response is used exclusively in the context of Web Distributed Authoring and Versioning (WebDAV)
There was a problem hiding this comment.
Returning "200 OK" when an ID doesn't exist is ok-ish but not a good option when the request partially fails for other reasons. In addition, you can't simply forward the downstream response codes because it could a mix of "200 OK", "400 Bad Request" and "500 Server Unavailable".
IMHO it just shows how problematic a bulk delete API endpoint is. In general I'd be more in a favor of supporting individual deletes.
There was a problem hiding this comment.
I think it just reflects that HTTP codes were created with single resource operations in mind, different from what we have today or in this context. A "200" could indicate that the bulk action itself was successful, but the results of individual updates can be included inside the payload.
openshift-ci
Bot
added
the
needs-rebase
Requests to the backend may fail for a variety of reasons such as network connectivity, API blips, etc. Instead of failing a test, this commit implements retries for API requests. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
330236d to
09b14a5
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/cc @PeterYurkovich |
|
/test e2e-monitoring |
|
/override-sticky ci/prow/e2e-monitoring |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/e2e-monitoring These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use DetailsIn 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. |
|
@simonpasquier: 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. |
4 participants
Requests to the backend may fail for a variety of reasons such as network connectivity, API blips, etc. Instead of failing a test, this commit implements retries for API requests.
Summary by CodeRabbit
build-e2etarget to compile the e2e test suite into a standalone binary.e2e.testbinary in version control.