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

Do basic mutual exclusion check on vlan member and router interface c… #770

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

Conversation

jipanyang
Copy link
Contributor

…onfig for same port

Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com

What I did
Eventually cfgmgr should prevent such configuration being pushed to appDB. Here just to do basic preventive check in orchagent.

Why I did it
To avoid wrong configuration from messing up orchagent state handling.

How I verified it
VS
Details if related

…onfig for same port

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID)
{
SWSS_LOG_ERROR("Router Intfs config on vlan member %s is not allowed", alias.c_str());
it = consumer.m_toSync.erase(it);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should block and discard the configuration. Instead, why don't we retry here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The concern is that performing retry here will introduce too many error logs and extra system overhead if the error is persistent for a long time. While lowering the log level will leave the error unnoticed most likely.

@@ -1972,6 +1972,13 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer)
continue;
}

if (port.m_rif_id)
{
SWSS_LOG_ERROR("Vlan config on router interface %s is not allowed", port.m_alias.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

prsunny
prsunny previously approved these changes Mar 21, 2019
Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Config validation handles this scenario. Approving as this can temporarily resolve the transient issues.

@lguohan
Copy link
Contributor

lguohan commented Mar 23, 2019

it is strange that this PR changes vnet test cases.

@lguohan
Copy link
Contributor

lguohan commented Mar 23, 2019

The test case should be added to check the syslog error message for invalid configuration. I do not see such test cases added.

Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

The test case should be added to check the syslog error message for invalid configuration. I do not see such test cases added.

@jipanyang
Copy link
Contributor Author

it is strange that this PR changes vnet test cases.

Before the change "100.102.1.1/24" was configured on "Ethernet4" first, then the second test case tries to add Ethernet4 to Vlan1002. It hit the exact problem of one port being put in both rif and vlan.

@lguohan
Copy link
Contributor

lguohan commented Aug 1, 2019

retest this please

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
sonic-net#770)

* A generic JSON file updater, which can add/update-existing attributes.
This tool would be used to update /etc/sonic/core_analyzer.rc.json file to add
credentials by HW proxy.

* Updated per review comments.
The option is better named.
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
Missing MACsec Create Port action in SwitchStateBase::create. This bug will cause MACsec SC Crate action fail.

Signed-off-by: Ze Gan <ganze718@gmail.com>
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.

4 participants