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.

Sign up for GitHub

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

[config] Do not stop or restart dependent services when reloading config #582

Merged
merged 1 commit into from
Jul 25, 2019
Merged

[config] Do not stop or restart dependent services when reloading config #582

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions config/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,13 +288,10 @@ def _abort_if_false(ctx, param, value):

def _stop_services():
services = [
'dhcp_relay',
'swss',
'snmp',
'lldp',
'pmon',
'bgp',
Copy link
Collaborator

@nazariig nazariig Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

@jleveque jleveque Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jleveque If swss service restarts in warm mode does it restart teamd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jleveque I believe this breaks swss service level warm restart because teamd will recreate portchannels. Is it going to be fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yxieca / @pavel-shirshov to comment on teamd warm restart.

Copy link
Contributor

@yxieca yxieca Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@jleveque teamd is not a dependent of swss from what I see in current code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

'teamd',
'hostcfgd',
]
for service in services:
Expand All @@ -312,11 +309,8 @@ def _restart_services():
'rsyslog-config',
'swss',
'bgp',
'teamd',
'pmon',
'lldp',
'snmp',
'dhcp_relay',
'hostcfgd',
]
for service in services:
Expand Down