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

[intfsorch]: Add setRouterIntfsMtu function #574

Merged
merged 1 commit into from
Aug 14, 2018

Conversation

stcheng
Copy link
Contributor

@stcheng stcheng commented Aug 10, 2018

This function allows the propagation of MTU from port MTU to router
interface MTU if the router interface is created based on this port.

Once the port MTU gets changes, the router interface MTU will also
get changed.

Signed-off-by: Shu0T1an ChenG [email protected]

@@ -1532,6 +1534,10 @@ void PortsOrch::doPortTask(Consumer &consumer)
p.m_mtu = mtu;
m_portList[alias] = p;
SWSS_LOG_NOTICE("Set port %s MTU to %u", alias.c_str(), mtu);
if (!p.m_rif_id)
{
gIntfsOrch->setRouterIntfsMtu(p, mtu);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need to pass the value? Isnt it the same as p.m_mtu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching this. i'll update it.

port.m_alias.c_str(), mtu, status);
return true;
}
SWSS_LOG_NOTICE("Set router interface %s MTU to %u",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be INFO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'd like to keep it as NOTICE since it's possible to change port channel MTUs or VLAN mtus and we'd like to know the corresponding MTU changes for the router interfaces.

{
SWSS_LOG_ERROR("Failed to set router interface %s MTU to %u, rv:%d",
port.m_alias.c_str(), mtu, status);
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

return False.

}
SWSS_LOG_NOTICE("Set router interface %s MTU to %u",
port.m_alias.c_str(), mtu);
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

return true

@@ -1532,6 +1534,10 @@ void PortsOrch::doPortTask(Consumer &consumer)
p.m_mtu = mtu;
m_portList[alias] = p;
SWSS_LOG_NOTICE("Set port %s MTU to %u", alias.c_str(), mtu);
if (!p.m_rif_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

if (p.m_rif_id) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. updated.

@lguohan
Copy link
Contributor

lguohan commented Aug 10, 2018

code looks good. please add vs test.

@jipanyang
Copy link
Contributor

Change looks good to me, now the ethernet port router intf mtu setting should work as before during system init.

Just curious, since in https://github.com/Azure/sonic-swss/pull/545/files you also have lag mtu setting, are you going to address that in orchagent for lag mtu in separate PR?

@stcheng
Copy link
Contributor Author

stcheng commented Aug 10, 2018

@jipanyang yes. since the current implementation also doesn't support LAG MTU changes. I'll do it in the separate PRs.

This function allows the propagation of MTU from port MTU to router
interface MTU if the router interface is created based on this port.

Once the port MTU gets changes, the router interface MTU will also
get changed.

Update the VS unit test test_InterfaceChangeMtu case. It will check
the data flow from application database to ASIC database.

Signed-off-by: Shu0T1an ChenG <[email protected]>

tbl = swsscommon.ProducerStateTable(pdb, "PORT_TABLE")
fvs = swsscommon.FieldValuePairs([("mtu", "8888")])
tbl.set("Ethernet8", fvs)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to configure ethernet8 into rif or it is already in rif.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the first test case when the ip is assigned, the associated rif is created

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@stcheng stcheng merged commit 4eeecfd into sonic-net:master Aug 14, 2018
@stcheng stcheng deleted the routerintfs branch August 14, 2018 16:57
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.

5 participants