Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PVST Feature commit #3316

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

divyachandralekha
Copy link

Added new stpmgr for STP config handling
Added changes in orchagent for STP programming via SAI APIs

What I did
Added new stpmgr under cfgmgr for handling STP config notifications
Added STP changes in stporch component to handle STP operations (STP Instance creation/deletion, STP Port Add/Update/Del to STP instance) via SAI

Why I did it
PVST Feature support

@divyachandralekha divyachandralekha changed the title Pvst co pr PVST Feature enable Oct 4, 2024
@divyachandralekha divyachandralekha changed the title PVST Feature enable PVST Feature commit Oct 4, 2024
SWSS_LOG_ENTER();

if (stpGlobalTask == false)
stpGlobalTask = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please explain what is the role of this variable?

I see it being used in StpPortTask and other functions and return if this is not set. Is this to ensure that STP is enabled globally before its enabled on port - during config load?

If yes, then a better option would be to check the linux stp bridge state/some underlying resource which would be created in kernel and based on then decide whether to "defer & retry" in other task.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, It is to ensure sequence of the processing of STP config DB s.
I feel the kernel checks or interactions are more expensive than simply checking an in-memory variable.
Also repeatedly performing these checks can introduce performance overhead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stpGlobalTask should be done before any other stp tasks like vlan, port configurations are performed. Reason is, stpGlobalTask initializes the global stp bridge config, setsup stpmgr stack with all required bridge initialization config, so that per VLAN or Port can be added on top. Hence without this set , all other tasks are returned without action.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct; there will be no action on VLAN or port-related configurations when stpGlobalTask is not set. However, these requests will remain in the queue, and once stpGlobalTask is set, the VLAN and port-related configurations will be processed from the queue. This sequencing is necessary in the case of a device reboot with a saved configuration, as Redis-DB can be read in any order. Therefore, ensuring the correct sequence of events is essential.

@divyachandralekha
Copy link
Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

@muhammadalihussnain muhammadalihussnain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me. Please re trigger pipelines to pass the checks

@divyachandralekha
Copy link
Author

stp repo should be merged to resolve the compilation errors here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants