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.

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

wrappers/services: RemainAfterExit=yes for oneshot daemons w/ stop cmds #2920

Merged
merged 4 commits into from Feb 23, 2017

Conversation

ssweeny
Copy link
Contributor

@ssweeny ssweeny commented Feb 22, 2017

systemd complains if you generate a service file for a oneshot daemon
that also includes an ExecStop directive but doesn't specify
RemainAfterExit=yes. systemd needs this keep track of the service after
the command exits.

Also adds a test for this and fixes a typo in the opening comment of the generated systemd unit file.

Fixes: https://bugs.launchpad.net/snappy/+bug/1647169

Scott Sweeny added 2 commits February 22, 2017 15:26
systemd complains if you generate a service file for a oneshot daemon
that also includes an ExecStop directive but doesn't specify
RemainAfterExit=yes. systemd needs this keep track of the service after
the command exits.

Also adds a test for this.

Fixes: https://bugs.launchpad.net/snappy/+bug/1647169
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's get a second review before landing this. I've requested a small change for testing but consider this approved with that change in place.

@@ -170,7 +170,7 @@ func RemoveSnapServices(s *snap.Info, inter interacter) error {

func genServiceFile(appInfo *snap.AppInfo) string {
serviceTemplate := `[Unit]
# Auto-generated, DO NO EDIT
# Auto-generated, DO NOT EDIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :-)

@@ -204,6 +205,17 @@ WantedBy={{.ServicesTarget}}
restartCond = systemd.RestartOnFailure.String()
}

var remain string
if appInfo.Daemon == "oneshot" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just thinking aloud. Is there any other condition where we would like to have remain-after-exit set?

daemon: oneshot
`

info, err := snap.InfoFromSnapYaml([]byte(yamlText))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have snaptest.MockYaml from snap/snaptest for this purpose. Please use it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zyga Do you mean snaptest.MockSnap? I don't see a MockYaml anywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I meant MockInfo :-) They differ subtly but I think all you need is MockInfo

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Looks good, please do merge master into this branch, that should fix the test failure.

@chipaca chipaca merged commit e8fc228 into snapcore:master Feb 23, 2017
@ssweeny ssweeny deleted the fix-oneshot-daemons branch February 23, 2017 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants