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: end-to-end support for the bare base snap #3625
Conversation
mvo5
added some commits
Jul 25, 2017
All checks have failed
| @@ -217,6 +217,7 @@ struct sc_mount_config { | ||
| // The struct is terminated with an entry with NULL path. | ||
| const struct sc_mount *mounts; | ||
| bool on_classic_distro; | ||
| + bool uses_base_snap; |
zyga
Jul 27, 2017
Contributor
I'd like to have code that makes this work without any flags. In essence all snaps use base snap so our approach should be more universal.
mvo5
Aug 31, 2017
Collaborator
The flag is currently only used to ensure /usr/lib/snapd (and later snapctl) is available for the snap. We need /usr/lib/snapd/snap-exec to actually running anything in a base so that is important. On a normal core system we do not have to make /usr/lib/snapd available, its already there.
| @@ -333,11 +334,34 @@ static void sc_bootstrap_mount_namespace(const struct sc_mount_config *config) | ||
| config->rootfs_dir, dir); | ||
| sc_must_snprintf(dst, sizeof dst, "%s%s", | ||
| scratch_dir, dir); | ||
| - sc_do_mount(src, dst, NULL, MS_BIND, NULL); | ||
| - sc_do_mount("none", dst, NULL, MS_SLAVE, NULL); | ||
| + if (access(src, F_OK) == 0 |
zyga
Jul 27, 2017
Contributor
We should be extra careful with symlinks here (the base snap may contain symlinks that will "pass" the F_OK test. I'd suggest using lstat instead, which should not be much longer but will give us the predictability.
mvo5
Aug 31, 2017
Collaborator
I'm find with lstat(), I kept it in sync with the lines before that use access() as well.
| } | ||
| } | ||
| } | ||
| + if (config->uses_base_snap) { | ||
| + // snapd dir | ||
| + const char *src = "/usr/lib/snapd"; |
mvo5
Aug 31, 2017
Collaborator
Indeed, I changed the code now to use the libexecdir configure option. However I think this needs to be even smarter, i.e. we need to take re-exec into account as well. I added a fixme. we could of course mount dirname(sys.argv[0]) which is probably the easiest and most sensible thing to do.
| + sc_do_mount(src, dst, NULL, MS_REC | MS_BIND, NULL); | ||
| + sc_do_mount("none", dst, NULL, MS_REC | MS_SLAVE, NULL); | ||
| + | ||
| + // FIXME: snapctl tool - our apparmor policy wants it in |
zyga
Jul 27, 2017
Contributor
How about this: let's always use /var/lib/snapd/tools as a place where snapd keeps it tooling. Snap confine could then mount tmpfs there and create a symlink or bind mount farm from each tool (wherever it is, even from the file that is invisible in the base snap) to an empty file here.
As long as we can require /var/lib/snapd to exist in base snaps we could then run our tools reliably.
| @@ -0,0 +1,4 @@ | ||
| +name: test-snapd-base-bare | ||
| +version: 1.0 | ||
| +type: base |
sergiusens
Aug 1, 2017
Contributor
this new type isn't added in any snapcraft PR, only the base field, but not the base snap type
All checks have failed
mvo5
referenced this pull request
in snapcore/snapcraft
Aug 28, 2017
Merged
add new "no-wrapper" property to apps #1420
pedronis
added
the
Blocked
label
Aug 29, 2017
mvo5
added some commits
Aug 31, 2017
All checks have failed
All checks have failed
|
@zyga I addressed your comments and replied. I would love to have this as an early preview for bases in 2.28. It would allow e.g. Neal to start experimenting with the fedora core snap for real. |
mvo5
added some commits
Aug 31, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
codecov-io
commented
Sep 1, 2017
•
Edited 1 time
-
codecov-io
Sep 1, 2017
-
Sep 1, 2017
Codecov Report
@@ Coverage Diff @@
## master #3625 +/- ##
==========================================
- Coverage 75.73% 75.72% -0.02%
==========================================
Files 413 413
Lines 35531 35549 +18
==========================================
+ Hits 26909 26918 +9
- Misses 6722 6728 +6
- Partials 1900 1903 +3
Continue to review full report at Codecov.
|
Some checks were not successful
zyga
reviewed
Sep 1, 2017
LGTM with two changes
- drop the
MS_RECflags from the extra mount call - let's please rename the
.gitignorefiles to something else (e.g..keep)
| @@ -43,14 +43,16 @@ | ||
| #define MAX_BUF 1000 | ||
| +#ifndef LIBEXECDIR |
|
@zyga MS_REC is now removed |
mvo5
removed
the
Blocked
label
Sep 1, 2017
All checks have passed
niemeyer
approved these changes
Sep 1, 2017
A few details, and LGTM. I'd prefer to not have the dozens of .gitignore files on empty directories committed into the tree.. we should be able to easily create the content dynamically.
| @@ -43,14 +43,16 @@ | ||
| #define MAX_BUF 1000 | ||
| +#ifndef LIBEXECDIR | ||
| +LIBEXECDIR = "/usr/lib/snapd" |
| + if (config->uses_base_snap) { | ||
| + // snapd dir: we need it so that we can actually run | ||
| + // anything because snap-exec needs to be visible the | ||
| + // base snap |
niemeyer
Sep 1, 2017
Contributor
Seems slightly misleading. We don't want snap-exec to ship in base snaps, right?
| @@ -128,6 +128,13 @@ override_dh_auto_build: | ||
| mkdir -p _build/src/$(DH_GOPKG)/cmd/snap/test-data | ||
| cp -a cmd/snap/test-data/*.gpg _build/src/$(DH_GOPKG)/cmd/snap/test-data/ | ||
| dh_auto_build -- $(BUILDFLAGS) $(TAGS) $(GCCGOFLAGS) | ||
| + # Generate static snap-exec - it somehow includes CGO so we must | ||
| + # force a static build here. We need a static snap-exec inside | ||
| + # the core snap because not all bases will have a libc |
niemeyer
Sep 1, 2017
Contributor
The base field is on the app snap itself, while the base type is on the underlying snap. These sound special enough to be worth having a new type for them. We want to hide them in the default listing, for example.
| @@ -0,0 +1,4 @@ | ||
| +name: test-snapd-base-bare |
niemeyer
Sep 1, 2017
Contributor
Can't comment on the empty files/dirs above, so adding here:
Wouldn't it be better to create them dynamically inside the test instead of having these in the tree?
mvo5
Sep 4, 2017
Collaborator
Yes, thank you! I added a snapcraft.yaml that creates them now instead of manually doing it here.
mvo5
added some commits
Sep 4, 2017
All checks have passed
mvo5 commentedJul 26, 2017
This branch adds the missing pieces to enable the "bare" base snap with a proper integration test.
The tests will require that the following snapcraft features get merged:
snapcore/snapcraft#1420
snapcore/snapcraft#1419