overlord/patch: support for sublevel patches #5683

stolowski · 2018-08-20T10:20:09Z

Support for sub-level patches and that do not prevent rollback. The patches become a map of arrays, e.g. patches[level] = {sublevelPatch1, sublevelPatch2...}.

This is needed for #5497.

codecov-io · 2018-08-20T10:50:07Z

Codecov Report

Merging #5683 into master will decrease coverage by 0.01%.
The diff coverage is 74.74%.

@@            Coverage Diff            @@
##           master   #5683      +/-   ##
=========================================
- Coverage   79.11%   79.1%   -0.02%     
=========================================
  Files         530     530              
  Lines       40444   40520      +76     
=========================================
+ Hits        31999   32052      +53     
- Misses       5853    5865      +12     
- Partials     2592    2603      +11

Impacted Files	Coverage Δ
overlord/patch/patch2.go	`86.2% <100%> (ø)`	⬆️
overlord/patch/patch3.go	`100% <100%> (ø)`	⬆️
overlord/patch/patch1.go	`58.06% <100%> (ø)`	⬆️
overlord/patch/patch5.go	`6.66% <100%> (ø)`	⬆️
overlord/patch/patch6.go	`85.36% <100%> (ø)`	⬆️
overlord/patch/patch4.go	`65.18% <100%> (ø)`	⬆️
overlord/patch/patch.go	`76.03% <73.11%> (-15.08%)`	⬇️
overlord/hookstate/hookmgr.go	`74.51% <0%> (+0.96%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7f2b64...b6c366f. Read the comment docs.

chipaca

Say there's a backward-compatible transformation that you need to apply state. You do it with a sublevel patch and release it in version N. A user in on version M<N; they refresh to N, but then roll back to M for a while before refreshing to N once again. The second time they refresh to N, the transformation isn't applied (because the sublevel is already there), so they now can have bits of their state untransformed.

So, we need to handle untransformed state anyway. Why have subpatches at all?

stolowski · 2018-08-21T10:46:21Z

@chipaca dang, you do have a point, I missed this scenario. I suppose we could update state sublevel when we detect a downgrade, so that subpatches are re-applied later when the user updates again.
The main motivation/advantage is to have all the state transformations in well defined place (and not have to iterate/inspect state if we know there is nothning to transform).

niemeyer · 2018-08-21T16:31:08Z

So, we need to handle untransformed state anyway. Why have subpatches at all?

For the same reason we have patches, I think. We could always lump everything together in a single changeset that then needs all the smarts internally to verify when it really has to be applied. But with a patch system we can tell exactly what needs to be applied right now.

With sublevels, the situation arguably gets a bit more difficult because we're allowing the changeset to be "redone" when rolling backwards and forwards again, but it's still not so different. Which exact patches need to be applied right now when we just upgraded? Well, it's the delta between the prior level+sublevel and the current level+sublevel.

niemeyer

Looking pretty good, thanks Pawel.

A few suggestions:

niemeyer · 2018-08-21T16:32:37Z

overlord/patch/patch.go

+type PatchFunc func(s *state.State) error
+
+// patches maps from patch level L to the list of sublevel patches.
+var patches = make(map[int][]PatchFunc)


Nice structure.

niemeyer · 2018-08-21T16:51:16Z

overlord/patch/patch.go

+	if err == nil || err == state.ErrNoState {
+		err = s.Get("patch-sublevel", &stateSublevel)
+		if err == state.ErrNoState && stateLevel <= 6 {
+			// accommodate for the fact that sublevel patches got introduced at patch level 6.


We shouldn't have to special case patch 6. We can represent the fact that a level has a single sublevel in exactly the same, before and after patch 6, and no matter what the current sublevel is. Although I haven't finished reading the logic, this is probably important as we need to keep in mind that the current system may not actually be in patch level 6. So if there's something special about it, it's probably a bug.

niemeyer · 2018-08-21T17:01:23Z

overlord/patch/patch.go

-// patches maps from patch level L to the function that moves from L-1 to L.
-var patches = make(map[int]func(s *state.State) error)
+// Sublevel is the current implemented sublevel for the Level. Sublevel patches do not prevent rollbacks.
+var Sublevel = 1


It seems to make more sense to say that the first sublevel on a level is zero (6.0). Seems more natural both in terms of the number (6.0) and also in the sense that this is the number we cannot go backwards from, as it introduced the changes of level N. It also makes more sense for indexing.. (patches[level][0] is the first patch of the given level).

Logic below will need to be adapted for that.

niemeyer · 2018-08-21T17:10:18Z

overlord/patch/patch.go

+
+func applySublevelPatches(level, start int, s *state.State) error {
+	for sublevel := start; sublevel < len(patches[level]); sublevel++ {
+		logger.Noticef("Patching system state from level %d, sublevel %d to sublevel %d", level, sublevel, sublevel+1)


We just logged the level we're coming from and the one we're going to.. we can be more terse here, and perhaps avoid saying something for the first first sublevel, even because the first sublevel will be wrong otherwise (there's no sublevel FIRST-1).

How about:

if sublevel > 0 { logger.Noticef("Patching system state level %d to sublevel %d...", level, sublevel) }

niemeyer · 2018-08-21T17:11:07Z

overlord/patch/patch.go

+}
+
+func applySublevelPatches(level, start int, s *state.State) error {
+	for sublevel := start; sublevel < len(patches[level]); sublevel++ {


Curiously, this logic already assumes that the first level is zero. Minor inconsistency, or hidden bug?

This method is used to apply a series of patches for given level; we start from 0 when upgrading to a new level. I hope it makes more sense now after I adjusted all the surrounding code per your other comments.

niemeyer · 2018-08-21T17:17:34Z

overlord/patch/patch.go

+
+	// at the lower Level - apply all new level and sublevel patches
+	for level := stateLevel; level < Level; level++ {
+		pp := patches[level+1]


pp?

I suggest something like sublevels instead.

niemeyer · 2018-08-21T17:20:09Z

overlord/patch/patch.go

+	// at the lower Level - apply all new level and sublevel patches
+	for level := stateLevel; level < Level; level++ {
+		pp := patches[level+1]
+		logger.Noticef("Patching system state from level %d to %d, sublevel", level, level+1, len(pp))


Wrong number of parameters.. string is missing a placeholder.

I suggest dropping the sublevels and keeping the string as it is here. This is indeed moving from patch level N to N+1, which is the incompatible change and the most important one. Further sublevels will then log their individual messages per the other suggestion above.

niemeyer · 2018-08-22T10:20:40Z

Per discussion with @pedronis online, there's also the issue of detecting when a revert to a 6.0 release happens. The old code won't know to reset the sublevel back to zero, which means that when an upgrade happens next sublevel 1 and on won't apply again.

To fix that, we can look at the state and see the history of revisions for core. Something along those lines:

Is state level 6 and state sublevel > 0? If so, continue with the following...
In the history of the core snap in state, is the previously installed core revision known as a patch 6 sublevel 0? If so, continue with the following...
Is the last core refresh timestamp different from the stored patch-sublevel-reset? If so, continue with the following
Update patch-sublevel to zero
Update patch-sublevel-reset to the current core refresh timestamp

@pedronis @stolowski Does that sound right?

…ack.

stolowski · 2018-08-24T07:57:38Z

This is ready for re-review.

pedronis

some questions/comments

pedronis · 2018-08-28T18:47:56Z

overlord/overlord_test.go

@@ -61,7 +61,7 @@ func (ovs *overlordSuite) TearDownTest(c *C) {
 }

 func (ovs *overlordSuite) TestNew(c *C) {
-	restore := patch.Mock(42, nil)
+	restore := patch.Mock(42, 2, nil)


this test should also check sublevel for completeness I think

pedronis · 2018-08-29T07:40:39Z

overlord/patch/patch.go

+		return fmt.Errorf("internal error: couldn't find current core revision in the snap sequence")
+	}
+
+	prevRev := snapst.Sequence[currentIndex-1].Revision


in the case of a/because of revert the values in Sequence after currentIndex might count too

pedronis · 2018-08-29T07:40:49Z

overlord/patch/patch.go

+		return err
+	}
+	if len(snapst.Sequence) < 2 {
+		return nil


this case doesn't seem to be tested

pedronis · 2018-08-29T07:44:25Z

overlord/patch/patch.go

+
+	var sublevelResetTime time.Time
+	var lastRefresh time.Time
+	if err := s.Get("last-refresh", &lastRefresh); err != nil && err != state.ErrNoState {


last-refresh is the last auto-refresh only, is not the last core refresh time, the closest to that would be the /snap/core/current symlink time

pedronis · 2018-08-29T07:53:32Z

overlord/patch/patch.go

+
+	// at the lower Level - apply all new level and sublevel patches
+	for level := stateLevel; level < Level; level++ {
+		sublevels := patches[level+1]


the +1 could be moved to the for loop

level := stateLevel+1 ; level <= Level

no?

Yes. Done (small downside of doing it is "-1" needed for the debug messages though).

niemeyer · 2018-08-30T12:53:48Z

overlord/patch/patch.go

 	"github.com/snapcore/snapd/logger"
+	"github.com/snapcore/snapd/overlord/snapstate"


I've reviewed this as part of a follow up PR by mistake. This cannot be here.. please see the review there for details.

Thanks, fixed.

… revision in sequence.

pedronis

some small comments

pedronis · 2018-08-31T12:10:35Z

overlord/patch/patch.go

+	}
+
+	raw, ok := snaps["core"]
+	if err == state.ErrNoState || !ok {


why the err check here? is checked just above

Indeed, not needed, thanks.

pedronis · 2018-08-31T12:11:33Z

overlord/patch/patch.go

+
+func getCoreRefreshTime() (time.Time, error) {
+	path := filepath.Join(dirs.SnapMountDir, "core", "current")
+	info, err := os.Stat(path)


I think we want Lstat here, no?

Yes we do, thanks for spotting!

pedronis · 2018-08-31T12:12:46Z

overlord/patch/patch_test.go

+		os.Remove(currentPath)
+	}
+}
+func (s *patchSuite) TestRefreshBackFromLevel60ShortSequence(c *C) {


missing separator empty line before this?

pedronis

thank you

niemeyer

LGTM, thanks!

Support for sublevel patches.

118787b

stolowski closed this Aug 20, 2018

stolowski added 2 commits August 21, 2018 09:11

Merge branch 'master' into patch-sublevel

7d1edc4

Support sublevel patches for every future level.

03400dc

stolowski reopened this Aug 21, 2018

Use int in the test to make sure the patch is applied exactly once.

05feabd

chipaca reviewed Aug 21, 2018

View reviewed changes

Update patch-sublevel when downgrading within same level.

2b6ee77

niemeyer requested changes Aug 21, 2018

View reviewed changes

niemeyer requested a review from pedronis August 22, 2018 13:01

stolowski added 6 commits August 22, 2018 18:21

Review comments, handle the case of reverting to 6.0 and refreshing b…

a7d60a9

…ack.

Handle revision 6.0, more tests.

6e92aa6

Merge branch 'master' into patch-sublevel

8e42c06

Added a comment.

77bf8b1

Simplify the tests.

822e314

Updated core rev number.

b5d4338

stolowski added 2 commits August 24, 2018 09:58

Merge branch 'master' into patch-sublevel

c38c2cc

Fix off-by-one bug concerning current state sublevel.

9dc1721

stolowski mentioned this pull request Aug 24, 2018

overlord/patch: patch for static plug/slot attributes #5497

Merged

stolowski added 2 commits August 24, 2018 13:15

Fix expected sublevel in the test.

3054e1c

Merge branch 'master' into patch-sublevel

6eb86c4

pedronis reviewed Aug 29, 2018

View reviewed changes

stolowski added 3 commits August 30, 2018 11:34

Merge branch 'master' into patch-sublevel

d55ea8d

TestRefreshBackFromLevel60ShortSequence test.

b92d60c

Moved +1 to the loop.

f94fac4

Determine core refresh time from current symlink.

208369e

niemeyer requested changes Aug 30, 2018

View reviewed changes

stolowski added 4 commits August 30, 2018 18:54

Don't depend on snapstate struct definition. Check both prev and next…

10b6633

… revision in sequence.

Improved the test.

5d12c0d

Use time.Equal for time comparision.

086fdfe

Merge branch 'master' into patch-sublevel

1cc7b86

pedronis reviewed Aug 31, 2018

View reviewed changes

stolowski added 2 commits September 3, 2018 09:59

Merge branch 'master' into patch-sublevel

d527263

Review feedback (use Lstat, remove unneeded error check).

ce0a94c

pedronis approved these changes Sep 3, 2018

View reviewed changes

Merge branch 'master' into patch-sublevel

afc81a6

niemeyer approved these changes Sep 7, 2018

View reviewed changes

Merge branch 'master' into patch-sublevel

b6c366f

stolowski merged commit 600bb07 into snapcore:master Sep 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord/patch: support for sublevel patches #5683

overlord/patch: support for sublevel patches #5683

stolowski commented Aug 20, 2018 •

edited

codecov-io commented Aug 20, 2018 •

edited

chipaca left a comment

stolowski commented Aug 21, 2018 •

edited

niemeyer commented Aug 21, 2018

niemeyer left a comment

niemeyer Aug 21, 2018

niemeyer Aug 21, 2018

niemeyer Aug 21, 2018

niemeyer Aug 21, 2018

niemeyer Aug 21, 2018

stolowski Aug 23, 2018

niemeyer Aug 21, 2018

niemeyer Aug 21, 2018

niemeyer commented Aug 22, 2018

stolowski commented Aug 24, 2018

pedronis left a comment

pedronis Aug 28, 2018

pedronis Aug 29, 2018

stolowski Aug 31, 2018

pedronis Aug 29, 2018

pedronis Aug 29, 2018

pedronis Aug 29, 2018

stolowski Aug 30, 2018

niemeyer Aug 30, 2018

stolowski Aug 30, 2018

pedronis left a comment

pedronis Aug 31, 2018

stolowski Sep 3, 2018

pedronis Aug 31, 2018

stolowski Sep 3, 2018

pedronis Aug 31, 2018

stolowski Sep 3, 2018

pedronis left a comment

niemeyer left a comment

		"github.com/snapcore/snapd/logger"
		"github.com/snapcore/snapd/overlord/snapstate"

overlord/patch: support for sublevel patches #5683

overlord/patch: support for sublevel patches #5683

Conversation

stolowski commented Aug 20, 2018 • edited

codecov-io commented Aug 20, 2018 • edited

Codecov Report

chipaca left a comment

Choose a reason for hiding this comment

stolowski commented Aug 21, 2018 • edited

niemeyer commented Aug 21, 2018

niemeyer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer commented Aug 22, 2018

stolowski commented Aug 24, 2018

pedronis 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

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

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

niemeyer left a comment

Choose a reason for hiding this comment

stolowski commented Aug 20, 2018 •

edited

codecov-io commented Aug 20, 2018 •

edited

stolowski commented Aug 21, 2018 •

edited