[config] Do not stop or restart dependent services when reloading config #582
Conversation
jleveque
changed the title
| 'swss', | ||
| 'snmp', | ||
| 'lldp', | ||
| 'pmon', | ||
| 'bgp', |
There was a problem hiding this comment.
@jleveque can we also add swss.service dependency for bgp/lldp and eventually have similar behaviour as for teamd? The idea here is to have network and platform parts separated and fully managed by systemd. What do you think?
There was a problem hiding this comment.
@nazariig: Good suggestions. I'm all in favor of simplifying the service management. I opened a PR to add the same dependency for BGP here (sonic-net/sonic-buildimage#3078), but as you can see from the review comments, it appears there are corner cases, so we will need to think about a more intricate approach.
Regarding LLDP, it's not as imperative that we restart it along with SwSS. Now that we have lldpmgrd, anytime SwSS restarts it should get notified of port state changes and configure LLDP appropriately. It's not as critical as dhcp_relay or radv which have no way of being updated to port changes, but the dependency wouldn't hurt and would help simplify the service management, so I'd like to get @lguohan's opinion on this.
Update: Discussed with Guohan and confirmed that the eventual goal is to remove dependencies altogether, and make services independent as much as possible. As I mentioned above, with the addition of lldpmgrd, the LLDP service is no longer dependent on SwSS. The LLDP service can continue running while restarting SwSS. lldpmgrd will receive notifications when port statuses change, and it will reconfigure LLDP as appropriate. Thus, we should also be able to remove lldp from these lists, and not add it as a dependency of SwSS. I will work on this in a separate PR.
There was a problem hiding this comment.
@jleveque If swss service restarts in warm mode does it restart teamd?
There was a problem hiding this comment.
As long as there is a call to systemctl [re]start swss, teamd and the other services which are configured with the same dependency on swss will also be [re]started.
There was a problem hiding this comment.
@jleveque I believe this breaks swss service level warm restart because teamd will recreate portchannels. Is it going to be fixed?
There was a problem hiding this comment.
@yxieca / @pavel-shirshov to comment on teamd warm restart.
There was a problem hiding this comment.
@jleveque teamd is not a dependent of swss from what I see in current code.
There was a problem hiding this comment.
Discussed offline. teamd is still dependent on swss. We will need a more elegant dependency solution in sonic-buildimage to handle the warm-restart case. But this PR should still be valid.
|
Retest this please |
|
@lguohan please have a look |
|
Retest this please |
1 similar comment
|
Retest this please |
|
@yxieca: Now that sonic-net/sonic-buildimage#2852 has been merged into the 201811 branch, this PR should also be cherry-picked into the 201811 branch. Hopefully it will pick cleanly. |
- What I did
Remove explicit calls to stop/restart services which are now dependent on SwSS.
As of sonic-net/sonic-buildimage#2546, teamd, snmp and dhcp_relay services will be stopped/started/restarted along with swss when it is stopped/started/restarted, respectively.
This prevents issues due to double restarts (e.g., this script would call
systemctl restart swss, which would now cause teamd to restart, then shortly thereafter it would callsystemctl restart teamd, which could lead to instability).Now, stopping/starting/restarting swss will also stop/start/restart teamd, snmp, dhcp_relay and radv services, so we should no longer be stopping/starting/restarting them along with swss.