Static Networking Config#96
Static Networking Config#96openshift-merge-bot[bot] merged 1 commit intoopenshift-assisted:masterfrom
Conversation
WalkthroughAdds NMState static-networking support: introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as Server (server.py)
participant U as StaticNet Utils (service_client/static_net.py)
participant M as Metrics
participant IE as InfraEnv Store
rect rgba(200,230,255,0.20)
note over C,S: Generate NMState YAML
C->>S: generate_nmstate_yaml(params)
S->>M: track_tool_usage("generate_nmstate_yaml")
S->>U: generate_nmstate_from_template(params)
U-->>S: nmstate_yaml or TemplateError
S-->>C: nmstate_yaml or error
end
rect rgba(220,255,220,0.20)
note over C,S: Validate NMState YAML
C->>S: validate_nmstate_yaml(nmstate_yaml)
S->>M: track_tool_usage("validate_nmstate_yaml")
S->>U: validate_and_parse_nmstate(nmstate_yaml)
U-->>S: parsed_dict or error
S-->>C: "OK" or error
end
rect rgba(255,240,200,0.20)
note over C,S: Alter host static config
C->>S: alter_static_network_config_nmstate_for_host(cluster_id, index?, yaml?)
S->>M: track_tool_usage("alter_static_network_config_nmstate_for_host")
S->>U: add_or_replace_static_host_config_yaml(existing, index?, yaml?)
U-->>S: updated_list or error
S->>IE: update infra_env(cluster_id, updated_list)
IE-->>S: ack
S-->>C: result JSON or error
end
rect rgba(255,220,230,0.20)
note over C,S: List InfraEnvs
C->>S: list_infra_envs(cluster_id)
S->>M: track_tool_usage("list_infra_envs")
S->>IE: fetch infra_envs(cluster_id)
IE-->>S: list
S-->>C: JSON list
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Poem
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 |
openshift-ci
Bot
added
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
6-6: Python version mismatch with base image (blocks container build).Dockerfile uses UBI9 Python 3.11, but pyproject requires >=3.13. Align to 3.11 (or bump the image).
Apply:
-requires-python = ">=3.13" +requires-python = ">=3.11"
🧹 Nitpick comments (4)
service_client/static_net.py (4)
43-71: Disallow negative indexes to avoid accidental Python negative indexing semantics.Currently, -1 will replace the last entry, which is surprising.
def add_or_replace_static_host_config_yaml( existing_static_network_config: str | None, index: int | None, new_nmstate_yaml: str, ) -> list[HostStaticNetworkConfig]: @@ - if index is None: + if index is None: config.append(host_config) else: + if index < 0: + raise IndexError("negative indexes are not supported") if index >= len(config): raise IndexError( f"static network config only has {len(config)} elements, cannot replace index {index}" ) config[index] = host_config
27-41: Likewise, forbid negative index on delete.def remove_static_host_config_by_index( existing_static_network_config: str, index: int ) -> list[HostStaticNetworkConfig]: @@ - if index >= len(config): + if index < 0: + raise ValueError("negative indexes are not supported") + if index >= len(config): raise ValueError( f"static network config only has {len(config)} elements, cannot delete index {index}" )
133-141: Make ipv4_address optional with a default.Avoid forcing callers to always supply None explicitly; aligns with usage elsewhere.
class EthernetInterfaceParams(BaseModel): @@ - ipv4_address: IPV4AddressWithSubnet | None + ipv4_address: IPV4AddressWithSubnet | None = None
167-175: Optional: match Ethernet default for VLAN ipv4_address.- ipv4_address: IPV4AddressWithSubnet | None = Field( + ipv4_address: IPV4AddressWithSubnet | None = Field( description="If the user supplies an IP address for the vlan interface, don't reuse that same address on the base ethernet interface" )(If intended optional with default None, set
= Noneexplicitly.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Dockerfile(1 hunks)pyproject.toml(3 hunks)server.py(2 hunks)service_client/static_net.py(1 hunks)tests/test_static_net.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server.py (3)
metrics/metrics.py (3)
metrics(69-71)track_tool_usage(51-65)initiate_metrics(44-48)service_client/assisted_service_api.py (3)
get_infra_env(203-218)update_infra_env(306-327)list_infra_envs(221-239)service_client/static_net.py (5)
NMStateTemplateParams(193-209)add_or_replace_static_host_config_yaml(43-70)generate_nmstate_from_template(212-214)remove_static_host_config_by_index(27-40)validate_and_parse_nmstate(94-106)
tests/test_static_net.py (1)
service_client/static_net.py (11)
remove_static_host_config_by_index(27-40)add_or_replace_static_host_config_yaml(43-70)validate_and_parse_nmstate(94-106)generate_nmstate_from_template(212-214)NMStateTemplateParams(193-209)EthernetInterfaceParams(133-140)IPV4AddressWithSubnet(126-130)RouteParams(109-123)DNSParams(182-190)BondInterfaceParams(143-164)VLANInterfaceParams(167-179)
🪛 GitHub Actions: Unit Tests
server.py
[error] 16-16: OSError during test collection: libnmstate.so.2 cannot be opened (No such file or directory) while importing 'server'
tests/test_static_net.py
[error] 12-12: OSError during test collection: libnmstate.so.2 cannot be opened (No such file or directory) while importing 'service_client.static_net'
⏰ 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). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (5)
pyproject.toml (1)
15-18: nmstate Git dep requires system libs; tests fail without them.The Python binding loads libnmstate.so at import-time. Since unit tests run outside the container, they lack nmstate-libs, causing ImportError/OSError. I’m proposing a code-side fallback in static_net.py to avoid hard system deps during tests. No change needed here if that fallback is accepted.
server.py (3)
396-441: LGTM on nmstate tools wiring.Interfaces are clean and errors are handled. These will work once the static_net import fallback and template fixes land.
Re-run unit tests after applying static_net fixes; the current CI failure stems from libnmstate import at module import time.
443-483: Alter/replace/remove host config flow looks correct.Index/None semantics and update call are sound. Negative index handling will inherit from static_net stricter checks.
487-503: InfraEnv listing tool is straightforward.No issues; returns JSON string as advertised.
tests/test_static_net.py (1)
1-524: Tests look comprehensive; they’ll fail until template/context and import fallback are fixed.No changes needed here. After the static_net fixes, these should pass; consider adding a negative-index test later.
keitwb
force-pushed
the
static-hosts-nmstate
branch
from
db04fa2 to
8987266
Compare
openshift-ci
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
service_client/static_net.py (2)
218-221: Fix Jinja render: unpack params so top‑level names (dns/routes/ethernet_ifaces/etc.) are availableThe template references top‑level variables (dns, routes, ethernet_ifaces, …), but
render(params)only exposes a single positional object. Unpack the model to kwargs. Alternatively, change the template to referenceparams.*. Given the current template, unpacking is the minimal fix.Apply this diff:
def generate_nmstate_from_template(params: NMStateTemplateParams) -> str: """Generate the nmstate yaml based on the params""" - return NMSTATE_TEMPLATE.render(params) + return NMSTATE_TEMPLATE.render(**params.model_dump())
76-89: Defensive parsing: validate 'interfaces' is a list and require both name+MACCurrent code assumes
interfacesexists and is iterable; missing/invalid types will raise at runtime. Also ensure both fields exist to avoidNoneentries.Apply this diff:
def _generate_host_static_config(nmstate_yaml: str) -> HostStaticNetworkConfig: nmstate = validate_and_parse_nmstate(nmstate_yaml) - interfaces = nmstate.get("interfaces") - name_and_mac_list: list[MacInterfaceMap] = [ - { - "mac_address": i.get("mac-address"), - "logical_nic_name": i.get("name"), - } - for i in interfaces - if i.get("mac-address") - ] + interfaces = nmstate.get("interfaces") + if not isinstance(interfaces, list): + raise ValueError("nmstate yaml must contain an 'interfaces' list") + name_and_mac_list: list[MacInterfaceMap] = [ + { + "mac_address": cast(str, i.get("mac-address")), + "logical_nic_name": cast(str, i.get("name")), + } + for i in interfaces + if isinstance(i, dict) + and i.get("mac-address") + and i.get("name") + ] if not name_and_mac_list: - raise ValueError("At least one interface must be associated to a MAC Address") + raise ValueError("At least one interface must include both name and MAC address")
🧹 Nitpick comments (9)
service_client/static_net.py (7)
30-41: Normalize exception types and harden JSON parsing
- Use IndexError for out‑of‑range (consistent with add/replace docstring and Python semantics).
- Catch JSON decode errors and raise a clear ValueError.
Apply this diff:
def remove_static_host_config_by_index( existing_static_network_config: str, index: int ) -> list[HostStaticNetworkConfig]: """Remove a single host's config by index position.""" - config = [ - cast(HostStaticNetworkConfig, d) - for d in json.loads(existing_static_network_config) - ] + try: + parsed = json.loads(existing_static_network_config) + except json.JSONDecodeError as e: + raise ValueError("existing_static_network_config must be valid JSON") from e + config = [cast(HostStaticNetworkConfig, d) for d in parsed] if index < 0: raise IndexError("negative indexes are not allowed") if index >= len(config): - raise ValueError( + raise IndexError( f"static network config only has {len(config)} elements, cannot delete index {index}" ) del config[index] return config
97-110: Optional: enforce mapping type from YAMLIf
safe_loadreturns a non‑mapping (e.g., a list), downstream.get()calls break. Guard early.Apply this diff:
try: - return yaml.safe_load(nmstate_yaml) + data = yaml.safe_load(nmstate_yaml) + if not isinstance(data, dict): + raise ValueError("nmstate YAML must be a mapping at the top level") + return data
179-181: Make VLAN ipv4_address truly optionalWithout an explicit default, Pydantic may treat Optional as required. Set
= Nonefor clarity and UX.Apply this diff:
- ipv4_address: IPV4AddressWithSubnet | None = Field( + ipv4_address: IPV4AddressWithSubnet | None = Field( + None, description="If the user supplies an IP address for the vlan interface, don't reuse that same address on the base ethernet interface" )
165-167: Bond should require at least two portsBonds with <2 ports are invalid for most modes. Enforce via schema.
Apply this diff:
- port_interface_names: list[str] = Field( + port_interface_names: list[str] = Field( + min_length=2, description="The interface names that are aggregated for this bond." )
257-267: Only emitdhcp: falsewhen IPv4 is enabledWhen IPv4 is disabled, the
dhcpkey is redundant; keeping it conditional avoids noise.Apply this diff:
ipv4: {% if i.ipv4_address %} address: - ip: {{i.ipv4_address.address}} prefix-length: {{i.ipv4_address.cidr_length}} enabled: true + dhcp: false {% else %} enabled: false {% endif %} - dhcp: falseAnd for bond:
ipv4: {% if i.ipv4_address %} address: - ip: {{i.ipv4_address.address}} prefix-length: {{i.ipv4_address.cidr_length}} - {% endif %} - enabled: true - dhcp: false + enabled: true + dhcp: false + {% else %} + enabled: false + {% endif %}Also applies to: 295-303
115-117: Typo: “addreses” → “addresses”Apply this diff:
- destination: str = Field( - "0.0.0.0/0", description="The destination addreses for which this route applies" - ) + destination: str = Field( + "0.0.0.0/0", description="The destination addresses for which this route applies" + )
101-102: Grammar nit: docstring“invalid is some way” → “invalid in some way”.
Apply this diff:
- - ValueError: If the yaml is invalid is some way + - ValueError: If the yaml is invalid in some wayserver.py (2)
430-441: Avoid logging full generated YAML at info levelContains MACs/IPs; lower to debug or truncate to reduce sensitive data in logs.
Apply this diff:
- log.info("Generate nmstate yaml with params: %s", params.model_dump_json(indent=2)) + log.debug("Generate nmstate yaml with params: %s", params.model_dump_json(indent=2)) try: generated = generate_nmstate_from_template(params) - log.info("Generated yaml: %s", generated) + log.debug("Generated yaml (truncated): %s", generated[:2000]) return generated
435-441: Return a more actionable error without leaking internalsSurface the error class to the caller; keeps logs detailed while the response is concise.
Apply this diff:
- except TemplateError as e: + except TemplateError as e: log.error("Failed to render nmstate template", exc_info=e) - return "ERROR: Failed to generate nmstate yaml" + return f"ERROR: Failed to generate nmstate yaml ({e.__class__.__name__})" except Exception as e: log.error("Exception generating nmstate yaml", exc_info=e) - return "ERROR: Unknown error" + return f"ERROR: Unknown error ({e.__class__.__name__})"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Dockerfile(1 hunks)pyproject.toml(3 hunks)server.py(2 hunks)service_client/static_net.py(1 hunks)tests/test_static_net.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Dockerfile
- tests/test_static_net.py
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T18:35:37.169Z
Learnt from: keitwb
PR: openshift-assisted/assisted-service-mcp#96
File: service_client/static_net.py:212-215
Timestamp: 2025-09-17T18:35:37.169Z
Learning: Jinja2's template.render() method can accept Pydantic BaseModel objects directly as positional arguments. The template can then access the model's fields using dot notation (e.g., {{params.field_name}}). There's no need to unpack the model using model_dump() or dict() methods.
Applied to files:
service_client/static_net.py
🧬 Code graph analysis (1)
server.py (3)
metrics/metrics.py (3)
metrics(69-71)track_tool_usage(51-65)initiate_metrics(44-48)service_client/assisted_service_api.py (4)
InventoryClient(25-536)get_infra_env(203-218)update_infra_env(306-327)list_infra_envs(221-239)service_client/static_net.py (5)
NMStateTemplateParams(199-215)add_or_replace_static_host_config_yaml(44-73)generate_nmstate_from_template(218-220)remove_static_host_config_by_index(26-41)validate_and_parse_nmstate(97-109)
⏰ 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). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (1)
server.py (1)
456-477: Rely on consistent IndexError from helpers for out‑of‑range indicesAfter normalizing exceptions in
remove_static_host_config_by_index, this tool can document/catch just IndexError for invalid indices.Do you want me to update docstrings/handlers here to reflect IndexError only and add a brief user‑facing message when indices are invalid?
keitwb
force-pushed
the
static-hosts-nmstate
branch
from
8987266 to
dbdb3ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
service_client/static_net.py (1)
76-95: Defensive parsing: validate root mapping, enforce 'interfaces' list, and require both name+MAC.Current code will crash on missing/invalid 'interfaces' (TypeError/AttributeError) and accepts entries without a name. Raise clear ValueErrors and filter strictly.
Apply:
def _generate_host_static_config(nmstate_yaml: str) -> HostStaticNetworkConfig: - nmstate = validate_and_parse_nmstate(nmstate_yaml) - interfaces = nmstate.get("interfaces") - name_and_mac_list: list[MacInterfaceMap] = [ - { - "mac_address": i.get("mac-address"), - "logical_nic_name": i.get("name"), - } - for i in interfaces - if i.get("mac-address") - ] - if not name_and_mac_list: - raise ValueError("At least one interface must be associated to a MAC Address") + nmstate = validate_and_parse_nmstate(nmstate_yaml) + if not isinstance(nmstate, dict): + raise ValueError("nmstate yaml must be a mapping at the top level") + interfaces = nmstate.get("interfaces") + if not isinstance(interfaces, list): + raise ValueError("nmstate yaml must contain an 'interfaces' list") + name_and_mac_list: list[MacInterfaceMap] = [ + { + "mac_address": cast(str, i.get("mac-address")), + "logical_nic_name": cast(str, i.get("name")), + } + for i in interfaces + if isinstance(i, dict) and i.get("mac-address") and i.get("name") + ] + if not name_and_mac_list: + raise ValueError("At least one interface must include both name and MAC address")
🧹 Nitpick comments (9)
service_client/static_net.py (9)
55-60: Handle invalid JSON/type for existing_static_network_config.Wrap json.loads, ensure array type, and surface a clear ValueError.
Apply:
- if existing_static_network_config: - config = [ - cast(HostStaticNetworkConfig, d) - for d in json.loads(existing_static_network_config) - ] + if existing_static_network_config: + try: + parsed = json.loads(existing_static_network_config) + except json.JSONDecodeError as e: + raise ValueError("existing_static_network_config must be a JSON array") from e + if not isinstance(parsed, list): + raise ValueError("existing_static_network_config must be a JSON array") + config = [cast(HostStaticNetworkConfig, d) for d in parsed]
97-110: Tighten return type and doc for validate_and_parse_nmstate.Reflect actual return (dict or None) to aid callers and type-checkers.
Apply:
-def validate_and_parse_nmstate(nmstate_yaml: str) -> Any: +def validate_and_parse_nmstate(nmstate_yaml: str) -> dict[str, Any] | None: """Validate nmstate yaml and return it parsed. Raises: - ValueError: If the yaml is invalid is some way """
173-181: ipv4_address is declared Optional but required; default to None.Without a default, Pydantic treats it as required even with Optional.
Apply:
- ipv4_address: IPV4AddressWithSubnet | None = Field( - description="If the user supplies an IP address for the vlan interface, don't reuse that same address on the base ethernet interface" - ) + ipv4_address: IPV4AddressWithSubnet | None = Field( + None, + description="If the user supplies an IP address for the vlan interface, don't reuse that same address on the base ethernet interface" + )
129-134: Consider disallowing /0 masks for interface addresses.CIDR 0 is not valid for an assigned interface address (it’s a route). Recommend 1..32.
Apply (if aligned with nmstate expectations):
- cidr_length: int = Field(ge=0, le=32) + cidr_length: int = Field(ge=1, le=32)
188-196: Clarify DNS description (not optional once dns is provided).The field is required (min_length=1). Drop “optional” to avoid confusion.
Apply:
- dns_servers: list[str] = Field( - min_length=1, description="An optional list of DNS server ip addresses" - ) + dns_servers: list[str] = Field( + min_length=1, description="A non-empty list of DNS server IP addresses" + )
275-283: VLAN IPv4: disable when no address is supplied (align with ethernet/bond).Emitting enabled: true with no address is inconsistent and can confuse nmstate/apply flows.
Apply:
ipv4: - {% if i.ipv4_address %} - address: - - ip: {{i.ipv4_address.address}} - prefix-length: {{i.ipv4_address.cidr_length}} - {% endif %} - enabled: true - dhcp: false + {% if i.ipv4_address %} + address: + - ip: {{i.ipv4_address.address}} + prefix-length: {{i.ipv4_address.cidr_length}} + enabled: true + dhcp: false + {% else %} + enabled: false + dhcp: false + {% endif %}
295-303: Bond IPv4: disable when no address is supplied.Mirror ethernet behavior to avoid “enabled with no address”.
Apply:
ipv4: - {% if i.ipv4_address %} - address: - - ip: {{i.ipv4_address.address}} - prefix-length: {{i.ipv4_address.cidr_length}} - {% endif %} - enabled: true - dhcp: false + {% if i.ipv4_address %} + address: + - ip: {{i.ipv4_address.address}} + prefix-length: {{i.ipv4_address.cidr_length}} + enabled: true + dhcp: false + {% else %} + enabled: false + dhcp: false + {% endif %}
307-315: Whitespace/format nits in template (harmless but noisy in diffs).Trim trailing space after “port:” and option values.
Apply:
- port: + port: @@ - {{k}}: {{v}} + {{k}}: {{v}}
26-42: Unify out-of-range exception types — raise IndexError in remove_static_host_config_by_indexadd_or_replace_static_host_config_yaml is tested to raise IndexError on OOB (tests/test_static_net.py:178–181); remove_static_host_config_by_index currently raises ValueError for index >= len(config). Change remove_static_host_config_by_index to raise IndexError for consistency (or document the intentional difference). File: service_client/static_net.py lines 26–42.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Dockerfile(1 hunks)pyproject.toml(3 hunks)server.py(2 hunks)service_client/static_net.py(1 hunks)tests/test_static_net.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server.py
- pyproject.toml
- Dockerfile
- tests/test_static_net.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T18:35:37.169Z
Learnt from: keitwb
PR: openshift-assisted/assisted-service-mcp#96
File: service_client/static_net.py:212-215
Timestamp: 2025-09-17T18:35:37.169Z
Learning: Jinja2's template.render() method can accept Pydantic BaseModel objects directly as positional arguments. The template can then access the model's fields using dot notation (e.g., {{params.field_name}}). There's no need to unpack the model using model_dump() or dict() methods.
Applied to files:
service_client/static_net.py
⏰ 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). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (1)
service_client/static_net.py (1)
218-221: LGTM: template rendering API.Passing the Pydantic model directly is acceptable given the current template usage and tests.
keitwb
force-pushed
the
static-hosts-nmstate
branch
3 times, most recently
from
06d38a8 to
5ab9575
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
service_client/static_net.py (2)
218-221: LGTM on render(params).Per prior decision and tests, passing the Pydantic model directly works for this template.
76-94: Defensive parsing: validate 'interfaces' list and require both name+MAC.Avoid TypeError on missing/invalid 'interfaces' and prevent None in mac_interface_map. Build the list only when both fields exist.
def _generate_host_static_config(nmstate_yaml: str) -> HostStaticNetworkConfig: nmstate = validate_and_parse_nmstate(nmstate_yaml) - interfaces = nmstate.get("interfaces") - name_and_mac_list: list[MacInterfaceMap] = [ - { - "mac_address": i.get("mac-address"), - "logical_nic_name": i.get("name"), - } - for i in interfaces - if i.get("mac-address") - ] - if not name_and_mac_list: - raise ValueError("At least one interface must be associated to a MAC Address") + interfaces = nmstate.get("interfaces") + if not isinstance(interfaces, list): + raise ValueError("nmstate yaml must contain an 'interfaces' list") + name_and_mac_list: list[MacInterfaceMap] = [ + { + "mac_address": cast(str, i.get("mac-address")), + "logical_nic_name": cast(str, i.get("name")), + } + for i in interfaces + if isinstance(i, dict) and i.get("mac-address") and i.get("name") + ] + if not name_and_mac_list: + raise ValueError("At least one interface must include both name and MAC address")
🧹 Nitpick comments (6)
service_client/static_net.py (5)
96-109: Validate top-level nmstate structure is a mapping.safe_load may return non-dict (e.g., list/null). Enforce mapping for downstream code.
def validate_and_parse_nmstate(nmstate_yaml: str) -> Any: @@ - try: - return yaml.safe_load(nmstate_yaml) + try: + obj = yaml.safe_load(nmstate_yaml) except yaml.YAMLError as e: raise ValueError("Invalid YAML") from e + if not isinstance(obj, dict): + raise ValueError("Invalid nmstate structure: top-level must be a mapping") + return obj
173-186: Validate VLAN ID range (1–4094).Adds basic guard aligned with IEEE 802.1Q.
- vlan_id: int + vlan_id: int = Field(ge=1, le=4094)
26-41: Use consistent exception type for out-of-range index (IndexError).replace-delete APIs should raise IndexError for bounds issues (negative or OOB) for consistency with add/replace.
def remove_static_host_config_by_index( - existing_static_network_config: str, index: int + existing_static_network_config: str, index: int ) -> list[HostStaticNetworkConfig]: @@ - if index >= len(config): - raise ValueError( + if index >= len(config): + raise IndexError( f"static network config only has {len(config)} elements, cannot delete index {index}" )
26-33: Companion change: accept list input for remove helper.Mirror the robustness for deletion path.
-def remove_static_host_config_by_index( - existing_static_network_config: str, index: int +def remove_static_host_config_by_index( + existing_static_network_config: str | list[HostStaticNetworkConfig], index: int ) -> list[HostStaticNetworkConfig]: @@ - config = [ - cast(HostStaticNetworkConfig, d) - for d in json.loads(existing_static_network_config) - ] + if isinstance(existing_static_network_config, str): + config = [cast(HostStaticNetworkConfig, d) for d in json.loads(existing_static_network_config)] + else: + config = [cast(HostStaticNetworkConfig, d) for d in existing_static_network_config]
115-121: Typo in description.“addreses” → “addresses”.
- destination: str = Field( - "0.0.0.0/0", description="The destination addreses for which this route applies" - ) + destination: str = Field( + "0.0.0.0/0", description="The destination addresses for which this route applies" + )server.py (1)
435-441: Include error details in generation failures (without stack).Helps users fix template issues faster.
- except TemplateError as e: + except TemplateError as e: log.error("Failed to render nmstate template", exc_info=e) - return "ERROR: Failed to generate nmstate yaml" - except Exception as e: + return f"ERROR: Failed to generate nmstate yaml: {e}" + except Exception as e: log.error("Exception generating nmstate yaml", exc_info=e) - return "ERROR: Unknown error" + return f"ERROR: Unknown error: {e.__class__.__name__}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Dockerfile(1 hunks)pyproject.toml(3 hunks)server.py(2 hunks)service_client/static_net.py(1 hunks)tests/test_static_net.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Dockerfile
- pyproject.toml
- tests/test_static_net.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T18:35:37.169Z
Learnt from: keitwb
PR: openshift-assisted/assisted-service-mcp#96
File: service_client/static_net.py:212-215
Timestamp: 2025-09-17T18:35:37.169Z
Learning: Jinja2's template.render() method can accept Pydantic BaseModel objects directly as positional arguments. The template can then access the model's fields using dot notation (e.g., {{params.field_name}}). There's no need to unpack the model using model_dump() or dict() methods.
Applied to files:
service_client/static_net.py
🧬 Code graph analysis (1)
server.py (3)
metrics/metrics.py (2)
metrics(69-71)track_tool_usage(51-65)service_client/assisted_service_api.py (3)
get_infra_env(203-218)update_infra_env(306-327)list_infra_envs(221-239)service_client/static_net.py (5)
NMStateTemplateParams(199-215)add_or_replace_static_host_config_yaml(44-73)generate_nmstate_from_template(218-220)remove_static_host_config_by_index(26-41)validate_and_parse_nmstate(97-109)
⏰ 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). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
I'm not really an nmstate expert, but maybe @linoyaslan or @AlonaKaplan could take a look?
| @track_tool_usage() | ||
| async def list_infra_envs(cluster_id: str) -> str: | ||
| """ | ||
| List all of the infra_envs associated with the given cluster_id. |
There was a problem hiding this comment.
We've so far tried pretty hard to hide infraenv as a concept in this server. Should this maybe be getting the static network config for a cluster instead?
There was a problem hiding this comment.
Ok yeah I just make the method return the static net configs for a given cluster id to completely hide the concept of infra env altogether.
| @@ -0,0 +1,324 @@ | |||
| """Static networking related functionality.""" | |||
There was a problem hiding this comment.
I feel like this doesn't really belong in service_client.
I know the organization in this project is a bit of a mess, but the idea was for this to include just the stuff that talked to assisted service and this is more of a utility for editing and generating nmstate in isolation.
There was a problem hiding this comment.
Moved it to a new static_net package and split up the templating from the field/mac address map handling.
| - ValueError: If the yaml is invalid is some way | ||
| """ | ||
| # Eventually when nmstate 2.2.51 is released we should be able to validate the nmstate by doing: | ||
| # libnmstate.validate(nmstate_yaml) |
There was a problem hiding this comment.
Instead of leaving these comments in here, can we open a followup issue with this info if we intend to merge this without the nmstate validation changes?
There was a problem hiding this comment.
(Same goes for the Dockerfile and pyproject.toml changes)
There was a problem hiding this comment.
ok, removed them. It isn't super important as they just added validation to the assisted installer API that would use the same logic. The bot still is going to be instructed to call the validate method which at least checks that it is good yaml.
keitwb
force-pushed
the
static-hosts-nmstate
branch
from
5ab9575 to
0f5f57a
Compare
Add tools for CRUD operations on a host's static network config. Also add a tool to validate the static network nmstate YAML, and a tool to generate an initial nmstate YAML for the most common configurations used. The API handling is a bit awkward given the MAC to IP mapping but all of that complexity is hidden from the LLM.
keitwb
force-pushed
the
static-hosts-nmstate
branch
from
0f5f57a to
efca965
Compare
openshift-ci
Bot
added
size/XL
|
@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. |
|
[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 |
openshift-merge-bot
Bot
merged commit 7e55ae4
into
openshift-assisted:master
2 participants
Add tools for CRUD operations on a host's static network config.
Also add a tool to validate the static network nmstate YAML, and a tool to generate an initial nmstate YAML for the most common configurations used.
The API handling is a bit awkward given the MAC to IP mapping but all of that complexity is hidden from the LLM.
Summary by CodeRabbit
New Features
Tests
Chores