OCPBUGS-77949: OCPBUGS-77948: TNF node replacement test updates#30846
OCPBUGS-77949: OCPBUGS-77948: TNF node replacement test updates#30846jaypoulz wants to merge 4 commits intoopenshift:mainfrom
Conversation
…logs - Use PcsStatusFull / PcsStatusFullViaDebug for human-readable pacemaker status instead of XML - Truncate long SSH stdout/stderr in logs (e.g. avoid pcs status xml dumps) - On WaitForNodesOnline timeout, log full pacemaker status for debugging Made-with: Cursor
…nHostPort for IPv6
- Use net.JoinHostPort(RedfishIP, port) so IPv6 addresses are bracketed (RFC 3986)
- BMH template placeholder {REDFISH_HOST_PORT} replaces {REDFISH_IP}; port 8000 in code
Made-with: Cursor
…e replacement - Capture job pod logs and gate on survivor update-setup run - Force-delete stuck BMH/Machine after 1m; shorten recovery and BMH timeouts; rename timeouts to match duration (3m/7m/10m) - Wait for survivor and target update-setup jobs in parallel Made-with: Cursor
CSR approval was never observed in testing; remove the wait to simplify recovery. Made-with: Cursor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/valid-reference
|
@jaypoulz: This pull request references Jira Issue OCPBUGS-77949, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughEnhanced the two-node test suite with improved concurrency handling, extended timeout management, and better diagnostic logging. Updated BMC address construction, added new job and resource lifecycle helpers, and strengthened error reporting and cleanup mechanisms across test utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jaypoulz 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jaypoulz: This pull request references Jira Issue OCPBUGS-77949, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (dhensel@redhat.com), skipping review request. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@jaypoulz: This pull request references Jira Issue OCPBUGS-77949, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (dhensel@redhat.com), skipping review request. 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 openshift-eng/jira-lifecycle-plugin repository. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/two_node/utils/core/ssh.go`:
- Around line 136-141: The current log truncation slices the UTF-8 string bytes
with out[:maxLogOutputBytes], which can cut a multi-byte rune and produce
invalid UTF-8 in logs; update the truncation logic around stdout.String()/out
and the e2e.Logf call to perform rune-safe truncation (for example, convert to
runes or iterate runes until adding the next rune would exceed
maxLogOutputBytes) and then log the safely truncated string along with the total
byte length using the existing maxLogOutputBytes and e2e.Logf call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ecbedc9-3c0f-4263-b6e6-a6a05314b2c2
📒 Files selected for processing (6)
test/extended/testdata/two_node/baremetalhost-template.yamltest/extended/two_node/tnf_node_replacement.gotest/extended/two_node/utils/common.gotest/extended/two_node/utils/core/ssh.gotest/extended/two_node/utils/services/etcd.gotest/extended/two_node/utils/services/pacemaker.go
| out := stdout.String() | ||
| if len(out) > maxLogOutputBytes { | ||
| e2e.Logf("SSH stdout: %q ... (truncated, total %d bytes)", out[:maxLogOutputBytes], len(out)) | ||
| } else { | ||
| e2e.Logf("SSH stdout: %q", out) | ||
| } |
There was a problem hiding this comment.
Minor: Truncation may split multi-byte UTF-8 characters.
Slicing out[:maxLogOutputBytes] by byte count could split a multi-byte UTF-8 character (e.g., if present in file paths or error messages), producing malformed output in logs. Since this is debug logging and the full output is returned to the caller unchanged, the impact is cosmetic.
💡 Optional fix using rune-safe truncation
- if len(out) > maxLogOutputBytes {
- e2e.Logf("SSH stdout: %q ... (truncated, total %d bytes)", out[:maxLogOutputBytes], len(out))
+ if len(out) > maxLogOutputBytes {
+ truncated := out[:maxLogOutputBytes]
+ // Ensure we don't split a UTF-8 multi-byte character
+ for len(truncated) > 0 && truncated[len(truncated)-1]&0xC0 == 0x80 {
+ truncated = truncated[:len(truncated)-1]
+ }
+ e2e.Logf("SSH stdout: %q ... (truncated, total %d bytes)", truncated, len(out))📝 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.
| out := stdout.String() | |
| if len(out) > maxLogOutputBytes { | |
| e2e.Logf("SSH stdout: %q ... (truncated, total %d bytes)", out[:maxLogOutputBytes], len(out)) | |
| } else { | |
| e2e.Logf("SSH stdout: %q", out) | |
| } | |
| out := stdout.String() | |
| if len(out) > maxLogOutputBytes { | |
| truncated := out[:maxLogOutputBytes] | |
| // Ensure we don't split a UTF-8 multi-byte character | |
| for len(truncated) > 0 && truncated[len(truncated)-1]&0xC0 == 0x80 { | |
| truncated = truncated[:len(truncated)-1] | |
| } | |
| e2e.Logf("SSH stdout: %q ... (truncated, total %d bytes)", truncated, len(out)) | |
| } else { | |
| e2e.Logf("SSH stdout: %q", out) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/two_node/utils/core/ssh.go` around lines 136 - 141, The current
log truncation slices the UTF-8 string bytes with out[:maxLogOutputBytes], which
can cut a multi-byte rune and produce invalid UTF-8 in logs; update the
truncation logic around stdout.String()/out and the e2e.Logf call to perform
rune-safe truncation (for example, convert to runes or iterate runes until
adding the next rune would exceed maxLogOutputBytes) and then log the safely
truncated string along with the total byte length using the existing
maxLogOutputBytes and e2e.Logf call sites.
|
@jaypoulz: The following test 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. |
eggfoobar
left a comment
eggfoobar
left a comment
There was a problem hiding this comment.
Looking great, just had some small suggestions.
| automatedCleaningMode: metadata | ||
| bmc: | ||
| address: redfish+https://{REDFISH_IP}:8000/redfish/v1/Systems/{UUID} | ||
| address: redfish+https://{REDFISH_HOST_PORT}/redfish/v1/Systems/{UUID} |
There was a problem hiding this comment.
Maybe, we should just rename this to REDFISH_HOST, the port can cause confusion
| // to empty so the test can proceed without blocking on controller cleanup. | ||
| func deleteOcResourceWithRetry(oc *exutil.CLI, resourceType, resourceName, namespace string) error { | ||
| return core.RetryWithOptions(func() error { | ||
| done := make(chan error, 1) |
There was a problem hiding this comment.
Instead of doing this timer, can we just use the old core.RetryWithOptions and if that fails we fall to force delete?
| "{CREDENTIALS_NAME}": testConfig.TargetNode.BMCSecretName, | ||
| "{BOOT_MAC_ADDRESS}": newMACAddress, | ||
| "{BMH_NAME}": testConfig.TargetNode.BMHName, | ||
| "{REDFISH_HOST_PORT}": redfishHostPort, |
There was a problem hiding this comment.
Same here on updating to REDFISH_HOST
3 participants
Summary by CodeRabbit
Release Notes