snap: add `snap refresh --time` option #3240

mvo5 · 2017-04-26T08:28:06Z

Based on #2833

This branch adds a new snap refresh --time option that will display information around the refresh times. This is useful for users to make things more transparent and also useful for debugging.

Here is the example output:

$ snap refresh --time
schedule: 00:00-04:59/5:00-10:59/11:00-16:59/17:00-23:59
last: 2017-04-26 07:47:00 +0200 CEST
next: 2017-04-26 16:33:00 +0200 CEST

One thing I wonder about is if this information should be part of sysinfo or if we should create a dedicated endpoint. The branch is currently using sysinfo, but it seems a dedicated endpoint might be needed anyway when we implement something like snap refresh --delay 2h. Opinions welcome.

…y ran

…refresh-schedule

…tion)

…refresh-schedule

…nger needed

… next refresh is calculated earlier

…l-refresh-schedule-info

chipaca

+1 with a comment :-)

chipaca · 2017-04-26T08:45:47Z

cmd/snap/cmd_snap_op.go

@@ -919,6 +941,7 @@ func init() {
 		waitDescs.also(channelDescs).also(modeDescs).also(map[string]string{
 			"revision":          i18n.G("Refresh to the given revision"),
 			"list":              i18n.G("Show available snaps for refresh"),
+			"time":              i18n.G("Show auto refresh information"),


I'd mention here that adding --time will not perform a refresh

(I see that list has the same problem of not saying “this will not perform the action”)

10:57 < mvo_> Chipaca: I like your suggestion about the --time help output. how about "Only show refresh time information" or "Only show refresh times information but not perform any refresh"? 10:59 < zyga> mvo_: maybe `snap refresh --query`

zyga

One question for Schedule.String below

zyga · 2017-04-26T08:59:54Z

cmd/snap/cmd_snap_op.go

@@ -919,6 +941,7 @@ func init() {
 		waitDescs.also(channelDescs).also(modeDescs).also(map[string]string{
 			"revision":          i18n.G("Refresh to the given revision"),
 			"list":              i18n.G("Show available snaps for refresh"),
+			"time":              i18n.G("Show auto refresh information"),


10:57 < mvo_> Chipaca: I like your suggestion about the --time help output. how about "Only show refresh time information" or "Only show refresh times information but not perform any refresh"? 10:59 < zyga> mvo_: maybe `snap refresh --query`

zyga · 2017-04-26T09:15:38Z

timeutil/schedule.go

@@ -62,6 +66,13 @@ type Schedule struct {
 	Weekday string
 }

+func (sched *Schedule) String() string {
+	if sched.Weekday == "" {
+		return fmt.Sprintf("%s:%s", sched.Start, sched.End)


Is this a typo? The start:end vs start-end below?

Yes, thank you very much for spotting this!

…l-refresh-schedule-info

zyga · 2017-04-26T10:09:59Z

tests/main/auto-refresh/task.yaml

            break
        fi
-        sleep 1
+        echo "Doing something that triggers ensure"
+        snap install test-snapd-python-webserver


Don't we have snap debug ensure-state-soon for this?

…refresh-schedule-info

…test

pedronis · 2017-04-28T08:44:41Z

overlord/snapstate/snapmgr.go

@@ -450,15 +450,23 @@ func autoRefreshInFlight(st *state.State) bool {
 	return false
 }

-func lastRefresh(st *state.State) (time.Time, error) {
+func (m *SnapManager) LastRefresh() (time.Time, error) {


they should grow a doc comment I think, that also explains they need to be used with the state lock

pedronis

lgtm

pedronis · 2017-04-28T08:54:54Z

Personally I think it's fine to expose this on /system-info, and maybe/likely later repeat it on GET of some kind of /refresh(-schedule) endpoint as well, once we need an endpoint to control this as well

robert-ancell · 2017-06-14T00:38:11Z

I just noticed this change, I've put some documentation in https://github.com/snapcore/snapd/wiki/REST-API but a couple of questions:

The time formats are inconsistent with the rest of snapd - e.g. they don't match install-date from snap objects.

Only my system:
system-info: 2017-06-14 09:04:00 +1200 NZST
install-date: 2017-02-07T06:49:09+13:00

Could we change them to use the RFC3339 UTC format?

What format is the schedule in?

robert-ancell · 2017-06-26T15:10:34Z

PR for date formats #3528

mvo5 added 30 commits January 31, 2017 16:15

initial version of the scheduleSpec parser

b8d83cf

Merge branch 'feature/timeutil' into feature/internal-refresh-schedule

b503fa6

draft WIP

f0fed9e

initial version of the scheduleSpec parser

0d9ace7

add new ParseTime()

6a99254

add timeutil.TimeOnly data structure

55b0bb3

add support for timeutil.Schedule.Matches() for weekdays

94b295c

improve docstrings

dad7d88

Merge branch 'feature/timeutil' into feature/internal-refresh-schedule

29f6c6a

port code to use new timeutil.Schedule.Matches()

e08e16a

rename TimeOnly -> TimeOfDay

64740ad

rename TimeOnly -> TimeOfDay

fe4646a

WIP add randomness to schedule

e9b77d0

run gofmt -s -w

bad75e6

add support for seconds in TimeOfDay

5ef4b1a

Merge branch 'feature/timeutil' into feature/internal-refresh-schedule

3af62d7

update tests for new interval behaviour

b17a935

make TestScheduleMatches() nicer

d13176a

implement Schedule.SameInterval()

7c26db6

Merge branch 'feature/timeutil' into feature/internal-refresh-schedule

da28a43

use matchedSchedule.SameInterval() to determine if the refresh alread…

552690a

…y ran

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

3fadca7

…refresh-schedule

use time.Time.Equals() instead of check.DeepEquals to compare two times

d62843e

add missing error check (thanks zyga!)

3203259

rework parse* helpers

04e3226

Merge branch 'feature/timeutil' into feature/internal-refresh-schedule

8373c92

add test that ensures snapstate.DefaultRefreshSchedule is correct

2001789

ensure refresh window ends in 30s (to avoid schedule window randomiza…

5879e04

…tion)

fix debug output for next scheduled refresh

1a25744

really fix auto-refresh test

a1811ae

mvo5 added 14 commits April 25, 2017 12:02

extract helpers from ensureRefreshes()

b176c55

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

621de8a

…refresh-schedule

rework refresh attempt computation

29fc44c

add tests that ensure correct behavior of SnapMgr.nextRefresh

9b7991a

implement snap refresh --time

3b2b1d5

address review feedback from Gustavo (thanks!)

0cb5177

remove a bunch of sleeps in the refresh schedule tests that are no lo…

1e4599b

…nger needed

address review feedback from Samuele (thanks!)

f720eca

move the backoff check slightly later in ensureRefreshes, so that the…

d399f8c

… next refresh is calculated earlier

address more review feedback (thanks to Samuele)

dade333

Merge branch 'feature/internal-refresh-schedule' into feature/interna…

faddc31

…l-refresh-schedule-info

remove some more dead code

4c720c9

Merge branch 'feature/internal-refresh-schedule' into feature/interna…

db6f22e

…l-refresh-schedule-info

add test for snap refresh --time

fd7b5b6

chipaca approved these changes Apr 26, 2017

View reviewed changes

clarify that snap refresh --{list,time} do not perform any action

24a1642

zyga reviewed Apr 26, 2017

View reviewed changes

mvo5 added 3 commits April 26, 2017 11:28

fix typo, thanks to zyga!

b1aad95

Merge branch 'feature/internal-refresh-schedule' into feature/interna…

b997e20

…l-refresh-schedule-info

deal with {next,last}Refresh being zero

fadba13

zyga reviewed Apr 26, 2017

View reviewed changes

mvo5 added 2 commits April 26, 2017 15:48

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

050527f

…refresh-schedule-info

use "snap debug ensure-state-soon" in internal-refresh-schedule-info …

8f45a83

…test

pedronis requested a review from niemeyer April 27, 2017 16:06

pedronis reviewed Apr 28, 2017

View reviewed changes

pedronis approved these changes Apr 28, 2017

View reviewed changes

chipaca merged commit cbd7b51 into snapcore:master May 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snap: add `snap refresh --time` option #3240

snap: add `snap refresh --time` option #3240

mvo5 commented Apr 26, 2017 •

edited

chipaca left a comment

chipaca Apr 26, 2017

chipaca Apr 26, 2017

zyga Apr 26, 2017

zyga left a comment

zyga Apr 26, 2017

zyga Apr 26, 2017

mvo5 Apr 26, 2017

zyga Apr 26, 2017

pedronis Apr 28, 2017

pedronis left a comment

pedronis commented Apr 28, 2017

robert-ancell commented Jun 14, 2017

robert-ancell commented Jun 26, 2017

snap: add snap refresh --time option #3240

snap: add snap refresh --time option #3240

Conversation

mvo5 commented Apr 26, 2017 • edited

chipaca 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

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

pedronis left a comment

Choose a reason for hiding this comment

pedronis commented Apr 28, 2017

robert-ancell commented Jun 14, 2017

robert-ancell commented Jun 26, 2017

snap: add `snap refresh --time` option #3240

snap: add `snap refresh --time` option #3240

mvo5 commented Apr 26, 2017 •

edited