Skip to content

NO-ISSUE: retry transient registry errors in opm QE #1272

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

Merged
openshift-merge-bot merged 1 commit into from
Apr 7, 2026
Merged

NO-ISSUE: retry transient registry errors in opm QE #1272

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
111 changes: 79 additions & 32 deletions tests-extension/test/qe/util/opmcli/opm_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package opmcli

import (
"bytes"
"errors"
"fmt"
"io"
"os"
Expand All @@ -13,6 +14,7 @@ import (
"runtime/debug"
"strings"
"sync"
"time"

g "github.com/onsi/ginkgo/v2"
e2e "k8s.io/kubernetes/test/e2e/framework"
Expand Down Expand Up @@ -153,42 +155,87 @@ func (c *CLI) Output() (string, error) {
if c.verbose {
e2e.Logf("DEBUG: %s %s\n", c.execPath, c.printCmd())
}
// Create the command with the executable and arguments
cmd := exec.Command(c.execPath, c.finalArgs...)
// Set registry authentication file if configured
if c.podmanAuthfile != "" {
cmd.Env = append(os.Environ(), "REGISTRY_AUTH_FILE="+c.podmanAuthfile)

const maxRetries = 3
const retryDelay = 2 * time.Second

var lastErr error
var lastTrimmed string

for attempt := 1; attempt <= maxRetries; attempt++ {
// Create the command with the executable and arguments
cmd := exec.Command(c.execPath, c.finalArgs...)
// Set registry authentication file if configured
if c.podmanAuthfile != "" {
cmd.Env = append(os.Environ(), "REGISTRY_AUTH_FILE="+c.podmanAuthfile)
}
// Set working directory if specified
if c.ExecCommandPath != "" {
e2e.Logf("set exec command path is %s\n", c.ExecCommandPath)
cmd.Dir = c.ExecCommandPath
}
// Set stdin buffer
cmd.Stdin = c.stdin
Comment on lines +162 to +178
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Reset stdin for each retry.

Lines 177-178 reuse the same *bytes.Buffer across attempts. CombinedOutput() consumes cmd.Stdin, so a retried command will see EOF/empty input instead of the original payload.

Suggested fix 
 	var lastErr error
 	var lastTrimmed string
+	var stdinBytes []byte
+	if c.stdin != nil {
+		stdinBytes = append([]byte(nil), c.stdin.Bytes()...)
+	}
 
 	for attempt := 1; attempt <= maxRetries; attempt++ {
 		// Create the command with the executable and arguments
 		cmd := exec.Command(c.execPath, c.finalArgs...)
@@
-		// Set stdin buffer
-		cmd.Stdin = c.stdin
+		// Reset stdin for each attempt
+		if stdinBytes != nil {
+			cmd.Stdin = bytes.NewReader(stdinBytes)
+		}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests-extension/test/qe/util/opmcli/opm_client.go` around lines 162 - 178,
The stdin buffer is reused across retries so CombinedOutput() on a failed
attempt leaves EOF for subsequent attempts; inside the retry loop (before
assigning cmd.Stdin/use of exec.Command) clone/reset stdin for each attempt: if
c.stdin != nil and is a bytes.Buffer (or supports Bytes()), create a fresh
bytes.NewBuffer(copyOfBytes) and assign that to cmd.Stdin (instead of reusing
c.stdin) so each retry gets the original payload; update the retry loop around
exec.Command creation (symbols: c.stdin, exec.Command, CombinedOutput(),
maxRetries) to perform this reset.

// Log command execution if info logging is enabled
if c.showInfo && attempt == 1 {
e2e.Logf("Running '%s %s'", c.execPath, strings.Join(c.finalArgs, " "))
}
// Execute command and capture combined output
out, err := cmd.CombinedOutput()
trimmed := strings.TrimSpace(string(out))

if err == nil {
c.stdout = bytes.NewBuffer(out)
if attempt > 1 {
e2e.Logf("Command succeeded on retry attempt %d/%d", attempt, maxRetries)
}
return trimmed, nil
}

lastErr = err
lastTrimmed = trimmed

if isTransientRegistryError(trimmed) && attempt < maxRetries {
e2e.Logf("Transient registry error detected (attempt %d/%d), retrying after %v: %s",
attempt, maxRetries, retryDelay, trimmed)
time.Sleep(retryDelay)
continue
}

break
}
// Set working directory if specified
if c.ExecCommandPath != "" {
e2e.Logf("set exec command path is %s\n", c.ExecCommandPath)
cmd.Dir = c.ExecCommandPath

var exitErr *exec.ExitError
if errors.As(lastErr, &exitErr) {
e2e.Logf("Error running %s %s:\n%s", c.execPath, strings.Join(c.finalArgs, " "), lastTrimmed)
return lastTrimmed, &ExitError{ExitError: exitErr, Cmd: c.execPath + " " + strings.Join(c.finalArgs, " "), StdErr: lastTrimmed}
}
// Set stdin buffer
cmd.Stdin = c.stdin
// Log command execution if info logging is enabled
if c.showInfo {
e2e.Logf("Running '%s %s'", c.execPath, strings.Join(c.finalArgs, " "))

// Fatal error preventing command execution
FatalErr(fmt.Errorf("unable to execute %q: %v", c.execPath, lastErr))
// unreachable code
return "", nil
}

func isTransientRegistryError(output string) bool {
transientErrors := []string{
"503 Service Unavailable",
"received unexpected HTTP status: 503",
"502 Bad Gateway",
"504 Gateway Timeout",
"429 Too Many Requests",
"TLS handshake timeout",
"i/o timeout",
"connection reset by peer",
"unexpected EOF",
"EOF",
}
// Execute command and capture combined output
out, err := cmd.CombinedOutput()
trimmed := strings.TrimSpace(string(out))
// Handle different types of execution results
switch exitErr := err.(type) {
case nil:
// Successful execution
c.stdout = bytes.NewBuffer(out)
return trimmed, nil
case *exec.ExitError:
// Command executed but returned non-zero exit code
e2e.Logf("Error running %v:\n%s", cmd, trimmed)
return trimmed, &ExitError{ExitError: exitErr, Cmd: c.execPath + " " + strings.Join(c.finalArgs, " "), StdErr: trimmed}
default:
// Fatal error preventing command execution
FatalErr(fmt.Errorf("unable to execute %q: %v", c.execPath, err))
// unreachable code
return "", nil
for _, errMsg := range transientErrors {
if strings.Contains(output, errMsg) {
return true
}
}
return false
}
Comment on lines +220 to 239
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Overly broad "EOF" substring match may cause false positive retries.

The bare "EOF" pattern matches any string containing those characters (e.g., "GEOFFREY", "EOF_MARKER", or unrelated error messages). This could trigger retries on non-transient failures.

Consider using more specific patterns that reflect actual error messages from Go's standard library or container runtimes:

Proposed fix 
 	transientErrors := []string{
 		"503 Service Unavailable",
 		"received unexpected HTTP status: 503",
 		"502 Bad Gateway",
 		"504 Gateway Timeout",
 		"429 Too Many Requests",
 		"TLS handshake timeout",
 		"i/o timeout",
 		"connection reset by peer",
-		"unexpected EOF",
-		"EOF",
+		"unexpected EOF",
+		": EOF",
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func isTransientRegistryError(output string) bool {
transientErrors := []string{
"503 Service Unavailable",
"received unexpected HTTP status: 503",
"502 Bad Gateway",
"504 Gateway Timeout",
"429 Too Many Requests",
"TLS handshake timeout",
"i/o timeout",
"connection reset by peer",
"unexpected EOF",
"EOF",
}
// Execute command and capture combined output
out, err := cmd.CombinedOutput()
trimmed := strings.TrimSpace(string(out))
// Handle different types of execution results
switch exitErr := err.(type) {
case nil:
// Successful execution
c.stdout = bytes.NewBuffer(out)
return trimmed, nil
case *exec.ExitError:
// Command executed but returned non-zero exit code
e2e.Logf("Error running %v:\n%s", cmd, trimmed)
return trimmed, &ExitError{ExitError: exitErr, Cmd: c.execPath + " " + strings.Join(c.finalArgs, " "), StdErr: trimmed}
default:
// Fatal error preventing command execution
FatalErr(fmt.Errorf("unable to execute %q: %v", c.execPath, err))
// unreachable code
return "", nil
for _, errMsg := range transientErrors {
if strings.Contains(output, errMsg) {
return true
}
}
return false
}
func isTransientRegistryError(output string) bool {
transientErrors := []string{
"503 Service Unavailable",
"received unexpected HTTP status: 503",
"502 Bad Gateway",
"504 Gateway Timeout",
"429 Too Many Requests",
"TLS handshake timeout",
"i/o timeout",
"connection reset by peer",
"unexpected EOF",
": EOF",
}
for _, errMsg := range transientErrors {
if strings.Contains(output, errMsg) {
return true
}
}
return false
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests-extension/test/qe/util/opmcli/opm_client.go` around lines 223 - 242,
The current isTransientRegistryError function uses a bare "EOF" in the
transientErrors slice which can match substrings (e.g., "GEOFFREY") and cause
false-positive retries; update the logic in isTransientRegistryError (and the
transientErrors slice) to remove the broad "EOF" entry and instead detect EOF
only as a standalone token or exact error (e.g., use a regexp with word
boundaries like `\bEOF\b` or check for output == "EOF" or lines trimmed equal
"EOF"), preserving other entries like "unexpected EOF"; ensure you reference and
update the transientErrors variable and the loop that scans output so only
genuine EOF errors trigger a retry.


// GetDirPath recursively searches for a directory containing a file/directory with the specified prefix.
Expand Down