Add timeout and retry to CLI log/describe commands in e2e tests #2049
Conversation
WalkthroughAdds timeout-and-retry support to E2E CLI helpers: new execution primitives in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (2)tests/e2e/lib/restore_cli.go (1)
tests/e2e/lib/backup_cli.go (1)
🔇 Additional comments (8)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc 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 |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/e2e/lib/cli_common.go (2)
103-150: LGTM: Retry logic is correctly implemented.The retry loops properly handle failures with appropriate delays between attempts and informative logging.
Note: The parameter name
maxRetriesrepresents the total number of attempts (including the initial attempt), not the number of retries. For example,maxRetries=3results in 3 total attempts (1 initial + 2 retries). This is consistent throughout the implementation but could be clearer if renamed tomaxAttempts.
28-28: Remove or utilize the unusedTimeoutfield.The
Timeoutfield in theCLICommandstruct is declared but never referenced. All timeout-enabled methods accepttimeoutas an explicit parameter instead.Consider either removing the field if unused, using it as a default when methods are called without an explicit timeout, or documenting if it's reserved for future use.
tests/e2e/lib/restore_cli.go (1)
202-242: LGTM: Restore logs methods are correctly implemented.The restore operations follow the same robust timeout/retry pattern as backup operations, ensuring consistent behavior across the test suite.
Note: There's significant code duplication between the backup and restore CLI methods. Consider extracting a generic helper function to reduce duplication:
func executeWithOptionsHelper(cmd *CLICommand, timeout time.Duration, maxRetries int, retryDelay time.Duration, useOutput bool) ([]byte, error) { if maxRetries > 1 { if useOutput { return cmd.ExecuteOutputWithTimeoutAndRetry(timeout, maxRetries, retryDelay) } return cmd.ExecuteWithTimeoutAndRetry(timeout, maxRetries, retryDelay) } if useOutput { return cmd.ExecuteOutputWithTimeout(timeout) } return cmd.ExecuteWithTimeout(timeout) }This would reduce duplication across both backup and restore operations, though it's acceptable to defer this refactor given this is test code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
tests/e2e/lib/backup_cli.go(2 hunks)tests/e2e/lib/cli_common.go(2 hunks)tests/e2e/lib/restore_cli.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
tests/e2e/lib/backup_cli.gotests/e2e/lib/restore_cli.gotests/e2e/lib/cli_common.go
🧬 Code graph analysis (3)
tests/e2e/lib/backup_cli.go (1)
tests/e2e/lib/cli_common.go (4)
DefaultCLITimeout(15-15)DefaultCLIRetries(19-19)DefaultCLIRetryDelay(20-20)CLICommand(23-29)
tests/e2e/lib/restore_cli.go (1)
tests/e2e/lib/cli_common.go (4)
DefaultCLITimeout(15-15)DefaultCLIRetries(19-19)DefaultCLIRetryDelay(20-20)CLICommand(23-29)
tests/e2e/lib/cli_common.go (1)
must-gather/pkg/cli.go (1)
Timeout(39-39)
🪛 golangci-lint (2.5.0)
tests/e2e/lib/cli_common.go
[error] 19-19: File is not properly formatted
(gci)
🔇 Additional comments (5)
tests/e2e/lib/cli_common.go (2)
14-21: LGTM: Timeout and retry defaults are appropriate.The 5-minute timeout and 3-retry configuration with 10-second delays are reasonable defaults for operations that stream from object storage and may experience transient network issues.
55-101: LGTM: Timeout implementation is correct.Both
ExecuteWithTimeoutandExecuteOutputWithTimeoutproperly usecontext.WithTimeoutandexec.CommandContextto enable command cancellation. The explicit check forcontext.DeadlineExceededprovides clear timeout error messages.tests/e2e/lib/backup_cli.go (2)
195-225: LGTM: Describe methods properly implement timeout and retry.The wrapper pattern preserves backward compatibility while
DescribeBackupViaCLIWithOptionsprovides configurable timeout/retry behavior. The conditional logic correctly distinguishes between single-attempt (timeout-only) and multi-attempt (timeout + retry) scenarios.
227-267: LGTM: Logs methods follow consistent pattern.The three-tier API design (simple wrapper, timeout-only variant, full options) provides good flexibility while maintaining backward compatibility. The implementation correctly uses
ExecuteOutputWithTimeoutvariants to capture stdout.tests/e2e/lib/restore_cli.go (1)
170-200: LGTM: Restore describe methods mirror backup implementation.The implementation is consistent with the backup operations and correctly applies the timeout/retry pattern.
Prevents tests from hanging indefinitely when streaming logs from object storage. Uses 5-minute timeout with 3 retries. Co-Authored-By: Claude <[email protected]> Signed-off-by: Michal Pryc <[email protected]>
mpryc
force-pushed
the
cli_timeout_retry
branch
from
f95c9cc to
51683de
Compare
|
@mpryc: The following tests failed, say
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. |
|
if its 5 minutes here you're suggesting CLI itself should use even lower timeout? or is this a catchall if cli does not implement timeout |
for CI I think we are fine 5min as we know the download speed is pretty fast, but on the CLI side I would not assume so, so timeout here is separate to CLI itself and we do not look here at the CLI. Another option would be to use env from CLI here, but than we need to have CLI change in and new CLI available |
|
So should CLI have default timeout higher than 5? |
2 participants
Prevents tests from hanging indefinitely when streaming logs from object storage. Uses 5-minute timeout with 3 retries.