cmd/snap-update-ns: teach update logic to handle synthetic changes #4224

zyga · Nov 15, 2017

This patch teaches the mount namespace update logic to cope with
synthetic changes. Such changes are are tagged with x-snapd.synethetic
field. Each syntethic change is tied via the x-snapd.parent-id field
to a non-synthetic change matching the x-snapd.id field.

As long as the non synthetic change is around all synthetic changes
that refer to it are preserved during updates. When the non-synthetic
change is no longer desired all the synthetic changes are undone in the
right order.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

codecov-io · Nov 15, 2017

Codecov Report

Merging #4224 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #4224     +/-   ##
========================================
+ Coverage   76.09%   76.2%   +0.1%     
========================================
  Files         442     444      +2     
  Lines       38675   38721     +46     
========================================
+ Hits        29431   29508     +77     
+ Misses       7222    7196     -26     
+ Partials     2022    2017      -5

Impacted Files	Coverage Δ
cmd/snap-update-ns/utils.go	`100% <100%> (ø)`	⬆️
cmd/snap-update-ns/entry.go	`94% <100%> (+0.25%)`	⬆️
cmd/snap-update-ns/change.go	`98.26% <100%> (+0.67%)`	⬆️
httputil/redirect18.go	`100% <0%> (ø)`
httputil/transport17.go	`100% <0%> (ø)`
overlord/snapstate/snapstate.go	`81.78% <0%> (+0.24%)`	⬆️
overlord/ifacestate/helpers.go	`60.26% <0%> (+0.66%)`	⬆️
cmd/snap-update-ns/main.go	`42.96% <0%> (+3.9%)`	⬆️
cmd/snap-seccomp/main.go	`54.21% <0%> (+7.22%)`	⬆️
httputil/useragent.go	`85% <0%> (+7.5%)`	⬆️

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 4466a78...e370d84. Read the comment docs.

stolowski · Nov 17, 2017

stolowski reviewed Nov 17, 2017

View changes

First quick pass.

It's slightly baffling that XSnapdEntryID and XSnapdParentID can return empty strings if missing, but we don't seem to handle that here. Is this guaranteed not to happen? What happens if we have an antry with snapd.parent-id=e1, but "e1" doesn't exist? Would it make sense to have a test for this?

I changed the approach to make mount IDs implicit. This is actually both easier to handle and works better in practice where existing desired mount profiles don't have them.

jdstrand · Nov 21, 2017

jdstrand requested changes Nov 21, 2017

View changes

I clearly have a lot of questions with this PR. I think this is partly because this is an intermediate PR where other pieces are missing and I don't have the full context. It seems there might be opportunities to improve comments, variable names and the use of the term 'parent'.

I'm going to mark this as 'request changes' for the code comments. Please consider the number of questions as reason to clarify things. I'm happy to review again after my questions are answered and code clarified.

Can you add a comment on why synthetic entries should be reused only if the parent is desired? AIUI, a synthetic can't exist without the parent and so in that case it can't be reused. I think part of the problem for understanding is that a syntethic mount is described as "entries [that] are created by snap-update-ns itself, separately from what snapd instructed. Such entries are needed to make other things possible", but that isn't very specific and I'm not sure what that means wrt this code. Maybe a more specific comment in entry.go is warranted....

I don't see where snapd is setting x-snapd.parent-id or x-snapd.id-- I only see the helpers to parse these. Is this being done in a follow-up PR?

Done

The assignments of needed-by (nee parent-id) and id thing is indeed a follow up.

Perhaps:

// The tmpfs that lets us write into immutable squashfs. We mock x-snapd.parent-id
// to the squashfs mount. Mark it synthetic since it is a helper mount that is needed
// to facilitate the following mounts.

Done

'tmpfs' has 'tmpfs' for the fs_spec/mnt_fsname, not 'none'.

Done

Perhaps:

// A bind mount to preserve a directory hidden by the tmpfs (the mountpoint
// is created elsewhere). We mock x-snapd.parent-id to the tmpfs mount

What is interesting here is that the parent-id of tmpfs mount in the previous profile is for the squashfs mount and the parent-id of this bind mount is for the tmpfs, but they both have the same parent-id. Is this intended? Might this cause confusion?

Perhaps append to the comment: (the mountpoint ('created') is created elsewhere

Based on the placement within the current profiles list, I was expecting that this would have a parent-id on tmpfs.

Also, why don't these (and the other) example mount profiles use x-snapd.id? I was thinking every mount would have x-snapd.id, most would have x-snapd.parent-id and some would have x-snapd.synthetic....

I thought the unmounts were supposed to happen in the reverse order. The current has: 'tmpfs, existing, created', but the unmount is 'existing, created, tmpfs'. Is this intended?

Perhaps more detail on '.../created' would make this test clearer.

s/none/tmpfs/

Done

Why are tmpfs, existing and created all kept (and again, not in the expected order) when only created is desired? I don't see anything in 'created' about a parent-id. If it is because the prefix is the same, can you add a comment stating this?

Ok, this clarifies some of my questions on why x-snapd.id isn't being set in the testsuite, but then, why is x-snapd.id needed at all if we are just going to use e.Dir here? When is x-snapd.id set to the hash? How is that hash calculated?

This is something I was experimenting with. Initially it was just an opaque ID that snapd would generate. Then I realized it cannot be so because that identifier has to be stable so I resorted to hashing the whole entry. Then I realized it actually needs to be implicitly defined so that existing installations upgrade correctly (and by correctly I mean that there are no spurious changes). In the end this just became the mount point. I think that we can simplify this aspect and just use the e.Dir (ill-named for files but still) directly.

"I think that we can simplify this aspect and just use the e.Dir (ill-named for files but still) directly."

Will this happen in a follow-up PR?

I'll do it in a follow up.

s/here/are/

I assume you meant s/are/here/

This comment is illuminating. I think it deserves to be in the actual code, not just the testsuite.

Done, moved to NeededChanges and reworded keeping the meaning.

s/syntetic/synthetic/

Done

Why is /usr/share/mysnap the parent-id? Is the parent-id intended to be the thing that caused this mount? (sorry, I don't see where we set parent-id anywhere so I'm not sure what it is supposed to be). If parent-id is meant to be the thing that caused this mount, I would suggest not calling it 'parent' since the term is overloaded with the term 'parent dir'. Ie, '/usr/share/mysnap' is not the parent dir of '/usr/share', but within the context of this code, it is the parent operation. I think this is the cause of some of my confusion and why I had so many questions up above.

I agree, it should not be called parent. Perhaps needed-by= would suffice?

Again, this comment is illuminating. I think discussing this in the code that does the operation would be worthwhile.

jdstrand · Nov 28, 2017

jdstrand approved these changes Nov 28, 2017

View changes

I'm going to mark this as approved, but please change 'parent-id' to 'needed-by' before committing. There is also one open question inline regarding x-snapd.id and e.Dir and when you would perform that cleanup. It would be nice to have here but can be in a follow-up PR.

zyga · Nov 28, 2017

@jdstrand I changed all parent-id to needed-by. I'll change to e.Dir in a follow-up.

zyga referenced this pull request Nov 28, 2017
Merged
cmd/snap-update-ns: add execWritableMimic #4315

zyga merged commit 42c55de into snapcore:master Nov 29, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

zyga deleted the zyga:feature/synthetic-do-undo branch Nov 29, 2017

+        +	// Collect the IDs of desired changes.
+        +	// We need that below to keep implicit changes from the current profile.
+        +	for i := range desired {
+        +		desiredIDs[XSnapdEntryID(&desired[i])] = true

          			continue
          		}
          		skipDir = "" // reset skip prefix as it no longer applies
+        +
+        +		// Reuse synthetic entries if their parent is desired.
+        +		if XSnapdSynthetic(&current[i]) && desiredIDs[XSnapdParentID(&current[i])] {

+        +// Unused bind mount farms are unmounted.
+        +func (s *changeSuite) TestNeededChangesTmpfsBindMountFarmUnused(c *C) {
+        +	current := &mount.Profile{Entries: []mount.Entry{{
+        +		// The tmpfs that lets us write into immutable squashfs.

+        +func (s *changeSuite) TestNeededChangesTmpfsBindMountFarmUnused(c *C) {
+        +	current := &mount.Profile{Entries: []mount.Entry{{
+        +		// The tmpfs that lets us write into immutable squashfs.
+        +		Name:    "none",

+        +		Type:    "tmpfs",
+        +		Options: []string{"x-snapd.parent-id=/snap/name/42/subdir", "x-snapd.synthetic"},
+        +	}, {
+        +		// A bind mount to preserve a directory hidden by the tmpfs.

snapcore/snapd

cmd/snap-update-ns: teach update logic to handle synthetic changes #4224

zyga commented Nov 15, 2017

codecov-io commented Nov 15, 2017 •

Edited 7 times

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 20, 2017

codecov-io
Nov 20, 2017

stolowski reviewed Nov 17, 2017

View changes

zyga added some commits Nov 20, 2017

jdstrand requested changes Nov 21, 2017

View changes

zyga added some commits Nov 28, 2017

zyga referenced this pull request Nov 28, 2017

cmd/snap-update-ns: add execWritableMimic #4315

jdstrand approved these changes Nov 28, 2017

View changes

zyga commented Nov 28, 2017

zyga merged commit `42c55de` into snapcore:master Nov 29, 2017
1 check passed

1 check passed

zyga deleted the zyga:feature/synthetic-do-undo branch Nov 29, 2017

snapcore/snapd

Join GitHub today

cmd/snap-update-ns: teach update logic to handle synthetic changes #4224

Conversation

zyga commented Nov 15, 2017

codecov-io commented Nov 15, 2017 • edited Edited 7 times codecov-io Nov 28, 2017 codecov-io Nov 28, 2017 codecov-io Nov 28, 2017 codecov-io Nov 28, 2017 codecov-io Nov 28, 2017 codecov-io Nov 20, 2017 codecov-io Nov 20, 2017

Codecov Report

stolowski reviewed Nov 17, 2017 View changes

zyga added some commits Nov 20, 2017

Some checks were not successful

jdstrand requested changes Nov 21, 2017 View changes

zyga Nov 28, 2017 • edited Edited 1 time zyga Nov 28, 2017

zyga added some commits Nov 28, 2017

zyga referenced this pull request Nov 28, 2017

jdstrand approved these changes Nov 28, 2017 View changes

zyga commented Nov 28, 2017

Hide details View details zyga merged commit 42c55de into snapcore:master Nov 29, 2017 1 check passed

1 check passed

zyga deleted the zyga:feature/synthetic-do-undo branch Nov 29, 2017

codecov-io commented Nov 15, 2017 •

Edited 7 times

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 28, 2017

codecov-io
Nov 20, 2017

codecov-io
Nov 20, 2017

stolowski reviewed Nov 17, 2017

View changes

jdstrand requested changes Nov 21, 2017

View changes

zyga Nov 28, 2017 •

Edited 1 time

zyga
Nov 28, 2017

jdstrand approved these changes Nov 28, 2017

View changes

zyga merged commit `42c55de` into snapcore:master Nov 29, 2017
1 check passed