Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: allow core refresh.schedule setting #2833
Conversation
mvo5
added some commits
Jan 31, 2017
Some checks were not successful
Some checks were not successful
All checks have passed
All checks have passed
Some checks were not successful
All checks have failed
All checks have failed
zyga
requested changes
Feb 13, 2017
I think we need one extra test and there's one trivial error at the end of the diff. The "time in interval" thing is somewhat confusing and I don't quite understand one of the functions. Perhaps some comment on that logic could help (at least me :-).
| + logger.Noticef("cannot use refresh.schedule: %s", err) | ||
| + refreshSchedule, err = timeutil.ParseSchedule(defaultRefreshSchedule) | ||
| + if err != nil { | ||
| + panic(fmt.Sprintf("defaultRefreshSchedule cannot be parsed: %s", err)) |
zyga
Feb 13, 2017
Contributor
We should have a test that ensures we can parse defaultRefreshSchedule
| + return false | ||
| + } | ||
| + | ||
| + if sched.Weekday != "" { |
zyga
Feb 14, 2017
Contributor
The condition above (!sched.Matches(t1) || !sched.Matches(t2)) confused me as without it the Schedule object didn't matter at all.
| + "fri": 5, "friday": 5, | ||
| + "sat": 6, "saturday": 6, | ||
| +} | ||
| + |
zyga
Feb 13, 2017
Contributor
Question, any reason those are not func (schedule *Schedule) parseWeekday(s string) err?
mvo5
Feb 13, 2017
Collaborator
No good reason. In fact the current mix is strange. After looking at this a bit I made them real functions that do not know yet about *Schedule.
| + | ||
| + return rest, nil | ||
| +} | ||
| + |
zyga
Feb 13, 2017
Contributor
Personal nitpick. It would be awesome if each of the parseXXX function had a one-line comment that shows roughly what they parse.
| + | ||
| +func parseSingleSchedule(s string) (*Schedule, error) { | ||
| + var cur Schedule | ||
| + |
| + // 2017-02-07 is a Tuesday | ||
| + {"mon@9:00-11:00", "2017-02-07 9:00", false}, | ||
| + } { | ||
| + fakeNow, err := time.Parse(shortForm, t.fakeNow) |
mvo5
added some commits
Feb 13, 2017
All checks have passed
All checks have failed
All checks have failed
All checks have failed
Some checks were not successful
Some checks were not successful
Some checks were not successful
| + return false | ||
| + } | ||
| + | ||
| + if sched.Weekday != "" { |
zyga
Feb 14, 2017
Contributor
The condition above (!sched.Matches(t1) || !sched.Matches(t2)) confused me as without it the Schedule object didn't matter at all.
| + "fri": 5, "friday": 5, | ||
| + "sat": 6, "saturday": 6, | ||
| +} | ||
| + |
zyga
Feb 13, 2017
Contributor
Question, any reason those are not func (schedule *Schedule) parseWeekday(s string) err?
mvo5
Feb 13, 2017
Collaborator
No good reason. In fact the current mix is strange. After looking at this a bit I made them real functions that do not know yet about *Schedule.
mvo5
added this to the
2.23 milestone
Feb 14, 2017
mvo5
added some commits
Feb 14, 2017
All checks have passed
Some checks were not successful
All checks have passed
mvo5
referenced this pull request
Feb 14, 2017
Merged
timeutil: a bunch of helpers for the scheduled refreshes #2782
mvo5
added some commits
Feb 15, 2017
Some checks were not successful
niemeyer
changed the title from
many: allow core.refresh.schedule setting
to
many: allow core refresh.schedule setting
Feb 15, 2017
mvo5
added some commits
Feb 15, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
All checks have passed
All checks have passed
All checks have passed
Some checks were not successful
Some checks were not successful
All checks have passed
Some checks were not successful
All checks have passed
Some checks were not successful
Some checks were not successful
Some checks were not successful
| - return nil | ||
| - } | ||
| + // already have a refresh timer | ||
| + if m.nextRefresh != nil { |
niemeyer
Feb 16, 2017
Contributor
I couldn't find logic that reload the next refresh when the value changes, and stops the old timer while doing that to ensure it doesn't get called out of turn. Does that exist and I missed, or do we still need it?
| @@ -484,6 +483,18 @@ func (m *SnapManager) ensureRefreshes() error { | ||
| return err | ||
| } | ||
| + when := timeutil.Next(refreshSchedule, lastRefresh) |
| + when := timeutil.Next(refreshSchedule, lastRefresh) | ||
| + | ||
| + m.nextRefresh = time.AfterFunc(when, func() { m.doAutoRefresh(updated, tasksets) }) | ||
| + logger.Debugf("schedule next refresh in %s", when) |
niemeyer
Feb 16, 2017
Contributor
Phrasing is a bit curious. Perhaps ("Next refresh scheduled for %s.", time.Now().Add(delta))
| @@ -510,6 +521,8 @@ func (m *SnapManager) ensureRefreshes() error { | ||
| } | ||
| chg.Set("snap-names", updated) | ||
| chg.Set("api-data", map[string]interface{}{"snap-names": updated}) | ||
| + | ||
| + m.nextRefresh = nil |
niemeyer
Feb 16, 2017
Contributor
Shouldn't we ensure this is stopped before we unset it? Otherwise the function is still called.
mvo5
Feb 16, 2017
Collaborator
AIUI a go timer is only called once (c.f. https://play.golang.org/p/OuAh1Ua6db). A ticker would be callled more often. So I think we don't need to cancel.
niemeyer
Feb 16, 2017
Contributor
Sorry, I assumed this was also called on global refreshes ("snap refresh" alone), but looking at the terminology above, it makes sense that it's only called on "auto".
| - tr := config.NewTransaction(st) | ||
| - tr.Set("core", "refresh.last", time.Now()) | ||
| - tr.Commit() | ||
| + st.Set("last-refresh", time.Now()) |
| @@ -10,29 +10,32 @@ execute: | | ||
| echo "Install a snap from stable" | ||
| snap install test-snapd-tools | ||
| snap list | MATCH 'test-snapd-tools +[0-9]+\.[0-9]+' | ||
| + snap set core refresh.schedule="0:00-$(date +%H:%M:%S --date=30seconds)" |
niemeyer
Feb 16, 2017
Contributor
We don't take seconds anymore, I think? Also not clear what the goal is here. This probably shouldn't be working as is, so perhaps unnecessary?
mvo5
Feb 16, 2017
Collaborator
We still take seconds currently, they are used in the unit tests still. But I fixed the test, now that we have the 14day max interval the test does not need to set this schedule, it can use a different oen like 00:00-23:59.
mvo5
modified the milestones:
2.24,
2.23
Feb 17, 2017
niemeyer
removed this from the
2.24 milestone
Feb 23, 2017
niemeyer
added
the
Blocked
label
Feb 23, 2017
|
Closing for now until the high level discussion has happened. |
mvo5
closed this
Feb 23, 2017
mvo5
added some commits
Mar 17, 2017
mvo5
removed
the
Blocked
label
Mar 30, 2017
mvo5
reopened this
Mar 30, 2017
mvo5
added this to the
2.24 milestone
Apr 4, 2017
Some checks were not successful
| + m.lastRefreshAttempt = time.Now() | ||
| + m.state.Lock() | ||
| + updated, tasksets, err := AutoRefresh(m.state) | ||
| + m.state.Unlock() |
pedronis
Apr 4, 2017
•
Edited 1 time
-
pedronis
Apr 5, 2017
Contributor
-
Apr 5, 2017
the unlocking looks wrong here, we need to make the Change based on tasksets while we still hold the same lock
| + return nil | ||
| +} | ||
| + | ||
| +func (m *SnapManager) doAutoRefresh(updated []string, tasksets []*state.TaskSet) error { |
pedronis
Apr 4, 2017
Contributor
this is a strange name given that it's not a task handler, also a bit unclear why the code is split this way between the the function passed to AfterFunc and this one
| @@ -466,6 +488,8 @@ func (m *SnapManager) ensureRefreshes() error { | ||
| } | ||
| chg.Set("snap-names", updated) | ||
| chg.Set("api-data", map[string]interface{}{"snap-names": updated}) | ||
| + |
|
Unit tests failed on: |
niemeyer
reviewed
Apr 5, 2017
A few comments below.
For the validation question, I suggest just adding the parsing in the snapd set config pipeline, quick and dirty.
In the near future we'll have configuration schemas for everybody, and can leverage that for core itself too.
For the rest, let's just make sure we have a LGTM from @pedronis.
| - // random interval on top of the minmum time between refreshes | ||
| - defaultRefreshRandomness = 4 * time.Hour | ||
| +var ( | ||
| + defaultRefreshSchedule = "00:00-04:59/5:00-10:59/11:00-16:59/17:00-23:59" |
| if err != nil && !config.IsNoOption(err) { | ||
| return err | ||
| } | ||
| + refreshSchedule, err := timeutil.ParseSchedule(refreshScheduleStr) |
niemeyer
Apr 5, 2017
Contributor
Don't we have to validate/discard the weekday somewhere around here?
| @@ -434,11 +441,26 @@ func (m *SnapManager) ensureRefreshes() error { | ||
| } | ||
| // store attempts in memory so that we can backoff a |
| // Ensure() also runs ensureRefreshes() | ||
| s.state.Unlock() | ||
| s.snapmgr.Ensure() | ||
| + // give the timer a bit of time | ||
| + time.Sleep(100 * time.Millisecond) |
niemeyer
Apr 5, 2017
Contributor
This will likely create issues in slow systems. If last-refresh is supposed to get updated, can we test this inside a time-bounded loop with shorter sleeps to decide when we're done waiting?
| s.state.Unlock() | ||
| s.snapmgr.Ensure() | ||
| + // give the timer a bit of time | ||
| + time.Sleep(300 * time.Millisecond) |
niemeyer
Apr 5, 2017
Contributor
Here it looks like it already created problems even on a high end system?
Perhaps same approach here and elsewhere if we have the same pattern.
mvo5
modified the milestones:
2.25,
2.24
Apr 6, 2017
mvo5
added some commits
Apr 7, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
| -var ( | ||
| - defaultRefreshSchedule = "00:00-04:59/5:00-10:59/11:00-16:59/17:00-23:59" | ||
| -) | ||
| +var defaultRefreshSchedule = "00:00-04:59/5:00-10:59/11:00-16:59/17:00-23:59" |
Some checks were not successful
|
Fails with |
|
Fails with |
mvo5
added some commits
Apr 11, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
Some checks were not successful
All checks have passed
| + logger.Debugf("Option refresh-schedule changed, reloading.") | ||
| + m.nextRefresh.Stop() | ||
| + m.nextRefresh = nil | ||
| + m.lastRefreshAttempt = time.Time{} |
pedronis
Apr 24, 2017
Contributor
also not sure:
// 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).After(time.Now()) {
return nil
}
makes sense where it is now, time.Now() is not when we will run the refresh attempt, it's just the time we are trying to schedule it, should it be moved inside the AfterFunc callback? or the logic tweaked some other way?
pedronis
Apr 24, 2017
Contributor
I'm also not sure 100% why we need the timer? wouldn't checking if the nextRefresh time is in the past be enough and try to do the change creation job here directly?
mvo5
added some commits
Apr 25, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
All checks have passed
|
A branch with |
| - return nil | ||
| + refreshSchedule, err := timeutil.ParseSchedule(refreshScheduleStr) | ||
| + if err == nil { | ||
| + err = refreshScheduleUsesWeekdays(refreshSchedule) |
niemeyer
Apr 25, 2017
Contributor
s/Uses/No/ would make this line more readable I think, as right now the error is happening when the function gets what it asks for (with weekdays), which is a bit misleading.
| - // change already in motion | ||
| - return nil | ||
| + if err != nil { | ||
| + logger.Noticef("cannot use refresh.schedule: %s", err) |
niemeyer
Apr 25, 2017
Contributor
s/:/configuration:/ maybe, to give a hint that this is a configuration option?
| + if !m.nextRefresh.IsZero() { | ||
| + if m.currentRefreshSchedule != refreshScheduleStr { | ||
| + // the refresh schedule has changed | ||
| + logger.Debugf("Option refresh-schedule changed, reloading.") |
niemeyer
Apr 25, 2017
Contributor
s/-/./, and it's not clear what this is reloading? Perhaps end at "changed."?
mvo5
Apr 26, 2017
Collaborator
Yeah, good point. There will be a debug output in the next line in this case anyway that prints the next scheduled refresh time so ending at "changed." is a good idea.
| m.lastRefreshAttempt = time.Now() | ||
| updated, tasksets, err := AutoRefresh(m.state) | ||
| if err != nil { | ||
| + logger.Noticef("AutoRefresh failed with: %s", err) |
niemeyer
Apr 25, 2017
Contributor
This should probably be a bit more oriented towards the user, since AutoRefresh is an internal method which the user will have no idea about. Perhaps something along the lines of
``"Cannot prepare auto-refresh change: %v"`
| m.lastRefreshAttempt = time.Now() | ||
| updated, tasksets, err := AutoRefresh(m.state) | ||
| if err != nil { | ||
| + logger.Noticef("AutoRefresh failed with: %s", err) | ||
| return err | ||
| } | ||
| // Do setLastRefresh() only if the store (in AutoRefresh) gave |
niemeyer
Apr 25, 2017
Contributor
Comment is now out-of-date relative to the code. Perhaps s/Do setLastRefresh()/Set last refresh time/
pedronis
reviewed
Apr 25, 2017
lgtm, but not sure I understand the time.Sleep after Ensure left in the tests given that we don't have a timer anymore
| - // store attempts in memory so that we can backoff a | ||
| +func (m *SnapManager) doAutoRefresh() error { |
pedronis
Apr 25, 2017
Contributor
I remarked this before, but it seems an odd name for something that is not a handler.
I would call it something like: just autoRefresh or initiateAutoRefresh
mvo5
Apr 26, 2017
Collaborator
Good point, sorry that I failed to address this earlier. How do you feel about runAutoRefresh() ?
| + // do refresh attempt (if needed) | ||
| + if m.nextRefresh.Before(time.Now()) { | ||
| + err = m.doAutoRefresh() | ||
| + m.nextRefresh = time.Time{} |
| + if canAutoRefreshCalled { | ||
| + break | ||
| + } | ||
| + time.Sleep(10 * time.Millisecond) |
| @@ -4470,6 +4538,8 @@ func (s *snapmgrTestSuite) TestEnsureRefreshesWithUpdateError(c *C) { | ||
| // update for the "some-snap" in our fake store | ||
| s.state.Unlock() | ||
| s.snapmgr.Ensure() | ||
| + // give the timer a bit of time | ||
| + time.Sleep(100 * time.Millisecond) |
mvo5
added some commits
Apr 26, 2017
Some checks were not successful
| - // store attempts in memory so that we can backoff a | ||
| +func (m *SnapManager) runAutoRefresh() error { |
pedronis
Apr 26, 2017
Contributor
we don't have a lot of other examples like this, usually the other managers create the change in ensure* directly,
I can live with run, the other options a bit shorter that my previous proposal would be "launchAutoRefresh"
| @@ -457,9 +435,86 @@ func (m *SnapManager) ensureRefreshes() error { | ||
| } | ||
| chg.Set("snap-names", updated) | ||
| chg.Set("api-data", map[string]interface{}{"snap-names": updated}) | ||
| + | ||
| + m.state.EnsureBefore(0) |
pedronis
Apr 26, 2017
Contributor
now that this is called from Ensure directly without the timer indirection I think we don't need this anymore
mvo5
added some commits
Apr 26, 2017
Some checks were not successful
Some checks were not successful
mvo5
added some commits
Apr 26, 2017
Some checks were not successful
All checks have passed
| @@ -68,7 +68,7 @@ type Schedule struct { | ||
| func (sched *Schedule) String() string { | ||
| if sched.Weekday == "" { | ||
| - return fmt.Sprintf("%s:%s", sched.Start, sched.End) | ||
| + return fmt.Sprintf("%s-%s", sched.Start, sched.End) |
mvo5 commentedFeb 10, 2017
•
Edited 1 time
-
mvo5
Mar 30, 2017
This is a new iteration of the new core.refresh.schedule support - it is limited currently to a schedule within a single day.
This allows to setup a schedule for refreshes like:
The snapd system will only auto-refresh within these schedule windows unless the window got missed and in such cases it will refresh immediately.
Open questions/issues:
snap set core refresh.schedulewould reject invalid settings right away but there is no easy for this right now - except for adding the validation in the configure hook which duplicates a bit of code as the hook is shell(or python) and our code is go.