Skip to content

Commit fd1e310

Browse files
Port configuration incremental update support (#985)
Due to historical reason, portsyncd and portmgrd both handle PORT table changes in CONFIG_DB and write APPL_DB according to configuration change. portmgrd handles fields including "mtu", "admin_status" and "learn_mode"; portsyncd handles all fields. There are a few issues here: portsyncd is designed to listen to kernel port status change and fire the change event to high level, it should not handle CONFIG_DB change. portmgrd is the right place to handle port configuration change according to SONiC architecture. Configuration change for "mtu", "admin_status" and "learn_mode" will be handled twice which is not necessary portsyncd put all configuration to APPL_DB even if user only changes part of them. E.g. user changes "speed" of the port, portsyncd will put configuration like "mtu", "admin_status", "autoneg" to APPL_DB. This is not necessary too. This HLD describes how to fix them.
1 parent 03976a9 commit fd1e310

File tree

1 file changed

+44
-21
lines changed

1 file changed

+44
-21
lines changed

doc/port_auto_neg/port-auto-negotiation-design.md

+44-21
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
| 0.1 | | Junchao Chen | Initial version |
4242
| 0.2 | | Junchao Chen | Fix review comment |
4343
| 0.3 | | Dante (Kuo-Jung) Su | Add support for SFPs and operational states|
44+
| 0.4 | | Junchao Chen | Port incremental configuration |
4445

4546
### Scope
4647
This document is the design document for port auto negotiation feature on SONiC. This includes the requirements, CLI change, DB schema change, DB migrator change, yang model change and swss change.
@@ -516,26 +517,39 @@ The new SONiC speed setting flow can be described in following pseudo code:
516517
port = getPort(alias)
517518
if autoneg changed:
518519
setPortAutoNeg(port, autoneg)
519-
520-
if autoneg == true:
521-
speed_list = vector()
522-
if adv_speeds changed or autoneg changed:
523-
// if adv_speeds == "all", leave speed_list empty which means all supported speeds
524-
if adv_speeds != "all":
525-
speed_list = adv_speeds
526-
setPortAdvSpeed(port, speed_list)
527-
528-
interface_type_list = vector()
529-
if adv_interface_types changed or autoneg changed:
530-
// if adv_interface_types == "all", leave interface_type_list empty which means all supported types
531-
if adv_interface_types != "all"
532-
interface_type_list = adv_interface_types
533-
setPortAdvInterfaceType(port, interface_type_list)
534-
else if autoneg == false:
535-
if speed changed or autoneg changed:
536-
setPortSpeed(port, speed)
537-
if interface_type changed or autoneg changed:
520+
if autoneg == on:
521+
// Due to incremental port configuration support, we might only get an "autoneg" field change from APP_DB.
522+
// In this case, we will need to apply previous saved adv_speeds and adv_interface_types to SAI. If there
523+
// is no previous configuration, will use default empty adv_speeds and adv_interface_types which indicates
524+
// all supported speeds and all supported interface types.
525+
setPortAdvSpeed(port, port.adv_speeds)
526+
setPortAdvInterfaceType(port, port.adv_interface_types)
527+
elif autoneg == off:
528+
// Due to incremental port configuration support, we might only get an "autoneg" field change from APP_DB.
529+
// In this case, we will need to apply previous saved speed and interface_type to SAI. Speed is a mandatory configuration.
530+
// If there is no previous configuration for interface_type, it shall use SAI_PORT_INTERFACE_TYPE_NONE by default.
531+
setPortSpeed(port, port.speed)
532+
setInterfaceType(port, port.interface_type)
533+
534+
if adv_speeds changed:
535+
if autoneg == on:
536+
setPortAdvSpeed(port, adv_speeds) // empty adv_speeds means all supported speeds
537+
port.adv_speeds = adv_speeds
538+
539+
if adv_interface_types changed:
540+
if autoneg == on:
541+
setPortAdvInterfaceType(port, adv_interface_types)
542+
port.adv_interface_types = adv_interface_types
543+
544+
if speed changed:
545+
if autoneg != on:
546+
setPortSpeed(port, speed) // for autoneg is off/not_set, apply the speed to SAI, this is for backward compatible
547+
port.speed = speed
548+
549+
if interface_type changed:
550+
if autoneg == off:
538551
setInterfaceType(port, interface_type)
552+
port.interface_type = interface_type
539553
```
540554

541555
##### Getting Remote Advertisement
@@ -558,9 +572,18 @@ swss will do validation for auto negotiation related fields, although it still C
558572

559573
#### portsyncd and portmgrd Consideration
560574

561-
No changes for portsyncd and portmgrd.
575+
Due to historical reason, portsyncd and portmgrd both handle **PORT** table changes in **CONFIG_DB** and write **APPL_DB** according to configuration change. portmgrd handles fields including "mtu", "admin_status" and "learn_mode"; portsyncd handles all fields. There are a few issues here:
576+
577+
1. portsyncd is designed to listen to kernel port status change and fire the change event to high level, it should not handle **CONFIG_DB** change. portmgrd is the right place to handle port configuration change according to SONiC architecture.
578+
2. Configuration change for "mtu", "admin_status" and "learn_mode" will be handled twice which is not necessary
579+
3. portsyncd put all configuration to **APPL_DB** even if user only changes part of them. E.g. user changes "speed" of the port, portsyncd will put configuration like "mtu", "admin_status", "autoneg" to **APPL_DB**. This is not necessary too.
580+
581+
To address these issues, a few changes shall be made:
562582

563-
Due to historical reason, portsyncd and portmgrd both handle **PORT** table changes in **CONFIG_DB** and write **APPL_DB** according to configuration change. portmgrd handles fields including "mtu", "admin_status" and "learn_mode"; portsyncd handles the rest fields.
583+
1. portsyncd no longer listen to **CONFIG_DB** changes
584+
2. portmgrd shall be extended to handle all port configuration changes
585+
3. portmgrd shall implement incremental configuration update. It means that portmgrd shall not put configuration to **APPL_DB** if the field is not changed compare to previous value.
586+
4. portsorch shall be changed to handle incremental port configuration changes
564587

565588
#### Port Breakout Consideration
566589

0 commit comments

Comments
 (0)