[yang-models] Add YANG model for SYSTEM_PORT#12689
[yang-models] Add YANG model for SYSTEM_PORT#12689qiluo-msft merged 6 commits intosonic-net:masterfrom
Conversation
|
@rlhui for awareness |
kenneth-arista
force-pushed
the
master-system-port-yang
branch
2 times, most recently
from
de3d1a6 to
2d93191
Compare
|
@arlakshm @praveen-li would you please help to review this PR? Thanks. |
@arlakshm @praveen-li kindly ping. |
praveen-li
left a comment
praveen-li
left a comment
There was a problem hiding this comment.
Looks good, barring few questions\doubts. Thanks.
There was a problem hiding this comment.
SYSTEM_PORT_WRONG_SPEED_CONFIG -> SYSTEM_PORT_WRONG_SPEED_PATTERN
Preferred if test name has condition being tested.
There was a problem hiding this comment.
kindly point to the source of truth for config, e.g. HLD, Orchagent processing functions etc. Thx
There was a problem hiding this comment.
Please see PortsOrch::addSystemPorts() in sonic-swss/orchagent/portsorch.cpp
There was a problem hiding this comment.
Suggestion: any constraints needed ? e.g. asic[0-9].
There was a problem hiding this comment.
Does it need to be a leafref to PORT\Interface?
There was a problem hiding this comment.
Yes, I can use a leafref. I had it as follows previously but ran into a build problem. Let's try again.
There was a problem hiding this comment.
I encountered the same build failure; e.g.
ERROR:YANG-TEST: Exception >Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "Ethernet0" points to a non-existing leaf. in not empty< in /sonic/src/sonic-yang-models/tests/yang_model_tests/test_yang_model.py:208
There was a problem hiding this comment.
You need to add Ethernet0 in your json config. For example:
https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/tests/yang_model_tests/tests_config/lldp.json#L82-L109
There was a problem hiding this comment.
8-bits give us flexibility to support more cores in future ASICs. Admittedly, 255 cores is not likely. If you prefer, I can constrain to maximum 8 cores as so:
type uint8 {
range 0..7;
}
There was a problem hiding this comment.
Yes, similar to core_index we would like flexibility to support logical indices that can exceed 8 bits.
There was a problem hiding this comment.
I am happy to constrain this. The number of VOQs per port is typically 8 but can be more. I'll constrain this to between 1..8 for now.
kenneth-arista
force-pushed
the
master-system-port-yang
branch
from
2d93191 to
d3c0c2e
Compare
|
@praveen-li @kenneth-arista do we still have open questions for this PR? Or just need to fix build failure? Thanks. |
|
I don't have any open questions. /azpw run Azure.sonic-buildimage |
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Can you add description? And what's the unit? kbps?
There was a problem hiding this comment.
A VOQ is a Virtual Output Queue. Typically one VOQ is allocated per traffic class per port. The unit is the number of queues per port. For example, 8 corresponds to 8 VOQs per port. This number is not related to the speed or a port.
There was a problem hiding this comment.
Then why do we have speed for VOQ?
There was a problem hiding this comment.
This is a YANG model for a system port and not for a VOQ. A system port has many attributes, some of which are core_index (which core the port is associated with in a multicore ASIC), number of VOQs associated with the port, and the speed of the port.
Are you asking about this description Number of VoQs associated with a port? Or are you asking about the description for leaf speed?
There was a problem hiding this comment.
I'm asking about leaf speed.
There was a problem hiding this comment.
Oops. I meant to say the units are in kbps. Max speed for future compatibility is 800Gbps.
There was a problem hiding this comment.
If unit is kbps, max speed is 800Mbps. Plese include unit in description.
There was a problem hiding this comment.
Sorry I'm not thinking straight. The listed range is 1..800000. So, units are million bits per second since 800,000 mbps == 800,000,000,000 bps.
There was a problem hiding this comment.
Ok, please add description with unit.
|
@kenneth-arista would you please update doc/Configuration.md? |
|
@kenneth-arista |
|
Hi @ganglyu, I see a number of failures in For example, |
We have fixed a related issue, would you please merge the latest code? |
Add YANG model for SYSTEM_PORT. Signed-off-by: Kenneth Cheung <[email protected]>
Revert leafref for ifname because of build failure.
- ifname should be a leafref - adding missing port info in json test_config - add missing description for speed attribute
kenneth-arista
force-pushed
the
master-system-port-yang
branch
from
0fe0dc9 to
9554f70
Compare
|
@kenneth-arista
|
System ports also includes Cpu0 ports, which are not defined in port_config.ini and thus the YANG was modified accordingly.
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@praveen-li are you ok to approve this PR? Thanks. |
praveen-li
left a comment
praveen-li
left a comment
There was a problem hiding this comment.
Thx for contribution. Looks good to me now.
|
Can we get this PR back ported to 202205 if needed? @arlakshm |
arlakshm
added
the
Chassis for 202205 branch
gechiang
added
the
Included in Chassis for 202205 Branch
Add YANG model for SYSTEM_PORT. Resolves sonic-net#12458 YANG model for SYSTEM_PORT in CONFIG_DB was missing. Added new YANG model and associated unit tests. Passing unit tests
[yang-models] Add YANG model for SYSTEM_PORT (sonic-net#12689)
9 participants
Add YANG model for SYSTEM_PORT.
Resolves #12458
Signed-off-by: Kenneth Cheung [email protected]
Why I did it
YANG model for SYSTEM_PORT in CONFIG_DB was missing.
How I did it
Added new YANG model and associated unit tests.
How to verify it
Passing unit tests
Which release branch to backport (provide reason below if selected)