Skip to content

test: retry operations in e2e tests #1071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
simonpasquier wants to merge 1 commit into
base: main-alerts-management-api
Choose a base branch
from
+142 −87
Open

test: retry operations in e2e tests #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ web/screenshots/
web/cypress/videos/
web/dist/
web/node_modules/
e2e.test
plugin-backend
# Leave these permanently for backwards compatability for backporting
node_modules/
Expand Down
4 changes: 4 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ start-backend:
test-backend:
go test ./pkg/... ./internal/... -v

.PHONY: build-e2e
build-e2e:
go test -c -tags e2e ./test/e2e

.PHONY: test-e2e
test-e2e:
go test -tags e2e -v -timeout=150m -count=1 ./test/e2e
Expand Down
70 changes: 35 additions & 35 deletions test/e2e/create_alert_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ package e2e

import (
"context"
"errors"
"fmt"
"testing"
"time"

"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/openshift/monitoring-plugin/internal/managementrouter"
Expand All @@ -27,7 +31,7 @@ func TestCreateUserDefinedAlertRule(t *testing.T) {
defer cleanup()

createExpr := "vector(1) or vector(0)"
id, err := createRuleViaAPI(ctx, f, managementrouter.CreateAlertRuleRequest{
createAlertRuleRequest := managementrouter.CreateAlertRuleRequest{
AlertingRule: &managementrouter.AlertRuleSpec{
Alert: new("E2ECreateAlert"),
Expr: &createExpr,
Expand All @@ -43,47 +47,43 @@ func TestCreateUserDefinedAlertRule(t *testing.T) {
PrometheusRuleName: "e2e-create-pr",
PrometheusRuleNamespace: testNamespace,
},
})
if err != nil {
t.Fatalf("Failed to create alert rule: %v", err)
}
id, err := createRuleViaAPIWithRetry(ctx, f, createAlertRuleRequest)
require.NoError(t, err)
require.NotEmpty(t, id)

t.Logf("Created rule with ID: %s", id)

promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get(
ctx, "e2e-create-pr", metav1.GetOptions{},
)
if err != nil {
t.Fatalf("Failed to get PrometheusRule: %v", err)
}
err = poll(time.Second, time.Minute, func() error {
promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get(
ctx, "e2e-create-pr", metav1.GetOptions{},
)
if err != nil {
return fmt.Errorf("failed to get PrometheusRule: %w", err)
}

if len(promRule.Spec.Groups) == 0 {
t.Fatal("Expected at least one rule group in PrometheusRule")
}
for _, group := range promRule.Spec.Groups {
for _, rule := range group.Rules {
if rule.Alert == "E2ECreateAlert" {
if rule.Expr.String() != createExpr {
return fmt.Errorf("expected expr %q, got %q", createExpr, rule.Expr.String())
}
if rule.For == nil || string(*rule.For) != "1m" {
return fmt.Errorf("expected for '1m', got %v", rule.For)
}
if rule.Labels["severity"] != "info" {
return fmt.Errorf("expected severity=info, got %q", rule.Labels["severity"])
}
if rule.Annotations["summary"] != "E2E test alert for create-rule" {
return fmt.Errorf("expected summary annotation, got %q", rule.Annotations["summary"])
}

var foundAlert bool
for _, group := range promRule.Spec.Groups {
for _, rule := range group.Rules {
if rule.Alert == "E2ECreateAlert" {
foundAlert = true
if rule.Expr.String() != createExpr {
t.Errorf("Expected expr %q, got %q", createExpr, rule.Expr.String())
}
if rule.For == nil || string(*rule.For) != "1m" {
t.Errorf("Expected for '1m', got %v", rule.For)
}
if rule.Labels["severity"] != "info" {
t.Errorf("Expected severity=info, got %q", rule.Labels["severity"])
}
if rule.Annotations["summary"] != "E2E test alert for create-rule" {
t.Errorf("Expected summary annotation, got %q", rule.Annotations["summary"])
return nil
}
}
}
}

if !foundAlert {
t.Fatal("Alert 'E2ECreateAlert' not found in PrometheusRule")
}

t.Log("Create alert rule e2e test passed successfully")
return errors.New("alerting rule 'E2ECreateAlert' not found in PrometheusRule")
})
require.NoError(t, err)
}
118 changes: 66 additions & 52 deletions test/e2e/delete_alert_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"testing"
"time"

"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/openshift/monitoring-plugin/internal/managementrouter"
Expand All @@ -37,7 +38,7 @@ func TestDeleteAlertRule(t *testing.T) {

for _, name := range ruleNames {
expr := fmt.Sprintf("absent(nonexistent{e2e_rule=%q})", name)
id, err := createRuleViaAPI(ctx, f, managementrouter.CreateAlertRuleRequest{
alertRuleRequest := managementrouter.CreateAlertRuleRequest{
AlertingRule: &managementrouter.AlertRuleSpec{
Alert: new(name),
Expr: &expr,
Expand All @@ -50,17 +51,18 @@ func TestDeleteAlertRule(t *testing.T) {
PrometheusRuleName: "e2e-delete-pr",
PrometheusRuleNamespace: testNamespace,
},
})
}

id, err := createRuleViaAPIWithRetry(ctx, f, alertRuleRequest)
if err != nil {
t.Fatalf("Failed to create alert rule %s: %v", name, err)
t.Fatalf("failed to create alert rule %s: %v", name, err)
}
ruleIDs = append(ruleIDs, id)

}

t.Logf("Created 3 rules with IDs: %v", ruleIDs)

time.Sleep(2 * time.Second)

deleteReq := managementrouter.BulkDeleteAlertRulesRequest{
RuleIds: []string{ruleIDs[0], ruleIDs[1]},
}
Expand All @@ -69,61 +71,73 @@ func TestDeleteAlertRule(t *testing.T) {
t.Fatalf("Failed to marshal delete request: %v", err)
}

deleteURL := f.PluginURL + "/api/v1/alerting/rules"
req, err := http.NewRequestWithContext(ctx, http.MethodDelete, deleteURL, bytes.NewBuffer(reqBody))
if err != nil {
t.Fatalf("Failed to create delete request: %v", err)
}
req.Header.Set("Content-Type", "application/json")
if f.BearerToken != "" {
req.Header.Set("Authorization", "Bearer "+f.BearerToken)
}
err = poll(time.Second, time.Minute, func() error {
deleteURL := f.PluginURL + "/api/v1/alerting/rules"
req, err := http.NewRequestWithContext(ctx, http.MethodDelete, deleteURL, bytes.NewBuffer(reqBody))
if err != nil {
return fmt.Errorf("failed to create delete request: %w", err)
}
req.Header.Set("Content-Type", "application/json")
if f.BearerToken != "" {
req.Header.Set("Authorization", "Bearer "+f.BearerToken)
}

resp, err := f.HTTPClient().Do(req)
if err != nil {
t.Fatalf("Failed to make delete request: %v", err)
}
defer resp.Body.Close()
resp, err := f.HTTPClient().Do(req)
if err != nil {
return fmt.Errorf("failed to make delete request: %w", err)
}
defer resp.Body.Close()

@coderabbitai coderabbitai Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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


if resp.StatusCode != http.StatusOK {
body, err := io.ReadAll(resp.Body)
if err != nil {
return fmt.Errorf("failed to read body: %w", err)
}
return fmt.Errorf("expected status 200, got %d (body: %s)", resp.StatusCode, string(body))
}

if resp.StatusCode != http.StatusOK {
body, _ := io.ReadAll(resp.Body)
t.Fatalf("Expected status 200, got %d. Body: %s", resp.StatusCode, string(body))
}
var deleteResp managementrouter.BulkDeleteAlertRulesResponse
if err := json.NewDecoder(resp.Body).Decode(&deleteResp); err != nil {
return fmt.Errorf("failed to decode delete response: %w", err)
}

var deleteResp managementrouter.BulkDeleteAlertRulesResponse
if err := json.NewDecoder(resp.Body).Decode(&deleteResp); err != nil {
t.Fatalf("Failed to decode delete response: %v", err)
}
if len(deleteResp.Rules) != 2 {
return fmt.Errorf("expected 2 results, got %d", len(deleteResp.Rules))
}
for _, result := range deleteResp.Rules {
if result.StatusCode != http.StatusNoContent {
return fmt.Errorf("rule %s deletion failed with status %d: %v", result.Id, result.StatusCode, result.Message)
}
}

if len(deleteResp.Rules) != 2 {
t.Fatalf("Expected 2 results, got %d", len(deleteResp.Rules))
}
for _, result := range deleteResp.Rules {
if result.StatusCode != http.StatusNoContent {
t.Errorf("Rule %s deletion failed with status %d: %v", result.Id, result.StatusCode, result.Message)
return nil
})
require.NoError(t, err)

err = poll(time.Second, 20*time.Second, func() error {
promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get(
ctx, "e2e-delete-pr", metav1.GetOptions{},
)
if err != nil {
return fmt.Errorf("failed to get PrometheusRule after deletion: %w", err)
}
}

promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get(
ctx, "e2e-delete-pr", metav1.GetOptions{},
)
if err != nil {
t.Fatalf("Failed to get PrometheusRule after deletion: %v", err)
}
var remainingAlerts []string
for _, group := range promRule.Spec.Groups {
for _, rule := range group.Rules {
remainingAlerts = append(remainingAlerts, rule.Alert)
}
}

var remainingAlerts []string
for _, group := range promRule.Spec.Groups {
for _, rule := range group.Rules {
remainingAlerts = append(remainingAlerts, rule.Alert)
if len(remainingAlerts) != 1 {
return fmt.Errorf("expected 1 remaining rule, got %d: %v", len(remainingAlerts), remainingAlerts)
}
}

if len(remainingAlerts) != 1 {
t.Fatalf("Expected 1 remaining rule, got %d: %v", len(remainingAlerts), remainingAlerts)
}
if remainingAlerts[0] != "KeepAlert3" {
t.Errorf("Expected remaining rule 'KeepAlert3', got %q", remainingAlerts[0])
}
if remainingAlerts[0] != "KeepAlert3" {
return fmt.Errorf("expected remaining rule 'KeepAlert3', got %q", remainingAlerts[0])
}

t.Log("Delete alert rule e2e test passed successfully")
return nil
})
require.NoError(t, err)
}
36 changes: 36 additions & 0 deletions test/e2e/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,47 @@ import (
"io"
"net/http"
"net/url"
"time"

"k8s.io/apimachinery/pkg/util/wait"

"github.com/openshift/monitoring-plugin/internal/managementrouter"
"github.com/openshift/monitoring-plugin/test/e2e/framework"
)

// poll calls the given function f() every given interval
// until it returns no error or the given timeout occurs.
// When a timeout occurs, the last observed error is returned
// wrapped in a wait.ErrWaitTimeout.
func poll(interval, timeout time.Duration, f func() error) error {
var lastErr error
err := wait.PollUntilContextTimeout(context.Background(), interval, timeout, true, func(context.Context) (bool, error) {
if lastErr = f(); lastErr != nil {
return false, nil
}

return true, nil
})
if err != nil && lastErr != nil {
return fmt.Errorf("%w: %w", err, lastErr)
}

return err
}

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
}
Comment on lines +41 to +52

@coderabbitai coderabbitai Bot Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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: createRuleViaAPIWithRetry may 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-delete poll resends the same fixed ruleIDs[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.

@simonpasquier simonpasquier Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@PeterYurkovich PeterYurkovich Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@coderabbitai coderabbitai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@jgbernalp jgbernalp Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a "new" http code for multi status. Which internally assigns status to individual operations.

@PeterYurkovich PeterYurkovich Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@simonpasquier simonpasquier Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jgbernalp jgbernalp Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func createRuleViaAPI(ctx context.Context, f *framework.Framework, payload managementrouter.CreateAlertRuleRequest) (string, error) {
reqBody, err := json.Marshal(payload)
if err != nil {
Expand Down