Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snapstate: add support for refresh.schedule=managed #4161
Conversation
mvo5
added some commits
Oct 30, 2017
All checks have failed
mvo5
added this to the 2.30 milestone
Nov 7, 2017
| const snapdControlConnectedPlugAppArmor = ` | ||
| # Description: Can manage snaps via snapd. | ||
| /run/snapd.socket rw, | ||
| ` | ||
| +type snapControlInterface struct { | ||
| + commonInterface |
pedronis
Nov 9, 2017
Contributor
afaict seems we haven't used this kind of embedding pattern so far, wondering if there's a preference, I suppose it's a question for @zyga
zyga
Nov 13, 2017
Contributor
I noticed this and I was thinking about it myself. While we haven't done this before I don't think it is a bad practice myself (unless there's something golang-y that I'm not familiar with). Doing it would allow us to simplify a few interfaces that just need to override one method and are happy with the semantics of common interface for others.
| + | ||
| + refreshSchedule, ok := plug.Attrs["refresh-schedule"].(string) | ||
| + if ok { | ||
| + if refreshSchedule != "" && refreshSchedule != "managed" { |
pedronis
Nov 9, 2017
Contributor
I wouldn't accept refresh-schedule to be set to an empty "" string, it seems strange (easier to leave it just out) and it's not a pattern that our plug/slot rule language supports well
| @@ -489,6 +498,7 @@ func (m *SnapManager) NextRefresh() time.Time { | ||
| // RefreshSchedule returns the current refresh schedule. | ||
| // The caller should be holding the state lock. | ||
| func (m *SnapManager) RefreshSchedule() string { | ||
| + m.checkRefreshSchedule() |
pedronis
Nov 9, 2017
Contributor
are we sure we can ignore errors here? anyway I think the one place using this could survive this returning an error, it's not used all over the place
| + // the snap must come from the store | ||
| + // FIXME: should we use | ||
| + // assertstate.SnapDeclaration(info.SideInfo.SnapID) | ||
| + // here? |
| + . $TESTSLIB/snaps.sh | ||
| + install_local test-snapd-control-consumer | ||
| + | ||
| + # now pretend test-snapd-control-consumer comes from the store (fugly HACK) |
pedronis
Nov 9, 2017
Contributor
we could use the fakestore here instead (it could help also with setting up assertions), it's a bit of work though
|
We also need to revert the emergency weekly refresh timer |
| + if refreshScheduleStr != "managed" { | ||
| + return false | ||
| + } | ||
| + snapStates, err := All(m.state) |
pedronis
Nov 13, 2017
Contributor
I think everything from here and below belongs to an ifacestate helper that gets hooked into snapstate
mvo5
Nov 14, 2017
Collaborator
Thanks, I pushed a variant of your suggestion. It seems like a device property if the refreshes are managed or not so I moved it into devicestate and hook from there. But happy to go with your original suggestion if you feel strongly about it. I also moved to us snapdeclaration and started the store hints (in a separate branch in https://github.com/snapcore/snapd/compare/master...mvo5:refresh-candidates-managed?expand=1.
pedronis
Nov 14, 2017
•
Edited 1 time
-
pedronis
Nov 14, 2017
Contributor
-
Nov 14, 2017
no, that seems reasonable as well (devicestate)
| @@ -87,6 +87,12 @@ func (iface *commonInterface) SanitizeSlot(slot *interfaces.Slot) error { | ||
| return nil | ||
| } | ||
| +// SanitizePlug checks and possibly modifies a plug. |
mvo5
Nov 14, 2017
Collaborator
It is here to make things future proof, but your question is valid and YAGNI etc - so I think its fine to remove it as long as we remember that if we add SanitizePlug to commonInterface we need to look over the existing commonInterface embedders.
| +} | ||
| + | ||
| +func (iface *snapControlInterface) SanitizePlug(plug *interfaces.Plug) error { | ||
| + if err := iface.commonInterface.SanitizePlug(plug); err != nil { |
zyga
Nov 13, 2017
Contributor
Aha, for future-proofing, I assume. In such case can you comment in the common code so that things are clear. Alternatively just don't do this :)
| + return err | ||
| + } | ||
| + | ||
| + refreshSchedule, ok := plug.Attrs["refresh-schedule"].(string) |
| @@ -389,8 +390,15 @@ func refreshScheduleNoWeekdays(rs []*timeutil.Schedule) error { | ||
| } | ||
| func (m *SnapManager) checkRefreshSchedule() ([]*timeutil.Schedule, error) { | ||
| - refreshScheduleStr := defaultRefreshSchedule | ||
| + if m.refreshScheduleManaged() { | ||
| + if m.currentRefreshSchedule != "managed" { |
zyga
Nov 13, 2017
Contributor
Curious if this should exclude ability to manually manage a system. What do you think?
| + tr.Set("core", "refresh.schedule", "managed") | ||
| + tr.Commit() | ||
| + | ||
| + // using MockSnap, we want to read the bits on disk |
mvo5
Nov 14, 2017
Collaborator
I updated the comment now, there are a bunch of places this is copyied^Wcargo-culted^Wused, if we are happy with the update text I can fix it in all places (probably in a separate PR).
pedronis
added
the
Blocked
label
Nov 13, 2017
mvo5
added some commits
Nov 14, 2017
mvo5
referenced this pull request
Nov 15, 2017
Merged
tests: add new `fakestore new-snap-{declaration,revision}` helpers #4222
mvo5
referenced this pull request
Nov 16, 2017
Merged
interfaces: add "refresh-schedule" attribute to snapd-control #4231
mvo5
added some commits
Nov 16, 2017
codecov-io
commented
Nov 17, 2017
•
Edited 14 times
-
codecov-io
Nov 29, 2017
-
codecov-io
Nov 28, 2017
-
codecov-io
Nov 28, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 24, 2017
-
codecov-io
Nov 24, 2017
-
codecov-io
Nov 24, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 21, 2017
-
codecov-io
Nov 17, 2017
-
Nov 29, 2017 -
Nov 28, 2017 -
Nov 28, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 24, 2017 -
Nov 24, 2017 -
Nov 24, 2017 -
Nov 23, 2017 -
Nov 23, 2017 -
Nov 23, 2017 -
Nov 21, 2017 -
Nov 17, 2017
Codecov Report
@@ Coverage Diff @@
## master #4161 +/- ##
==========================================
+ Coverage 76.17% 76.21% +0.03%
==========================================
Files 443 445 +2
Lines 38591 38837 +246
==========================================
+ Hits 29398 29600 +202
- Misses 7174 7214 +40
- Partials 2019 2023 +4
Continue to review full report at Codecov.
|
| @@ -7,5 +7,5 @@ Documentation=man:snap(1) | ||
| [Service] | ||
| Type=oneshot | ||
| -ExecStart=@bindir@/snap refresh | ||
| +ExecStart=/bin/sh -c 'if ! @bindir@/snap refresh --list|grep "schedule:.*managed"; then @bindir@/snap refresh; fi' |
mvo5
added some commits
Nov 17, 2017
| @@ -223,3 +225,60 @@ func ProxyStore(st *state.State) (*asserts.Store, error) { | ||
| return a.(*asserts.Store), nil | ||
| } | ||
| + | ||
| +func plugConnected(st *state.State, snapName, plugName string) bool { |
pedronis
Nov 17, 2017
Contributor
seems this def should be directly before or after CanSetRefreshScheduleManaged
| @@ -1929,3 +1932,78 @@ func (s *deviceMgrSuite) TestCanAutoRefreshOnClassic(c *C) { | ||
| s.state.Set("seeded", false) | ||
| c.Check(canAutoRefresh(), Equals, false) | ||
| } | ||
| + | ||
| +func (s *deviceMgrSuite) TestRefreshControlManaged(c *C) { |
pedronis
Nov 17, 2017
Contributor
does CanSetRefreshScheduleManaged merit its separate unit tests now?
| + err = assertstate.Add(s.state, snapdWithSnapControlDecl) | ||
| + c.Assert(err, IsNil) | ||
| + | ||
| + c.Check(devicestate.RefreshScheduleManaged(st), Equals, true) |
pedronis
Nov 17, 2017
Contributor
would probably be good to check some of the false cases too or maybe those would go with the CanSetRefreshScheduleManaged tests
| @@ -491,6 +503,9 @@ func (m *SnapManager) NextRefresh() time.Time { | ||
| // RefreshSchedule returns the current refresh schedule. | ||
| // The caller should be holding the state lock. | ||
| func (m *SnapManager) RefreshSchedule() string { | ||
| + // This call ensures "m.currentRefreshSchedule" is up-to-date | ||
| + // with the latest configuration settings. | ||
| + m.checkRefreshSchedule() |
mvo5
Nov 21, 2017
Collaborator
I think this shows that the code here is a bit problematic and need a slight refactor. I'm not sure yet about the "how" but the pattern above smells funny and the fact that there is a comment to explain the intend does not make it better. I will try to come up with something better.
| + // check for refresh hints from the store every 24h | ||
| + // to be able to warn in the future | ||
| + var lastRefreshHints time.Time | ||
| + if err := m.state.Get("last-refresh-hints", &lastRefreshHints); err != nil && err != state.ErrNoState { |
pedronis
Nov 17, 2017
•
Edited 1 time
-
pedronis
Nov 17, 2017
Contributor
-
Nov 17, 2017
two comments,
- as far as I understand we always want to do this (because the schedule can be slower than once a day even in other cases)
- this is a strange place to do this anyway in terms of sane error reporting among other things (for example that we call this from the accessor)
mvo5
Nov 21, 2017
Collaborator
Thanks, I misunderstood and was not aware of (1). Fixed now. And agreed to (2), I moved it but want to look at this a bit harder to see if the existing code can be written in a slightly cleaner way (its a bit entangled right now).
|
some go vet failures;
|
mvo5
added some commits
Nov 21, 2017
mvo5
referenced this pull request
Nov 22, 2017
Merged
snapstate: ensure RefreshSchedule() gives accurate results #4272
mvo5
added some commits
Nov 23, 2017
mvo5
removed
the
Blocked
label
Nov 23, 2017
mvo5
referenced this pull request
Nov 23, 2017
Closed
snapstate: move autorefresh code into autoRefresh helper #4289
bboozzoo
and others
added some commits
Nov 3, 2017
| + | ||
| +// refreshScheduleManaged returns true if the refresh schedule of the | ||
| +// device is managed by an external snap | ||
| +func refreshScheduleManaged(st *state.State) bool { |
pedronis
Nov 27, 2017
Contributor
no strong opinion either way, but this could be moved to snapstate in the new autorefresh.go file and keep only CanSetRefreshScheduleManaged as the hook?
mvo5
Nov 27, 2017
Collaborator
I like your suggestion, I think:
snapstate.CanAutoRefresh = canAutoRefresh
- snapstate.RefreshScheduleManaged = refreshScheduleManaged
+ snapstate.CanSetRefreshScheduleManaged = CanSetRefreshScheduleManaged
alone is worth it, nice and symmetrical.
niemeyer
approved these changes
Nov 28, 2017
A few details, and LGTM once you address them to your own satisfaction. Thanks for implementing this.
| +type cmdCanSetRefreshScheduleManaged struct{} | ||
| + | ||
| +func init() { | ||
| + cmd := addDebugCommand("can-set-refresh-schedule-managed", |
| @@ -20,6 +20,9 @@ | ||
| package corecfg |
niemeyer
Nov 28, 2017
Contributor
Let's please not change this right now as it's an unrelated change that will move a lot of code around, and thus make this unrelated PR more confusing, but it feels like this package is misplaced at the top level. This is quite dependent on the whole infrastructure of the overlord being live, and also strongly tied to two pieces of it: the hook manager and the config manager. Something like "configstate/configcore" might be a more appropriate place for it (homage to the io/ioutil and path/filepath naming style).
| @@ -223,3 +225,44 @@ func ProxyStore(st *state.State) (*asserts.Store, error) { | ||
| return a.(*asserts.Store), nil | ||
| } | ||
| + | ||
| +// plugConnected returns true if the given snap/plug names are connected | ||
| +func plugConnected(st *state.State, snapName, plugName string) bool { |
niemeyer
Nov 28, 2017
Contributor
Is this really plugConnected/plugName or more like interfaceConnected/ifaceName?
| + | ||
| +// CanSetRefreshScheduleManaged returns true if the device can be | ||
| +// switched to the "core.refresh.schedule=managed" mode. | ||
| +func CanSetRefreshScheduleManaged(st *state.State) bool { |
| + // The snap must have a snap declaration (implies that | ||
| + // its from the store) | ||
| + if _, err := assertstate.SnapDeclaration(st, info.SideInfo.SnapID); err != nil { | ||
| + continue |
mvo5 commentedNov 7, 2017
This PR allows setting
refresh.schedule: managedif we have a connected plug on the system for the snapd-control interface with therefresh-schedule: managedproperty.This is got get a first review, the PR needs some more work, i.e. it needs to contact the store for refreshes (even if we not act on them) so that local warnings can be issued in the future.