USHIFT-6849: Add C2CC IPv6 tests scenarios#6750
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vanhalenar: This pull request references USHIFT-6849 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an ChangesIPv6 Support for C2CC Cluster-to-Cluster Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vanhalenar 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 |
![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/suites/c2cc/reconciliation.robot (1)
76-86: ⚡ Quick winAdd guard for empty FOREIGN_CIDR.
If FOREIGN_CIDR is ${EMPTY}, this test will inject an empty string into the annotation and
Should Containwill spuriously pass (empty string is contained in any string). Consider adding a precondition check.🛡️ Suggested guard
Add at the start of the test case:
Reconcile SNAT Annotation Preserves Foreign Subnets [Documentation] Inject a foreign subnet into the SNAT annotation, then remove only ... the C2CC CIDRs. Verify the controller merges C2CC CIDRs back without ... removing the foreign subnet. + Skip If '${FOREIGN_CIDR}' == '${EMPTY}' Test requires FOREIGN_CIDR variable [Setup] Inject Foreign Subnet Into SNAT Annotation cluster-a ${FOREIGN_CIDR}🤖 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/suites/c2cc/reconciliation.robot` around lines 76 - 86, Add a precondition guard at the top of the "Reconcile SNAT Annotation Preserves Foreign Subnets" test to ensure FOREIGN_CIDR is not empty: before calling Inject Foreign Subnet Into SNAT Annotation, run a check like Fail If '${FOREIGN_CIDR}' == '${EMPTY}' FOREIGN_CIDR must be set (or use Skip Test If to skip) so that Get Node SNAT Annotation / Should Contain can't pass spuriously when FOREIGN_CIDR is empty.
🤖 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/scenarios-bootc/el10/presubmits/el102-src`@c2cc-ipv6.sh:
- Around line 94-95: The call to wait_for_greenboot_on_hosts
"c2cc_pre_greenboot" can fail but its failure is currently ignored; update the
caller so that if wait_for_greenboot_on_hosts returns a non-zero status it is
propagated (e.g., return or exit with that status) before proceeding to any C2CC
reconfiguration steps—ensure the caller checks the return value of
wait_for_greenboot_on_hosts and aborts further actions when it fails so hosts
are not reconfigured while in a bad state.
In `@test/scenarios-bootc/el9/presubmits/el98-src`@c2cc-ipv6.sh:
- Around line 94-95: The call to wait_for_greenboot_on_hosts
"c2cc_pre_greenboot" must not be ignored; check its exit status and propagate
failure (exit non‑zero or return) before proceeding to any C2CC reconfiguration
steps. Update the script so after invoking wait_for_greenboot_on_hosts you test
its result (e.g., if ! wait_for_greenboot_on_hosts "c2cc_pre_greenboot"; then
log an error and exit 1) so the subsequent C2CC config application cannot run
when greenboot did not succeed.
---
Nitpick comments:
In `@test/suites/c2cc/reconciliation.robot`:
- Around line 76-86: Add a precondition guard at the top of the "Reconcile SNAT
Annotation Preserves Foreign Subnets" test to ensure FOREIGN_CIDR is not empty:
before calling Inject Foreign Subnet Into SNAT Annotation, run a check like Fail
If '${FOREIGN_CIDR}' == '${EMPTY}' FOREIGN_CIDR must be set (or use Skip
Test If to skip) so that Get Node SNAT Annotation / Should Contain can't pass
spuriously when FOREIGN_CIDR is empty.
🪄 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: 08e63990-2158-4f4e-a16d-2f3984368f54
📒 Files selected for processing (7)
test/resources/c2cc.resourcetest/scenarios-bootc/el10/presubmits/el102-src@c2cc-ipv6.shtest/scenarios-bootc/el10/presubmits/el102-src@c2cc.shtest/scenarios-bootc/el9/presubmits/el98-src@c2cc-ipv6.shtest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/cleanup.robottest/suites/c2cc/reconciliation.robot
| wait_for_greenboot_on_hosts "c2cc_pre_greenboot" | ||
|
|
There was a problem hiding this comment.
Propagate pre-greenboot failure before applying C2CC config.
Line 94 ignores wait_for_greenboot_on_hosts failure and can continue with host reconfiguration in a bad state.
Suggested fix
- wait_for_greenboot_on_hosts "c2cc_pre_greenboot"
+ if ! wait_for_greenboot_on_hosts "c2cc_pre_greenboot"; then
+ return 1
+ fi🤖 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/scenarios-bootc/el10/presubmits/el102-src`@c2cc-ipv6.sh around lines 94
- 95, The call to wait_for_greenboot_on_hosts "c2cc_pre_greenboot" can fail but
its failure is currently ignored; update the caller so that if
wait_for_greenboot_on_hosts returns a non-zero status it is propagated (e.g.,
return or exit with that status) before proceeding to any C2CC reconfiguration
steps—ensure the caller checks the return value of wait_for_greenboot_on_hosts
and aborts further actions when it fails so hosts are not reconfigured while in
a bad state.
| wait_for_greenboot_on_hosts "c2cc_pre_greenboot" | ||
|
|
There was a problem hiding this comment.
Propagate pre-greenboot failure before applying C2CC config.
Line 94 ignores wait_for_greenboot_on_hosts failure and can continue with host reconfiguration in a bad state.
Suggested fix
- wait_for_greenboot_on_hosts "c2cc_pre_greenboot"
+ if ! wait_for_greenboot_on_hosts "c2cc_pre_greenboot"; then
+ return 1
+ fi📝 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.
| wait_for_greenboot_on_hosts "c2cc_pre_greenboot" | |
| if ! wait_for_greenboot_on_hosts "c2cc_pre_greenboot"; then | |
| return 1 | |
| fi | |
🤖 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/scenarios-bootc/el9/presubmits/el98-src`@c2cc-ipv6.sh around lines 94 -
95, The call to wait_for_greenboot_on_hosts "c2cc_pre_greenboot" must not be
ignored; check its exit status and propagate failure (exit non‑zero or return)
before proceeding to any C2CC reconfiguration steps. Update the script so after
invoking wait_for_greenboot_on_hosts you test its result (e.g., if !
wait_for_greenboot_on_hosts "c2cc_pre_greenboot"; then log an error and exit 1)
so the subsequent C2CC config application cannot run when greenboot did not
succeed.
|
@vanhalenar: 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. |
2 participants
Summary by CodeRabbit