-
Notifications
You must be signed in to change notification settings - Fork 547
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
[mtu] Update the mtu on host interfaces if config DB changes #375
Conversation
portsyncd/portsyncd.cpp
Outdated
{ | ||
char system_cmd[1024]; | ||
snprintf(system_cmd, sizeof(system_cmd), "ip link set %s mtu %s", key.c_str(), value.c_str()); | ||
int ret_val = system(system_cmd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I've seen the usage of swss::exec to execute commands instead of direct system call. Can you check? Also, you could use stringstream for the command which would be convenient instead of allocating a buffer and then use snprintf
You could refer: IntfMgr::setIntfIp in intfmgr.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Sunny.
Will check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @prsunny ,
Just updated the diff with exec. Thanks for your suggestion.
Please help review the latest diff.
Cheers,
Haiyang
Signed-off-by: Haiyang Zheng <haiyang.z@alibaba-inc.com>
Hi @prsunny , Just updated the diff with exec. Thanks for your suggestion. Please help review the latest diff. Cheers, |
{ | ||
string cmd, res; | ||
cmd = "ip link set " + key + " mtu " + value; | ||
swss::exec(cmd, res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to check for return value?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sunny @prsunny ,
The return value will be checked inside swss::exec, if failed swss::exec will throw an error.
And most places calling swss::exec don't check the res except those show cmd.
some test log for the failure case.
Nov 9 02:45:07.294323 sonic-1 ERR portsyncd: :- exec: ip link set Ethernet8 mtu 9800: Success
Thanks,
Haiyang
…net#375) "Ethernet0|4": { "wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]", "scheduler": "[SCHEDULER|scheduler.0]" }, Signed-off-by: Wenda <wenni@microsoft.com>
* add missing object types * refactor stats to be generic
Signed-off-by: Haiyang Zheng haiyang.z@alibaba-inc.com
What I did
Now, portsyncd is subscribed to config DB, and use handlePortConfig to copy port config from config DB to APP DB. Here, we also need to update the host interfaces MTU in linux kernel if the port config changes.
Why I did it
Otherwise, there will be discrepancy between linux kernel MTU and port MTU in config DB/app DB.
How I verified it
Update the port inside config_db.json file with new mtu field, and verify both kernel MTU and ASIC port MTU has been updated properly.
Details if related
This pull request require the following pull request
sonic-net/sonic-buildimage#1128
With both changes, the MTU from config DB will be the only source for Kernel MTU and port MTU on ASIC.