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: add `snap revert` #1275
Conversation
mvo5
added some commits
May 19, 2016
|
retest this please |
pedronis
reviewed
Jun 6, 2016
| + | ||
| + // RollbackMode means the given snap got rolled back | ||
| + RollbackMode | ||
| + |
pedronis
Jun 6, 2016
Contributor
not sure we should call Mode something like this also because it seems it's SnapSetup only flag, so it should follow my suggestion in the comment below in the code (if it's really a mode like trymode then the previous definition should go away and we should grab the value of firstInterimUsableFlagValue here instead of the one following)
pedronis
reviewed
Jun 6, 2016
| @@ -302,8 +306,68 @@ func Remove(s *state.State, name string) (*state.TaskSet, error) { | ||
| // Rollback returns a set of tasks for rolling back a snap. | ||
| // Note that the state must be locked by the caller. | ||
| -func Rollback(s *state.State, snap, ver string) (*state.TaskSet, error) { | ||
| - return nil, fmt.Errorf("rollback not implemented") | ||
| +func Rollback(s *state.State, name, ver string) (*state.TaskSet, error) { |
pedronis
Jun 6, 2016
Contributor
bit confused about what should happen when ones rollback again? should we stop that here?
mvo5
Jun 7, 2016
Collaborator
@pedronis Yeah, that is a really good question. So far we only keep a single previous revision so you get only one rollback. Once we support more then of course RollbackR in the snapst is no longer good and it needs to become a list or a map. I can do this now so that we are ready?
pedronis
Jun 7, 2016
Contributor
I don't have a strong opinion either way, but if keep it to just one rollback recorded, we should error here if a 2nd is tried. Also do we need to reset RollbackR somewhere?
mvo5
Jun 7, 2016
Collaborator
Yes, it is reset in https://github.com/snapcore/snapd/pull/1275/files#diff-82a44be6f243493a55d2ee2fe6f0addfR660. I added code that will prevent two rollbacks now.
pedronis
reviewed
Jun 7, 2016
| - // 0x40000000 >> iota | ||
| + // Flags for SnapSetup: | ||
| + // RollbackOp means the given snap got rolled back | ||
| + RollbackOp = 0x40000000 >> iota |
pedronis
Jun 7, 2016
Contributor
could you double check, this is correct? if I read the lang ref correctly you need a new const block to get the right effect
mvo5
added some commits
Jun 7, 2016
zyga
reviewed
Jun 7, 2016
| @@ -120,6 +125,14 @@ func (snapst *SnapState) Current() *snap.SideInfo { | ||
| return snapst.Sequence[n-1] | ||
| } | ||
| +func (snapst *SnapState) Previous() *snap.SideInfo { |
pedronis
reviewed
Jun 7, 2016
| @@ -677,6 +698,9 @@ func (m *SnapManager) doLinkSnap(t *state.Task, _ *tomb.Tomb) error { | ||
| Set(st, ss.Name, snapst) | ||
| // Make sure if state commits and snapst is mutated we won't be rerun | ||
| t.SetStatus(state.DoneStatus) | ||
| + // FIXME: this triggers a stale in the followup tasks in the | ||
| + // taskrunner code, i.e. tasks after link-snap will | ||
| + // only be run after the next EnsureBefore() |
pedronis
reviewed
Jun 8, 2016
| @@ -107,6 +111,7 @@ type SnapState struct { | ||
| Active bool `json:"active,omitempty"` | ||
| Channel string `json:"channel,omitempty"` | ||
| Flags SnapStateFlags `json:"flags,omitempty"` | ||
| + RollbackR snap.Revision `json:"rollback,omitempty"` |
pedronis
Jun 8, 2016
Contributor
if we think we'll need to support more than one rollback later, maybe this should start as []snap.Revision from day 1? to avoid the UnmarshalJSON juggling, though it wouldn't be too bad either
mvo5
added some commits
Jun 9, 2016
|
this is missing snapmgr_test.go tests and would be good if we started adding tests in the style of the ones in link_snap_test.go tests for link-snap handlers for any new handlers we introduce (I still plan to work through the preexisting handlers and add tests/fixes before RTM) |
mvo5
added some commits
Jun 10, 2016
|
@pedronis thanks again! tests added :) |
pedronis
reviewed
Jun 10, 2016
| +// SnapSetup specific flags | ||
| +const ( | ||
| + // RollbackOp means the given snap got rolled back | ||
| + RollbackOp = 0x40000000 >> iota |
pedronis
Jun 10, 2016
Contributor
same comment as I mentioned in first boot, given that this is only for SnapSetup flags it can be typed as SnapSetupFlags
pedronis
reviewed
Jun 10, 2016
| + | ||
| +const ( | ||
| + // RollbackOp means the given snap got rolled back | ||
| + RollbackOp = 0x40000000 >> iota |
pedronis
Jun 10, 2016
Contributor
sorry, probably was confusing but I also meant:
RollbackOp SnapSetupFlags = 0x40000000 >> iota
so it cannot be mixed easily with SnapStateFlags
|
retest this please |
mvo5
added some commits
Jun 13, 2016
|
LGTM FWIW |
niemeyer
reviewed
Jun 17, 2016
| + } `positional-args:"yes"` | ||
| +} | ||
| + | ||
| +var shortRollbackHelp = i18n.G("Rollback the given snap to the previous version") |
niemeyer
reviewed
Jun 17, 2016
| + | ||
| +var shortRollbackHelp = i18n.G("Rollback the given snap to the previous version") | ||
| +var longRollbackHelp = i18n.G(` | ||
| +The rollback command will rollback the given snap to the previous version. |
niemeyer
Jun 17, 2016
•
Edited 1 time
-
niemeyer
Jun 17, 2016
Contributor
-
Jun 17, 2016
The rollback command will revert the given snap to its state before the latest refresh.
This will reactivate the previous snap revision, and will use the original data that was
associated with that revision, discarding any data changes that were done by the
latest revision. As an exception, data which the snap explicitly chooses to share
across revisions is not touched by the rollback process.
niemeyer
reviewed
Jun 17, 2016
| + common.SnappySuite | ||
| +} | ||
| + | ||
| +func (s *rollbackAppSuite) TestInstallUpdateRollback(c *check.C) { |
niemeyer
Jun 17, 2016
Contributor
This needs to be moved to spread, but let's not block on it. I see it depends on the fake store, which will need a bit of churn for us to establish the pattern. So a follow up seems more appropriate.
|
@mvo5 When you have a spare moment I'd like to have an interactive conversation with you to see whether we might take a slightly different approach for this work which would be both more flexible and require less state tracking. Before going into the details of that, though can we replace the "rollback" terminology by "revert", as a shorter and nicer companion to "refresh"? Then, here are the details:
With all of that in place, I believe the whole process would work fine with arbitrary revisions, so I suggest accepting this as the initial form of revert: If the revision is not provided, it becomes the previous one in the sequence. If the current revision is the oldest one (again, sequence not numeric order) revert errors saying "error: revision %d is the oldest one for snap %q". Note that the provided revision can be both older or newer than the current revision. As long as it's part of the currently installed sequence, we accept it. What do you think? |
|
what happens in this model to the data of the reverted revisions, we just what happens on a new install? we put the it just after Current in the On Fri, Jun 17, 2016 at 9:14 PM, Gustavo Niemeyer notifications@github.com
|
|
Yes, I think we'd keep the data of the revert revisions until something else happens. The new install is a good question, though. The only sane behavior I can think of is killing everything after the current one (effectively removing those revisions), and then appending as usual. This would make it look from the user perspective as if those revisions were never there, still preserving the correct sequence which was observed by the client as coming from the server. A practical example to become more clear:
Sounds sane? Any other ideas? |
|
There's an open edge there which we need to handle appropriately: how do we get back to the current revision without losing the latest changes that are sitting on the older revision that we reverted to. I think the answer here is that we need to distinguish an automatic global refresh from an explicit one. That is, this should work assuming 48 is still the latest revision in the store:
This is not a revert, though. In other words, in this case the data is again copied out from revision 35 exactly as it would be if 48 was in the store and not locally. |
|
Another idea: we should probably support a few options to offer some flexibility without requiring explicit revisions to be stated:
In fact, this is so much nicer than explicit revisions. We probably don't even need explicit revisions with that? |
|
ok, with the idea of wiping the disabled (because reverted from) revisions when we install/something happens it seems reasonable, I think we need to have some prerequisite before though:
|
mvo5
added some commits
Jun 29, 2016
|
Thanks for the review! I addressed the points raised, please let me know if there is anything else I can improve. |
mvo5
referenced this pull request
in mvo5/snappy
Jun 29, 2016
Closed
snapstate: drop revisions after "current" on refresh #12
niemeyer
reviewed
Jun 29, 2016
| + for _, si := range snapst.Sequence { | ||
| + if si.Revision == ss.Revision { | ||
| + snapst.Candidate = si | ||
| + } | ||
| } |
niemeyer
Jun 29, 2016
Contributor
We need some error checking logic here I think. Something along the lines of:
if snapst.Candidate == nil {
return fmt.Errorf("cannot prepare snap %q with unknown revision %s", ss.Name, ss.Revision)
}
|
Just one simple error checking and LGTM! |
pedronis
reviewed
Jun 29, 2016
| @@ -772,6 +813,7 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error { | ||
| snapst.Current = oldCurrent | ||
| snapst.Active = false | ||
| snapst.Channel = oldChannel | ||
| + snapst.Current = oldCurrent |
pedronis
reviewed
Jun 29, 2016
| + c.Assert(ts, IsNil) | ||
| +} | ||
| + | ||
| +func (s *snapmgrTestSuite) TestBlockunThrough(c *C) { |
pedronis
reviewed
Jun 29, 2016
| @@ -1284,7 +1484,7 @@ func (s *snapmgrQuerySuite) TestInfo(c *C) { | ||
| c.Check(info.Description(), Equals, "Lots of text") | ||
| } | ||
| -func (s *snapmgrQuerySuite) TestCurrent(c *C) { | ||
| +func (s *snapmgrQuerySuite) TestCurrentSideInfo(c *C) { |
pedronis
reviewed
Jun 29, 2016
| @@ -52,9 +52,6 @@ const ( | ||
| // for use in the interim time while we have the backward compatible | ||
| // support | ||
| firstInterimUsableFlagValue | ||
| - // if we need flags for just SnapSetup it may be easier | ||
| - // to start a new sequence from the other end with: | ||
| - // 0x40000000 >> iota | ||
| ) |
pedronis
Jun 29, 2016
Contributor
these comments need to be put back given that we aren't using this yet
pedronis
reviewed
Jun 29, 2016
| - return err | ||
| + for _, si := range snapst.Sequence { | ||
| + if si.Revision == ss.Revision { | ||
| + snapst.Candidate = si |
pedronis
reviewed
Jun 29, 2016
| + for _, si := range snapst.Sequence[currentIndex:] { | ||
| + out = append(out, si.Revision) | ||
| + } | ||
| + return out |
pedronis
Jun 29, 2016
•
Edited 1 time
-
pedronis
Jun 29, 2016
Contributor
-
Jun 29, 2016
this can be out = append(out, snapst.Sequence[currentIndex:]...) I think
mvo5
Jun 29, 2016
Collaborator
These are different types, out is a []snap.Revision but snapst.Sequence is a []*snap.SideInfo.
|
I made some small comments, but I'm bit confused with some tests, I need to play with the branch locally tomorrow |
pedronis
reviewed
Jun 29, 2016
| + c.Assert(chg.Status(), Equals, state.DoneStatus, Commentf("revert-snap change failed with: %v", chg.Err())) | ||
| + | ||
| + // ensure we are back to x1 after the revert | ||
| + c.Assert(osutil.FileExists(filepath.Join(dirs.SnapBlobDir, "foo_x1.snap")), Equals, true) |
pedronis
Jun 29, 2016
Contributor
I don't understand this test, is checking only things that are true independently of what is active (mount units, snap blob), which is ok, but it should check also something that changes like wrappers (I think some other tests here do that),
atm I can comment out the revert invocation and it passes anyway
mvo5
Jun 30, 2016
Collaborator
Thanks, indeed, this test was useless, sorry for overlooking that. Fixed now.
|
it seems we have a problem, and are missing some tests or missing testing some details: this line in undoLinkSnap: isn't correct if the candidate was already in the sequence |
|
sorry, same for the other line there: |
pedronis
reviewed
Jun 29, 2016
| + common.SnappySuite | ||
| +} | ||
| + | ||
| +func (s *revertAppSuite) TestInstallUpdateRevert(c *check.C) { |
niemeyer
Jun 30, 2016
Contributor
Yeah, @fgimenez can give us a hand with that so we don't block this month-long branch any further.
fgimenez
Jun 30, 2016
Contributor
Yep, I'll take care of this one with the remaining fake store tests.
pedronis
reviewed
Jun 29, 2016
| @@ -66,49 +66,53 @@ func doInstall(s *state.State, curActive bool, ss *SnapSetup) (*state.TaskSet, e | ||
| ss.Channel = "stable" | ||
| } | ||
| - var prepare *state.Task | ||
| - if ss.SnapPath != "" { | ||
| + var prepare, prev *state.Task |
pedronis
Jun 29, 2016
Contributor
given the changes to doInstall maybe we need a test in the style of TestInstallTasks like TestRevertTasks, to make sure the chaining is right
|
finished my pass, mostly some issues with tests, proposing some more testing of tasks/chaining in doInstall and the undoLinkSnap issue |
pedronis
reviewed
Jun 29, 2016
| + | ||
| + if !snapst.Active { | ||
| + return nil, fmt.Errorf("cannot revert inactive snaps") | ||
| + } |
pedronis
Jun 29, 2016
Contributor
not an issue atm but we should check if we are not trying to revert to snapst.Current and either do nothing? or error?
|
undoLinkSnap needs a test for the case when the candidate is already in the sequence (and one where its appended if we don't have that already). we need to store the old sequence length so that we know if we added something or not. |
mvo5
added some commits
Jun 30, 2016
pedronis
reviewed
Jun 30, 2016
| + var newSnapst snapstate.SnapState | ||
| + snapstate.Get(st, snapName, &newSnapst) | ||
| + c.Assert(newSnapst.CurrentSideInfo().Revision.Unset(), Equals, false) | ||
| + c.Assert(snapst.CurrentSideInfo(), Not(DeepEquals), newSnapst.CurrentSideInfo()) |
mvo5
added some commits
Jun 30, 2016
pedronis
reviewed
Jun 30, 2016
| m.backend.Candidate(snapst.Candidate) | ||
| - snapst.Sequence = append(snapst.Sequence, snapst.Candidate) | ||
| + hadCandidateInSeq := false |
pedronis
Jun 30, 2016
Contributor
this seems reversed, it true by default (and false if we had to change seq to add the cand there)
pedronis
reviewed
Jun 30, 2016
| @@ -775,11 +824,17 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| + var hadCandidateInSeq bool | ||
| + err = t.Get("had-candidate-in-seq", &hadCandidateInSeq) | ||
| + if err != nil { |
pedronis
reviewed
Jun 30, 2016
| - snapst.Candidate = snapst.Sequence[len(snapst.Sequence)-1] | ||
| - snapst.Sequence = snapst.Sequence[:len(snapst.Sequence)-1] | ||
| + snapst.Candidate = snapst.Sequence[snapst.findIndex(snapst.Current)] | ||
| + if hadCandidateInSeq { |
pedronis
reviewed
Jun 30, 2016
| - snapst.Sequence = snapst.Sequence[:len(snapst.Sequence)-1] | ||
| + snapst.Candidate = snapst.Sequence[snapst.findIndex(snapst.Current)] | ||
| + if hadCandidateInSeq { | ||
| + snapst.Sequence = snapst.Sequence[:len(snapst.Sequence)-1] |
pedronis
Jun 30, 2016
Contributor
I think Gustavo suggestion was to remove it by revision and not assume it was at the end
pedronis
reviewed
Jun 30, 2016
| @@ -780,7 +780,7 @@ func (m *SnapManager) doLinkSnap(t *state.Task, _ *tomb.Tomb) error { | ||
| t.Set("old-trymode", oldTryMode) | ||
| t.Set("old-channel", oldChannel) | ||
| t.Set("old-current", oldCurrent) | ||
| - t.Set("had-candidate-in-seq", hadCandidateInSeq) | ||
| + t.Set("added-candidate", addedCandidate) |
pedronis
Jun 30, 2016
Contributor
bit confused here, this is the same a snapst.Current and snapst.Candidate.Current so it's already available in undo
This was referenced Jun 30, 2016
|
undo code looks right now, needs some test in link_snap_test.go though ? |
|
I'm updating the title but description needs updating too |
pedronis
changed the title from
many: add `snap rollback`
to
many: add `snap revert`
Jun 30, 2016
|
apart description and missing test LGTM |
pedronis
reviewed
Jun 30, 2016
| @@ -161,6 +162,43 @@ func (snapst *SnapState) CurrentSideInfo() *snap.SideInfo { | ||
| panic("cannot find snapst.Current in the snapst.Sequence") | ||
| } | ||
| +func (snapst *SnapState) PreviousSideInfo() *snap.SideInfo { |
pedronis
Jun 30, 2016
Contributor
should this be private? or have a comment?
should maybe start with a check for HasCurrent as CurrentSideInfo does
mvo5
added some commits
Jun 30, 2016
niemeyer
reviewed
Jun 30, 2016
| + if !hadCandidate { | ||
| + i := snapst.findIndex(snapst.Current) | ||
| + if i < 0 { | ||
| + return fmt.Errorf("internal error: cannot find revision %d in %v for undoing the added revision", snapst.Candidate.Revision, snapst.Sequence) |
niemeyer
Jun 30, 2016
Contributor
This is unreachable. If i < 0, Sequence was indexed with a negative number just above, and so we had a panic.
The error checking is good, though. Let's just move this up and put the index on a var:
currentIndex := snapst.findIndex(snapst.Current)
if currentIndex < 0 { ... }
snapst.Candidate = ...
if !hadCandidate { ... }
|
Just that one trivial detail, and LGTM |
pedronis
reviewed
Jun 30, 2016
| @@ -247,3 +247,95 @@ func (s *linkSnapSuite) TestDoLinkSnapSuccessCoreRestarts(c *C) { | ||
| c.Check(t.Log()[0], Matches, `.*INFO snap "core" at revision 33 made available to the system.`) | ||
| c.Check(t.Log()[1], Matches, `.*INFO Restarting snapd\.\.\.`) | ||
| } | ||
| + | ||
| +func (s *linkSnapSuite) TestDoUndoLinkSnapRevertNotHadIndex(c *C) { |
pedronis
reviewed
Jun 30, 2016
| + c.Check(t.Status(), Equals, state.UndoneStatus) | ||
| +} | ||
| + | ||
| +func (s *linkSnapSuite) TestDoUndoLinkSnapRevertHadIndex(c *C) { |
pedronis
reviewed
Jun 30, 2016
| + var prepare, prev *state.Task | ||
| + // if we have a revision here we know we need to go back to that | ||
| + // revision | ||
| + if ss.SnapPath != "" || !ss.Revision.Unset() { | ||
| prepare = s.NewTask("prepare-snap", fmt.Sprintf(i18n.G("Prepare snap %q"), ss.SnapPath)) |
mvo5 commentedJun 6, 2016
•
Edited 1 time
-
mvo5
Jun 30, 2016
This branch adds a
snap revertcommand that allows going back to the previous version of a given snap.