interfaces,overlord: support unversioned data

[kyrofa]

The current versioned data directories are an important aspect of the upgrade/rollback strategy, but they don't scale well for applications that need to hold a vast amount of non-version-specific data (LP: #1559248). This PR is an attempt to implement support for unversioned data (i.e. data that is shared between versions of a given snap), conforming to Jamie's suggestion (note that the naming is of course up for discussion). The shared directories are removed by overlord only once the final version is removed.

Note that this will also require changes to ubuntu-core-launcher to create the user-specific shared data directory (as it creates the versioned data directory). That can come once this PR is approved.

[zyga]

MountDir feels odd here

[kyrofa]

You mean because it includes the version? Perhaps this is reason enough to create an info.BaseDir() function, then.

[zyga]

My comment was based on just the name: ... MountDir connection but I see that other tests use this as well.

[kyrofa]

Yeah, it's nice for testing that a given operation happens on a given revision of the snap. We may want to leave this as MountDir(), then.

[niemeyer]

Yeah, let's not overthink this too much for now. It's just a way to print out what the backend had at hand when the given method was called.

[zyga]

Hmmm, I'm not sure this looks nice.

[kyrofa]

Yeah I wasn't a huge fan either, but the only nice alternative I could think of was to stop being clever using SnapPath for everything and actually specify each of these explicitly, which is quite a bit more invasive (e.g. create an info.BasePath() to get /snap/snapname without the version, and so on). Would that be better? I'll see if I can simplify this.

[kyrofa]

Fixed.

[zyga]

Ditto

[kyrofa]

Also fixed.

[zyga]

Based on

zyga@zyga-thinkpad-w510:~$ go doc os.RemoveAll
func RemoveAll(path string) error
    RemoveAll removes path and any children it contains. It removes everything
    it can but returns the first error it encounters. If the path does not
    exist, RemoveAll returns nil (no error).

I don't think you have to check for the os.IsNotExist or call os.Remove again later.

[kyrofa]

Agreed, this was taken from RemoveSnapData(). They should use the same function anyway, so I'll refactor them both.

[kyrofa]

Done. Though note: the later os.Remove() call is what removes the parent directories when they're empty, so I've left that, but added a comment to clarify.

[zyga]

Wow, this stuff is stale! /apps/app/1.0 Where did that come from?

In either case I don't think /var/appps is what we want today. I'd look at something in /var/lib/snapd/

[kyrofa]

Indeed, lots of test cruft here. Note that it's using version instead of revision as well. I thought cleaning it would make for a noisy diff, though. Should I anyway?

[zyga]

Perhaps as a side branch. I think it's well worth not adding more /apps test data.

[kyrofa]

Alright, I'll leave this alone here, then.

[zyga]

Some comments inline.

[niemeyer]

Although I'd like to sync up with everyone on this, the approach looks nice and simple.

One initial comment: shared is perhaps not the best word. It also means "make it public" (share with whom?), and it may imply concurrent use which will never be the case here (only the active snap will touch it). As a preliminary strawman, I'd suggest "common".

[kyrofa]

shared is perhaps not the best word. It also means "make it public" (share with whom?), and it may imply concurrent use which will never be the case here (only the active snap will touch it).

Great point, I agree.

As a preliminary strawman, I'd suggest "common".

I'm good with "common." How about the variable name format: SNAP_COMMON_DATA and SNAP_USER_COMMON_DATA? I'm concerned the latter gives the impression that the data is common among users, which of course isn't the case. Can you think of something more clear?

[kyrofa]

I guess "unversioned" is an option as well.

[niemeyer]

Quickly discussed this with the team and the response was positive.

So let's go ahead with the proposal, just replacing the "shared" term by "common" everywhere please.

[didrocks]

Reading about this, "common" makes sense from an internal code perspective for sure, I'm less sure from a developer perspective it would though.
"common" is implicit (as for "shared") that multiple entities can access to it at the same time, which isn't the case as @niemeyer suggested.

What you do actually end up with are versioned and version-independant areas. It's even more obvious when you ls in the data directory as you will have:
version1/, version2/, …, current. Why not then rather using unversioned which I guess will live better next to those versions and the current symlink? Thoughts?

[niemeyer]

"common" doesn't really have any access-related concurrency connotation that I'm aware of, and searching for these terms seems to agree with that. Do you have any reference pointing out otherwise?

The fact multiple entities can access it is intended, though. That's exactly what "not versioned" means, and it's always more pleasing to label something after what it means than after what it does not mean.

[didrocks]

I partially agree with it's more pleasing to label something in a non negative way. However, I do fell that "unversioned" underly a fact that "common" doesn't hilight well enough: you need to ensure data you put there are forward compatible.

The "unversioned" term will be a little bit more scary to developers for this and it's a good reason: you really need to ensure that data you put there are working with any available version of your application. You don't want non forward-compatible schema of your database for instance (I'm afraid that this directory will be used a lot for databases in particular).
"common" is a little bit more gentle in this regard and don't underline this risk… at least from a non native speaker perspective (so take that with a grain of salt ;))

[niemeyer]

This is a bit problematic, and makes me want to think further about the whole approach we're taking. Imagine a button that says "Reset snap data".. if we keep the shared data around we're not actually resetting the data. We'll restart the snap, and its data is still there, partially! How do we reset the data for a snap, ever, once we have a shared directory?

[kyrofa]

That sounds like new functionality, though (i.e. not involving this particular bit of logic)-- do we currently support clearing data out from under an installed snap? As part of that feature I expect we'd also clear the shared data.

[niemeyer]

Okay, the answer is that we'll need a separate task that kills the data for all snaps at once. If we intentionally want to reset the data for a snap, it should do so for all snaps rather than just the current revision. The experience would probably be awkward otherwise.

So this is fine for now, thanks.

[kyrofa]

Agreed.

[niemeyer]

We had a BaseDir very recently, which got renamed because it was unclear. It doesn't feel much more clear now. Which base dir?

[kyrofa]

Agreed. This is part of why I used filepath.Clean initially-- this directory really has no meaning except for building paths. ParentDir()? MountsDir()? MountParentDir()?

[niemeyer]

Let's please drop all BaseDir entries. The use case in the template is better handled by exposing App to the template directly, and using the proper method names instead of rebuilding the paths inside the template.

[niemeyer]

Same as prior point above.

[niemeyer]

Same as prior point above.

[niemeyer]

Why are we building this variable here when we have a method in info that returns the exact correct path? We should have the info here and use its methods (templates can do that) instead of inventing a new way to get the same data for each one of these variables.

[kyrofa]

That would involve adding stripGlobalRootDir as a template function which makes this a little less pretty. Is that okay?

[kyrofa]

Note that touches a lot of tests as well. It may be better suited for another PR.

[niemeyer]

Per prior note, let's expose the App to the template and use the existing methods directly here instead of rebuilding the path locally.

[kyrofa]

Sounds good.

[niemeyer]

Let's please have a single field here, "App", and kill all of these fields which are simply accessing it by doing so in the template itself.

[kyrofa]

+1, but ditto my comment about stripGlobalRootDir and the fact that this will add a lot of diff noise. Do you want me to do that here, or in another PR?

[zyga]

I can start a parallel branch that does that and we can see where it leads.

[niemeyer]

This also shouldn't be here. We can have the string in the template itself.

[kyrofa]

Well, it changes to /root if it's a service, though.

[zyga]

That is still a "$HOME" constant in the template.

[kyrofa]

I'm talking about https://github.com/ubuntu-core/snappy/blob/master/systemd/systemd.go#L388 . You can't use $HOME or %h in the service file.

[kyrofa]

As agreed in Telegram, I've pushed updates renaming "shared" to "common," and removed the variables from the launchers.

[niemeyer]

LGTM

[jdstrand]

FYI, these changes look fine security policy-wise.

[zyga]

s/versions/revisions/

[niemeyer]

Good catch. Even better: "common across revisions"

[kyrofa]

Done.

[zyga]

ditto (revisions)

[kyrofa]

Done.

[zyga]

revisions

[kyrofa]

Done.

[zyga]

revisions

[kyrofa]

Done.

[zyga]

This would return /home/*/snap/$snap/common. This is not a new problem but it's not clear that the returned value is a glob.

[kyrofa]

Indeed. I think both this and DataHomeDir() should have "Glob" at the end of their names. I can do that here if you like. I actually already did it once and decided it made too much noise that didn't relate to the PR, but it's really not that invasive.

[niemeyer]

Instead of having a Glob in the name, let's please make it take a username parameter instead which can be set to "*" in current use cases.

[kyrofa]

I actually did that once as well. You run into problems when you try to use '$HOME' with a global root. I couldn't make it pretty.

[kyrofa]

Also, you actually need the globs for removal, so that would require new functions (probably part of @zyga's refactor).

[pedronis]

DataHomeDir is my fault refactoring away and not noticing that it involved actually a glob, not sure one method and one dirs pkg variable can cover all our use cases though, it seems we need to produce:
/home/*/snap/... "/root/snap/..." and "$HOME/snap/..."

[niemeyer]

We already have that problem today, so let's merge this and tackle that separately.

[zyga]

FYI: note the glob remark on CommonDataHomeDir below

[zyga]

LGTM