Remap user-level environment to system-level for services. #269

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Member

kyrofa commented Dec 18, 2015

Currently Snappy services define $SNAP_APP_USER_DATA_PATH to be within /root/apps, however, there is no logic to create the $SNAP_APP_USER_DATA_PATH like there is within binary wrappers, which means any service that tries to use $SNAP_APP_USER_DATA_PATH is trying to use a non-existing directory.

Having services write caches or logs into /root doesn't really make sense anyway, so this PR remaps the user-level environment into the system-level environment (i.e. $SNAP_APP_USER_DATA_PATH now =
$SNAP_APP_DATA_PATH) for services.

Fixes LP: #1527612

Contributor

snappy-m-o commented Dec 18, 2015

Can one of the admins verify this patch?

systemd/systemd.go
@@ -352,7 +352,7 @@ WantedBy={{.ServiceSystemdTarget}}
servicesSystemdTarget,
origin,
arch.UbuntuArchitecture(),
- "%h",
+ "/var/lib", // Remap user-level environment to system-level for services
@mvo5

mvo5 Dec 21, 2015

Collaborator

Thanks for adding a comment here, maybe add a tiny bit more like "Remap user-level environment to system-level for services. The code will append "apps/$appname/$version" to this path"

Collaborator

mvo5 commented Dec 21, 2015

Thanks for this branch! Looks excellent :) One tiny suggestion for the comment inline, but no blocker at all.

Remap user-level environment to system-level for services.
Currently Snappy services define $SNAP_APP_USER_DATA_PATH to be
within /root/apps, however, there is no logic to create the
$SNAP_APP_USER_DATA_PATH like there is within binary wrappers,
which means any service that tries to use $SNAP_APP_USER_DATA_PATH
is trying to use a non-existing directory.

Having services write caches or logs into /root doesn't really make
sense anyway, so this commit remaps the user-level environment into
the system-level environment (i.e. $SNAP_APP_USER_DATA_PATH now =
$SNAP_APP_DATA_PATH) for services.

Fixes LP: #1527612

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Member

kyrofa commented Dec 21, 2015

@mvo5 thanks for the review! I've updated the comment as you suggested.

Member

kyrofa commented Dec 21, 2015

It would be great if we can get this back into 15.04 as well.

Member

chipaca commented Dec 22, 2015

ok to test

Member

chipaca commented Dec 22, 2015

👍

Member

chipaca commented Jan 4, 2016

retest this please

Member

elopio commented Jan 5, 2016

The integration tests error is not related to this branch. I'm fixing it here: ubuntu-core#284

Also, I propose we add kyrofa to the whitelist: ubuntu-core/snappy-jenkins#35

Member

elopio commented Jan 5, 2016

@kyrofa could you write an integration test that installs and runs a snap with a service that logs something?
We have a bunch of simple snaps for testing here: https://github.com/ubuntu-core/snappy/tree/master/integration-tests/data/snaps
And here are some tests that you can use as a base: https://github.com/ubuntu-core/snappy/blob/master/integration-tests/tests/examples_test.go

Let me know if you need a hand. :D

Member

kyrofa commented Jan 5, 2016

@elopio sure! Thanks for fixing the integration tests 😛 .

Contributor

niemeyer commented Jan 6, 2016

A couple of questions: does it make sense for a service application to be using the user data directory, if there's no associated user? How/why would that happen?

Then, if the application does make a distinction between those, isn't it problematic that we're mapping two paths that are generally taken to be in different places pointing to the same directory? I can easily imagine an application using conflicting names within the two directories (e.g. "config" in both).

Member

kyrofa commented Jan 6, 2016

A couple of questions: does it make sense for a service application to be using the user data directory, if there's no associated user? How/why would that happen?

@niemeyer Ideally no. My particular use-case is ROS. roscore writes a log to a location determined by an environment variable, which by default is $HOME/.ros. The snapcraft plugin sets it up to log to $SNAP_APP_USER_DATA_PATH/ros. This works for binaries, but for services it hits the bug this PR is trying to fix.

You have a good point, though. I don't think we should have an environment variable pointing to an invalid location (like we do now), but perhaps the better fix would be to actually create that directory instead of remapping it. Perhaps the creation of user data paths should be moved within ubuntu-core-launcher instead of the binary wrappers, which would allow services to have that functionality as well. Thoughts?

Contributor

niemeyer commented Jan 6, 2016

Agreed it would be nice to have the user data directory present at all times, so applications wouldn't have to worry themselves about which context they're running under. I'm concerned about the interaction between this issue and what we're addressing in #264, though. At the time, it sounded like using /root was the right thing to do for sudo. But now we have another facet of the same problem, because /root ends up being picked for the services merely because that's the user they run as. Isn't it awkward that when run as "a service" (a concept we're trying to deprecate slightly, see context in snappy-devel@) an application would use /var/lib as its user data path, and when run under sudo, it would use /root?

Member

kyrofa commented Jan 6, 2016

Agreed, the inconsistency is bothersome. We can resolve it if we scrap this PR and move the user data directory creation into ubuntu-core-launcher, yes? That at least allows ROS services to run, even if they don't log to an ideal location.

Contributor

niemeyer commented Jan 6, 2016

Please let me have a conversation with the team tomorrow first, so you don't waste any time. First thing to do is to decide what is the convention we want for snap data storage in general.

Member

kyrofa commented Jan 6, 2016

Sounds good, let me know.

Contributor

niemeyer commented Jan 7, 2016

I'm closing this so we can more easily wait for it (or another PR) to reflect the agreement in the mailing list.

@niemeyer niemeyer closed this Jan 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment