Show route targets on Spectrum-X overlay details#93
Draft
ryanheffernan wants to merge 3 commits into
Draft
Conversation
Signed-off-by: rheffernan <rheffernan@nvidia.com>
📝 WalkthroughWalkthroughAdds Spectrum-X overlay VRF routing display, and extends VRF provisioning to derive, create, reuse, and clean up route targets from site ASN data. ChangesSpectrum-X VXLAN VRF routing display
Spectrum-X VRF provisioning route targets
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/nautobot/nautobot-app-overlays/nautobot_app_overlays/tables.py (1)
91-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate RT template logic (minor DRY opportunity).
VRF_IMPORT_RT_TEMPLATEandVRF_EXPORT_RT_TEMPLATEare nearly identical, differing only byimport_targets/export_targets. django-tables2'sTemplateColumnsupports anextra_contextargument to parametrize a single shared template, which would remove the duplication.♻️ Optional consolidation using extra_context
-VRF_IMPORT_RT_TEMPLATE = """ +VRF_RT_TEMPLATE = """ {% load helpers %} {% if record.vrf %} - {% for rt in record.vrf.import_targets.all %}{{ rt|hyperlinked_object }}{% if not forloop.last %}, {% endif %}{% empty %}{{ None|placeholder }}{% endfor %} + {% for rt in target_accessor %}{{ rt|hyperlinked_object }}{% if not forloop.last %}, {% endif %}{% empty %}{{ None|placeholder }}{% endfor %} {% else %} {{ None|placeholder }} {% endif %} """ - -VRF_EXPORT_RT_TEMPLATE = """ -{% load helpers %} -{% if record.vrf %} - {% for rt in record.vrf.export_targets.all %}{{ rt|hyperlinked_object }}{% if not forloop.last %}, {% endif %}{% empty %}{{ None|placeholder }}{% endfor %} -{% else %} - {{ None|placeholder }} -{% endif %} -"""Then in the table:
import_targets = tables.TemplateColumn( template_code=VRF_RT_TEMPLATE, extra_context={"target_accessor": None}, # override via render_ hook, or pass a lambda accessor verbose_name="Import RTs (VRF)", orderable=False, )Note
extra_contextis static per-column, not per-record, so this requires either a smallrender_import_targets/render_export_targetsmethod on the table instead of separate templates, which is arguably simpler thanextra_contexthere.🤖 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 `@components/nautobot/nautobot-app-overlays/nautobot_app_overlays/tables.py` around lines 91 - 113, `VRF_IMPORT_RT_TEMPLATE` and `VRF_EXPORT_RT_TEMPLATE` duplicate the same rendering logic and should be consolidated. Replace the two nearly identical templates with a single shared RT template and use the table’s `render_` methods or a small shared helper in `tables.py` to select `record.vrf.import_targets` vs `record.vrf.export_targets` for `TemplateColumn`. Keep the existing `VRF_RD_TEMPLATE` unchanged and update the `TemplateColumn` definitions to point at the shared logic.
🤖 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 `@components/nautobot/nautobot-app-overlays/nautobot_app_overlays/views.py`:
- Around line 76-84: The Spectrum X VXLAN panel in the view handling instance
isolation filtering is currently pulling all VXLANs via instance.vxlans, which
can render non-L3 entries in the “L3 VXLANs and Route Targets” table. Update the
queryset used in the Spectrum X branch to filter for VNITypeChoices.L3_VNI, or
otherwise enforce that only L3 VNIs reach SpectrumXVXLANInlineTable, so the
rendered rows match the table’s RD/RT columns.
---
Nitpick comments:
In `@components/nautobot/nautobot-app-overlays/nautobot_app_overlays/tables.py`:
- Around line 91-113: `VRF_IMPORT_RT_TEMPLATE` and `VRF_EXPORT_RT_TEMPLATE`
duplicate the same rendering logic and should be consolidated. Replace the two
nearly identical templates with a single shared RT template and use the table’s
`render_` methods or a small shared helper in `tables.py` to select
`record.vrf.import_targets` vs `record.vrf.export_targets` for `TemplateColumn`.
Keep the existing `VRF_RD_TEMPLATE` unchanged and update the `TemplateColumn`
definitions to point at the shared logic.
🪄 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: 94ca4f55-c51b-4e82-a250-741b77ec02aa
📒 Files selected for processing (5)
components/nautobot/nautobot-app-overlays/nautobot_app_overlays/tables.pycomponents/nautobot/nautobot-app-overlays/nautobot_app_overlays/templates/nautobot_app_overlays/overlay_retrieve.htmlcomponents/nautobot/nautobot-app-overlays/nautobot_app_overlays/tests/test_tables.pycomponents/nautobot/nautobot-app-overlays/nautobot_app_overlays/tests/test_views.pycomponents/nautobot/nautobot-app-overlays/nautobot_app_overlays/views.py
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tests/temporal/ngc/activities/test_spx_overlay_activities.py (1)
470-479: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting the reused route target is actually wired into the VRF payload.
The test mocks a GET returning an existing route target and comments that creation is skipped, but the visible portion of the test (through line 495) doesn't assert the
/api/ipam/vrfs/POST body usesROUTE_TARGET_IDforimport_targets/export_targets. Since aioresponses raises a connection error on any unmocked call, a stray create-POST would already fail the test, but an explicit assertion would directly verify the reuse path wires the correct ID rather than just verifying no extra request was made.✅ Suggested additional assertion (place after line 495)
await provision_vrf( ProvisionVrfInput( namespaces=[NS_ID], route_distinguisher="*:60004", overlay_id="test-overlay-001", site=LOCATION_ID, tenant="Public Demo", ) ) + + assert _request_json(m, "post", f"{NAUTOBOT}/api/ipam/vrfs/") == { + "name": "SpXTenant60004", + "rd": "*:60004", + "namespace": NS_ID, + "tenant": TENANT_ID, + "import_targets": [ROUTE_TARGET_ID], + "export_targets": [ROUTE_TARGET_ID], + }🤖 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 `@src/tests/temporal/ngc/activities/test_spx_overlay_activities.py` around lines 470 - 479, The test for the overlay reuse path should explicitly verify that the existing route target is reused in the VRF creation payload. In test_spx_overlay_activities, after the /api/ipam/vrfs/ POST is exercised, assert the request body includes ROUTE_TARGET_ID in both import_targets and export_targets so the behavior is tied to the actual payload, not just the absence of an extra create request.
🤖 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 `@src/nv_config_manager/temporal/ngc/activities/nautobot.py`:
- Around line 64-66: The site_asn handling in the route-target derivation path
is accepting blank or whitespace-only values, which can lead to invalid names.
Update the logic in the relevant activity helper in nautobot.py that reads
site_asn from context.get("data") so it only returns a value when site_asn is a
non-empty, non-whitespace string; otherwise fall through to the existing
fallback behavior before building the route target.
---
Nitpick comments:
In `@src/tests/temporal/ngc/activities/test_spx_overlay_activities.py`:
- Around line 470-479: The test for the overlay reuse path should explicitly
verify that the existing route target is reused in the VRF creation payload. In
test_spx_overlay_activities, after the /api/ipam/vrfs/ POST is exercised, assert
the request body includes ROUTE_TARGET_ID in both import_targets and
export_targets so the behavior is tied to the actual payload, not just the
absence of an extra create request.
🪄 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: a61e33be-fdf5-4e1e-a0c1-ab1be204bcbc
📒 Files selected for processing (3)
src/nv_config_manager/temporal/ngc/activities/nautobot.pysrc/nv_config_manager/temporal/ngc/workflows/spx_overlay.pysrc/tests/temporal/ngc/activities/test_spx_overlay_activities.py
✅ Files skipped from review due to trivial changes (1)
- src/nv_config_manager/temporal/ngc/workflows/spx_overlay.py
Comment on lines
+64
to
+66
| site_asn = (context.get("data") or {}).get("site_asn") | ||
| if site_asn is not None: | ||
| return str(site_asn) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject blank site_asn values before deriving the route target.
Line 65 accepts "" or whitespace because it only checks is not None, which can produce invalid route-target names like :60004 or :60004.
Proposed fix
- site_asn = (context.get("data") or {}).get("site_asn")
- if site_asn is not None:
- return str(site_asn)
+ site_asn = (context.get("data") or {}).get("site_asn")
+ if site_asn is None:
+ continue
+ site_asn = str(site_asn).strip()
+ if site_asn:
+ return site_asn🤖 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 `@src/nv_config_manager/temporal/ngc/activities/nautobot.py` around lines 64 -
66, The site_asn handling in the route-target derivation path is accepting blank
or whitespace-only values, which can lead to invalid names. Update the logic in
the relevant activity helper in nautobot.py that reads site_asn from
context.get("data") so it only returns a value when site_asn is a non-empty,
non-whitespace string; otherwise fall through to the existing fallback behavior
before building the route target.
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.
Summary
Add a Spectrum-X L3 VXLAN panel to overlay detail page, show the linked VRF route distinguisher and import/export route targets and derive the Spectrum-X route target as
<site_asn>:<vni>from the active site config contextValidation
Kind integration was not run locally; this is a focused workflow-state fix covered by the deploy workflow suite.
Passing Kind Integration run:
Checklist
CONTRIBUTING.md.Summary by CodeRabbit