overlord/snapstate: parallel snap install #5561

bboozzoo · 2018-07-26T12:08:26Z

A set of changes to snapstate that add support for parallel snap installation. The PR will currently conflict with #5452 regarding SnapAction struct fields. Once #5452 lands I'll update this PR as needed.

There is also a branch with changes to snapstate.InstallPath() right here https://github.com/bboozzoo/snapd/commits/bboozzoo/parallel-install-daemon I'll push those changes as a separate PR.

Feel free to indicate where more testing is needed.

codecov-io · 2018-07-26T12:35:57Z

Codecov Report

Merging #5561 into master will decrease coverage by <.01%.
The diff coverage is 97.67%.

@@            Coverage Diff             @@
##           master    #5561      +/-   ##
==========================================
- Coverage   78.96%   78.95%   -0.01%     
==========================================
  Files         522      522              
  Lines       39694    39711      +17     
==========================================
+ Hits        31343    31355      +12     
- Misses       5805     5809       +4     
- Partials     2546     2547       +1

Impacted Files	Coverage Δ
overlord/snapstate/check_snap.go	`81.13% <100%> (+0.48%)`	⬆️
overlord/snapstate/snapstate.go	`81.5% <100%> (+0.18%)`	⬆️
overlord/snapstate/backend/setup.go	`52.63% <100%> (+1.28%)`	⬆️
overlord/snapstate/handlers.go	`71.88% <80%> (+0.02%)`	⬆️
cmd/snap/cmd_aliases.go	`91.37% <0%> (-1.73%)`	⬇️
overlord/hookstate/hookmgr.go	`73.55% <0%> (-0.97%)`	⬇️
overlord/ifacestate/handlers.go	`64.88% <0%> (-0.4%)`	⬇️

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 3fdeae0...d07429f. Read the comment docs.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…-install-overlord-wip

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…nstall-overlord-wip

pedronis

Seems there's a real issue with download bits or test confusion.

Also not sure if alias code bits don't need changes or they are not here and will be in a follow up?

The SnapSetup construction in applyAutoAliasesDelta looks wrong for sure, missing InstanceKey.

pedronis · 2018-07-27T15:01:05Z

overlord/snapstate/snapstate.go

-		Channel:  channel,
+		SideInfo:    snapst.CurrentSideInfo(),
+		Channel:     channel,
+		InstanceKey: snapst.InstanceKey,


is this change tested? I see tests for most changes here, but not this one

Added a run through test for switch.

pedronis · 2018-07-27T15:06:59Z

overlord/snapstate/backend/setup.go

+
+	_, s.InstanceKey = snap.SplitInstanceName(instanceName)
+
+	// TODO parallel-install: enforce snap.Info.SnapName() == snap name from instanceName


it seems slightly late to do that here though, it would go earlier in checkSnap

There's a PR with some changes to snap.Validate() that will catch this. OTOH it's no harm to check it in snapstate as well.

pedronis · 2018-07-27T15:10:52Z

overlord/snapstate/backend_test.go

@@ -142,7 +142,7 @@ func (f *fakeStore) SnapInfo(spec store.SnapSpec, user *auth.UserState) (*snap.I
 	f.pokeStateLock()

 	sspec := snapSpec{
-		Name: spec.Name,
+		Name: snap.InstanceSnap(spec.Name),


I was confused, here we don't need this, we need to check that Name doesn't have an instance-key, this code might also be unused right now though

pedronis · 2018-07-27T15:14:09Z

overlord/snapstate/snapstate_test.go

+		},
+		{
+			op:   "storesvc-download",
+			name: "some-snap_instance",


this looks like a real bug or test confusion, download shouldn't receive an instance name. I don't think we changed store to support that.

either way, we probably should add a check about that in the fake download code

I rechecked download uses the name only for errors afaict, on the other end some of those would be strange with the instance name, like the ones about paid snaps (you pay once, not per instance), something to think about

I'll tweak the code to use snap name instead. Also will record target path in the tests.

as I wrote, the question is whether to pass instance-name or snap-name to Download, it matters mostly for error messages but some might be confusing with instance-name, and some might be better with, I don't know if it means we should teach it about instance-name and splitting. Easiest for a start would be to pass snap-name

…pInfo Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis · 2018-07-30T12:32:51Z

The SnapSetup construction in applyAutoAliasesDelta looks wrong for sure, missing InstanceKey.

could you add at least a general TODO about alias code there?

punctual issues have been addressed, won't be able to do a full deep 2nd pass

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…file Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

chipaca · 2018-08-01T11:56:46Z

overlord/snapstate/check_snap.go

@@ -208,6 +208,13 @@ func checkSnap(st *state.State, snapFilePath string, si *snap.SideInfo, curInfo
 		return err
 	}

+	snapName, instanceKey := snap.SplitInstanceName(instanceName)
+	if instanceKey != "" && snapName != s.SnapName() {
+		return fmt.Errorf("cannot install snap %q using instance name %q", s.SnapName(), instanceName)


why do we only error here if instanceKey != ""?

After going a bit back and forth with this one, I've moved the check down, after all callbacks are executed. The assumption is that the callbacks are likely to yield more detailed errors (if any), as for instance happens with checkGadgetOrKernel() which returns cannot replace gadget snap with a different one while the snap name check would return cannot install snap "zgadget" using instance name "gadget". I'm happy to adjust it further if needed,

Oh and dropped instanceKey != "" too.

stolowski

Wow, that's a massive change! First pass, just some minors.

stolowski · 2018-08-07T15:37:45Z

overlord/snapstate/backend_test.go

+	if len(split) >= 2 {
+		// <snap>_<rev>.snap
+		// <snap>_<instance-key>_<rev>.snap
+		name = split[0]


Looks like the logic of SplitInstanceName helper, can you use it instead?

I'm afraid it might be a bit confusing as the input is not an instance name but a file name.

I see, thanks.

stolowski · 2018-08-07T15:48:05Z

overlord/snapstate/snapstate_test.go

+	err = s.state.Get("snaps", &snaps)
+	c.Assert(err, IsNil)
+
+	snapst := snaps["some-snap_instance"]


Can you add c.Assert(snapst, NotNil) before other checks? Will give quicker diagnosis if something breaks, instead of panic'ing.

…nstall-overlord-wip

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Add support for opening snaps from directory (as in snap try). Otherwise snap info is populated with incorrect data and snap name checks will fail. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Tweak the handling of snap name in snap cheks. Allow registered callbacks to run first as they may return more precise/informative errors. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

stolowski

Looks good (but probably still needs review from Samuele). Thanks!

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis

lgtm, couple of comments left

pedronis · 2018-08-15T14:34:32Z

overlord/snapstate/check_snap.go

 	for _, check := range checkSnapCallbacks {
 		err := check(st, s, curInfo, flags)
 		if err != nil {
 			return err
 		}
 	}

+	if snapName != s.SnapName() {


do we need a test for this when instanceKey == ""

Test added.

pedronis · 2018-08-15T14:37:26Z

overlord/snapstate/snapstate.go

@@ -843,6 +845,7 @@ func applyAutoAliasesDelta(st *state.State, delta map[string][]string, op string

 		snapsup := &SnapSetup{
 			SideInfo: &snap.SideInfo{RealName: snapName},
+			// TODO parallel-install: include instance key


we probably want a more general TODO about the whole alias code vs instate keys and parallel installs

Tweaked the comment a little bit.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…nstall-overlord-wip

bboozzoo requested a review from pedronis July 26, 2018 12:09

bboozzoo mentioned this pull request Jul 27, 2018

store, overlord/snapstate: introduce instance name in store APIs #5452

Merged

bboozzoo added 4 commits July 27, 2018 10:35

overlord/snapstate: pass instance name to backend.SetupSnap()

c9a3596

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate/backend: support instance name in SetupSnap()

65222be

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: parallel snap instances

f939475

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: update to match changes in store

912b03d

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo force-pushed the bboozzoo/parallel-install-overlord branch from 18d926e to 912b03d Compare July 27, 2018 08:41

bboozzoo added 3 commits July 27, 2018 11:45

Merge branch 'bboozzoo/parallel-install-store' into bboozzoo/parallel…

9f7dca2

…-install-overlord-wip

overlord/snapstate: fix use of instance names in fake store

b03655d

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/parallel-i…

1a1c89b

…nstall-overlord-wip

pedronis previously requested changes Jul 27, 2018

View reviewed changes

bboozzoo added 4 commits July 30, 2018 14:07

overlord/snapstate: bail out if passed instance name in fakestore Sna…

2edfe9c

…pInfo Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: do not pass instance name to store.Download()

12c65f9

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: record target path of store downloads

6068fb2

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: add test for snap switch

7c992cd

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added 4 commits July 30, 2018 15:58

overlord/snapstate: aliases: leave TODO

56a9ccf

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate/backend: pass instance name to SetupSnap

74ac75f

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: pass instance name to checkSnap()

c075f5f

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: verify instance name matches with snap meta from …

44661b0

…file Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added the Parallel installs label Jul 31, 2018

chipaca approved these changes Aug 1, 2018

View reviewed changes

bboozzoo mentioned this pull request Aug 2, 2018

[WIP] Parallel installs integration #5596

Closed

stolowski suggested changes Aug 7, 2018

View reviewed changes

bboozzoo added 4 commits August 8, 2018 11:28

Merge remote-tracking branch 'origin/master' into bboozzoo/parallel-i…

0271f30

…nstall-overlord-wip

overlord/snapstate: add NotNil checks when indexing snaps map directly

3dcb553

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: support directory snaps in fake backend

3b15a12

Add support for opening snaps from directory (as in snap try). Otherwise snap info is populated with incorrect data and snap name checks will fail. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: tweak snap instance name checks

9be09a5

Tweak the handling of snap name in snap cheks. Allow registered callbacks to run first as they may return more precise/informative errors. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

stolowski approved these changes Aug 8, 2018

View reviewed changes

overlord/snapstate: gofmt

7349aae

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis approved these changes Aug 15, 2018

View reviewed changes

bboozzoo added 2 commits August 16, 2018 12:12

overlord/snapstate: extend check snap tests, tweak code comments

68d3825

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/parallel-i…

d07429f

…nstall-overlord-wip

bboozzoo merged commit 8f466e4 into snapcore:master Aug 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord/snapstate: parallel snap install #5561

overlord/snapstate: parallel snap install #5561

bboozzoo commented Jul 26, 2018

codecov-io commented Jul 26, 2018 •

edited

pedronis left a comment

pedronis Jul 27, 2018

bboozzoo Jul 30, 2018

pedronis Jul 27, 2018

bboozzoo Jul 30, 2018

pedronis Jul 27, 2018

pedronis Jul 27, 2018

pedronis Jul 27, 2018

pedronis Jul 27, 2018

bboozzoo Jul 30, 2018

pedronis Jul 30, 2018

pedronis commented Jul 30, 2018 •

edited

chipaca Aug 1, 2018

bboozzoo Aug 8, 2018 •

edited

stolowski left a comment

stolowski Aug 7, 2018

bboozzoo Aug 8, 2018

stolowski Aug 8, 2018

stolowski Aug 7, 2018

bboozzoo Aug 8, 2018

stolowski left a comment

pedronis left a comment

pedronis Aug 15, 2018

bboozzoo Aug 16, 2018

pedronis Aug 15, 2018

bboozzoo Aug 16, 2018


		_, s.InstanceKey = snap.SplitInstanceName(instanceName)

		// TODO parallel-install: enforce snap.Info.SnapName() == snap name from instanceName

overlord/snapstate: parallel snap install #5561

overlord/snapstate: parallel snap install #5561

Conversation

bboozzoo commented Jul 26, 2018

codecov-io commented Jul 26, 2018 • edited

Codecov Report

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

Choose a reason for hiding this comment

pedronis commented Jul 30, 2018 • edited

Choose a reason for hiding this comment

bboozzoo Aug 8, 2018 • edited

Choose a reason for hiding this comment

stolowski 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

stolowski left a 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

codecov-io commented Jul 26, 2018 •

edited

pedronis commented Jul 30, 2018 •

edited

bboozzoo Aug 8, 2018 •

edited