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

PVSTP feature implementation. #1058

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

Conversation

ph408077
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 portsorch component to handle STP operations (STP Instance creation/deletion, STP Port Add/Update/Del to STP instance) via SAI

Why I did it
To support PVSTP feature

How I verified it
Spytest automation and also manual verification on VS

Details if related

- Added new stpmgr for STP config handling
- Added changes in orchagent for STP programming via SAI APIs
@@ -3378,6 +3549,336 @@ bool PortsOrch::removeAclTableGroup(const Port &p)
return true;
}


sai_object_id_t PortsOrch::getStpInstanceOid(sai_uint16_t stp_instance)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding STP functions to portsorch has considerably increased the file size. I think STP functions should be handled separately and not overload portsorch. Like an stporch.cpp

Choose a reason for hiding this comment

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

Hi Prince, Considering STP has limited data and is part of vlan and port we included the code as part of portsorch. This has undergone extensive testing and might be risky to change at this point of time. We have additional spanning-tree features in the next release as part of which we will move this to separate file, hope thats acceptable. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to disagree. Portsorch is not intended to handle protocol aspects and this is an overload. It is better to address/finalize this now than a later churn.

Copy link
Author

Choose a reason for hiding this comment

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

Separated STP changes from portsorch. Please review the latest commit/changes

- Separated STP changes from portsorch
@ph408077
Copy link
Author

Build failure is because of dependency in sonic-swss-common:

orchdaemon.cpp: In member function 'bool OrchDaemon::init()':
orchdaemon.cpp:91:13: error: 'APP_STP_VLAN_INSTANCE_TABLE_NAME' was not declared in this scope
APP_STP_VLAN_INSTANCE_TABLE_NAME,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
orchdaemon.cpp:92:13: error: 'APP_STP_PORT_STATE_TABLE_NAME' was not declared in this scope
APP_STP_PORT_STATE_TABLE_NAME,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
orchdaemon.cpp:93:13: error: 'APP_STP_FASTAGEING_FLUSH_TABLE_NAME' was not declared in this scope
APP_STP_FASTAGEING_FLUSH_TABLE_NAME
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
orchdaemon.cpp:94:5: error: could not convert '{, , }' from '' to 'std::vector<std::__cxx11::basic_string >'
};

@lguohan
Copy link
Contributor

lguohan commented Oct 25, 2019

need vs test.

orchagent/Makefile.am Outdated Show resolved Hide resolved
orchagent/stporch.cpp Outdated Show resolved Hide resolved
orchagent/stporch.cpp Show resolved Hide resolved
orchagent/stporch.cpp Show resolved Hide resolved
@ByReaL
Copy link

ByReaL commented Jan 28, 2020

Spytest automation

would you be able to share a link to "spytest automation" please

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
Previously log rotate was performed after request and on first log line
that was recorded which caused delay and issues with syslog logrotate
when sending HUP signal, actual log rotate was not performed and handle
to a sairedis.rec was sill open preventing logrotate from happening.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants