many: implement "refresh-mode: {restart,endure,...}" for services #4617

mvo5 · 2018-02-06T11:56:24Z

This PR adds a new "refresh-mode" to apps/services. The supported
values are: {restart,survive} with restart being the default.

Any service that has "refresh-mode: survive" will not get restarted
during a snap refresh.

See also https://forum.snapcraft.io/t/140/21

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

This PR adds a new "refresh-mode" to apps/services. The supported values are: {restart,survive} with restart being the default. Any service that has "refresh-mode: survive" will not get restarted during a snap refresh. See also https://forum.snapcraft.io/t/140/21

zyga · 2018-02-06T12:52:09Z

++ snap pack /home/gopath/src/github.com/snapcore/snapd/tests/lib/snaps/test-snapd-service /home/gopath/src/github.com/snapcore/snapd/tests/lib/snaps/test-snapd-service
2018/02/06 12:09:45.808651 container.go:242: in snap "test-snapd-service": path "bin/stop" does not exist
error: cannot pack "/home/gopath/src/github.com/snapcore/snapd/tests/lib/snaps/test-snapd-service": snap is unusable due to missing files

zyga · 2018-02-06T12:53:29Z

overlord/snapstate/backend.go

@@ -55,7 +55,7 @@ type managerBackend interface {
 	CopySnapData(newSnap, oldSnap *snap.Info, meter progress.Meter) error
 	LinkSnap(info *snap.Info) error
 	StartServices(svcs []*snap.AppInfo, meter progress.Meter) error
-	StopServices(svcs []*snap.AppInfo, meter progress.Meter) error
+	StopServices(svcs []*snap.AppInfo, reason string, meter progress.Meter) error


Stop reason should be a distinct type with some constants, to avoid and deter typos.

Yeah, when I wrote this I was wondering this too. I dislike the extra verbosity but I agree about the usefulness of the static checking.

zyga · 2018-02-06T12:54:26Z

tests/lib/snaps/test-snapd-service/meta/snap.yaml

@@ -3,8 +3,14 @@ version: 1.0
 apps:
    test-snapd-service:
        command: bin/start
+        stop-command: bin/stop


This file is missing.

Thanks, fixed.

chipaca

LGTM. One question though: wouldn't it make sense to fire a reload to survive services, once the new revno is in place?

chipaca · 2018-02-06T17:51:29Z

snap/validate.go

+		return fmt.Errorf(`"refresh-mode" field contains invalid value %q`, app.RefreshMode)
+	}
+	if app.RefreshMode != "" && app.Daemon == "" {
+		return fmt.Errorf(`"refresh-mode" can only be used for services not in %q`, app.Name)


"refresh-mode" cannot be used for %q, only for services or something like that?

zyga

Some nitpicky name suggestions, mostly. Otherwise +1

zyga · 2018-02-06T20:10:59Z

overlord/patch/patch5.go

@@ -62,7 +63,7 @@ func patch5(st *state.State) error {
 			continue
 		}

-		err = wrappers.StopServices(svcs, log)
+		err = wrappers.StopServices(svcs, snap.ServiceStopReasonRefresh, log)


Maybe snap.StopForRefresh?

zyga · 2018-02-06T20:11:29Z

overlord/snapstate/snapstate.go

@@ -142,6 +142,7 @@ func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int
 	if snapst.IsInstalled() {
 		// unlink-current-snap (will stop services for copy-data)
 		stop := st.NewTask("stop-snap-services", fmt.Sprintf(i18n.G("Stop snap %q services"), snapsup.Name()))
+		stop.Set("stop-reason", "refresh")


snap.StopForRefresh

zyga · 2018-02-06T20:11:51Z

overlord/snapstate/snapstate.go

@@ -1085,6 +1086,7 @@ func Disable(st *state.State, name string) (*state.TaskSet, error) {

 	stopSnapServices := st.NewTask("stop-snap-services", fmt.Sprintf(i18n.G("Stop snap %q (%s) services"), snapsup.Name(), snapst.Current))
 	stopSnapServices.Set("snap-setup", &snapsup)
+	stopSnapServices.Set("stop-reason", "disable")


snap.StopDisabled

zyga · 2018-02-06T20:12:10Z

overlord/snapstate/snapstate.go

@@ -1246,6 +1248,7 @@ func Remove(st *state.State, name string, revision snap.Revision) (*state.TaskSe

 		stopSnapServices := st.NewTask("stop-snap-services", fmt.Sprintf(i18n.G("Stop snap %q services"), name))
 		stopSnapServices.Set("snap-setup", snapsup)
+		stopSnapServices.Set("stop-reason", "remove")


snap.StopRemoved

zyga · 2018-02-06T20:12:49Z

snap/validate.go

@@ -414,6 +414,17 @@ func ValidateApp(app *AppInfo) error {
 		return err
 	}

+	// validate refresh-mode
+	switch app.RefreshMode {
+	case "", "restart", "survive":


Could those be constans rather than literals?

Maybe, we don't use constants for e.g. app.Daemon - but if you have strong opinions I can use constants here.

mvo5 · 2018-02-07T09:15:01Z

@chipaca re reload> I'm inclined to leave leave this to the snap author by using e.g. the post-refresh hook.

codecov-io · 2018-02-07T09:43:21Z

Codecov Report

Merging #4617 into master will increase coverage by 0.13%.
The diff coverage is 89.09%.

@@            Coverage Diff             @@
##           master    #4617      +/-   ##
==========================================
+ Coverage   78.42%   78.55%   +0.13%     
==========================================
  Files         464      464              
  Lines       33144    33350     +206     
==========================================
+ Hits        25994    26199     +205     
  Misses       5008     5008              
- Partials     2142     2143       +1

Impacted Files	Coverage Δ
snap/info.go	`84.38% <ø> (ø)`	⬆️
snap/types.go	`82.35% <ø> (ø)`	⬆️
overlord/snapstate/backend/link.go	`56.16% <0%> (ø)`	⬆️
overlord/patch/patch5.go	`6.66% <0%> (ø)`	⬆️
overlord/snapstate/snapstate.go	`82.23% <100%> (+0.05%)`	⬆️
snap/validate.go	`96.35% <100%> (+0.06%)`	⬆️
snap/info_snap_yaml.go	`93.14% <100%> (+0.02%)`	⬆️
systemd/systemd.go	`84.72% <100%> (+0.15%)`	⬆️
overlord/snapstate/handlers.go	`70.08% <50%> (-0.12%)`	⬇️
wrappers/services.go	`84.58% <97.05%> (+1.44%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa2bbdc...f463b38. Read the comment docs.

niemeyer

A few comments, and LGTM when you're happy. Thank you!

niemeyer · 2018-02-13T16:46:51Z

wrappers/services.go

+		// Skip stop on refresh when refresh mode is "keep"
+		if reason == snap.StopReasonRefresh {
+			switch app.RefreshMode {
+			case "keep":


I'm sorry for the back and forth on this term, Michael. I'm still concerned about the current one.. the problem with both survive and keep is that it feels to innocent. I'd like to have a term that gives people a strange feeling about using it, and make them look for details on why that is so. As hopefully my last suggestion here, how about "endure". This comes close to "survive", but it has that feeling that it might not be that easy to keep it up, which is realistic given the constraints we've discussed for this (things changing behind the process, write access being lost, etc).

No worries, I'm happy about the attention to this. I changed this now in my PR to endure.

niemeyer · 2018-02-13T16:48:21Z

wrappers/services.go

+				sysd.Kill(app.ServiceName(), "USR1", "main")
+				continue
+			case "sigusr1-all":
+				sysd.Kill(app.ServiceName(), "USR1", "all")


This change ended up more contained than I expected, thank you!

niemeyer · 2018-02-13T16:48:43Z

tests/lib/snaps/test-snapd-service/bin/start-refresh-mode

+
+trap "echo got sigusr1" SIGUSR1
+trap "echo got sighup" SIGHUP
+trap "echo got sigterm" SIGTERM


niemeyer · 2018-02-13T16:50:43Z

wrappers/services.go

+			case "sighup-all":
+				sysd.Kill(app.ServiceName(), "HUP", "all")
+				continue
+			case "sigusr1":


Any reason to not support usr2 as well? Those are the typical signals I can think of for implementing custom behavior (hup, usr1, usr2).. any others?

Sure, I added sigusr2 now as well. I think (hup,usr1,usr2) are the ones used. We can easily add more if needed but it seems the other ones are really exotic (or inappropriate).

…meouts

zyga · 2018-02-14T16:20:13Z

snap/validate.go

 	if err := validateAppOrderNames(app, app.After); err != nil {
 		return err
 	}

+	// validate refresh-mode
+	switch app.RefreshMode {
+	case "", "endure", "restart", "sigterm", "sigterm-all", "sighup", "sighup-all", "sigusr1", "sigusr1-all", "sigusr2", "sigusr2-all":


This begs to be documented somewhere!

mvo5 requested a review from niemeyer February 6, 2018 11:56

zyga reviewed Feb 6, 2018

View reviewed changes

mvo5 added 2 commits February 6, 2018 14:41

tests: add missing file for test-snapd-service

5761d07

many: add new snap.ServiceStopReason and improve tests

bd7671f

chipaca approved these changes Feb 6, 2018

View reviewed changes

zyga approved these changes Feb 6, 2018

View reviewed changes

many: address review feedback and improve tests

b25c7f5

mvo5 changed the title ~~many: implement "refresh-mode: survive" for services~~ many: implement "refresh-mode: {restart,keep,...}" for services Feb 8, 2018

mvo5 added 2 commits February 8, 2018 07:42

Merge remote-tracking branch 'upstream/master' into dont-kill-me

7b774c6

snap: rename refresh-mode: survive to keep

dbc944a

mvo5 force-pushed the dont-kill-me branch 2 times, most recently from 8ae3fe5 to 0d9da90 Compare February 8, 2018 08:58

many: support refresh-mode: sig{term,hup,usr1}{,-all}

8622cac

mvo5 force-pushed the dont-kill-me branch from 0d9da90 to 8622cac Compare February 8, 2018 10:23

mvo5 added this to the 2.32 milestone Feb 12, 2018

Merge remote-tracking branch 'upstream/master' into dont-kill-me

157f7f5

niemeyer approved these changes Feb 13, 2018

View reviewed changes

address review feedback

2f75292

mvo5 changed the title ~~many: implement "refresh-mode: {restart,keep,...}" for services~~ many: implement "refresh-mode: {restart,endure,...}" for services Feb 13, 2018

mvo5 added 3 commits February 14, 2018 11:31

tests: do not trap TERM in snap-service-refresh-mode to avoid long ti…

7486f28

…meouts

tests: add debug to snap-service-refresh-mode

0c3020a

add debug to identify test failure

d83ffe4

zyga reviewed Feb 14, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into dont-kill-me

f463b38

mvo5 merged commit 410afe8 into canonical:master Feb 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: implement "refresh-mode: {restart,endure,...}" for services #4617

many: implement "refresh-mode: {restart,endure,...}" for services #4617

mvo5 commented Feb 6, 2018

zyga commented Feb 6, 2018

zyga Feb 6, 2018

mvo5 Feb 6, 2018

zyga Feb 6, 2018

mvo5 Feb 6, 2018

chipaca left a comment

chipaca Feb 6, 2018

zyga left a comment

zyga Feb 6, 2018

zyga Feb 6, 2018

zyga Feb 6, 2018

zyga Feb 6, 2018

zyga Feb 6, 2018

mvo5 Feb 7, 2018

mvo5 commented Feb 7, 2018

codecov-io commented Feb 7, 2018 •

edited

Loading

niemeyer left a comment

niemeyer Feb 13, 2018

mvo5 Feb 13, 2018

niemeyer Feb 13, 2018

niemeyer Feb 13, 2018

niemeyer Feb 13, 2018

mvo5 Feb 13, 2018

zyga Feb 14, 2018

many: implement "refresh-mode: {restart,endure,...}" for services #4617

many: implement "refresh-mode: {restart,endure,...}" for services #4617

Conversation

mvo5 commented Feb 6, 2018

zyga commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Feb 7, 2018

codecov-io commented Feb 7, 2018 • edited Loading

Codecov Report

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 7, 2018 •

edited

Loading