MGMT-21646: Static Networking Configuration#214
MGMT-21646: Static Networking Configuration#214keitwb merged 1 commit intorh-ecosystem-edge:mainfrom
Conversation
keitwb
requested review from
andrej1991,
carbonin,
eranco74,
omertuc and
zszabo-rh
|
@keitwb: This pull request references MGMT-21646 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 epic to target the "4.21.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. |
WalkthroughUpdated the assisted-service-mcp submodule pointer; expanded template.yaml with clarified abbreviation rules, explicit static-networking flows and pre-flight checks, Discovery ISO handling, and required cluster details; removed a prior static-networking-denial eval and added a VLAN static-networking eval for two hosts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as Assistant
participant G as Tool: generate_nmstate_yaml
participant V as Tool: validate_nmstate_yaml
participant C as Tool: alter_static_network_config_nmstate_for_host
participant S as Assisted Service
U->>A: Request static network configuration for hosts
A->>A: Check cluster details and existing static config
A->>G: Generate nmstate YAML per host (apply defaults if needed)
G-->>A: Return nmstate YAML
A->>U: Present YAML and allow user tweaks
U-->>A: Confirm apply
A->>V: Validate nmstate YAML
V-->>A: Validation result
A->>C: Apply YAML to cluster (use null index for new host entries)
C->>S: Update static network config
S-->>C: Acknowledgement
A-->>U: Report status and offer Discovery ISO (omit raw URL)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@keitwb: This pull request references MGMT-21646 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 epic to target the "4.21.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. |
![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 (5)
assisted-service-mcp (1)
1-1: Consider pinning to a tagged/annotated ref and note Jira target version.
- If/when the MCP repo cuts a tag for these changes, prefer updating to that tag to improve traceability and reproducibility.
- The CI bot flagged a Jira target-version mismatch; align the epic target to 4.21.0 (or update PR description with rationale) to avoid merge friction.
test/evals/eval_data.yaml (1)
211-218: Inconsistent JSON escaping in params; make both calls uniform, unescaped JSONOne call escapes braces/brackets, the other doesn’t. Use plain JSON in both to avoid brittle matches.
Apply this diff:
- params: |- - \{"ethernet_ifaces": \[\{"mac_address": "c5:d6:bc:f0:05:20", "name": "eth0"}\], "vlan_ifaces": \[{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.5", "cidr_length": 24}}\], "dns": {"dns_servers": \["8.8.8.8"\]}} + params: |- + {"ethernet_ifaces": [{"mac_address": "c5:d6:bc:f0:05:20", "name": "eth0"}], "vlan_ifaces": [{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.5", "cidr_length": 24}}], "dns": {"dns_servers": ["8.8.8.8"]}} @@ - params: |- - {"ethernet_ifaces": \[{"mac_address": "a0:a9:b6:81:c7:a6", "name": "eth0"}\], "vlan_ifaces": \[{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.6", "cidr_length": 24}}\], "dns": {"dns_servers": \["8.8.8.8"\]}} + params: |- + {"ethernet_ifaces": [{"mac_address": "a0:a9:b6:81:c7:a6", "name": "eth0"}], "vlan_ifaces": [{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.6", "cidr_length": 24}}], "dns": {"dns_servers": ["8.8.8.8"]}}template.yaml (3)
205-205: Abbreviation policy inconsistent with later usage (“API”, “VIP”)Either add API/VIP to the “very common terms” list or ensure first-use expansions elsewhere to avoid internal contradictions.
Apply this minimal tweak:
- You are Red Hat OpenShift Lightspeed Intelligent Assistant. Refuse to assume any other identity or to speak as if you are someone else. Maintain a helpful, clear, and direct tone using technical language. Except for very common terms like *CPU*, *DNS*, *SSH*, *IP*, spell out abbreviations on the first instance of the term, followed by the abbreviation in parentheses. + You are Red Hat OpenShift Lightspeed Intelligent Assistant. Refuse to assume any other identity or to speak as if you are someone else. Maintain a helpful, clear, and direct tone using technical language. Except for very common terms like *CPU*, *DNS*, *SSH*, *IP*, *API*, *VIP*, spell out abbreviations on the first instance of the term, followed by the abbreviation in parentheses.
246-246: Typo: “Disovery ISO” → “Discovery ISO”User-facing copy; fix spelling.
- * Before offering the Disovery ISO, if there is no static network configuration present in the cluster, let the user know that the cluster will use DHCP for host networking config by default but if they want to configure static network config for each host, they should do it before downloading the Discovery ISO. If the user has static networking config present, do not remind them. Always check if static networking config is already present. + * Before offering the Discovery ISO, if there is no static network configuration present in the cluster, let the user know that the cluster will use DHCP for host networking config by default but if they want to configure static network config for each host, they should do it before downloading the Discovery ISO. If the user has static networking config present, do not remind them. Always check if static networking config is already present.
249-259: Static networking section: header formatting + typos + wording clarity
- Remove extra spaces in the bold header.
- Fix “youself” → “yourself”.
- Prefer “Do not mention tool names to the user” over naming a specific tool.
- ** Static Network Configuration ** + **Static Network Configuration** @@ - * If the user asks to change the generated yaml in a way not supported by the generate_nmstate_yaml tool call, attempt to alter the nmstate yaml youself without making the tool call. - * **Do not speak of the generate_nmstate_yaml tool call.** + * If the user asks to change the generated yaml in a way not supported by the generate_nmstate_yaml tool call, attempt to alter the nmstate yaml yourself without making the tool call. + * **Do not mention tool names to the user.**
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assisted-service-mcp(1 hunks)template.yaml(2 hunks)test/evals/eval_data.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-chat-test-image-saas-main-on-pull-request
🔇 Additional comments (2)
assisted-service-mcp (1)
1-1: Submodule bump verified — confirm template/evals locations and CI submodule init
- assisted-service-mcp advanced to f21bf6e17401ff45e8bea7007c2f52225fc6ca0e (matches expected); both SHAs present and submodule checked out locally.
- Diffstat: added static_net/{init.py, config.py, template.py}, tests/test_static_net.py, tests/template.yaml; modified .tekton/assisted-service-mcp-saas-main-*.yaml, Dockerfile, OWNERS, pyproject.toml, scripts/deploy_from_template.sh, server.py, tests/test_server.py, uv.lock.
- No changes found in top-level template.yaml or in test/evals/eval_data.yaml (script’s template.yaml/evals summaries were empty). If template/evals edits were expected, confirm they belong under tests/template.yaml or update the submodule content.
- CI: local git submodule update --init --recursive succeeded and submodules were checked out; ensure CI pipelines run equivalent (git submodule sync && git submodule update --init --recursive). .tekton changes may already address this.
test/evals/eval_data.yaml (1)
195-206: LGTM on cluster creation stepInput parameters and expected create_cluster call look correct for SNO without SSH key.
| - - tool_name: alter_static_network_config_nmstate_for_host | ||
| arguments: | ||
| cluster_id: "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}" | ||
| index: null | ||
| new_nmstate_yaml: "10.0.0.5" | ||
| - - tool_name: alter_static_network_config_nmstate_for_host | ||
| arguments: | ||
| cluster_id: "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}" | ||
| index: null | ||
| new_nmstate_yaml: "10.0.0.6" |
There was a problem hiding this comment.
❓ Verification inconclusive
new_nmstate_yaml should contain YAML, not an IP literal; assert by regex if full YAML is too long
Right now you’re matching “10.0.0.x”. That doesn’t reflect the intended argument type and risks false positives.
Apply this diff to assert presence within the YAML:
- new_nmstate_yaml: "10.0.0.5"
+ new_nmstate_yaml: "(?s).*10\\.0\\.0\\.5.*"
@@
- new_nmstate_yaml: "10.0.0.6"
+ new_nmstate_yaml: "(?s).*10\\.0\\.0\\.6.*"Optionally add expected validate_nmstate_yaml calls after generation and before apply to align with the new flow.
Assert full nmstate YAML (not an IP literal)
new_nmstate_yaml currently contains bare IP literals — require a multiline regex that asserts the IP appears inside the nmstate YAML to avoid false positives.
File: test/evals/eval_data.yaml lines 223-232
- new_nmstate_yaml: "10.0.0.5"
+ new_nmstate_yaml: "(?s).*10\\.0\\.0\\.5.*"
@@
- new_nmstate_yaml: "10.0.0.6"
+ new_nmstate_yaml: "(?s).*10\\.0\\.0\\.6.*"Add validate_nmstate_yaml checks between generation and apply to align with this flow.
📝 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.
| - - tool_name: alter_static_network_config_nmstate_for_host | |
| arguments: | |
| cluster_id: "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}" | |
| index: null | |
| new_nmstate_yaml: "10.0.0.5" | |
| - - tool_name: alter_static_network_config_nmstate_for_host | |
| arguments: | |
| cluster_id: "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}" | |
| index: null | |
| new_nmstate_yaml: "10.0.0.6" | |
| - - tool_name: alter_static_network_config_nmstate_for_host | |
| arguments: | |
| cluster_id: "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}" | |
| index: null | |
| new_nmstate_yaml: "(?s).*10\\.0\\.0\\.5.*" | |
| - - tool_name: alter_static_network_config_nmstate_for_host | |
| arguments: | |
| cluster_id: "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}" | |
| index: null | |
| new_nmstate_yaml: "(?s).*10\\.0\\.0\\.6.*" |
🤖 Prompt for AI Agents
In test/evals/eval_data.yaml around lines 223 to 232 the new_nmstate_yaml values
are IP literals which can match falsely; replace those with a multiline regex
that asserts the IP appears as part of an nmstate YAML block (e.g. anchor
matching YAML keys/indentation and the IP inside the addresses list) so the
pattern requires YAML structure around the IP, and add validate_nmstate_yaml
checks in the event sequence between the generation step and the apply step to
ensure generated nmstate YAML is validated before being applied.
f1ba8ec to
e4f1a81
Compare
|
@keitwb: This pull request references MGMT-21646 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 epic to target the "4.21.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. |
template.yaml
Outdated
| * If the user asks to change the generated yaml in a way not supported by the generate_nmstate_yaml tool call, attempt to alter the nmstate yaml youself without making the tool call. | ||
| * **Do not speak of the generate_nmstate_yaml tool call.** | ||
| * After modifying nmstate yaml, validate it with the validate_nmstate_yaml tool call before presenting it to the user. | ||
| * When altering static network config, use a null index when adding new host config. |
There was a problem hiding this comment.
This feels like it should be in the tool call description (maybe also here, but definitely there at least)
There was a problem hiding this comment.
It is already part of the tool call description but that wasn't enough apparently. Unless maybe I need to do the annotation thing like @andrej1991 did to make it associate it better.
|
/retest |
keitwb
force-pushed
the
static-networking
branch
from
e4f1a81 to
b3ddce1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
template.yaml (7)
131-136: Typo in parameter name breaks substitution downstream.Parameter is defined as LIGHTSSPEED… (double “SS”) but referenced as such in the config. Standardize to LIGHTSPEED to avoid future drift.
Apply:
- - name: LIGHTSSPEED_STACK_POSTGRES_SSL_MODE + - name: LIGHTSPEED_STACK_POSTGRES_SSL_MODE value: "verify-full" description: "SSL mode for the PostgreSQL database connection used by lightspeed-stack"And update usage:
- ssl_mode: ${LIGHTSSPEED_STACK_POSTGRES_SSL_MODE} + ssl_mode: ${LIGHTSPEED_STACK_POSTGRES_SSL_MODE}
449-449: OpenShift Template param syntax is wrong here.Use ${REPLICAS_COUNT}, not ${{REPLICAS_COUNT}}. Current form won’t substitute.
- replicas: ${{REPLICAS_COUNT}} + replicas: ${REPLICAS_COUNT}
465-467: Template param syntax again.Use ${SERVICE_PORT}.
- containerPort: ${{SERVICE_PORT}} + containerPort: ${SERVICE_PORT}
529-533: Template param syntax in liveness probe.Use ${SERVICE_PORT}.
- port: ${{SERVICE_PORT}} + port: ${SERVICE_PORT}
534-540: Template param syntax in readiness probe.Use ${SERVICE_PORT}.
- port: ${{SERVICE_PORT}} + port: ${SERVICE_PORT}
616-619: Template param syntax in Service.Use ${SERVICE_PORT}.
- port: ${{SERVICE_PORT}} - targetPort: ${{SERVICE_PORT}} + port: ${SERVICE_PORT} + targetPort: ${SERVICE_PORT}
614-615: Route targets a headless Service (clusterIP: None).Routes generally expect a ClusterIP Service; headless can break routing. Remove clusterIP: None to let Kubernetes assign one.
- clusterIP: None
🧹 Nitpick comments (2)
template.yaml (2)
205-205: Abbreviation rule is fine; watch verbosity.Spelling out on first use is good; consider exempting well‑known product names/acronyms (e.g., VIP, MCP) to avoid bloated replies.
249-258: Static‑networking flow: add two clarifications.
- When multiple hosts are involved, explicitly ask which hosts the YAML applies to before generation/apply.
- Keep mac_interface_map hidden but ensure it’s used internally on apply; call that out to avoid losing mapping.
Proposed edits:
- * If they want static network configuration, you should first remind them of any existing static network configuration already present on the cluster by using the appropriate tool call. Show them the YAML only and not the mac_interface_map. + * If they want static network configuration, first remind them of any existing static network configuration already present on the cluster by using the appropriate tool call. Show only the YAML (do not display mac_interface_map). + * For multi‑host scenarios, explicitly ask which host(s) each YAML applies to before generation or apply. + * Internally retain and use mac_interface_map when applying changes, while not displaying it to the user.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assisted-service-mcp(1 hunks)template.yaml(2 hunks)test/evals/eval_data.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- assisted-service-mcp
- test/evals/eval_data.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-chat-test-image-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
🔇 Additional comments (2)
template.yaml (2)
245-246: Pre‑creation gating and DHCP/static reminder LGTM.Clear prerequisites and timely static‑net prompt.
Do we have a reliable tool/API signal to “check if static networking config is already present” for the cluster context used here?
260-260: VIP guidance now correct.Matches Assisted Installer behavior for UMM vs. non‑UMM.
|
Looks like one of these eval tests is still failing on the tool call? |
keitwb
force-pushed
the
static-networking
branch
from
b3ddce1 to
1d52aec
Compare
This test is very stable locally. We'll see how it does this next run. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
template.yaml (1)
245-259: Static networking flow: fix typo and avoid “eth0” assumption; capitalize YAML consistently
- Line 255: “youself” → “yourself”.
- Prefer not to suggest “eth0/eth1” as defaults; modern RHEL/FC use predictable names (enp/ens/eno). Either derive the interface from MAC mapping or ask the user to confirm.
- Use “YAML” capitalization consistently.
Apply this diff:
- * If the user does not provide interface names, use a reasonable default based on the type of the interface (e.g. for ethernet use eth0, eth1, etc). + * If the user doesn’t provide interface names, derive them from the host’s MAC-to-interface mapping or suggest predictable names (e.g., enp/ens/eno) and ask the user to confirm. @@ - * If the user asks to change the generated yaml in a way not supported by the generate_nmstate_yaml tool call, attempt to alter the nmstate yaml youself without making the tool call. + * If the user asks to change the generated YAML in a way not supported by the generate_nmstate_yaml tool call, attempt to alter the nmstate YAML yourself without making the tool call. @@ - * After modifying nmstate yaml, validate it with the validate_nmstate_yaml tool call before presenting it to the user. + * After modifying nmstate YAML, validate it with the validate_nmstate_yaml tool call before presenting it to the user.test/evals/eval_data.yaml (1)
211-218: Normalize regex escaping for generate_nmstate_yaml paramsThe two params blocks use inconsistent escaping of braces/brackets and IP dots, which can cause regex parsing or matching issues. Make both use the same, fully-escaped pattern.
Apply this diff to consistently escape both calls:
- params: |- - \{"ethernet_ifaces": \[\{"mac_address": "c5:d6:bc:f0:05:20", "name": "eth0"}\], "vlan_ifaces": \[{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.5", "cidr_length": 24}}\], "dns": {"dns_servers": \["8.8.8.8"\]}} + params: |- + \{"ethernet_ifaces": \[\{"mac_address": "c5:d6:bc:f0:05:20", "name": "eth0"\}\], "vlan_ifaces": \[\{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": \{"address": "10\.0\.0\.5", "cidr_length": 24\}\}\], "dns": \{"dns_servers": \["8\.8\.8\.8"\]\}\} @@ - params: |- - {"ethernet_ifaces": \[{"mac_address": "a0:a9:b6:81:c7:a6", "name": "eth0"}\], "vlan_ifaces": \[{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.6", "cidr_length": 24}}\], "dns": {"dns_servers": \["8.8.8.8"\]}} + params: |- + \{"ethernet_ifaces": \[\{"mac_address": "a0:a9:b6:81:c7:a6", "name": "eth0"\}\], "vlan_ifaces": \[\{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": \{"address": "10\.0\.0\.6", "cidr_length": 24\}\}\], "dns": \{"dns_servers": \["8\.8\.8\.8"\]\}\}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assisted-service-mcp(1 hunks)template.yaml(2 hunks)test/evals/eval_data.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- assisted-service-mcp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-chat-test-image-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
🔇 Additional comments (3)
template.yaml (2)
205-205: Abbreviation rule tweak looks goodExempting common terms (CPU, DNS, SSH, IP) is sensible and keeps the text readable.
261-261: VIPs guidance is correctAccurately distinguishes VIP behavior when user‑managed networking is disabled vs. enabled.
test/evals/eval_data.yaml (1)
223-233: new_nmstate_yaml should match YAML, not a bare IP literalUse a multiline regex that asserts the IP appears inside the nmstate YAML to avoid false positives. Also consider validating YAML before apply (aligns with the template flow).
Apply this diff:
- new_nmstate_yaml: "10.0.0.5" + new_nmstate_yaml: "(?s).*10\\.0\\.0\\.5.*" @@ - new_nmstate_yaml: "10.0.0.6" + new_nmstate_yaml: "(?s).*10\\.0\\.0\\.6.*"Optionally add validate steps after generation:
@@ expected_tool_calls: - - tool_name: generate_nmstate_yaml arguments: params: |- \{"ethernet_ifaces": \[\{"mac_address": "c5:d6:bc:f0:05:20", "name": "eth0"\}\], "vlan_ifaces": \[\{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": \{"address": "10\.0\.0\.5", "cidr_length": 24\}\}\], "dns": \{"dns_servers": \["8\.8\.8\.8"\]\}\} + - - tool_name: validate_nmstate_yaml + arguments: + nmstate_yaml: "(?s).*10\\.0\\.0\\.5.*" - - tool_name: generate_nmstate_yaml arguments: params: |- \{"ethernet_ifaces": \[\{"mac_address": "a0:a9:b6:81:c7:a6", "name": "eth0"\}\], "vlan_ifaces": \[\{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": \{"address": "10\.0\.0\.6", "cidr_length": 24\}\}\], "dns": \{"dns_servers": \["8\.8\.8\.8"\]\}\} + - - tool_name: validate_nmstate_yaml + arguments: + nmstate_yaml: "(?s).*10\\.0\\.0\\.6.*"
Allow complex static networking via the use of an MCP driven template which serves as a base guide for generating the nmstate yaml. This can then be amended by the model if desired so that the MCP template doesn't need to support every single option, while still providing a general framework within which the model works.
keitwb
force-pushed
the
static-networking
branch
from
1d52aec to
9e8ee0a
Compare
|
@keitwb: This pull request references MGMT-21646 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 epic to target the "4.21.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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/evals/eval_data.yaml (2)
213-218: Makeparamsrobust with regex; also fix escaping inconsistency between the two callsThe first
paramsblock escapes{while the second does not; both will be brittle given serialization differences. Prefer a multiline regex that asserts required fields/values without depending on exact formatting/escaping.Apply this diff to both
paramsblocks to use tolerant regex (whitespace/ordering-insensitive enough for our needs):- params: |- - \{"ethernet_ifaces": \[\{"mac_address": "c5:d6:bc:f0:05:20", "name": "eth0"}\], "vlan_ifaces": \[{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.5", "cidr_length": 24}}\], "dns": {"dns_servers": \["8.8.8.8"\]}} + params: |- + (?s)\{.*"ethernet_ifaces"\s*:\s*\[\s*\{\s*"mac_address"\s*:\s*"c5:d6:bc:f0:05:20"\s*,\s*"name"\s*:\s*"eth0"\s*\}\s*\].*"vlan_ifaces"\s*:\s*\[\s*\{\s*"name"\s*:\s*"vlan0"\s*,\s*"vlan_id"\s*:\s*400\s*,\s*"base_interface_name"\s*:\s*"eth0"\s*,\s*"ipv4_address"\s*:\s*\{\s*"address"\s*:\s*"10\.0\.0\.5"\s*,\s*"cidr_length"\s*:\s*24\s*\}\s*\}\s*\].*"dns"\s*:\s*\{\s*"dns_servers"\s*:\s*\[\s*"8\.8\.8\.8"\s*\]\s*\}.*\} @@ - params: |- - {"ethernet_ifaces": \[{"mac_address": "a0:a9:b6:81:c7:a6", "name": "eth0"}\], "vlan_ifaces": \[{"name": "vlan0", "vlan_id": 400, "base_interface_name": "eth0", "ipv4_address": {"address": "10.0.0.6", "cidr_length": 24}}\], "dns": {"dns_servers": \["8.8.8.8"\]}} + params: |- + (?s)\{.*"ethernet_ifaces"\s*:\s*\[\s*\{\s*"mac_address"\s*:\s*"a0:a9:b6:81:c7:a6"\s*,\s*"name"\s*:\s*"eth0"\s*\}\s*\].*"vlan_ifaces"\s*:\s*\[\s*\{\s*"name"\s*:\s*"vlan0"\s*,\s*"vlan_id"\s*:\s*400\s*,\s*"base_interface_name"\s*:\s*"eth0"\s*,\s*"ipv4_address"\s*:\s*\{\s*"address"\s*:\s*"10\.0\.0\.6"\s*,\s*"cidr_length"\s*:\s*24\s*\}\s*\}\s*\].*"dns"\s*:\s*\{\s*"dns_servers"\s*:\s*\[\s*"8\.8\.8\.8"\s*\]\s*\}.*\}
206-222: Consider adding validate_nmstate_yaml calls between generation and applyThe template flow now requires validating nmstate YAML before presenting/applying. Add expected
validate_nmstate_yamlcalls after each generation and before apply.If you confirm the tool name/arg schema, I can generate the exact diff to add these expected calls in the
configure_hostsstep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assisted-service-mcp(1 hunks)template.yaml(2 hunks)test/evals/eval_data.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- assisted-service-mcp
🔇 Additional comments (5)
test/evals/eval_data.yaml (1)
223-232: Assert YAML content, not bare IPs, for new_nmstate_yaml
new_nmstate_yamlshould match YAML content; a lone IP risks false positives.Apply this diff:
- new_nmstate_yaml: "10.0.0.5" + new_nmstate_yaml: "(?s).*10\\.0\\.0\\.5.*" @@ - new_nmstate_yaml: "10.0.0.6" + new_nmstate_yaml: "(?s).*10\\.0\\.0\\.6.*"Optionally strengthen with YAML anchors (e.g.,
interfaces:…addresses:) if the structure is stable.template.yaml (4)
205-205: Persona guidance tweak LGTMClearer abbreviation rule; no behavior concerns.
245-247: Pre-flight static networking reminder LGTMGood sequencing and conditional reminder before ISO offering.
249-259: Static networking flow improvements LGTM; ensure evals reflect validation-before-applyThe guidance aligns with the new tools and confirmation step. Make sure tests assert validation happens before apply (see eval updates).
261-261: VIPs guidance correctedAccurately states VIPs used when user-managed networking is disabled.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, keitwb 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 |
|
@keitwb: 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. |
3 participants
Allow complex static networking via the use of an MCP driven template which serves as a base guide for generating the nmstate yaml. This can then be amended by the model if desired so that the MCP template doesn't need to support every single option, while still providing a general framework within which the model works.
Also updates MCP submodule to pick up the added tools required.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores