Conversation
Signed-off-by: Zoe Blevins <zblevins@nvidia.com>
Signed-off-by: Zoe Blevins <zblevins@nvidia.com>
…generator Signed-off-by: Zoe Blevins <zblevins@nvidia.com>
zsblevins
requested review from
dmathur0,
jojung1,
polarweasel,
ryanheffernan,
spidercensus and
susanhooks
as code owners
📝 WalkthroughWalkthroughAdds a full AIR Simulation flow (TUI, CLI, orchestrator, cloud-init, topology builders, mock contexts, templates, tests), renames installer CLI to nvcm-installer, updates docs, introduces parallel image builds, and removes legacy AIR client, activities, workflows, API endpoints, and Helm secret wiring. ChangesAIR Simulation Enablement and Legacy AIR Removal
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Collaborator
Author
|
/ok to test 7b9b118 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (3)
development/mock_topology/context/air_superpod/devices/su00-tan-spine01.json-61-117 (1)
61-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate
eth0interface entries (overwrite risk) insu00-tan-spine01.json
This JSON defines two interfaces with the samename(eth0), samemac_address, sameip_addresses, and sameconnected_interface, differing only in fields likemgmt_only/role. The generatordevelopment/mock_topology/jobs/designs/interfaces.yaml.j2keys interfaces by!create_or_update:nameand!ref:intf-{{ device.name }}-{{ intf.name }}, so both entries target the same interface object and will overwrite each other. Collapse to a singleeth0entry (keep the intendedmgmt_only/role).🤖 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 `@development/mock_topology/context/air_superpod/devices/su00-tan-spine01.json` around lines 61 - 117, The file contains two duplicate interface objects named "eth0" (same mac_address, ip_addresses, connected_interface) which will collide because the generator (interfaces.yaml.j2) keys interfaces by !create_or_update:name and !ref:intf-{{ device.name }}-{{ intf.name }}; collapse these into a single "eth0" object by merging the differing fields (choose the correct mgmt_only and role values you intend to keep, e.g., mgmt_only: true vs false and include the role: { name: "Management" } if needed), remove the duplicate entry, and ensure the resulting interface includes name, type, status, enabled, mgmt_only, ip_addresses, mac_address, description/mtu/role as required and the same connected_interface block so there is no overwrite during generation.installer/src/nv_config_manager_installer/air_sim/sim_manager.py-414-416 (1)
414-416:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn type contract is violated in
prepare_server().The method is annotated
str | None, but Line 465 returnsFalse. This introduces mixed return semantics and can cause incorrect callers.💡 Suggested fix
- return False + return NoneAlso applies to: 465-466
🤖 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 `@installer/src/nv_config_manager_installer/air_sim/sim_manager.py` around lines 414 - 416, prepare_server currently declares a return type of "str | None" but returns a boolean False in some failure paths; change those failure returns to None (or alternatively change the annotation to "str | bool" if boolean semantics are intentional) so the return type contract is consistent; locate the prepare_server function and replace any "return False" (and similar boolean returns) with "return None", and run a quick grep for callers of prepare_server to adjust any truthy/falsey checks to explicit None checks if needed.installer/src/nv_config_manager_installer/tui/air_sim/screens/options.py-158-165 (1)
158-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider providing user feedback for invalid timeout values.
The silent
ValueErrorhandling preserves the existing config value when the user enters invalid input, but provides no indication that the input was rejected. Users might be confused when their changes don't take effect.💡 Suggested improvement
Consider using Textual's validation system or showing a notification when integer parsing fails:
try: config.wait_timeout = int(self.query_one("`#wait-timeout`", Input).value) except ValueError: self.app.notify("Invalid wait timeout - must be an integer", severity="warning")🤖 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 `@installer/src/nv_config_manager_installer/tui/air_sim/screens/options.py` around lines 158 - 165, The current try/except blocks around reading "`#wait-timeout`" and "`#deploy-timeout`" swallow ValueError silently; update the handlers in the method where you call self.query_one("`#wait-timeout`", Input) and self.query_one("`#deploy-timeout`", Input) so that when int(...) raises ValueError you preserve the existing config value but also surface feedback to the user (e.g., call self.app.notify or use Textual validation) with a clear message like "Invalid wait timeout — must be an integer" and "Invalid deploy timeout — must be an integer" so users know their input was rejected; keep the existing behavior of not overwriting config but add the notification in the except blocks for both config.wait_timeout and config.deploy_timeout.
🧹 Nitpick comments (1)
development/mock_topology/jobs/mock_topology_design.py (1)
92-99: ⚡ Quick winConsider logging invalid content type strings.
The method silently returns
Nonefor invalid content type strings. Configuration errors (typos, missing models) will be silently ignored. Consider logging a warning whenValueErrororContentType.DoesNotExistis caught to aid debugging.📝 Suggested enhancement
+import logging + +logger = logging.getLogger(__name__) + `@staticmethod` def _get_content_type(content_type: str) -> ContentType | None: """Resolve an app.model content type string.""" try: app_label, model = content_type.split(".") return ContentType.objects.get(app_label=app_label, model=model) except (ValueError, ContentType.DoesNotExist): + logger.warning("Invalid content type string: %s", content_type) return None🤖 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 `@development/mock_topology/jobs/mock_topology_design.py` around lines 92 - 99, The _get_content_type function currently swallows ValueError and ContentType.DoesNotExist; modify it to log a warning including the invalid content_type string and the exception details before returning None. Use a module logger (e.g., logger = logging.getLogger(__name__)) or an existing logger and call logger.warning(...) inside the except block, referencing the caught exception and the content_type variable; keep the existing return None behavior. Ensure you import logging if not present and catch both ValueError and ContentType.DoesNotExist in the same except to log them.
🤖 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
`@development/mock_topology/context/air_superpod/devices/su00-cin-spine01.json`:
- Around line 61-117: There are two duplicate interface objects named "eth0" on
this device (both using address 10.100.1.25/25, same MAC and connected peer
su01-oob-mleaf01:swp6) which will conflict in Nautobot and evade the
_require_cumulus_eth0_mac validator that only checks the first match; remove the
redundant entry and keep the fuller definition (the one with mgmt_only:false
plus role/mtu/description) or merge any needed fields into that single eth0
object so only one eth0 interface exists for the device.
In
`@development/mock_topology/context/air_superpod/devices/su00-cin-spine02.json`:
- Around line 61-117: Remove the redundant eth0 interface entry: there are two
interface objects with "name": "eth0" (both using IP 10.100.1.149/25, MAC
44:38:39:00:00:03 and connected_interface su01-oob-mleaf02:swp6); keep the
intended single definition (prefer the one with the desired fields such as
"mgmt_only": false, "description", "mtu", "role") and delete the other block so
the device has only one eth0 entry and no duplicate IP/MAC/peer config.
In
`@development/mock_topology/context/air_superpod/devices/su01-tan-bleaf01.json`:
- Around line 61-117: The interfaces array contains two entries both named
"eth0" with identical IP 10.100.1.19/25, MAC 44:38:39:00:00:13 and
connected_interface swp3 -> device su01-oob-mleaf01; remove the duplicate by
either merging the two objects into a single interface entry for "eth0"
(combining fields like mgmt_only, description, mtu, role) or rename one entry to
a distinct interface name (e.g., "eth1" or "eth0:mgmt") and update its fields
accordingly so each interface name is unique.
In
`@development/mock_topology/context/air_superpod/devices/su01-tan-bleaf02.json`:
- Around line 61-117: The interfaces array contains two entries with the same
"name": "eth0" (both have address 10.100.1.146/25, mac_address 44:38:39:00:00:14
and connected_interface.name "swp3"), which must be merged or renamed; update
the JSON so there is only one unique interface entry for "eth0" by consolidating
fields (combine mgmt_only, enabled, description, mtu, role, ip_addresses and
connected_interface) into a single object (or rename the second entry to a
distinct interface name if it represents a different logical interface) ensuring
no duplicated IPs/MACs and preserving connected_interface.device/role
information.
In
`@development/mock_topology/context/air_superpod/devices/su01-tan-hleaf01.json`:
- Around line 61-117: The interfaces array contains two entries with the same
name "eth0" (both with address 10.100.1.21/25 and MAC 44:38:39:00:00:15), which
is invalid; resolve by merging the two objects into a single "eth0" entry that
combines differing fields (e.g., include mgmt_only, description, mtu, role) or
by renaming one entry to a unique interface name (and adjust its
description/role accordingly), ensuring there is only one "name": "eth0" object
and that IP, mac, connected_interface, and role fields are consistent and not
duplicated.
In
`@development/mock_topology/context/air_superpod/devices/su01-tan-hleaf02.json`:
- Around line 61-117: There are two interface objects both named "eth0" with
identical IP/MAC/connected_interface but conflicting mgmt_only values; remove or
merge the duplicate so each interface name is unique: pick the correct mgmt_only
value (true or false) and consolidate properties into a single "eth0" entry
(preserve fields like ip_addresses, mac_address, mtu, description, role,
connected_interface) or rename one interface if both are needed; update the
object that currently contains "name": "eth0" to remove the duplicate and ensure
only one interface object with that name remains.
In
`@development/mock_topology/context/air_superpod/devices/su01-tan-sleaf01.json`:
- Around line 61-117: The JSON defines the same interface name "eth0" twice with
identical IP/MAC/connected_interface but conflicting mgmt_only and other
properties; remove or merge the duplicate so only one "eth0" entry exists:
either delete the redundant block (the second or first) or merge fields into a
single object for "eth0" preserving the correct values for mgmt_only,
description, mtu, role, ip_addresses, mac_address and connected_interface;
ensure the final interface object uses the unique name "eth0" and consistent
properties to avoid validation/parsing errors.
In
`@development/mock_topology/context/air_superpod/devices/su01-tan-sleaf02.json`:
- Around line 61-117: There are two interface objects both named "eth0" with
identical ip_addresses, mac_address and connected_interface but conflicting
mgmt_only values; fix by merging these into a single "eth0" entry (or remove the
duplicate) so only one interface object exists, consolidate fields
(ip_addresses, mac_address, connected_interface, description, mtu, role) into
that single "eth0" object and set mgmt_only to the correct boolean value you
intend to model.
In `@installer/nvcm-air-sim.yaml`:
- Line 24: Replace the hardcoded NGC API key value for ngc_api_key in the YAML
(the line setting ngc_api_key) with a reference to a secrets source (e.g., an
environment variable or placeholder) and remove the literal token from the repo;
update any deployment/config loading to read process.env.NGC_API_KEY (or your
secret manager) and add a non-sensitive example entry (e.g.,
NGc_API_KEY=REPLACE_ME) to your env/sample file or docs so users know how to
supply the key, and ensure you scan for and remove any other hard-coded
credentials.
In `@installer/src/nv_config_manager_installer/air_sim/cloud_init.py`:
- Line 294: The FRR config template currently hardcodes the BGP password
"NVCMBgp1!" in _SETUP_SCRIPT_TEMPLATE; change the template to use a placeholder
(e.g. {bgp_password}) instead of the literal string, and update
generate_setup_script to substitute NVCM_NETWORK_SECRETS["bgp_password"] into
that placeholder before returning the script, ensuring the template uses the
placeholder and generate_setup_script imports/reads NVCM_NETWORK_SECRETS and
performs a safe format/replace to inject the secret into the
_SETUP_SCRIPT_TEMPLATE.
- Around line 334-366: The download loop for node_exporter (uses variables
tarball, gh_arch, NE_VERSION, NE_BASE, NE_DIR and temp file "/tmp/${tarball}")
does not verify integrity against the hardcoded SHA256 values in the manifest;
after each curl download compute the SHA256 of "/tmp/${tarball}" (eg via
sha256sum or shasum -a 256) and compare it to the expected checksum for that
arch (the values currently embedded in the jq manifest for node_exporter_amd64
and node_exporter_armv5), abort with a non‑zero exit and log an error if the
checksums differ, and only proceed to tar/x extract and mv the binary when the
checksum matches (also ensure you remove the temp file on failure). Ensure the
checksum lookup maps gh_arch or out_name to the correct expected hash before
comparing so the correct manifest value is used.
- Around line 539-542: The script currently injects unescaped dynamic values via
script.replace("__OOB_SWITCH_GW__", oob_gateway or "UNSET") and similar replaces
for "__LB_ALLOWED_PREFIXES__", "__RELAY_RETURN_NETWORKS__", and "__BGP_ASN__",
which risks shell injection; fix by importing shlex and wrapping each
substituted value with shlex.quote(...) (ensure the fallback "UNSET" is quoted
as well) before calling script.replace so all dynamic inputs are safely
shell-escaped.
In `@installer/src/nv_config_manager_installer/air_sim/sim_manager.py`:
- Around line 1342-1344: The public method currently hard-codes a default
password ("demo") in the signature (the parameters username, password, namespace
where CONFIG_MANAGER_NAMESPACE is used); remove the hard-coded default by making
password (and optionally username) required (or default to None) and add a
runtime validation that raises a clear exception if password is not provided, or
pull the value from a secure source (env var or secret manager) instead; update
all callers of this method to pass credentials and ensure documentation/comments
note the required parameter change so callers are fixed accordingly.
- Around line 525-527: Remove the hard-coded BGP password string "neighbor {gw}
password NVCMBgp1!" in sim_manager.py and instead inject the BGP password from a
secret/config input; e.g., add a parameter or read from a secrets/config object
(bgp_password) or environment variable (BGP_PASSWORD) in the function that
builds the FRR config where the neighbor {gw} lines are composed, replace the
literal with that variable, ensure the secret is sourced from your secret
manager or rotated per run, and avoid printing or logging the password.
- Around line 480-481: The route calculation currently slices internal_ip
assuming a /24; replace that with proper CIDR math using Python's ipaddress
module: parse internal_ip with ipaddress.ip_network(internal_ip, strict=False),
derive the canonical network as f"{net.network_address}/{net.prefixlen}" (or
str(net)), assign that to internal_network, and then call _ssh exactly as before
with iface_name; use strict=False to allow host addresses to resolve to their
correct network. Ensure you import ipaddress at top of sim_manager.py and update
the internal_network assignment where it currently uses rsplit and split.
In `@installer/src/nv_config_manager_installer/air_sim/topology.py`:
- Around line 301-315: The code calls description.lower() assuming description
is a string (description = intf.get("description", "")) but YAML null yields
None; update the handling so description is coerced to a string or validated
before calling .lower() — e.g., after reading description from intf, set
description = description if isinstance(description, str) else "" (or
description = (intf.get("description") or "")), then use description.lower()
when checking for "exit"; this change affects the block where description,
raw_mac, mac_address, needs_auto are computed and where
self.exit_interfaces.append((device_name, intf_name)) is conditional.
In `@installer/src/nv_config_manager_installer/deployer.py`:
- Around line 893-906: The code builds a docker command using build_output_args
created from build_env["BUILDX_BUILDER"] and then constructs the command in the
_ParallelCommand for each entry in images, but it erroneously appends --load to
a plain docker build; update the logic around build_output_args and the command
construction so that when build_env.get("BUILDX_BUILDER") is truthy you either
switch the invoked binary to "docker buildx build" (i.e., use "docker",
"buildx", "build", ... and include "--load") or keep "docker", "build" and
remove "--load" (or use "--output=type=docker" instead); adjust the code that
builds the cmd list inside the loop that creates _ParallelCommand (and the
build_output_args assignment) so the CLI flags match the chosen command (search
for build_output_args, build_env, images, and _ParallelCommand to find where to
change).
In `@installer/src/nv_config_manager_installer/tui/air_sim/screens/launch.py`:
- Around line 772-780: The method _do_prov_refresh uses assert
isinstance(self._manager, AirSimulationManager) for a runtime check; replace
that assert with an explicit check (e.g., if not isinstance(self._manager,
AirSimulationManager): raise TypeError(...) or log an error and return) before
calling self._manager.get_provisioning_status(self._host, self._port), so the
code fails predictably when _manager is the wrong type and still reaches the
finally block that clears self._prov_polling; reference _do_prov_refresh,
_manager, AirSimulationManager, get_provisioning_status, _update_prov,
_prov_polling, _host, and _port when making the change.
- Around line 893-900: In _run_orchestrator, the conditional used for the
context manager should choose the pathname before calling open rather than
between two open calls; replace the current conditional expression with one that
passes the chosen path to open (e.g., open(log_path if log_path else os.devnull,
"w") as lf) so the with statement consistently binds lf to the opened file, then
construct _TuiCallback(self, log_file=lf) and pass it to
SimOrchestrator(self._config, cb).
- Around line 762-770: Replace the runtime assert in _do_refresh with an
explicit isinstance check: verify that self._manager is an AirSimulationManager
before calling get_pod_status; if the check fails, either raise a descriptive
TypeError (e.g., "expected AirSimulationManager for self._manager") or log the
error and return early to avoid calling get_pod_status with the wrong type, then
proceed to call self._manager.get_pod_status(self._host, self._port) and
self.app.call_from_thread(self._update_table, pods) when the check passes.
---
Minor comments:
In
`@development/mock_topology/context/air_superpod/devices/su00-tan-spine01.json`:
- Around line 61-117: The file contains two duplicate interface objects named
"eth0" (same mac_address, ip_addresses, connected_interface) which will collide
because the generator (interfaces.yaml.j2) keys interfaces by
!create_or_update:name and !ref:intf-{{ device.name }}-{{ intf.name }}; collapse
these into a single "eth0" object by merging the differing fields (choose the
correct mgmt_only and role values you intend to keep, e.g., mgmt_only: true vs
false and include the role: { name: "Management" } if needed), remove the
duplicate entry, and ensure the resulting interface includes name, type, status,
enabled, mgmt_only, ip_addresses, mac_address, description/mtu/role as required
and the same connected_interface block so there is no overwrite during
generation.
In `@installer/src/nv_config_manager_installer/air_sim/sim_manager.py`:
- Around line 414-416: prepare_server currently declares a return type of "str |
None" but returns a boolean False in some failure paths; change those failure
returns to None (or alternatively change the annotation to "str | bool" if
boolean semantics are intentional) so the return type contract is consistent;
locate the prepare_server function and replace any "return False" (and similar
boolean returns) with "return None", and run a quick grep for callers of
prepare_server to adjust any truthy/falsey checks to explicit None checks if
needed.
In `@installer/src/nv_config_manager_installer/tui/air_sim/screens/options.py`:
- Around line 158-165: The current try/except blocks around reading
"`#wait-timeout`" and "`#deploy-timeout`" swallow ValueError silently; update the
handlers in the method where you call self.query_one("`#wait-timeout`", Input) and
self.query_one("`#deploy-timeout`", Input) so that when int(...) raises ValueError
you preserve the existing config value but also surface feedback to the user
(e.g., call self.app.notify or use Textual validation) with a clear message like
"Invalid wait timeout — must be an integer" and "Invalid deploy timeout — must
be an integer" so users know their input was rejected; keep the existing
behavior of not overwriting config but add the notification in the except blocks
for both config.wait_timeout and config.deploy_timeout.
---
Nitpick comments:
In `@development/mock_topology/jobs/mock_topology_design.py`:
- Around line 92-99: The _get_content_type function currently swallows
ValueError and ContentType.DoesNotExist; modify it to log a warning including
the invalid content_type string and the exception details before returning None.
Use a module logger (e.g., logger = logging.getLogger(__name__)) or an existing
logger and call logger.warning(...) inside the except block, referencing the
caught exception and the content_type variable; keep the existing return None
behavior. Ensure you import logging if not present and catch both ValueError and
ContentType.DoesNotExist in the same except to log them.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fa0ac14d-0253-4dfc-8eef-152472649849
⛔ Files ignored due to path filters (11)
docs/assets/images/air-sim/01-topology.svgis excluded by!**/*.svgdocs/assets/images/air-sim/02-options.svgis excluded by!**/*.svgdocs/assets/images/air-sim/03-launch-ready.svgis excluded by!**/*.svgdocs/assets/images/air-sim/04-launch-running.svgis excluded by!**/*.svgdocs/assets/images/air-sim/05-launch-pod-status.svgis excluded by!**/*.svgdocs/assets/images/air-sim/06-launch-dhcp-log.svgis excluded by!**/*.svgdocs/assets/images/air-sim/07-launch-ztp-log.svgis excluded by!**/*.svgdocs/assets/images/air-sim/08-launch-access.svgis excluded by!**/*.svgdocs/assets/images/installer/19-values-preview-generated.svgis excluded by!**/*.svginstaller/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (210)
.github/copilot-instructions.mdMakefileTHIRD_PARTY_LICENSES.mdcomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/management/__init__.pycomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/migrations/__init__.pycomponents/nautobot/nv_config_manager_jobs/bootstrap/load_bootstrap_data.pycomponents/nautobot/nv_config_manager_jobs/data/config_contexts.yamlcomponents/nautobot/tests/test_load_bootstrap_data.pydeploy/airgapped/README.mddeploy/airgapped/create-airgapped.shdeploy/airgapped/manifests/load-airgapped-images.shdeploy/helm/sample-eso-config.yamldeploy/helm/sample-nv-config-manager.inideploy/helm/templates/_vault-agent.tpldeploy/helm/templates/kubernetes-secrets.yamldeploy/helm/templates/vault-secrets.yamldeploy/helm/values-rbac-open.yamldeploy/helm/values.yamldeploy/scripts/setup-vm-prereqs.shdevelopment/air_sim/configs/air_trial.yamldevelopment/air_sim/template_plugins/superpod-template-plugin/README.mddevelopment/air_sim/template_plugins/superpod-template-plugin/pyproject.tomldevelopment/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/__init__.pydevelopment/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/cin-leaf/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/cin-leaf/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/cin-spine/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/cin-spine/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/oob-hleaf/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/oob-hleaf/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/oob-mleaf/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/oob-mleaf/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/oob-spine/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/oob-spine/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/bridge.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/evpn.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/interface.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/nve.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/qos.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/router.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/service.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/system.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/superpod-demo-common/include/vrf.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-bleaf/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-bleaf/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-hleaf/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-hleaf/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-sleaf/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-sleaf/5.16.1/entrypoint/startup.yaml.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-spine/5.16.1/entrypoint/boot-script.j2development/air_sim/template_plugins/superpod-template-plugin/src/nv_config_manager_superpod_templates/templates/cumulus-linux/tan-spine/5.16.1/entrypoint/startup.yaml.j2development/mock_topology/context/__init__.pydevelopment/mock_topology/context/air_superpod/README.mddevelopment/mock_topology/context/air_superpod/devices/oob-mgmt-server.jsondevelopment/mock_topology/context/air_superpod/devices/su00-cin-spine01.jsondevelopment/mock_topology/context/air_superpod/devices/su00-cin-spine02.jsondevelopment/mock_topology/context/air_superpod/devices/su00-control01.jsondevelopment/mock_topology/context/air_superpod/devices/su00-control02.jsondevelopment/mock_topology/context/air_superpod/devices/su00-oob-spine01.jsondevelopment/mock_topology/context/air_superpod/devices/su00-oob-spine02.jsondevelopment/mock_topology/context/air_superpod/devices/su00-tan-spine01.jsondevelopment/mock_topology/context/air_superpod/devices/su00-tan-spine02.jsondevelopment/mock_topology/context/air_superpod/devices/su01-cin-leaf-r01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-cin-leaf-r02.jsondevelopment/mock_topology/context/air_superpod/devices/su01-compute-tray-r01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-compute-tray-r02.jsondevelopment/mock_topology/context/air_superpod/devices/su01-oob-hleaf01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-oob-hleaf02.jsondevelopment/mock_topology/context/air_superpod/devices/su01-oob-mleaf01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-oob-mleaf02.jsondevelopment/mock_topology/context/air_superpod/devices/su01-storage01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-tan-bleaf01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-tan-bleaf02.jsondevelopment/mock_topology/context/air_superpod/devices/su01-tan-hleaf01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-tan-hleaf02.jsondevelopment/mock_topology/context/air_superpod/devices/su01-tan-sleaf01.jsondevelopment/mock_topology/context/air_superpod/devices/su01-tan-sleaf02.jsondevelopment/mock_topology/context/air_superpod/locations.yamldevelopment/mock_topology/context/air_superpod/prefixes.yamldevelopment/mock_topology/context/air_trial/README.mddevelopment/mock_topology/context/air_trial/devices/oob-mgmt-server.jsondevelopment/mock_topology/context/air_trial/devices/oob-mleaf-01.jsondevelopment/mock_topology/context/air_trial/devices/tan-leaf-01.jsondevelopment/mock_topology/context/air_trial/devices/tan-leaf-02.jsondevelopment/mock_topology/context/air_trial/devices/tan-leaf-03.jsondevelopment/mock_topology/context/air_trial/devices/tan-leaf-04.jsondevelopment/mock_topology/context/air_trial/devices/tan-leaf-05.jsondevelopment/mock_topology/context/air_trial/locations.yamldevelopment/mock_topology/context/air_trial/prefixes.yamldevelopment/mock_topology/context/dgx_cloud/devices/core1-cg1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/core1-cg2-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/core1-cp1-smn1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/core2-cg1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/core2-cg2-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/core2-cp1-smn1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/leaf1-cno1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/leaf1-cp1-smn1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/leaf1-hss1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/leaf2-cno1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/leaf2-cp1-smn1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/leaf2-hss1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/spine1-cp1-smn1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/spine2-cp1-smn1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/spine3-cno1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/spine4-cno1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/dgx_cloud/devices/spine4-hss1-cp1-tan1-dc01.jsondevelopment/mock_topology/context/superpod/devices/a04-u44-p01-tor-01.jsondevelopment/mock_topology/context/superpod/devices/a08-u28-p01-oobspine-01.jsondevelopment/mock_topology/context/superpod/devices/a08-u32-p01-cleaf-01.jsondevelopment/mock_topology/context/superpod/devices/a08-u44-p01-mleaf-01.jsondevelopment/mock_topology/context/superpod/devices/a09-u28-p01-bleaf-01.jsondevelopment/mock_topology/context/superpod/devices/a09-u32-p01-sleaf-01.jsondevelopment/mock_topology/context/superpod/devices/a09-u36-p01-spine-01.jsondevelopment/mock_topology/context/superpod/devices/a09-u44-p01-pleaf-01.jsondevelopment/mock_topology/jobs/designs/devices.yaml.j2development/mock_topology/jobs/designs/interfaces.yaml.j2development/mock_topology/jobs/designs/ip_addresses.yaml.j2development/mock_topology/jobs/designs/managed_devices.yaml.j2development/mock_topology/jobs/designs/roles.yaml.j2development/mock_topology/jobs/mock_topology_design.pydocs/README.mddocs/api-specs/temporal.openapi.jsondocs/fern/docs.ymldocs/getting-started/index.mdxdocs/getting-started/local-development-quick-start.mdxdocs/i-want-to.mdxdocs/index.mdxdocs/install/index.mdxdocs/install/install-airgapped.mdxdocs/network-ztp/upload-images.mdxdocs/user-guides/air-simulation/index.mdxinstaller/README.mdinstaller/nvcm-air-sim.yamlinstaller/pyproject.tomlinstaller/scripts/screenshot_air_sim_tui.pyinstaller/scripts/screenshot_tui.pyinstaller/src/nv_config_manager_installer/air_sim/__init__.pyinstaller/src/nv_config_manager_installer/air_sim/cli.pyinstaller/src/nv_config_manager_installer/air_sim/cloud_init.pyinstaller/src/nv_config_manager_installer/air_sim/constants.pyinstaller/src/nv_config_manager_installer/air_sim/context_topology.pyinstaller/src/nv_config_manager_installer/air_sim/installer_config.pyinstaller/src/nv_config_manager_installer/air_sim/models.pyinstaller/src/nv_config_manager_installer/air_sim/orchestrator.pyinstaller/src/nv_config_manager_installer/air_sim/prebuilt_configs.pyinstaller/src/nv_config_manager_installer/air_sim/proxy.pyinstaller/src/nv_config_manager_installer/air_sim/sim_config.pyinstaller/src/nv_config_manager_installer/air_sim/sim_manager.pyinstaller/src/nv_config_manager_installer/air_sim/topology.pyinstaller/src/nv_config_manager_installer/cli.pyinstaller/src/nv_config_manager_installer/deployer.pyinstaller/src/nv_config_manager_installer/schema.pyinstaller/src/nv_config_manager_installer/secrets.pyinstaller/src/nv_config_manager_installer/tui/air_sim/__init__.pyinstaller/src/nv_config_manager_installer/tui/air_sim/app.pyinstaller/src/nv_config_manager_installer/tui/air_sim/app.tcssinstaller/src/nv_config_manager_installer/tui/air_sim/screens/__init__.pyinstaller/src/nv_config_manager_installer/tui/air_sim/screens/launch.pyinstaller/src/nv_config_manager_installer/tui/air_sim/screens/options.pyinstaller/src/nv_config_manager_installer/tui/air_sim/screens/topology.pyinstaller/src/nv_config_manager_installer/tui/screens/vault.pyinstaller/tests/__init__.pyinstaller/tests/air_sim/test_context_topology.pyinstaller/tests/air_sim/test_installer_config.pyinstaller/tests/air_sim/test_orchestrator.pyinstaller/tests/air_sim/test_tui_launch.pyinstaller/tests/conftest.pyinstaller/tests/test_cli.pyinstaller/tests/test_conditional_fields.pyinstaller/tests/test_deployer.pyinstaller/tests/test_helm_values.pyinstaller/tests/test_operator_versions.pyinstaller/tests/test_registry_client.pyinstaller/tests/test_schema.pyinstaller/tests/test_secrets.pyinstaller/tests/test_template_scanner.pyinstaller/tests/test_tui.pypyproject.tomlscripts/add_spdx_headers.pysrc/nv_config_manager/temporal/api/parameter_v1.pysrc/nv_config_manager/temporal/client/air.pysrc/nv_config_manager/temporal/ngc/activities/__init__.pysrc/nv_config_manager/temporal/ngc/activities/air.pysrc/nv_config_manager/temporal/ngc/workflows/__init__.pysrc/nv_config_manager/temporal/ngc/workflows/air.pysrc/tests/conftest.pysrc/tests/temporal/api/test_parameter.pysrc/tests/temporal/ngc/activities/test_air.pysrc/tests/temporal/ngc/workflows/test_air_workflow.pyui/src/app/workflows/aircreatesimulationworkflow/form/air-create-simulation-form.tsxui/src/app/workflows/aircreatesimulationworkflow/form/loading.tsxui/src/app/workflows/aircreatesimulationworkflow/form/page.tsxui/src/app/workflows/airdeletesimulationworkflow/form/air-delete-simulation-form.tsxui/src/app/workflows/airdeletesimulationworkflow/form/loading.tsxui/src/app/workflows/airdeletesimulationworkflow/form/page.tsxui/src/app/workflows/airvalidatesiteworkflow/form/air-validates-site-workflow-form.tsxui/src/app/workflows/airvalidatesiteworkflow/form/loading.tsxui/src/app/workflows/airvalidatesiteworkflow/form/page.tsxui/src/config/site.tsui/src/hooks/index.tsui/src/hooks/useSimulations.tsui/src/mocks/data/airSimulations.tsui/src/mocks/data/index.tsui/src/mocks/handlers/airHandlers.tsui/src/mocks/handlers/index.tsui/src/mocks/handlers/workflowHandlers.tsui/src/types/data-table.types.tsui/tests/e2e/airCreateSimulationForm.spec.tsui/tests/e2e/airDeleteSimulationForm.spec.tsui/tests/e2e/airValidateSiteForm.spec.tsui/tests/e2e/shared/apiMocks.ts
💤 Files with no reviewable changes (17)
- deploy/helm/sample-nv-config-manager.ini
- deploy/helm/sample-eso-config.yaml
- deploy/scripts/setup-vm-prereqs.sh
- components/nautobot/nv_config_manager_jobs/data/config_contexts.yaml
- src/nv_config_manager/temporal/client/air.py
- src/nv_config_manager/temporal/ngc/workflows/init.py
- src/nv_config_manager/temporal/ngc/activities/init.py
- deploy/helm/templates/kubernetes-secrets.yaml
- src/nv_config_manager/temporal/ngc/activities/air.py
- deploy/helm/values.yaml
- installer/src/nv_config_manager_installer/schema.py
- deploy/helm/templates/vault-secrets.yaml
- docs/api-specs/temporal.openapi.json
- src/nv_config_manager/temporal/api/parameter_v1.py
- deploy/helm/templates/_vault-agent.tpl
- pyproject.toml
- deploy/helm/values-rbac-open.yaml
Comment on lines
+61
to
+117
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": true, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.25/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "mac_address": "44:38:39:00:00:02", | ||
| "connected_interface": { | ||
| "name": "swp6", | ||
| "device": { | ||
| "name": "su01-oob-mleaf01", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": false, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.25/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "description": "su01-oob-mleaf01:swp6", | ||
| "mtu": 9216, | ||
| "role": { | ||
| "name": "Management" | ||
| }, | ||
| "mac_address": "44:38:39:00:00:02", | ||
| "connected_interface": { | ||
| "name": "swp6", | ||
| "device": { | ||
| "name": "su01-oob-mleaf01", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Duplicate eth0 interface on the same device.
This device defines eth0 twice (Lines 61-86 and 87-117) with the same address 10.100.1.25/25, same MAC, and the same connected peer su01-oob-mleaf01:swp6. Interface names must be unique per device in Nautobot, so the design-builder mock topology job will conflict (overwrite or fail) on the second definition. The new _require_cumulus_eth0_mac validator only inspects the first matching eth0, so it won't catch this. Keep a single eth0 entry (the fuller mgmt_only:false one with role/mtu/description appears intended).
🐛 Proposed fix: remove the duplicate eth0
- {
- "name": "eth0",
- "type": "1000base-t",
- "status": {
- "name": "Active"
- },
- "enabled": true,
- "mgmt_only": true,
- "ip_addresses": [
- {
- "address": "10.100.1.25/25",
- "mask_length": 25,
- "ip_version": 4
- }
- ],
- "mac_address": "44:38:39:00:00:02",
- "connected_interface": {
- "name": "swp6",
- "device": {
- "name": "su01-oob-mleaf01",
- "role": {
- "name": "OOB-MLEAF"
- }
- }
- }
- },
{
"name": "eth0",
"type": "1000base-t",🤖 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 `@development/mock_topology/context/air_superpod/devices/su00-cin-spine01.json`
around lines 61 - 117, There are two duplicate interface objects named "eth0" on
this device (both using address 10.100.1.25/25, same MAC and connected peer
su01-oob-mleaf01:swp6) which will conflict in Nautobot and evade the
_require_cumulus_eth0_mac validator that only checks the first match; remove the
redundant entry and keep the fuller definition (the one with mgmt_only:false
plus role/mtu/description) or merge any needed fields into that single eth0
object so only one eth0 interface exists for the device.
Comment on lines
+61
to
+117
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": true, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.149/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "mac_address": "44:38:39:00:00:03", | ||
| "connected_interface": { | ||
| "name": "swp6", | ||
| "device": { | ||
| "name": "su01-oob-mleaf02", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": false, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.149/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "description": "su01-oob-mleaf02:swp6", | ||
| "mtu": 9216, | ||
| "role": { | ||
| "name": "Management" | ||
| }, | ||
| "mac_address": "44:38:39:00:00:03", | ||
| "connected_interface": { | ||
| "name": "swp6", | ||
| "device": { | ||
| "name": "su01-oob-mleaf02", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Duplicate eth0 interface on the same device.
Same issue as su00-cin-spine01.json: eth0 is defined twice (Lines 61-86 and 87-117) with identical address 10.100.1.149/25, MAC, and peer su01-oob-mleaf02:swp6. Remove the redundant definition so the design-builder job doesn't conflict on duplicate interface names.
🐛 Proposed fix: remove the duplicate eth0
- {
- "name": "eth0",
- "type": "1000base-t",
- "status": {
- "name": "Active"
- },
- "enabled": true,
- "mgmt_only": true,
- "ip_addresses": [
- {
- "address": "10.100.1.149/25",
- "mask_length": 25,
- "ip_version": 4
- }
- ],
- "mac_address": "44:38:39:00:00:03",
- "connected_interface": {
- "name": "swp6",
- "device": {
- "name": "su01-oob-mleaf02",
- "role": {
- "name": "OOB-MLEAF"
- }
- }
- }
- },
{
"name": "eth0",
"type": "1000base-t",📝 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.
Suggested change
| { | |
| "name": "eth0", | |
| "type": "1000base-t", | |
| "status": { | |
| "name": "Active" | |
| }, | |
| "enabled": true, | |
| "mgmt_only": true, | |
| "ip_addresses": [ | |
| { | |
| "address": "10.100.1.149/25", | |
| "mask_length": 25, | |
| "ip_version": 4 | |
| } | |
| ], | |
| "mac_address": "44:38:39:00:00:03", | |
| "connected_interface": { | |
| "name": "swp6", | |
| "device": { | |
| "name": "su01-oob-mleaf02", | |
| "role": { | |
| "name": "OOB-MLEAF" | |
| } | |
| } | |
| } | |
| }, | |
| { | |
| "name": "eth0", | |
| "type": "1000base-t", | |
| "status": { | |
| "name": "Active" | |
| }, | |
| "enabled": true, | |
| "mgmt_only": false, | |
| "ip_addresses": [ | |
| { | |
| "address": "10.100.1.149/25", | |
| "mask_length": 25, | |
| "ip_version": 4 | |
| } | |
| ], | |
| "description": "su01-oob-mleaf02:swp6", | |
| "mtu": 9216, | |
| "role": { | |
| "name": "Management" | |
| }, | |
| "mac_address": "44:38:39:00:00:03", | |
| "connected_interface": { | |
| "name": "swp6", | |
| "device": { | |
| "name": "su01-oob-mleaf02", | |
| "role": { | |
| "name": "OOB-MLEAF" | |
| } | |
| } | |
| } | |
| }, | |
| { | |
| "name": "eth0", | |
| "type": "1000base-t", | |
| "status": { | |
| "name": "Active" | |
| }, | |
| "enabled": true, | |
| "mgmt_only": false, | |
| "ip_addresses": [ | |
| { | |
| "address": "10.100.1.149/25", | |
| "mask_length": 25, | |
| "ip_version": 4 | |
| } | |
| ], | |
| "description": "su01-oob-mleaf02:swp6", | |
| "mtu": 9216, | |
| "role": { | |
| "name": "Management" | |
| }, | |
| "mac_address": "44:38:39:00:00:03", | |
| "connected_interface": { | |
| "name": "swp6", | |
| "device": { | |
| "name": "su01-oob-mleaf02", | |
| "role": { | |
| "name": "OOB-MLEAF" | |
| } | |
| } | |
| } | |
| }, |
🤖 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 `@development/mock_topology/context/air_superpod/devices/su00-cin-spine02.json`
around lines 61 - 117, Remove the redundant eth0 interface entry: there are two
interface objects with "name": "eth0" (both using IP 10.100.1.149/25, MAC
44:38:39:00:00:03 and connected_interface su01-oob-mleaf02:swp6); keep the
intended single definition (prefer the one with the desired fields such as
"mgmt_only": false, "description", "mtu", "role") and delete the other block so
the device has only one eth0 entry and no duplicate IP/MAC/peer config.
Comment on lines
+61
to
+117
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": true, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.19/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "mac_address": "44:38:39:00:00:13", | ||
| "connected_interface": { | ||
| "name": "swp3", | ||
| "device": { | ||
| "name": "su01-oob-mleaf01", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": false, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.19/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "description": "su01-oob-mleaf01:swp3", | ||
| "mtu": 9216, | ||
| "role": { | ||
| "name": "Management" | ||
| }, | ||
| "mac_address": "44:38:39:00:00:13", | ||
| "connected_interface": { | ||
| "name": "swp3", | ||
| "device": { | ||
| "name": "su01-oob-mleaf01", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Duplicate interface name "eth0" in interfaces array.
Lines 61-86 and 87-117 both define an interface named "eth0". A device cannot have two interfaces with the same name, which will cause data integrity issues when this topology is loaded. Both interfaces share the same IP (10.100.1.19/25), MAC address, and connected peer.
Either merge these into a single interface definition or rename one of them (e.g., to a different logical interface).
🤖 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 `@development/mock_topology/context/air_superpod/devices/su01-tan-bleaf01.json`
around lines 61 - 117, The interfaces array contains two entries both named
"eth0" with identical IP 10.100.1.19/25, MAC 44:38:39:00:00:13 and
connected_interface swp3 -> device su01-oob-mleaf01; remove the duplicate by
either merging the two objects into a single interface entry for "eth0"
(combining fields like mgmt_only, description, mtu, role) or rename one entry to
a distinct interface name (e.g., "eth1" or "eth0:mgmt") and update its fields
accordingly so each interface name is unique.
Comment on lines
+61
to
+117
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": true, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.146/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "mac_address": "44:38:39:00:00:14", | ||
| "connected_interface": { | ||
| "name": "swp3", | ||
| "device": { | ||
| "name": "su01-oob-mleaf02", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": false, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.146/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "description": "su01-oob-mleaf02:swp3", | ||
| "mtu": 9216, | ||
| "role": { | ||
| "name": "Management" | ||
| }, | ||
| "mac_address": "44:38:39:00:00:14", | ||
| "connected_interface": { | ||
| "name": "swp3", | ||
| "device": { | ||
| "name": "su01-oob-mleaf02", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Duplicate interface name "eth0" in interfaces array.
Lines 61-86 and 87-117 both define an interface named "eth0". A device cannot have two interfaces with the same name, which will cause data integrity issues when this topology is loaded. Both interfaces share the same IP (10.100.1.146/25), MAC address, and connected peer.
Either merge these into a single interface definition or rename one of them (e.g., to a different logical interface).
🤖 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 `@development/mock_topology/context/air_superpod/devices/su01-tan-bleaf02.json`
around lines 61 - 117, The interfaces array contains two entries with the same
"name": "eth0" (both have address 10.100.1.146/25, mac_address 44:38:39:00:00:14
and connected_interface.name "swp3"), which must be merged or renamed; update
the JSON so there is only one unique interface entry for "eth0" by consolidating
fields (combine mgmt_only, enabled, description, mtu, role, ip_addresses and
connected_interface) into a single object (or rename the second entry to a
distinct interface name if it represents a different logical interface) ensuring
no duplicated IPs/MACs and preserving connected_interface.device/role
information.
Comment on lines
+61
to
+117
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": true, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.21/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "mac_address": "44:38:39:00:00:15", | ||
| "connected_interface": { | ||
| "name": "swp4", | ||
| "device": { | ||
| "name": "su01-oob-mleaf01", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "name": "eth0", | ||
| "type": "1000base-t", | ||
| "status": { | ||
| "name": "Active" | ||
| }, | ||
| "enabled": true, | ||
| "mgmt_only": false, | ||
| "ip_addresses": [ | ||
| { | ||
| "address": "10.100.1.21/25", | ||
| "mask_length": 25, | ||
| "ip_version": 4 | ||
| } | ||
| ], | ||
| "description": "su01-oob-mleaf01:swp4", | ||
| "mtu": 9216, | ||
| "role": { | ||
| "name": "Management" | ||
| }, | ||
| "mac_address": "44:38:39:00:00:15", | ||
| "connected_interface": { | ||
| "name": "swp4", | ||
| "device": { | ||
| "name": "su01-oob-mleaf01", | ||
| "role": { | ||
| "name": "OOB-MLEAF" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Duplicate interface name "eth0" in interfaces array.
Lines 61-86 and 87-117 both define an interface named "eth0". A device cannot have two interfaces with the same name, which will cause data integrity issues when this topology is loaded. Both interfaces share the same IP (10.100.1.21/25), MAC address, and connected peer.
Either merge these into a single interface definition or rename one of them (e.g., to a different logical interface).
🤖 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 `@development/mock_topology/context/air_superpod/devices/su01-tan-hleaf01.json`
around lines 61 - 117, The interfaces array contains two entries with the same
name "eth0" (both with address 10.100.1.21/25 and MAC 44:38:39:00:00:15), which
is invalid; resolve by merging the two objects into a single "eth0" entry that
combines differing fields (e.g., include mgmt_only, description, mtu, role) or
by renaming one entry to a unique interface name (and adjust its
description/role accordingly), ensuring there is only one "name": "eth0" object
and that IP, mac, connected_interface, and role fields are consistent and not
duplicated.
Comment on lines
+301
to
+315
| description = intf.get("description", "") | ||
| raw_mac = intf.get("mac_address") | ||
| if raw_mac is not None and not isinstance(raw_mac, str): | ||
| raise ValueError( | ||
| f"{device_name} {intf_name} mac_address must be a string; " | ||
| "quote MAC addresses in YAML" | ||
| ) | ||
| mac_address = None if raw_mac == "auto" else raw_mac | ||
| needs_auto = raw_mac == "auto" | ||
|
|
||
| if device_name not in self.devices: | ||
| continue | ||
|
|
||
| if description.lower() == "exit": | ||
| self.exit_interfaces.append((device_name, intf_name)) |
There was a problem hiding this comment.
Potential AttributeError when interface description is null/non-string.
Line 314 assumes description is a string. If YAML has description: null, .lower() will raise and abort topology parsing.
💡 Suggested fix
- description = intf.get("description", "")
+ description = intf.get("description") or ""
...
- if description.lower() == "exit":
+ if str(description).lower() == "exit":📝 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.
Suggested change
| description = intf.get("description", "") | |
| raw_mac = intf.get("mac_address") | |
| if raw_mac is not None and not isinstance(raw_mac, str): | |
| raise ValueError( | |
| f"{device_name} {intf_name} mac_address must be a string; " | |
| "quote MAC addresses in YAML" | |
| ) | |
| mac_address = None if raw_mac == "auto" else raw_mac | |
| needs_auto = raw_mac == "auto" | |
| if device_name not in self.devices: | |
| continue | |
| if description.lower() == "exit": | |
| self.exit_interfaces.append((device_name, intf_name)) | |
| description = intf.get("description") or "" | |
| raw_mac = intf.get("mac_address") | |
| if raw_mac is not None and not isinstance(raw_mac, str): | |
| raise ValueError( | |
| f"{device_name} {intf_name} mac_address must be a string; " | |
| "quote MAC addresses in YAML" | |
| ) | |
| mac_address = None if raw_mac == "auto" else raw_mac | |
| needs_auto = raw_mac == "auto" | |
| if device_name not in self.devices: | |
| continue | |
| if str(description).lower() == "exit": | |
| self.exit_interfaces.append((device_name, intf_name)) |
🤖 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 `@installer/src/nv_config_manager_installer/air_sim/topology.py` around lines
301 - 315, The code calls description.lower() assuming description is a string
(description = intf.get("description", "")) but YAML null yields None; update
the handling so description is coerced to a string or validated before calling
.lower() — e.g., after reading description from intf, set description =
description if isinstance(description, str) else "" (or description =
(intf.get("description") or "")), then use description.lower() when checking for
"exit"; this change affects the block where description, raw_mac, mac_address,
needs_auto are computed and where self.exit_interfaces.append((device_name,
intf_name)) is conditional.
Comment on lines
+893
to
+906
| build_output_args = ["--load"] if build_env.get("BUILDX_BUILDER") else [] | ||
| build_commands: list[_ParallelCommand] = [] | ||
| for name, dockerfile, context in images: | ||
| build_tag = f"{name}:local" | ||
| self.callback.on_log(f"Building {build_tag}...") | ||
| _run_logged( | ||
| [ | ||
| "docker", | ||
| "build", | ||
| "--provenance=false", | ||
| "--build-context", | ||
| "scripts=build/", | ||
| *apt_mirror_args, | ||
| "-t", | ||
| build_tag, | ||
| "-f", | ||
| dockerfile, | ||
| context, | ||
| ], | ||
| step, | ||
| self.callback, | ||
| timeout=900, | ||
| build_commands.append( | ||
| _ParallelCommand( | ||
| label=name, | ||
| cmd=[ | ||
| "docker", | ||
| "build", | ||
| "--provenance=false", | ||
| "--progress=plain", | ||
| *build_output_args, | ||
| "--build-context", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does docker build(notdocker buildx build) support the --load flag in current Docker CLI versions?
💡 Result:
No. In current Docker CLI versions, docker build (the same command as docker image build) does not support the --load flag; --load is a flag for docker buildx build. Docker’s official reference for docker image build / docker build lists supported options but does not include any --load option [1]. By contrast, the official reference for docker buildx build defines --load as “Shorthand for --output=type=docker” and explicitly documents loading the single-platform build result into docker images [2]. Docker’s Buildx “build drivers” documentation also describes that, when using non-default build drivers, you use --load to load the image into the local image store [3]. Practical implication: if you need --load behavior, use docker buildx build --load ... (and for multi-platform images, you typically use --push instead) [3].
Citations:
- 1: https://docs.docker.com/reference/cli/docker/image/build/
- 2: https://docs.docker.com/reference/cli/docker/buildx/build
- 3: https://docs.docker.com/build/builders/drivers/
Fix invalid Docker CLI flag usage (--load with docker build)
In installer/src/nv_config_manager_installer/deployer.py (~893-906), --load is appended to a docker build invocation when BUILDX_BUILDER is set, but --load is only valid for docker buildx build. This will cause the build command to fail.
- When
BUILDX_BUILDERis set, either switch the command todocker buildx build ... --load(or--output=type=docker) or stop adding--loadfordocker build.
🤖 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 `@installer/src/nv_config_manager_installer/deployer.py` around lines 893 -
906, The code builds a docker command using build_output_args created from
build_env["BUILDX_BUILDER"] and then constructs the command in the
_ParallelCommand for each entry in images, but it erroneously appends --load to
a plain docker build; update the logic around build_output_args and the command
construction so that when build_env.get("BUILDX_BUILDER") is truthy you either
switch the invoked binary to "docker buildx build" (i.e., use "docker",
"buildx", "build", ... and include "--load") or keep "docker", "build" and
remove "--load" (or use "--output=type=docker" instead); adjust the code that
builds the cmd list inside the loop that creates _ParallelCommand (and the
build_output_args assignment) so the CLI flags match the chosen command (search
for build_output_args, build_env, images, and _ParallelCommand to find where to
change).
Comment on lines
+762
to
+770
| @work(thread=True, exclusive=False) | ||
| def _do_refresh(self) -> None: | ||
| self._polling = True | ||
| try: | ||
| assert isinstance(self._manager, AirSimulationManager) | ||
| pods = self._manager.get_pod_status(self._host, self._port) | ||
| self.app.call_from_thread(self._update_table, pods) | ||
| finally: | ||
| self._polling = False |
There was a problem hiding this comment.
Replace assert with explicit runtime check.
Using assert for runtime type validation is unreliable because assertions are disabled when Python runs with the -O optimization flag, which would cause silent failures in production.
🛡️ Proposed fix
def _do_refresh(self) -> None:
self._polling = True
try:
- assert isinstance(self._manager, AirSimulationManager)
+ if not isinstance(self._manager, AirSimulationManager):
+ return
pods = self._manager.get_pod_status(self._host, self._port)📝 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.
Suggested change
| @work(thread=True, exclusive=False) | |
| def _do_refresh(self) -> None: | |
| self._polling = True | |
| try: | |
| assert isinstance(self._manager, AirSimulationManager) | |
| pods = self._manager.get_pod_status(self._host, self._port) | |
| self.app.call_from_thread(self._update_table, pods) | |
| finally: | |
| self._polling = False | |
| `@work`(thread=True, exclusive=False) | |
| def _do_refresh(self) -> None: | |
| self._polling = True | |
| try: | |
| if not isinstance(self._manager, AirSimulationManager): | |
| return | |
| pods = self._manager.get_pod_status(self._host, self._port) | |
| self.app.call_from_thread(self._update_table, pods) | |
| finally: | |
| self._polling = False |
🤖 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 `@installer/src/nv_config_manager_installer/tui/air_sim/screens/launch.py`
around lines 762 - 770, Replace the runtime assert in _do_refresh with an
explicit isinstance check: verify that self._manager is an AirSimulationManager
before calling get_pod_status; if the check fails, either raise a descriptive
TypeError (e.g., "expected AirSimulationManager for self._manager") or log the
error and return early to avoid calling get_pod_status with the wrong type, then
proceed to call self._manager.get_pod_status(self._host, self._port) and
self.app.call_from_thread(self._update_table, pods) when the check passes.
Comment on lines
+772
to
+780
| @work(thread=True, exclusive=False) | ||
| def _do_prov_refresh(self) -> None: | ||
| self._prov_polling = True | ||
| try: | ||
| assert isinstance(self._manager, AirSimulationManager) | ||
| prov, total, remaining = self._manager.get_provisioning_status(self._host, self._port) | ||
| self.app.call_from_thread(self._update_prov, prov, total, remaining) | ||
| finally: | ||
| self._prov_polling = False |
There was a problem hiding this comment.
Replace assert with explicit runtime check.
Using assert for runtime type validation is unreliable because assertions are disabled when Python runs with the -O optimization flag, which would cause silent failures in production.
🛡️ Proposed fix
def _do_prov_refresh(self) -> None:
self._prov_polling = True
try:
- assert isinstance(self._manager, AirSimulationManager)
+ if not isinstance(self._manager, AirSimulationManager):
+ return
prov, total, remaining = self._manager.get_provisioning_status(self._host, self._port)🤖 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 `@installer/src/nv_config_manager_installer/tui/air_sim/screens/launch.py`
around lines 772 - 780, The method _do_prov_refresh uses assert
isinstance(self._manager, AirSimulationManager) for a runtime check; replace
that assert with an explicit check (e.g., if not isinstance(self._manager,
AirSimulationManager): raise TypeError(...) or log an error and return) before
calling self._manager.get_provisioning_status(self._host, self._port), so the
code fails predictably when _manager is the wrong type and still reaches the
finally block that clears self._prov_polling; reference _do_prov_refresh,
_manager, AirSimulationManager, get_provisioning_status, _update_prov,
_prov_polling, _host, and _port when making the change.
Comment on lines
+893
to
+900
| @work(thread=True, exclusive=False) | ||
| def _run_orchestrator(self) -> None: | ||
| log_path = self._deploy_log_path | ||
| with open(log_path, "w") if log_path else open(os.devnull, "w") as lf: | ||
| cb = _TuiCallback(self, log_file=lf) | ||
| orchestrator = SimOrchestrator(self._config, cb) | ||
| orchestrator.run() | ||
|
|
There was a problem hiding this comment.
Fix incorrect context manager syntax.
The conditional expression is malformed. When log_path is truthy, the expression open(log_path, "w") if log_path else open(os.devnull, "w") evaluates to a file object, but when log_path is falsy it also evaluates to a file object from the else branch. However, the as lf binding only applies to the final result, making this work accidentally. The intent is clearer and more correct when the conditional is inside the open() call.
🔧 Proposed fix
- with open(log_path, "w") if log_path else open(os.devnull, "w") as lf:
+ with open(log_path if log_path else os.devnull, "w") as lf:
cb = _TuiCallback(self, log_file=lf)📝 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.
Suggested change
| @work(thread=True, exclusive=False) | |
| def _run_orchestrator(self) -> None: | |
| log_path = self._deploy_log_path | |
| with open(log_path, "w") if log_path else open(os.devnull, "w") as lf: | |
| cb = _TuiCallback(self, log_file=lf) | |
| orchestrator = SimOrchestrator(self._config, cb) | |
| orchestrator.run() | |
| `@work`(thread=True, exclusive=False) | |
| def _run_orchestrator(self) -> None: | |
| log_path = self._deploy_log_path | |
| with open(log_path if log_path else os.devnull, "w") as lf: | |
| cb = _TuiCallback(self, log_file=lf) | |
| orchestrator = SimOrchestrator(self._config, cb) | |
| orchestrator.run() |
🤖 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 `@installer/src/nv_config_manager_installer/tui/air_sim/screens/launch.py`
around lines 893 - 900, In _run_orchestrator, the conditional used for the
context manager should choose the pathname before calling open rather than
between two open calls; replace the current conditional expression with one that
passes the chosen path to open (e.g., open(log_path if log_path else os.devnull,
"w") as lf) so the with statement consistently binds lf to the opened file, then
construct _TuiCallback(self, log_file=lf) and pass it to
SimOrchestrator(self._config, cb).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Validation
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit
New Features
nvcm-installercommand alias for the installer CLI.Documentation
nvcm-installernaming.Refactor