snapstate: move refresh from a systemd timer to the internal snapstate Ensure() #2558

mvo5 · 2017-01-05T09:43:55Z

This branch is groundwork so that we can control how to delay refreshes. It moves the automatic refresh from a systemd timer into the snapstate and tracks the refresh-time in the internal state.

An extra careful review is appreciated as this has some potential for catastrophies. If the auto-refresh logic has a bug we can not fix that bug automatically via an update. We might consider keeping the systemd job around for a bit more (but with a lower update frequency of maybe once a week) to have a way out if disaster happens.

One nice side-effect of this change is that we do no longer generate empty changes if there is nothing to update. We probably still want to log those empty updates to syslog so that we can double check that our updates are really run. The branch is doing that currently and given that the checks only happen every 4h+rand(4h) that seems to be ok.

We now also have a lot more flexibility regarding the randomization of the update time. Currently its relatively static (4h fixed + rand(4h)) but we have many options here (unlike with systemd).

…er the refresh

…test

pedronis · 2017-01-05T10:25:30Z

overlord/snapstate/snapmgr.go

+	}
+
+	// time to refresh?
+	if !nextRefresh.IsZero() && time.Now().Before(nextRefresh) {


I suppose we shouldn't trigger a refresh before we are seeded?

it seems that if the refresh fails we will try immediately again instead of waiting a bit? because schedule-next-refresh is invoked only if everything is ok?

pedronis · 2017-01-05T11:09:59Z

overlord/snapstate/snapmgr.go

+		}
+	}
+
+	updated, tasksets, err := UpdateMany(m.state, nil, 0)


if you check in daemon refresh does more than this nowadays, it uses assertmgr to refresh snap-declaration as well as a first step, we need to reorg that somehow

…refresh

…muelle)

…refresh

Also disable autorefresh during tests and update spread test.

pedronis · 2017-02-01T16:31:58Z

overlord/devicestate/devicemgr.go

+	}
+
+	// and we also need to have a serial
+	_, err := Serial(st)


this is correct long term, otoh it means developer images with custom models will not auto-refresh atm

…refresh

niemeyer

A few comments, and LGTM once these are sorted to your satisfaction.

niemeyer · 2017-02-02T18:24:42Z

overlord/snapstate/snapstate.go

+// snaps on the system. In addition to that it will also refresh important
+// assertions.
+func AutoRefresh(st *state.State) ([]string, []*state.TaskSet, error) {
+	// FIXME: obtain it somehow?


As we discussed, the FIXME can be dropped here. This is doing the right thing. It's the internal implementation of userID handling in SnapSetup vs. SnapState that is broken.

Thanks, I removed the comment

niemeyer · 2017-02-02T18:28:55Z

overlord/snapstate/snapmgr.go

+
+	// for tests
+	var refreshDisabled bool
+	err := tr.Get("core", "refresh.disabled", &refreshDisabled)


That'll very quickly get widely used. We need to either agree that this is the direction we want to go, or we need to somehow make sure this can only possibly be used when testing, perhaps leveraging cmd.Version or something.

Agreed, we need a decision, for now it only works when SNAPD_DEBUG is set. I could also make that SNAPPY_TESTING or even only if we have a test build (but that will be slightly problematic in the tests we run against official builds in spread-cron).

niemeyer · 2017-02-02T18:30:24Z

overlord/snapstate/snapmgr.go

+	}
+
+	var refreshRand time.Duration
+	err = tr.Get("core", "refresh.rand", &refreshRand)


Can we please drop this? Given that the syntax in the FIXME above supports windows, we can keep this as an internal var or const while we don't finish the implementation of the full scheduling.

Yes, I removed it

niemeyer · 2017-02-02T18:35:46Z

overlord/snapstate/snapmgr.go

+	//  2. always set the last refresh regardless of errors
+	//     in "AutoRefresh()"
+	//
+	// This currently implements (2) because if there are e.g. store


Using (2) sounds a bit problematic now that we're giving people the option to dial down the frequency of retries. If someone has it set for once a week on Mondays, not retrying means it'll take another week until it tries again. Given how often we've been observing various kind of spurious errors, we should probably allow it to actually retry.

It should be super cheap for the store to say "go away, I have nothing for you!" if it really wants the retry to not happen. It can't easily say "Oh, come back please!", though.

Good point! Fixed.

niemeyer · 2017-02-02T18:36:23Z

overlord/snapstate/snapmgr.go

+	var msg string
+	switch len(updated) {
+	case 0:
+		// check in after some hours


Comment is outdated now that we're allowing configuration.

niemeyer · 2017-02-02T18:37:38Z

overlord/snapstate/snapmgr.go

+	default:
+		quoted := strutil.Quoted(updated)
+		// TRANSLATORS: the %s is a comma-separated list of quoted snap names
+		msg = fmt.Sprintf(i18n.G("Refresh snaps %s"), quoted)


We should probably do that only in, say, "case 2,3" or similar. Otherwise it might be better to say "Refresh snaps A, B and N others." instead.

I changed it to Refresh %d snaps. I can add the first one or two names if you want but it seems a bit random.

pedronis

some more comments

pedronis · 2017-02-03T09:40:46Z

overlord/snapstate/snapmgr.go

+	// allow disabling auto-refresh in the tests
+	if osutil.GetenvBool("SNAPD_DEBUG") {
+		var refreshDisabled bool
+		err := tr.Get("core", "refresh.disabled", &refreshDisabled)


shouldn't we use a less transparent name for this given that is for tests only? or use a env var for the setting as well?

That is a good question - the jury is still out if this is something we want to support for users as well or not. This is why this is so strangely written currently. I try to get clarification on that.

pedronis · 2017-02-03T09:42:49Z

overlord/snapstate/snapmgr.go

+
+	// FIXME: this is skewed because it runs every 10min but it will
+	//        be replaced soon by randomness based on the window
+	randomness := time.Duration(rand.Int63n(int64(refreshRandomness)))


I think (similarly to systemd) we should pick this at the start of snapd in the constructor of the manager, and log its value (to help debugging)

Good point, added. Please note that this will soon go away for a different randomness that takes the schedule intervall length into account.

pedronis · 2017-02-03T09:46:29Z

overlord/snapstate/snapmgr.go

+
+	// Do setLastRefresh() only if the store (in AutoRefresh) gave
+	// us no error. This means we retry on store errors every
+	// ~10min which will put some burden on the store. But if the


I think we forget and we have similar problems in bits of devicestate, but 10 minutes is very untrue when we have changes on the fly, because then we trigger Ensure a lot with EnsureBefore(0). I think what we want is a minmum time between tries. So setting a lastRefreshTry on the manager itself (doesn't need to be in state) and setting it before auto refresh, and bail out if the elapsed time form lastRefreshTry is lass than 19 minutes or something like that.

As I said we have a bunch of places already that might need code like that (mostly in devicestate I think)

Thank you, this is a very good point. I added the code but test is currently missing.

pedronis · 2017-02-03T09:59:38Z

overlord/snapstate/snapmgr.go

+		logger.Noticef(i18n.G("No snaps to auto-refresh found"))
+		return nil
+	case 1:
+		msg = fmt.Sprintf(i18n.G("Refresh snap %q"), updated[0])


should these say Auto-refresh ?

I like that, fixed!

pedronis · 2017-02-03T10:10:09Z

overlord/devicestate/devicemgr.go


+// canAutoRefresh is a helper that checks if the device is able to
+// auto-refresh
+func canAutoRefresh(st *state.State) bool {


just noting this seems not to be tested (the code is simple though)

Indeed, test is missing here too.

pedronis

a couple of the last comments I think are important

pedronis

thank you

pedronis · 2017-02-03T17:03:30Z

overlord/snapstate/snapmgr.go

+	// Check that we have reasonable delays between unsuccessful attempts.
+	// If the store is under stress we need to make sure we do not
+	// hammer it too often
+	if !m.lastRefreshAttempt.IsZero() && m.lastRefreshAttempt.Add(10*time.Minute).Before(time.Now()) {


Before should be After actually?

Yes, fixed and added a test.

pedronis

after trying something similar I think there's a buglet

pedronis

thanks

mvo5 added 16 commits January 3, 2017 17:03

move refresh-all inside the daemon

ff8bf32

remove refresh time now that refresh happens internally

35f2d12

add fixmes

874b4c8

add randomness

0f54e49

move the schedule of the next refresh out into a task that is run aft…

7e83fa1

…er the refresh

add test that tests the schedule-next-refresh task

b2898a2

add ensureRefresh tests

2fa1b59

ensure nextRefresh is scheduled regardless of refresh-errors and add …

06375ab

…test

fix test failures

cc5ea07

schedule next refresh in some hours if there are no updates

735617e

refactor and add stateNew() test helper

57df051

improve tests

00414c4

add spread test

90bf3dd

fix test/spread failures

70c15bf

remove unneeded nopHandler

b4c5de2

add syslog output when no updates are found

85407fb

pedronis reviewed Jan 5, 2017

View reviewed changes

mvo5 added 12 commits January 5, 2017 13:59

fix typo

8296809

Merge remote-tracking branch 'upstream/master' into feature/internal-…

862bdef

…refresh

always scheduleNextRefresh() regardless of errors (as suggested by Sa…

3b8f085

…muelle)

do not auto-refresh unless the systemd is seeded

8dfb6b7

add new snapstate.AutoRefresh() that will also refresh all assertions

1d7994d

fix typo

bec4c7a

remove merge artifacts

4719520

Merge remote-tracking branch 'upstream/master' into feature/internal-…

82dd275

…refresh

add new snapstate.CanAutoRefresh helper

9fd8fad

fix canAutoRefresh on classic

33c29ac

disable auto-refresh in daemon tests

f6a3b1c

add missing "jq" snap for auto-refresh task

98c298d

mvo5 closed this Jan 30, 2017

mvo5 mentioned this pull request Jan 30, 2017

overlord: move configstate.Transaction stuff into configstate.config.Transaction #2744

Merged

mvo5 added 2 commits January 31, 2017 16:56

Merge remote-tracking branch 'upstream/master' into feature/internal-…

1d6e4c6

…refresh

move to core.refresh.last instead of core.next-auto-refresh

ddb38ac

mvo5 reopened this Jan 31, 2017

mvo5 force-pushed the feature/internal-refresh branch from 101f980 to b531dc8 Compare February 1, 2017 07:01

remove snapd.refresh.timer from the tests and packaging

732269c

Also disable autorefresh during tests and update spread test.

mvo5 force-pushed the feature/internal-refresh branch from b531dc8 to 732269c Compare February 1, 2017 07:29

mvo5 added 2 commits February 1, 2017 09:34

adjust refresh-all-undo test now that core has a configure task

e678529

ignore SNAP_REFRESH_FROM_TIMER refreshes

d9e1ca0

pedronis reviewed Feb 1, 2017

View reviewed changes

mvo5 added 2 commits February 2, 2017 10:24

add FIXME based on Samueles comment

8ec180f

Merge remote-tracking branch 'upstream/master' into feature/internal-…

d35b40f

…refresh

niemeyer approved these changes Feb 2, 2017

View reviewed changes

pedronis self-assigned this Feb 2, 2017

mvo5 force-pushed the feature/internal-refresh branch from 3972ffe to c3d7b96 Compare February 3, 2017 07:33

address review feedback

1b43a02

mvo5 force-pushed the feature/internal-refresh branch from c3d7b96 to 1b43a02 Compare February 3, 2017 08:24

pedronis reviewed Feb 3, 2017

View reviewed changes

pedronis requested changes Feb 3, 2017

View reviewed changes

mvo5 added 2 commits February 3, 2017 15:40

address review feedback

e3f00c2

add FIXMEs

08b7a2a

pedronis approved these changes Feb 3, 2017

View reviewed changes

pedronis reviewed Feb 3, 2017

View reviewed changes

pedronis requested changes Feb 3, 2017

View reviewed changes

pedronis changed the title ~~snapstate: move refresh from a systemd timer to the inernal snapstate Ensure()~~ snapstate: move refresh from a systemd timer to the internal snapstate Ensure() Feb 4, 2017

add test for delays between unsuccessful AutoRefresh attempts

1f7aff4

pedronis approved these changes Feb 6, 2017

View reviewed changes

add test for canAutoRefresh

b1c727d

mvo5 merged commit 92958e1 into canonical:master Feb 7, 2017

snapstate: move refresh from a systemd timer to the internal snapstate Ensure() #2558

snapstate: move refresh from a systemd timer to the internal snapstate Ensure() #2558

Uh oh!

Conversation

mvo5 commented Jan 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees