USHIFT-7019: C2CC - Increase amount of interconnected VMs in from 2 to 3#6706
USHIFT-7019: C2CC - Increase amount of interconnected VMs in from 2 to 3#6706vanhalenar wants to merge 8 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vanhalenar: This pull request references USHIFT-7019 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a third remote cluster (cluster-c): new Robot variables, scenario provisioning and VM lifecycle for host3, refactored c2cc config to support multiple remotes, kubeconfig collection, and Robot suites refactored to run template-driven tests across clusters A/B/C. ChangesCluster C Test Infrastructure and Suite Updates
Sequence Diagram (high-level)sequenceDiagram
participant Runner
participant ScenarioScript
participant Host1
participant Host2
participant Host3
participant RobotSuites
Runner->>ScenarioScript: create & configure hosts (host1, host2, host3)
ScenarioScript->>Host3: inject cluster C networks and launch MicroShift
ScenarioScript->>Host1: configure C2CC routes for A/B/C
ScenarioScript->>Host2: configure C2CC routes for A/B/C
ScenarioScript->>Host3: configure C2CC routes for A/B/C
ScenarioScript->>ScenarioScript: collect kubeconfig-a/b/c
Runner->>RobotSuites: execute template-driven tests across cluster-a/b/c with KUBECONFIG_C and CLUSTER_C_* variables
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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/cleanup.robot (1)
89-89: ⚡ Quick winCleanup assertions should cover Cluster C CIDRs too.
Now that
cluster-cis registered, cleanup checks should also assert Cluster C CIDRs/rules are removed from cluster-a; currently only Cluster B is validated.✅ Minimal assertion additions
Should Not Contain ${stdout} ${CLUSTER_B_POD_CIDR} Should Not Contain ${stdout} ${CLUSTER_B_SVC_CIDR} +Should Not Contain ${stdout} ${CLUSTER_C_POD_CIDR} +Should Not Contain ${stdout} ${CLUSTER_C_SVC_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/cleanup.robot` at line 89, After registering cluster-c (the line using "Register Remote Cluster cluster-c ${HOST3_IP} ${HOST3_SSH_PORT} ${KUBECONFIG_C}"), add the same cleanup assertions that currently validate Cluster B's CIDRs/rules are removed from cluster-a but applied to Cluster C: duplicate the existing Cluster B cleanup checks and change their targets from cluster-b to cluster-c and from ${KUBECONFIG_B} (if used) to ${KUBECONFIG_C}, ensuring the assertions verify Cluster C CIDRs/rules are removed from cluster-a.
🤖 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.sh:
- Around line 66-71: The current configure_c2cc_host function overwrites
50-c2cc.yaml on each call so per-host remote config loses earlier entries;
modify configure_c2cc_host to collect or render both remote cluster entries for
a given host and write a single 50-c2cc.yaml containing both remote definitions
(instead of writing once per remote), and perform a single restart/apply after
that write; update logic where configure_c2cc_host is invoked for
host1/host2/host3 so each host’s call produces a combined file (or append-safe
rendering) rather than successive overwrites.
In `@test/suites/c2cc/sanity.robot`:
- Around line 17-43: The suite defines tests that reference "cluster-c" (e.g.,
test cases "Cluster Is Running", "All Pods On Cluster Are Ready", "Cluster Has
Expected Node", "C2CC Controller Is Running On Cluster") but setup never
registers that cluster, causing "Oc On Cluster cluster-c" lookups to fail;
update the suite setup to register/define cluster-c alongside cluster-a and
cluster-b (where clusters are registered for Oc On Cluster/template lookups) so
the templates and the Oc On Cluster keyword can resolve cluster-c during test
execution.
---
Nitpick comments:
In `@test/suites/c2cc/cleanup.robot`:
- Line 89: After registering cluster-c (the line using "Register Remote Cluster
cluster-c ${HOST3_IP} ${HOST3_SSH_PORT} ${KUBECONFIG_C}"), add the same
cleanup assertions that currently validate Cluster B's CIDRs/rules are removed
from cluster-a but applied to Cluster C: duplicate the existing Cluster B
cleanup checks and change their targets from cluster-b to cluster-c and from
${KUBECONFIG_B} (if used) to ${KUBECONFIG_C}, ensuring the assertions verify
Cluster C CIDRs/rules are removed from cluster-a.
🪄 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: 6fc450cd-8ba4-4914-afa2-c91e2df14697
📒 Files selected for processing (6)
test/resources/c2cc.resourcetest/scenarios-bootc/el10/presubmits/el102-src@c2cc.shtest/suites/c2cc/cleanup.robottest/suites/c2cc/connectivity.robottest/suites/c2cc/infrastructure.robottest/suites/c2cc/sanity.robot
|
this is great work, thanks @vanhalenar |
okay, I'll wait until that gets merged |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.sh:
- Around line 44-66: The yaml_content is being built with literal "\n" escapes
and then written with a quoted heredoc (<< 'EOF'), so the file ends up
containing backslash-n sequences; fix by building yaml_content with actual
newlines (e.g., use $'line\nnext\n' when concatenating into yaml_content or
accumulate lines with simple newline characters), then make sure the heredoc
used by run_command_on_vm allows variable expansion (use an unquoted <<EOF) or
alternatively pass the string through printf '%b' so escape sequences are
interpreted before writing to /etc/microshift/config.d/50-c2cc.yaml; reference
yaml_content, run_command_on_vm and the heredoc around 50-c2cc.yaml in your
changes.
In `@test/scenarios-bootc/el9/presubmits/el98-src`@c2cc.sh:
- Around line 44-66: The yaml_content string is built with literal "\n"
sequences but the current run_command_on_vm invocation uses a single-quoted
heredoc ('EOF') so those escapes are not expanded; update the write step to
expand backslash escapes (e.g. pipe printf '%b' "${yaml_content}" or echo -e
"${yaml_content}" into sudo tee /etc/microshift/config.d/50-c2cc.yaml) so
yaml_content becomes real newlines; locate the yaml_content variable and the
run_command_on_vm call to /etc/microshift/config.d/50-c2cc.yaml and replace the
heredoc approach with one that expands escapes (or remove the single quotes
around EOF and ensure expansion will occur).
🪄 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: b8003f59-889b-4d60-a618-935f9f1b63f9
📒 Files selected for processing (2)
test/scenarios-bootc/el10/presubmits/el102-src@c2cc.shtest/scenarios-bootc/el9/presubmits/el98-src@c2cc.sh
|
@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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
4 participants
Increase amount of interconnected VMs in from 2 to 3 in C2CC RF tests.
Summary by CodeRabbit
Tests
Chores