Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
docs: add usage guidelines for systemd targets, services and directives #240
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
base: main
Are you sure you want to change the base?
docs: add usage guidelines for systemd targets, services and directives #240
Changes from all commits
aace166a01eca7c5ef779File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
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.
Also, the "directions" of Wants/Requires and WantedBy/RequiredBy are opposite and it may only be appropriate/correct to change "one side".By this do you mean that we should only be having the relationship defined one way, correct? What I mean is that if unit A requires unit B. We would define in systemd either
But we shouldn't have both? I don't disagree with this statement.
Maybe we should clarify which one is preferred. from my understanding you're saying here we probably prefer RequiredBy.
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.
It's ok, what isn't this days :)
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.
Another reason for using barrier services is to make failure handling more explicit and resilient. If a target directly depends on all of its worker services, a single worker failure can prevent the target from being reached and stop the rest of the flow. With a barrier, the target can remain the stable stage boundary, while the barrier owns the responsibility of running the stage services, collecting their status, and deciding how failure is represented. This allows later stages, especially reporting, to still be reached so failures can be captured instead of skipping directly to shutdown or leaving the run incomplete.
Uh oh!
There was an error while loading. Please reload this page.
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.
I don't know if we need the explanation, but the reason After= does not behave as expected with non-oneshot services is that After= only waits for initialization, not completion. Since long-running services do not have a completion state, once the service initializes successfully, systemd considers it safe to start dependent services.
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.
I like this suggestion
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.
@copilot everything else can and does use Wants or Requires. The sev-certify stop targets boot strap this process.
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.
@copilot to me, RemainAfterExit=yes seems to fit sev-certify better and there doesn't seem to be a downside other than maybe salvaging more of a "bad boot".
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.
I have a few comments on this section.
First, I don't think TimeoutStartSec is necessarily the right criteria to decide between Type=oneshot and Type=simple. None of our current services rely on TimeoutStartSec, and I don't think service type selection should be driven by whether we can determine a timeout value.
Instead, I think the recommendation should be based on service behavior. For most of our services, Type=oneshot is the better fit because they are intended to perform finite work and run to completion once. I expect that to apply to the majority of services in this project.
Also, the main benefit of Type=oneshot in our architecture is ordering semantics. After= and Before= only wait for service activation, not completion. For long-running services (Type=simple), dependencies are satisfied as soon as the service initializes successfully. With Type=oneshot, ordering is only satisfied once the main process exits, which aligns better with our stage-based execution model.
RemainAfterExit=yes serves a different purpose. It keeps the service in the active state after execution has completed, allowing the unit to represent that a stage has finished. That is why barrier services use it by definition—they act as completion markers for all work associated with that target.
Without RemainAfterExit=yes, barrier services immediately transition to inactive after completion, which can lead to them being retriggered when revisiting target dependencies later in the flow.
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.
@copilot It's possible that the code isn't compliant with the guidelines. The code that I believe you're thinking of here was committed and merged before this PR was opened.
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.
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.
same comment as above. While it's true that guidelines are in flux, the sev-certify code being thought about here may not end up being compliant and isn't compliant with this version/commit of the guidelines.
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 have an example of where we use KillMode?
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.
Same thing, I don't know if we use this at all
Uh oh!
There was an error while loading. Please reload this page.