[chassis] add the scheduler config only on ports which are admin-up #24603
Conversation
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR modifies the QoS configuration template for VoQ chassis to conditionally apply scheduler configuration only on admin-up ports, addressing reference count issues during port speed changes (e.g., 400g to 100g).
Key Changes
- Introduces
SYSTEM_PORT_ACTIVEarray to track admin-up ports based onDEVICE_NEIGHBORpresence - Conditionally adds scheduler config for queues 3 and 4 on admin-up ports only
- Changes queues 0, 1, 2, 5, 6 to only be generated for admin-up ports instead of all ports
- Updates sample QoS output files to reflect the new configuration structure
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| files/build_templates/qos_config.j2 | Modified template to track active ports and conditionally generate scheduler configs based on port admin status |
| src/sonic-config-engine/tests/sample_output/py3/qos-nokia-ixr7250e-36x400g.json | Updated sample output showing scheduler configs only on active ports, with field reordering |
| src/sonic-config-engine/tests/sample_output/py3/qos-nokia-ixr7250e-36x100g.json | Updated sample output removing scheduler entries for inactive ports and non-lossless queues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
files/build_templates/qos_config.j2
Outdated
| "scheduler": "scheduler.1", | ||
| {% endif -%} | ||
| "wred_profile": "AZURE_LOSSLESS" | ||
| }, |
There was a problem hiding this comment.
This change creates a potential JSON syntax error. The loop at line 519 starts iterating over SYSTEM_PORT_ALL, but the closing {% endfor %} is missing before the next loop at line 526 starts. There should be a {% endfor %} after line 525 to properly close the queue|4 loop before starting the queue|0 loop.
| }, | |
| }, | |
| {% endfor %} |
| "{{ system_port }}|3": { | ||
| "scheduler" : "scheduler.1", | ||
| {% if system_port in SYSTEM_PORT_ACTIVE -%} | ||
| "scheduler": "scheduler.1", | ||
| {% endif -%} | ||
| "wred_profile": "AZURE_LOSSLESS" | ||
| }, |
There was a problem hiding this comment.
The conditional scheduler inclusion in queues 3 and 4 may produce invalid JSON when system_port is not in SYSTEM_PORT_ACTIVE. In such cases, the JSON would be {"wred_profile": "AZURE_LOSSLESS"} with a trailing comma from line 517 (,), creating invalid JSON. Consider restructuring to ensure valid JSON is always generated, e.g., by making the comma conditional or always including a scheduler field.
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| {%- set PORT_UPLINK = [] %} | ||
| {%- set PORT_DOWNLINK = [] %} | ||
| {%- set PORT_SERVICE = [] %} | ||
| {%- set SYSTEM_PORT_ACTIVE [] %} |
There was a problem hiding this comment.
Missing equals sign in list initialization. The syntax should be {%- set SYSTEM_PORT_ACTIVE = [] %} instead of {%- set SYSTEM_PORT_ACTIVE [] %}.
| {%- set SYSTEM_PORT_ACTIVE [] %} | |
| {%- set SYSTEM_PORT_ACTIVE = [] %} |
| }, | ||
| {% endfor %} | ||
| {% for system_port in SYSTEM_PORT_ALL %} | ||
| }{% if not loop.last %},{% endif %} |
There was a problem hiding this comment.
Missing comma after the closing brace. This will result in invalid JSON syntax. Should be }, to maintain proper JSON array structure.
2 participants
Why I did it
To avoid having ref count issues when the port speed is change from 400g to 100g we have to generate the scheduler only on the ports which are admin-up
Work item tracking
How I did it
add the scheduler config only on ports which are admin-up. Conditionally applies scheduler config based on admin status, updating qos_config.j2 and regenerating QoS sample output JSONs
How to verify it
UT
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)