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

skip generating auto_restart.conf for syncd #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

skip generating auto_restart.conf for syncd #51

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
4 changes: 4 additions & 0 deletions scripts/hostcfgd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,10 @@ class FeatureHandler(object):
# On multi-ASIC device, creates systemd configuration file for each feature instance
# residing in difference namespace.
for feature_name in feature_names:
# As to syncd service, swss will monitor its stop and restart it, don't need systemd to auto-restart it.
if "syncd" in feature_name:
Copy link
Contributor

Choose a reason for hiding this comment

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

This change will permanently disable syncd auto restart, right?

Intention for this change stems from the fact that swss restart will auto-trigger syncd restart .
Therefore, there should be a verification of such a dependency before disabling auto restart.

I think better design will be that syncd auto restart sequence checks (at runtime) the dependency of swss, and then aborts the syncd auto restart. Benefits:

Any future change on swss dependency will not break syncd autorestart logic as the check for dependency will happen at runtime and not at image build time.
So if syncd is removed from dependent service list then yncd autorestart will still work.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @prsunny , @yxieca for awareness.

Copy link
Author

@yanjundeng yanjundeng Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@vaibhavhd vaibhavhd, Currently, the dependency between syncd and swss is defined in swss.sh. Now the unnecessary auto-restart of syncd is triggered by systemd. And the swss also use systemctl to re-start syncd. Seems we could not differentiate these two auto-restart from syncd service's definition. Any idea to check the dependency and block one at runtime?

BTW, do we have any plan to remove the dependency between SWSS and syncd in future? If we could remove their dependency in future, for example, we could just restart syncd only, or just restart swss only. If we could make it, we must be able to ensure the ASIC DB is consistent and correct during these process. If so, it's safe to add the auto-restart flag back for syncd in future.

In fact, the current issue in syncd we hit is kinda like that we have removed the dependency. During that short window, the syncd is auto-restarted by systemd and it still tries to use the legacy ASIC_DB during startup.

Copy link
Author

Choose a reason for hiding this comment

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

Why we let swss take charge of auto-restart of syncd here? That's because before swss start syncd and other services, it always flush DB at first. So that the syncd's restart could get a clear ASIC DB.

Copy link
Contributor

Choose a reason for hiding this comment

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

Syncd and swss are designed to start/stop together due to they needs to be synchronized.

However, we do let the swss to take more control than syncd from day 1.

In this sense, I think syncd auto-stop is sufficient as long as swss can pick up the signal (by docker wait any) and restart both services.. @prsunny do you have concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yanjundeng do you know who added the syncd auto restart after 202012? Let's get the author of that change also chime in here.

continue

syslog.syslog(syslog.LOG_INFO, "Updating feature '{}' systemd config file related to auto-restart ..."
.format(feature_name))
feature_systemd_config_dir_path = self.SYSTEMD_SERVICE_CONF_DIR.format(feature_name)
Expand Down