feat: Add port management endpoints and UI #48
Conversation
- Introduced a new Ansible playbook to check port details, including speed and status. - Created a new role for checking port details with tasks to retrieve port speed and status. - Updated the Port model to include an 'is_up' field for tracking port status. - Added serializers for the Port model, including a dedicated serializer for updating link speed. - Implemented a new PortViewSet to handle port-related API actions, including syncing port details from the switch. - Enhanced utility functions to extract port speed and status from Ansible playbook results. - Added a migration to incorporate the new 'is_up' field in the database.
…dit page - Added a new PortManagementComponent for managing bridge ports, including functionalities for editing link speed and syncing port details. - Integrated PortManagementComponent into the SwitchEditPage to allow users to manage ports directly from the switch edit interface. - Updated the devices API to support fetching and updating bridge port details. - Removed console log statements for cleaner production code. - Enhanced localization with new strings for port management features.
|
Caution Review failedThe pull request is closed. WalkthroughAdds end-to-end port management: Ansible playbook/role to read port speed/status, backend Port.is_up field, serializers/views/URLs for port updates and sync, Ansible result parsers and test runner, new frontend PortManagementComponent with API client/types, and i18n entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as "UI: Port Management"
participant API as "Backend API"
participant DB as "Database"
participant Ansible as "Ansible Controller"
participant Device as "Network Device"
UI->>API: PATCH /ports/{id}/ (link_speed)
API->>DB: Update Port.link_speed
DB-->>API: OK
API-->>UI: Updated Port
UI->>API: POST /ports/{id}/sync/
API->>Ansible: run check-port-details(playbook) for port
Ansible->>Device: read /sys/class/net/{port}/speed
Ansible->>Device: run `ip link show {port}`
Device-->>Ansible: speed & status output
Ansible-->>API: playbook results
API->>API: parse results (speed, status)
API->>DB: Update Port.link_speed & Port.is_up
DB-->>API: OK
API-->>UI: Updated Port (sync result)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to focus on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
ui/ui/src/components/network/NetworkDiagramComponent.tsx (1)
75-75: Remove commented-out code entirely.Rather than commenting out the console.log, delete it completely. Version control preserves the history if you need to reference it later, and removing it keeps the codebase cleaner.
Apply this diff:
- // console.log("[NETWORKDIAGRAMCOMPONENT] fetching network map"); - const fetchData = async () => {ui/ui/src/components/devices/SwitchEditComponent.tsx (1)
196-236: Consider simplifying the conditional rendering logic.The current nested conditionals create redundancy. The outer condition already verifies that at least one version field exists, making the inner field-level checks somewhat redundant for rendering purposes. Consider this more concise approach:
-{!!( - (switchData.ovs_version && - String(switchData.ovs_version).trim()) || - (switchData.openflow_version && - String(switchData.openflow_version).trim()) -) && ( +{(switchData.ovs_version || switchData.openflow_version) && ( <> - {switchData.ovs_version && - String(switchData.ovs_version).trim() && ( + {switchData.ovs_version && ( <div> <Label htmlFor="ovs_version"> {getT("components.devices.switch_edit.ovs_version")} </Label> <Input id="ovs_version" name="ovs_version" value={formData.ovs_version || ""} onChange={handleInputChange} disabled={!isEditing} /> </div> )} - {switchData.openflow_version && - String(switchData.openflow_version).trim() && ( + {switchData.openflow_version && ( <div> <Label htmlFor="openflow_version"> {getT( "components.devices.switch_edit.openflow_version" )} </Label> <Input id="openflow_version" name="openflow_version" value={formData.openflow_version || ""} onChange={handleInputChange} disabled={!isEditing} /> </div> )} </> )}This maintains the same behavior while being more readable and less verbose.
backend/control_center/general/serializers.py (1)
45-72: Enforce link_speed bounds at the serializer levelRight now any authenticated client can PATCH an arbitrary integer (negative, zero, or absurdly large) into
link_speed. The UI guards against this, but the API should defend itself so we don't persist nonsense values. Please add explicit bounds (matching the UI: 10–100000 Mb/s and allow null) on the serializer field.class PortUpdateSerializer(serializers.ModelSerializer): """ Serializer for updating port link_speed only. All other fields are read-only. """ + link_speed = serializers.IntegerField( + allow_null=True, + min_value=10, + max_value=100_000, + required=False, + ) class Meta: model = Port fields = ['id', 'name', 'ovs_port_number', 'link_speed', 'is_up']backend/control_center/ansible/roles/check-port-details/tasks/main.yml (2)
2-21: Validate theport_namevariable to prevent unexpected behavior or command injection.The
port_namevariable is templated directly into shell commands without explicit validation. While Jinja2 templating provides some protection, explicit validation of the port name (e.g., alphanumeric + hyphen/underscore) prevents unexpected behavior if invalid values are passed. Consider adding a validation task at the beginning of the role or document the expected format ofport_name.You can add a validation task at the start of the role:
- name: Validate port_name variable assert: that: - port_name is defined - port_name | regex_search('^[a-zA-Z0-9._:-]+$') fail_msg: "port_name must be alphanumeric with only dots, hyphens, underscores, and colons"
2-15: Consider using Ansible'sstatandslurpmodules instead of shell for better idempotency.The port speed retrieval uses a shell script with complex bash logic. Ansible's
statmodule can check file existence andslurpmodule can read file contents more idempotently. This improves portability and reduces shell-related risks.You can refactor as follows:
- name: Check if port speed file exists stat: path: /sys/class/net/{{ port_name }}/speed register: speed_file - name: Get port speed from sysfs slurp: src: /sys/class/net/{{ port_name }}/speed register: port_speed_output when: speed_file.stat.exists - name: Validate and set port speed set_fact: port_speed: "{{ (port_speed_output.content | b64decode | trim) if (port_speed_output.content | b64decode | trim) not in ('', '-1') else '' }}" when: speed_file.stat.exists
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
backend/control_center/ansible/playbooks/check-port-details.yml(1 hunks)backend/control_center/ansible/roles/check-port-details/tasks/main.yml(1 hunks)backend/control_center/general/migrations/0032_port_is_up.py(1 hunks)backend/control_center/general/models.py(1 hunks)backend/control_center/general/serializers.py(1 hunks)backend/control_center/general/urls.py(1 hunks)backend/control_center/general/views.py(3 hunks)backend/control_center/utils/ansible_formtter.py(1 hunks)backend/control_center/utils/test_playbook.py(3 hunks)ui/ui/src/app/devices/controllers/page.tsx(1 hunks)ui/ui/src/app/devices/switches/[id]/page.tsx(2 hunks)ui/ui/src/components/devices/PortManagementComponent.tsx(1 hunks)ui/ui/src/components/devices/SwitchEditComponent.tsx(1 hunks)ui/ui/src/components/network/NetworkDiagramComponent.tsx(1 hunks)ui/ui/src/lib/devices.ts(11 hunks)ui/ui/src/lib/types.ts(1 hunks)ui/ui/src/locales/en.json(2 hunks)ui/ui/src/locales/es.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
ui/ui/src/components/devices/PortManagementComponent.tsx (3)
backend/control_center/general/views.py (1)
ports(592-602)ui/ui/src/lib/types.ts (2)
BridgePort(146-152)BridgePortsResponse(154-156)ui/ui/src/lib/devices.ts (3)
fetchBridgePorts(260-269)updatePortLinkSpeed(271-281)syncPortDetails(283-290)
backend/control_center/utils/test_playbook.py (2)
backend/control_center/utils/ansible_formtter.py (3)
get_interface_speeds_from_results(54-96)get_single_port_speed_from_results(187-215)get_port_status_from_results(218-279)backend/control_center/utils/ansible_utils.py (3)
create_inv_data(56-61)create_temp_inv(47-53)run_playbook_with_extravars(145-212)
ui/ui/src/lib/devices.ts (2)
ui/ui/src/lib/types.ts (2)
BridgePortsResponse(154-156)BridgePort(146-152)ui/ui/src/lib/axiosInstance.ts (1)
createAxiosInstanceWithToken(26-48)
backend/control_center/general/urls.py (1)
backend/control_center/general/views.py (2)
SwitchViewSet(577-635)PortViewSet(639-747)
backend/control_center/utils/ansible_formtter.py (1)
backend/control_center/ovs_management/views.py (5)
get(63-114)get(118-177)get(204-231)get(239-355)get(358-376)
backend/control_center/general/serializers.py (1)
backend/control_center/general/models.py (4)
Meta(54-56)Meta(71-72)Meta(86-87)Port(78-94)
backend/control_center/general/views.py (4)
backend/control_center/general/serializers.py (3)
PortSerializer(38-41)PortUpdateSerializer(44-72)save(55-72)backend/control_center/utils/ansible_formtter.py (2)
get_single_port_speed_from_results(187-215)get_port_status_from_results(218-279)backend/control_center/general/models.py (1)
Port(78-94)backend/control_center/utils/ansible_utils.py (3)
create_inv_data(56-61)create_temp_inv(47-53)run_playbook_with_extravars(145-212)
🪛 Ruff (0.14.4)
backend/control_center/general/migrations/0032_port_is_up.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
backend/control_center/general/serializers.py
41-41: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
53-53: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
backend/control_center/general/views.py
622-622: Unused method argument: request
(ARG002)
622-622: Unused method argument: pk
(ARG002)
656-656: Unused method argument: pk
(ARG002)
678-678: Use explicit conversion flag
Replace with conversion flag
(RUF010)
678-678: Redundant exception object included in logging.exception call
(TRY401)
682-682: Unused method argument: request
(ARG002)
682-682: Unused method argument: pk
(ARG002)
746-746: Use explicit conversion flag
Replace with conversion flag
(RUF010)
746-746: Redundant exception object included in logging.exception call
(TRY401)
⏰ 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: Docker Build Validation
🔇 Additional comments (10)
ui/ui/src/app/devices/controllers/page.tsx (1)
56-56: LGTM! Clean production code.Commenting out the debug log is appropriate for production code.
ui/ui/src/locales/en.json (2)
515-519: LGTM! Clear and consistent translation keys.The new translation keys for version fields and port count are well-named and consistent with the existing structure.
592-617: LGTM! Comprehensive port management translations.The new
port_managementtranslation section is thorough and well-structured, covering all necessary UI strings including:
- Field labels and placeholders
- Status indicators
- Action buttons
- Success/error messages
- Validation constraints
backend/control_center/general/models.py (1)
84-84: LGTM! Well-documented tri-state port status field.The
is_upfield properly uses a nullable Boolean to represent three states (up/down/unknown), with clear help text documenting the semantics.backend/control_center/general/migrations/0032_port_is_up.py (1)
1-19: LGTM! Standard Django migration.The migration correctly adds the
is_upfield to the Port model with appropriate parameters. The static analysis hints aboutClassVarannotations are false positives—the current structure follows standard Django migration patterns.ui/ui/src/app/devices/switches/[id]/page.tsx (2)
26-26: LGTM! Clean component import.
150-154: LGTM! Proper component integration.The
PortManagementComponentis correctly integrated with appropriate props:
switchIdproperly typed and passedfetchDatacallback enables data refresh after updatesisLoadingcombines both switch and bridge loading statesbackend/control_center/general/urls.py (2)
3-3: LGTM! Clean import addition.
11-11: LGTM! Proper viewset registration.The
PortViewSetis correctly registered with the router, enabling RESTful endpoints for port operations at/ports/.backend/control_center/ansible/playbooks/check-port-details.yml (1)
1-6: No issues found—role path and host group are correctly configured.The role directory
backend/control_center/ansible/roles/check-port-details/exists and contains valid tasks. Thelocalserverhost group is properly configured throughout the codebase and dynamically generated by utility scripts. The playbook structure is sound andbecome: trueis appropriate for reading sysfs port information.
| - name: Debug port speed | ||
| debug: | ||
| var: port_speed_output.stdout | ||
|
|
||
| - name: Debug port status | ||
| debug: | ||
| var: port_status_output.stdout_lines |
There was a problem hiding this comment.
Remove debug tasks—development artifacts for production code.
The debug tasks are development/troubleshooting artifacts that conflict with the PR objective to remove console log statements for cleaner production code. These should be removed entirely or wrapped in a conditional variable (e.g., when: debug_mode | default(false)) if they're needed for occasional troubleshooting.
Apply this diff to remove the debug tasks:
-
-- name: Debug port speed
- debug:
- var: port_speed_output.stdout
-
-- name: Debug port status
- debug:
- var: port_status_output.stdout_linesAlternatively, if debugging support is desired:
-- name: Debug port speed
+- name: Debug port speed
+ when: debug_mode | default(false)
debug:
var: port_speed_output.stdout
-- name: Debug port status
+- name: Debug port status
+ when: debug_mode | default(false)
debug:
var: port_status_output.stdout_lines📝 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.
| - name: Debug port speed | |
| debug: | |
| var: port_speed_output.stdout | |
| - name: Debug port status | |
| debug: | |
| var: port_status_output.stdout_lines |
🤖 Prompt for AI Agents
In backend/control_center/ansible/roles/check-port-details/tasks/main.yml around
lines 23 to 29 there are two debug tasks outputting port_speed_output.stdout and
port_status_output.stdout_lines which are development artifacts and must be
removed for production; either delete these two debug tasks entirely, or wrap
them with a conditional such as adding when: debug_mode | default(false) so they
only run when debug_mode is explicitly enabled, and ensure no other references
rely on these debug tasks being present.
- Added a line normalization step to convert output lines to lowercase before processing flags, ensuring consistent handling of port status results.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/control_center/utils/ansible_formtter.py (2)
205-205: Simplify redundant empty string check.The check
speed_output != ''is redundant after the truthy checkspeed_output, since an empty string is falsy in Python.Apply this diff:
- if speed_output and speed_output != '': + if speed_output:
260-273: Past issue resolved; consider consistent flag checking.The stale
line_lowerissue from the previous review has been fixed—line 261 now correctly setsline_lower = line.lower()for each iteration before it's used at line 271.However, line 269 mixes checking
'up' in flags_list(membership) with'no-carrier' not in flags_lower(substring search). For consistency and precision, both checks should useflags_list.Apply this diff:
- if 'up' in flags_list and 'no-carrier' not in flags_lower: + if 'up' in flags_list and 'no-carrier' not in flags_list:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/control_center/utils/ansible_formtter.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/control_center/utils/ansible_formtter.py (1)
backend/control_center/ovs_management/views.py (5)
get(63-114)get(118-177)get(204-231)get(239-355)get(358-376)
🔇 Additional comments (1)
backend/control_center/utils/ansible_formtter.py (1)
187-280: Well-structured helper functions with appropriate error handling.Both
get_single_port_speed_from_resultsandget_port_status_from_resultsare well-implemented:
- Clear docstrings with explicit parameter and return type documentation
- Proper normalization of input to handle different calling patterns (line 199, 230)
- Robust error handling with try/except blocks and None returns for missing data
- Effective use of logging for debugging and warnings
- Correct handling of edge cases (NOT_FOUND ports, invalid speed values, missing state information)
The functions integrate cleanly with the existing codebase and provide targeted extraction for single-port queries, complementing the existing batch functions like
get_interface_speeds_from_results.
- Updated the condition for checking the 'no-carrier' flag to ensure it references the flags list instead of the lowercased string, improving accuracy in port status extraction.
1 participant
Summary by CodeRabbit
New Features
UI Improvements
Localization