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: fix test cases to work with different DistroLibExecDir #3222
Conversation
| @@ -90,7 +91,7 @@ func ExecInCoreSnap() { | ||
| // ensure we do not re-exec into an older version of snapd, look | ||
| // for info file and ignore version of core that do not yet have | ||
| // it | ||
| - fullInfo := filepath.Join(corePath, "/usr/lib/snapd/info") |
zyga
Apr 24, 2017
Contributor
Wait, this feels wrong. If core path is the path where core is mounted the rest of the string can be a constant because it's not depending on the distribution anymore.
morphis
Apr 24, 2017
•
Edited 1 time
-
morphis
Apr 24, 2017
Contributor
-
Apr 24, 2017
I will look into this again. However all tests except refresh-delta-from-core on Ubuntu 14.04 seem to pass. However for Fedora/SUSE/Arch/... this might fail. We really need CI for all of these.
zyga
Apr 27, 2017
Contributor
Wait, this is not what I expected (the diff looked different before). So on Fedora the core path is /var/lib/snapd/snap/core/current/ but then the dirs.DistroLibExecDir will contain /usr/libexec/snapd, I don't understand how this is supposed to work.
morphis
Apr 27, 2017
Contributor
@zyga Please note that the code now uses dirs.CoreLibExecDir which will expand to /usr/lib/snapd even on Fedora. A previous version of the code was using dirs.DistroLibExecDir which is, as you pointed out, wrong.
In summary: The new code filepath.Join(corePath, dirs.CoreLibExecDir, "info") will take the corePath as determined before through /proc/self/exe and append dirs.CoreLibExecDir so that we end up with /snap/core/current/usr/libexec/snapd/info on Ubuntu and /var/lib/snapd/snap/core/current/usr/libexec/snapd/info
| @@ -449,8 +450,8 @@ func (s *SnapSuite) TestSnapRunIsReexeced(c *check.C) { | ||
| readlink string | ||
| expected bool | ||
| }{ | ||
| - {filepath.Join(dirs.SnapMountDir, "/usr/lib/snapd/snapd"), true}, | ||
| - {"/usr/lib/snapd/snapd", false}, | ||
| + {filepath.Join(dirs.SnapMountDir, dirs.DistroLibExecDir, "snapd"), true}, |
morphis
Apr 24, 2017
Contributor
For now yes, but when we have switch to different core/base snaps it may not. Using dirs.CoreLibExecDir here now.
zyga
Apr 24, 2017
Contributor
using CoreLibExecDir is fine. For core/base split I don't know how it will work yet so it's hard to say.
| @@ -90,7 +91,7 @@ func ExecInCoreSnap() { | ||
| // ensure we do not re-exec into an older version of snapd, look | ||
| // for info file and ignore version of core that do not yet have | ||
| // it | ||
| - fullInfo := filepath.Join(corePath, "/usr/lib/snapd/info") |
zyga
Apr 24, 2017
Contributor
Wait, this feels wrong. If core path is the path where core is mounted the rest of the string can be a constant because it's not depending on the distribution anymore.
morphis
Apr 24, 2017
•
Edited 1 time
-
morphis
Apr 24, 2017
Contributor
-
Apr 24, 2017
I will look into this again. However all tests except refresh-delta-from-core on Ubuntu 14.04 seem to pass. However for Fedora/SUSE/Arch/... this might fail. We really need CI for all of these.
zyga
Apr 27, 2017
Contributor
Wait, this is not what I expected (the diff looked different before). So on Fedora the core path is /var/lib/snapd/snap/core/current/ but then the dirs.DistroLibExecDir will contain /usr/libexec/snapd, I don't understand how this is supposed to work.
morphis
Apr 27, 2017
Contributor
@zyga Please note that the code now uses dirs.CoreLibExecDir which will expand to /usr/lib/snapd even on Fedora. A previous version of the code was using dirs.DistroLibExecDir which is, as you pointed out, wrong.
In summary: The new code filepath.Join(corePath, dirs.CoreLibExecDir, "info") will take the corePath as determined before through /proc/self/exe and append dirs.CoreLibExecDir so that we end up with /snap/core/current/usr/libexec/snapd/info on Ubuntu and /var/lib/snapd/snap/core/current/usr/libexec/snapd/info
| -func mockSnapConfine() func() { | ||
| - snapConfine := filepath.Join(dirs.DistroLibExecDir, "snap-confine") | ||
| - if err := os.MkdirAll(dirs.DistroLibExecDir, 0755); err != nil { | ||
| +func mockSnapConfine(reexec bool) func() { |
zyga
Apr 25, 2017
Contributor
Just nitpick. I'd prefer a flag like ReExecEnabled or something similar. The false/true that is in tests above is hard to grok just by looking.
| @@ -228,7 +228,11 @@ func isReexeced() bool { | ||
| } | ||
| func runSnapConfine(info *snap.Info, securityTag, snapApp, command, hook string, args []string) error { | ||
| - snapConfine := filepath.Join(dirs.DistroLibExecDir, "snap-confine") | ||
| + libExecDir := dirs.DistroLibExecDir | ||
| + if isReexeced() { |
niemeyer
Apr 26, 2017
Contributor
This should be done only once in this function. Right now it's being done twice, and it's also bogus because code right below will test for a path that does not exist. Proper way is likely to bring the code currently at the bottom here, and make sure snapConfine points to the proper thing upfront.
| -func mockSnapConfine() func() { | ||
| - snapConfine := filepath.Join(dirs.DistroLibExecDir, "snap-confine") | ||
| - if err := os.MkdirAll(dirs.DistroLibExecDir, 0755); err != nil { | ||
| +func mockSnapConfine(reexec bool) func() { |
zyga
Apr 25, 2017
Contributor
Just nitpick. I'd prefer a flag like ReExecEnabled or something similar. The false/true that is in tests above is hard to grok just by looking.
|
I'm confused... Why are so many things changing from DistroLibExecDir to CoreLibExecDir? That seems to indicate it'd be running/testing a completely different/wrong thing... |
|
@Conan-Kudo Many of the changes are for paths are in the code path for the re-execution feature which never worked on Fedora/Suse. So you never saw anything going wrong. This PR fixes those cases and brings us a bit closer to working re-execution too. |
|
Looks good, but some review comments (like #3222 (comment) are not addressed yet) |
|
Fixed all review comments and rebased on latest master |
|
Some unit test failures, e.g. https://travis-ci.org/snapcore/snapd/builds/226369117#L2580 |
|
@mvo5 Pushed a change to fix the failing unit tests. |
| @@ -195,11 +199,11 @@ apps: | ||
| func (s *ContentSuite) TestResolveSpecialVariable(c *C) { | ||
| info := snaptest.MockInfo(c, "name: name", &snap.SideInfo{Revision: snap.R(42)}) | ||
| - c.Check(builtin.ResolveSpecialVariable("foo", info), Equals, "/snap/name/42/foo") |
zyga
May 5, 2017
Contributor
Sorry for taking so long to verify this. This part is incorrect. The generated mount profile is only processed after pivot_root has happened so it must use the internal layout.
morphis
May 5, 2017
•
Edited 1 time
-
morphis
May 5, 2017
Contributor
-
May 5, 2017
How that? On Debian & Ubuntu we have /snap and on Fedora we have /var/lib/snapd/snap in and outside of the snap or do I miss anything?
zyga
May 5, 2017
Contributor
You are missing one thing. After pivot_root whatever the snap mount directory is it ends up being /snap. Only then do we process those mount directives so the hard-coded paths are actually always correct.
morphis
May 5, 2017
Contributor
Running the following
$ snap run --shell hello-world
bash-4.3$ echo $SNAP
/var/lib/snapd/snap/hello-world/27
Please also note that I didn't change builting.ResolveSpecialVariable. It is using dirs.BaseSnapMountDir already without this PR. See https://github.com/snapcore/snapd/blob/master/interfaces/builtin/content.go#L123
zyga
May 5, 2017
Contributor
As discussed on IRC there are a host of bugs here:
- snap execution environment should look the same everywhere
- $SNAP is always /snap/$SNAP_NAME/$SNAP_REVISION when seen from inside snap execution environment
- various other things are wrong but we'll discuss and fix them in separate PRs
mvo5
Jun 7, 2017
Collaborator
Silly question - if the code got changed to use /snap as static prefix, do we still need to change:
- "/snap/foo"
+ filepath.Join(dirs.CoreSnapMountDir, "foo")
I guess the answer is yes to make it more obvious when we mean "context-is-inside-core" and "context-is-outside"(?)
morphis
Jun 7, 2017
Contributor
I would say, we should do it this way, With that we always have a clear bind to the context and know what we're dealing with.
| execute: | | ||
| mkdir -p /tmp/unit-tests/src/github.com/snapcore | ||
| cp -ar $PROJECT_PATH /tmp/unit-tests/src/github.com/snapcore | ||
| chown -R test:12345 /tmp/unit-tests | ||
| su -l -c "cd /tmp/unit-tests/src/github.com/snapcore/snapd && GOPATH=/tmp/unit-tests ./run-checks --unit" test | ||
| + | ||
| + mv /etc/os-release /etc/os-release.orig |
zyga
May 5, 2017
Contributor
This is more of a prepare: stage to me but the actual section looks curious. Is this intentional?
| @@ -126,9 +128,11 @@ func SetRootDir(rootdir string) { | ||
| switch release.ReleaseInfo.ID { | ||
| case "fedora", "centos", "rhel", "arch": | ||
| - SnapMountDir = filepath.Join(rootdir, "/var/lib/snapd/snap") | ||
| + BaseSnapMountDir = "/var/lib/snapd/snap" |
niemeyer
May 10, 2017
Contributor
We can't use the "BaseSnap" term here, even more when we have a matching variable with "CoreSnap". Base snaps are around the block and this is unrelated.
morphis
May 11, 2017
Contributor
HostSnapMountDir still has a notion of not saying that it is absolute or not. BaseSnapMountDir was my attempt to word that. What about RawSnapMountDir?
niemeyer
May 12, 2017
Contributor
That makes it Raw, except other things raw are not Raw. Still pretty misleading and error prone. Not to mention "raw" doesn't really say much in this context.
See the suggestion in the other point.
| @@ -179,7 +183,8 @@ func SetRootDir(rootdir string) { | ||
| DistroLibExecDir = filepath.Join(rootdir, "/usr/lib/snapd") | ||
| } | ||
| - CoreLibExecDir = filepath.Join(rootdir, "/usr/lib/snapd") | ||
| + CoreLibExecDir = "/usr/lib/snapd" | ||
| + CoreSnapMountDir = "/snap" |
niemeyer
May 10, 2017
Contributor
What's the line for deciding what has a prefix or not? Why do we have BaseSnapMountDir with rootdir, but the ones above not? Hard to guess on call sites, which makes it quite error prone.
morphis
May 11, 2017
•
Edited 1 time
-
morphis
May 11, 2017
Contributor
-
May 11, 2017
We need a variable which has just the clean mount dir without the root prefix as otherwise we end up with something $prefix/$prefix/$snapmountdir in the unit tests. See https://github.com/snapcore/snapd/pull/3222/files#diff-76f691a17e89c1330742e51bb468ef76R203 as an example. The only place where it is ever being used are unit tests. I can move these variables into test code and keep it away from dirs/dirs.go but actually it would be nice to keep these things in a central place.
Also CoreLibExecDir/CoreSnapMountDir are needed as these describe where in the core snap those directories are located. These are different on Fedora than the outside SnapMountDir/LibExecDir so we have to keep them distinct.
pedronis
May 11, 2017
Contributor
if a unit test tries to write to those Core* dirs missing the rootdir will creates errors
morphis
May 11, 2017
Contributor
The unit test doesn't try to write into those dirs. They are used for example in the content interface where we check if the mount entries generated are ok. @zyga told me that those should never use SnapMountDir (which might be prefixed) but the mount dir without any prefix so /snap on Ubuntu and /var/lib/snapd/snap on Fedora.
niemeyer
May 12, 2017
Contributor
I suggest keeping the rootdir as originally, and stripping GlobalRootDir out of it when really needed. That makes it consistent.
| + // CoreLibExecDir is similar to CoreSnapMountDir but for the | ||
| + // LibExecDir used by snapd. | ||
| + CoreLibExecDir string | ||
| + DistroLibExecDir string | ||
| SnapBlobDir string |
added some commits
Apr 24, 2017
All checks have passed
codecov-io
commented
May 17, 2017
•
Edited 1 time
-
codecov-io
May 23, 2017
-
May 23, 2017
Codecov Report
@@ Coverage Diff @@
## master #3222 +/- ##
==========================================
- Coverage 77.16% 77.15% -0.01%
==========================================
Files 373 373
Lines 25793 25792 -1
==========================================
- Hits 19902 19900 -2
Misses 4134 4134
- Partials 1757 1758 +1
Continue to review full report at Codecov.
|
| + SnapMountDir string | ||
| + | ||
| + // CoreSnapMountDir is the path of the snap mount dir inside | ||
| + // the snap confinement environment. Give this nature it will |
| + // These are directories which are static inside the core snap and | ||
| + // can never be prefixed as they will be always absolute once we | ||
| + // are in the snap confinement environment. | ||
| + CoreLibExecDir = "/usr/lib/snapd" |
| @@ -116,13 +117,23 @@ func (iface *contentInterface) path(slot *interfaces.Slot, name string) []string | ||
| return out | ||
| } | ||
| +const ( |
| +const ( | ||
| + SnapMountDir = iota | ||
| + SnapDataDir | ||
| + SnapCommonDir |
added some commits
May 19, 2017
Some checks were not successful
Some checks were not successful
|
@pedronis @zyga @Conan-Kudo @niemeyer Can you have another look so we get this reviewed and merged soon? |
Some checks were not successful
mvo5
reviewed
Jun 7, 2017
Looks good, some comments inline. I am in favour of merging even if imperfect and do a followup to ease the burden of fixing this branch, it has a lot of conflicts potential.
| @@ -98,7 +98,7 @@ func (s *SnapSuite) TestSnapRunAppIntegration(c *check.C) { | ||
| // mock installed snap | ||
| dirs.SetRootDir(c.MkDir()) | ||
| defer func() { dirs.SetRootDir("/") }() | ||
| - defer mockSnapConfine()() | ||
| + defer mockSnapConfine(dirs.DistroLibExecDir)() |
mvo5
Jun 7, 2017
Collaborator
Pardon my ignorance, but it looks like the argument for mockSnapConfine() is always dirs.DistroLibExecDir - do we really need this new argument in this case?
morphis
Jun 7, 2017
Contributor
There is exactly one case where it isn't. See https://github.com/snapcore/snapd/pull/3222/files/f78d7918f26f71239000599fd11db988cd605cc0#diff-78f744cbc3eb83652f8ed773dceac016R466
mvo5
Jun 7, 2017
Collaborator
Aha, silly me. I even found this in subsequent reading but then forgot to delete the comment. Sorry for the noise.
| - {filepath.Join(dirs.SnapMountDir, "/usr/lib/snapd/snapd"), true}, | ||
| - {"/usr/lib/snapd/snapd", false}, | ||
| + {filepath.Join(dirs.SnapMountDir, dirs.CoreLibExecDir, "snapd"), true}, | ||
| + {fmt.Sprintf("%s/snapd", dirs.DistroLibExecDir), false}, |
| @@ -195,11 +199,11 @@ apps: | ||
| func (s *ContentSuite) TestResolveSpecialVariable(c *C) { | ||
| info := snaptest.MockInfo(c, "name: name", &snap.SideInfo{Revision: snap.R(42)}) | ||
| - c.Check(builtin.ResolveSpecialVariable("foo", info), Equals, "/snap/name/42/foo") |
zyga
May 5, 2017
Contributor
Sorry for taking so long to verify this. This part is incorrect. The generated mount profile is only processed after pivot_root has happened so it must use the internal layout.
morphis
May 5, 2017
•
Edited 1 time
-
morphis
May 5, 2017
Contributor
-
May 5, 2017
How that? On Debian & Ubuntu we have /snap and on Fedora we have /var/lib/snapd/snap in and outside of the snap or do I miss anything?
zyga
May 5, 2017
Contributor
You are missing one thing. After pivot_root whatever the snap mount directory is it ends up being /snap. Only then do we process those mount directives so the hard-coded paths are actually always correct.
morphis
May 5, 2017
Contributor
Running the following
$ snap run --shell hello-world
bash-4.3$ echo $SNAP
/var/lib/snapd/snap/hello-world/27
Please also note that I didn't change builting.ResolveSpecialVariable. It is using dirs.BaseSnapMountDir already without this PR. See https://github.com/snapcore/snapd/blob/master/interfaces/builtin/content.go#L123
zyga
May 5, 2017
Contributor
As discussed on IRC there are a host of bugs here:
- snap execution environment should look the same everywhere
- $SNAP is always /snap/$SNAP_NAME/$SNAP_REVISION when seen from inside snap execution environment
- various other things are wrong but we'll discuss and fix them in separate PRs
mvo5
Jun 7, 2017
Collaborator
Silly question - if the code got changed to use /snap as static prefix, do we still need to change:
- "/snap/foo"
+ filepath.Join(dirs.CoreSnapMountDir, "foo")
I guess the answer is yes to make it more obvious when we mean "context-is-inside-core" and "context-is-outside"(?)
morphis
Jun 7, 2017
Contributor
I would say, we should do it this way, With that we always have a clear bind to the context and know what we're dealing with.
| @@ -2689,7 +2719,7 @@ version: 1.0`) | ||
| { | ||
| // ensure only local install was run, i.e. first action is pseudo-action current | ||
| op: "current", | ||
| - old: "/snap/mock/100001", | ||
| + old: filepath.Join(dirs.StripRootDir(dirs.SnapMountDir), "mock/100001"), |
mvo5
Jun 7, 2017
Collaborator
This feels a bit repetitive and relatively long, I wonder if we could compute the dirs only once at the beginning of the test. OTOH it was repetitive before so maybe that is something for a later branch.
| @@ -4987,6 +5017,9 @@ func (s *snapmgrTestSuite) TestTrySetsTryModeJailMode(c *C) { | ||
| s.testTrySetsTryMode(snapstate.Flags{JailMode: true}, c) | ||
| } | ||
| func (s *snapmgrTestSuite) TestTrySetsTryModeClassic(c *C) { | ||
| + if !dirs.SupportsClassicConfinement() { |
mvo5
Jun 7, 2017
Collaborator
Would it be possible to move everything that needs classic confinement into a single snapmgrWithClassicConfinementTestSuite and simply skip the entire suite on the above condition?
morphis
Jun 7, 2017
Contributor
Not sure, something I would have to look into but would prefer to do it with a follow up PR to not put more burden on this one.
| @@ -5046,6 +5082,9 @@ func (s *snapmgrTestSuite) TestTryUndoRemovesTryFlagLeavesJailMode(c *C) { | ||
| s.testTrySetsTryMode(snapstate.Flags{JailMode: true}, c) | ||
| } | ||
| func (s *snapmgrTestSuite) TestTryUndoRemovesTryFlagLeavesClassic(c *C) { | ||
| + if !dirs.SupportsClassicConfinement() { |
mvo5
Jun 7, 2017
Collaborator
Slightly strange that SupportsClassicConfinement() is a member of dirs - I understand why but still feels wrong. Anyway, not something to be concerned about for this branch.
added some commits
Jun 7, 2017
Some checks were not successful
| +} | ||
| + | ||
| +// MountDirWithBasePath returns the base directory where it gets mounted of the snap with the given name and revision. | ||
| +func MountDirWithBasePath(basePath, name string, revision Revision) string { |
niemeyer
Jun 12, 2017
Contributor
Let's please drop this function. We don't need a special (and long) name for filepath.Join. :-)
morphis commentedApr 24, 2017
•
Edited 1 time
-
morphis
Apr 24, 2017
This is needed to allow execution of our unit tests within the package build on other distributions like Fedora.