overlord/snapshotstate/backend: introducing the snapshot backend #5066

chipaca · 2018-04-18T09:00:35Z

These are the low-level snapshot operations, that will get used by
snapshotstate to implement snapshot operations.

These are the low-level snapshot operations, that will get used by snapshotstate to implement snapshot operations.

codecov-io · 2018-04-18T11:01:59Z

Codecov Report

Merging #5066 into master will decrease coverage by 0.32%.
The diff coverage is 50.49%.

@@            Coverage Diff             @@
##           master    #5066      +/-   ##
==========================================
- Coverage   79.06%   78.74%   -0.33%     
==========================================
  Files         488      493       +5     
  Lines       36448    36854     +406     
==========================================
+ Hits        28818    29021     +203     
- Misses       5346     5488     +142     
- Partials     2284     2345      +61

Impacted Files	Coverage Δ
overlord/snapshotstate/backend/sizer.go	`100% <100%> (ø)`
dirs/dirs.go	`97.72% <100%> (+0.02%)`	⬆️
overlord/snapshotstate/backend/restorestate.go	`16.12% <16.12%> (ø)`
overlord/snapshotstate/backend/helpers.go	`38.35% <38.35%> (ø)`
overlord/snapshotstate/backend/reader.go	`43.18% <43.18%> (ø)`
overlord/snapshotstate/backend/backend.go	`74.78% <74.78%> (ø)`
overlord/hookstate/hookmgr.go	`74.87% <0%> (-1.03%)`	⬇️
... and 2 more

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 0e68d91...e43d1af. Read the comment docs.

bboozzoo

First pass

bboozzoo · 2018-04-18T09:20:22Z

dirs/dirs.go

@@ -259,4 +260,5 @@ func SetRootDir(rootdir string) {
 	SystemFontconfigCacheDir = filepath.Join(rootdir, "/var/cache/fontconfig")

 	FreezerCgroupDir = filepath.Join(rootdir, "/sys/fs/cgroup/freezer/")
+	SnapshotDir = filepath.Join(rootdir, "/var/spool/snapd/snapshots")


s/SnapshotDir/SnapshotsDir/ sounds more fitting

bboozzoo · 2018-04-18T09:52:18Z

overlord/snapshotstate/backend/backend.go

+	"github.com/snapcore/snapd/osutil"
+	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/strutil"
+	"regexp"


Can you group regex with other stdlib imports?

NO.

.
.
.

(yes)

bboozzoo · 2018-04-18T09:54:33Z

overlord/snapshotstate/backend/backend.go

+
+var (
+	dirOpen  = os.Open
+	dirNames = (*os.File).Readdirnames


This will make the call site slightly non obvious

Yes, I was aware of the tension between that and readability.

We do have a convention where we try to write those names close to their underlying packages and method/function names, precisely so that it's more readable.

The second one would be over the top with fileReaddirnames indeed and still unclear given that it's a method, so dirNames sounds good there, but the first one reads properly with osOpen.

Alternatively, given that osOpen is itself already mocked, it can return a different type, possibly an interface that has all the necessary methods.

Another alternative is to provide a different directory location with the desired data instead of mocking all methods. That might be better in some cases because it ends up testing the real code, including the backing logic behavior (readdirnames itself, for instance), instead of faking its behavior.

bboozzoo · 2018-04-18T10:05:19Z

overlord/snapshotstate/backend/backend.go

+var (
+	dirOpen  = os.Open
+	dirNames = (*os.File).Readdirnames
+	open     = Open


I'm a bit on on the fence with whether these should have their full type written down here.

Yeah, having both Open and open on the same package is indeed somewhat unclear. Perhaps backendOpen? This at least would make it slightly less tempting to use one or the other interchangeably without understanding why.

bboozzoo · 2018-04-18T10:21:38Z

overlord/snapshotstate/backend/helpers.go

+		return nil, -1, err
+	}
+
+	fi, err := f.Stat()


Error not checked

good catch, ouch

bboozzoo · 2018-04-18T10:23:40Z

overlord/snapshotstate/backend/helpers.go

+		return nil, -1, err
+	}
+
+	for _, f := range arch.File {


This f shadows the other f, made be go back and forth a bit.

bboozzoo · 2018-04-18T10:33:56Z

overlord/snapshotstate/backend/reader.go

+		return nil, err
+	}
+	defer func() {
+		if e != nil {


Not an issue but a bit of paranoia, I'd try if e != nill && f != nil { ... }

bboozzoo · 2018-04-18T10:58:01Z

overlord/snapshotstate/backend/reader.go

+			// for the system path, and we won't be creating the
+			// user's home (as we skip restore in that case).
+			// Also no chown happens for root/root.
+			if err := osutil.MkdirAllChown(parent, 0755, uid, gid); err != nil {


This will try to create $HOME/snap/<name>, shouldn't we check whether $HOME exists first?

as the comment says, we explicitly check for and skip the restore in the case the home does not exist.
(see the os.Stat(usr.HomeDir) above)

Missed that, thanks!

OTOH, should we check if SnapDataDir exists when restoring non-user data?

we check if the parent of si.DataDir() (that is, <SnapDataDir>/<snap>) exists and create it (as root:root) if it doesn't. I think this is reasonable; SnapDataDir not existing might indicate a brand new snapd (with the snapshot being 'imported' from elsewhere), which is probably fine.

Of course, <SnapDataDir>/<snap> might not exist because <snap> was removed, but you just asked to restore its data so that's probably ok, at least in backend when dealing with snapshots and not snapshot sets -- whether the frontend should exclude non-installed snaps from being restored unless explicitly requested is fair (and open) question.

bboozzoo · 2018-04-19T10:19:31Z

overlord/snapshotstate/snapshotmgr.go

+	return backend.Filename(skel)
+}
+
+func prepareSave(task *state.Task) (*snapshotState, *snap.Info, map[string]interface{}, error) {


Consider using named return values, the same goes for prepareRestore

this bit was pushed to this pr in error. I'm fixing it with a forced push. Please don't hate me!

bboozzoo · 2018-04-19T10:23:31Z

overlord/snapshotstate/snapshotmgr.go

+		return err
+	}
+	_, err = backend.Save(tomb.Context(nil), snapshot.SetID, cur, cfg, snapshot.Users)
+	return err


FWIW, looks like we do not need any extra cleanup here. The only 'artifact' created by backend.Save is a zip file which will go away on errors.

bboozzoo · 2018-04-19T10:26:48Z

overlord/snapshotstate/snapshotmgr.go

+		return err
+	}
+
+	trash, err := reader.RestoreLeavingTrash(tomb.Context(nil), snapshot.Users, task.Logf)


AFAICT this call also does not leave anything behind if errors occur inside.

…zoo)

bboozzoo

Actually it'd be nice to review this with the corresponding changes in handlers to see the big picture.

bboozzoo · 2018-04-23T08:33:44Z

overlord/snapshotstate/backend/backend.go

+// Filename of the given client.Snapshot in this backend.
+func Filename(sh *client.Snapshot) string {
+	// this _needs_ the snap name and version to be valid
+	return filepath.Join(dirs.SnapshotsDir, fmt.Sprintf("%d_%s_%s.zip", sh.SetID, sh.Snap, sh.Version))


Should revision be encoded in filename too?

Yeah, it sounds reasonable given it's the only thing that defines uniqueness, and it wouldn't hurt since it's just a few chars. We do have the revision inside, but the same would be true for the other fields.

bboozzoo · 2018-04-23T08:44:26Z

overlord/snapshotstate/backend/helpers.go

+		return nil, err
+	}
+	users[0] = root
+	seen := make(map[uint32]struct{}, len(ds)+1)


We could be really fancy and use osutil/sys.UserID here

bboozzoo · 2018-04-23T08:45:12Z

overlord/snapshotstate/backend/helpers.go

+			continue
+		}
+		seen[st.Uid] = yes
+		usr, err := user.LookupId(strconv.FormatUint(uint64(st.Uid), 10))


and osutil/sys.UserID.String() string (after adding it)

I'll leave these for a separate PR

niemeyer

This should be the final review. Let me know if you want to discuss any of those please.

niemeyer · 2018-04-23T12:51:44Z

dirs/dirs.go

@@ -259,4 +260,5 @@ func SetRootDir(rootdir string) {
 	SystemFontconfigCacheDir = filepath.Join(rootdir, "/var/cache/fontconfig")

 	FreezerCgroupDir = filepath.Join(rootdir, "/sys/fs/cgroup/freezer/")
+	SnapshotsDir = filepath.Join(rootdir, "/var/spool/snapd/snapshots")


It's not clear that this is the proper place for this. Spool is generally associated with things which are there just for a short while before going away into another system, such as email, print jobs, and similar.

From the Filesystem Hierarchy Standard:

/var/spool contains data which is awaiting some kind of later processing. Data in /var/spool represents work to be done in the future (by a program, user, or administrator); often data is deleted after it has been processed. [46]

It doesn't seem quite right for snapshots.

/var/lib/snapd/snapshots would be fine

niemeyer · 2018-04-23T12:54:29Z

overlord/snapshotstate/backend/backend.go

+
+var (
+	dirOpen  = os.Open
+	dirNames = (*os.File).Readdirnames


We do have a convention where we try to write those names close to their underlying packages and method/function names, precisely so that it's more readable.

The second one would be over the top with fileReaddirnames indeed and still unclear given that it's a method, so dirNames sounds good there, but the first one reads properly with osOpen.

Alternatively, given that osOpen is itself already mocked, it can return a different type, possibly an interface that has all the necessary methods.

Another alternative is to provide a different directory location with the desired data instead of mocking all methods. That might be better in some cases because it ends up testing the real code, including the backing logic behavior (readdirnames itself, for instance), instead of faking its behavior.

niemeyer · 2018-04-23T13:09:54Z

overlord/snapshotstate/backend/backend.go

+var (
+	dirOpen  = os.Open
+	dirNames = (*os.File).Readdirnames
+	open     = Open


Yeah, having both Open and open on the same package is indeed somewhat unclear. Perhaps backendOpen? This at least would make it slightly less tempting to use one or the other interchangeably without understanding why.

niemeyer · 2018-04-23T14:04:44Z

overlord/snapshotstate/backend/backend.go

+			if openError == nil {
+				// if openError was nil the snapshot was opened and needs closing
+				if closeError := rsh.Close(); err == nil {
+					err = closeError


It's probably okay to leave it like this, since we don't really expect the close error to ever be an issue since we're not writing, but FWIW there's something asymmetric here about open errors and close errors. An open error is ignored and passed onto f via broken, but a close error is transformed into a big deal and stops the iteration. The Close error seems like the least interesting one when we're not really touching the file here. But again, this will probably never fail, so perhaps okay.

niemeyer · 2018-04-23T14:05:48Z

overlord/snapshotstate/backend/backend.go

+		}
+	}
+
+	if err == io.EOF {


This should probably be checked inside the loop, right after dirNames. It would be strange to observe this error elsewhere still.

The wording on Readdirnames makes me think that it might at some point return a non-empty list and a non-nil error: it's documented that if names is empty, then the error is non-nil. It does not say that a non-nil error implies names is empty.

The implementation confirms this suspicion.

I'll add a comment explaining this.

niemeyer · 2018-04-23T18:01:03Z

overlord/snapshotstate/backend/trash.go

+// check that you're not trying to use trash info twice; data loss imminent.
+func (b *Trash) check() {
+	if b.Done {
+		panic("attempting to use a snapshot.Trash twice")


Doesn't feel worth crashing the daemon for that. Logging an internal error and preventing the actual data loss seems reasonable.

I've removed the method, and replaced it at the call site with appropriate logging

niemeyer · 2018-04-23T18:01:20Z

overlord/snapshotstate/backend/trash.go

+	b.check()
+	for _, dir := range b.Moved {
+		if err := os.RemoveAll(dir); err != nil {
+			logger.Noticef("cannot remove directory tree rooted at %q: %v", dir, err)


s/c/C/, for the logs

done for all of backend. Hope to propose a pr that does this everywhere sometime.

niemeyer · 2018-04-23T18:04:38Z

overlord/snapshotstate/backend/trash.go

+	Config *json.RawMessage `json:"config,omitempty"`
+}
+
+// check that you're not trying to use trash info twice; data loss imminent.


The documentation is phrased unconventionally and also a bit confusing in the sense that it's okay to use the information twice, otherwise we'd not have json fields above. There's something else that cannot be done twice, but it's not mentioned here.

dropping the function made this point moot \o/

niemeyer · 2018-04-23T18:07:28Z

overlord/snapshotstate/backend/trash.go

+	for _, dir := range b.Created {
+		logger.Debugf("removing %q", dir)
+		if err := os.RemoveAll(dir); err != nil {
+			logger.Noticef("while undoing changes because of a previous error: cannot remove %q: %v", dir, err)


Log messages start with a capital letter. Same above and below.

niemeyer · 2018-04-23T18:08:06Z

overlord/snapshotstate/backend/trash.go

+		orig := trash2orig(dir)
+		if orig == "" {
+			// dir is not a trash?!?
+			logger.Debugf("skipping restore of %q which seems to not be a trash", dir)


"not be a trash" :)

As I said, I'd originally called it a backup, but you told me to change it without telling me what to change it to (I agree backup might be ambiguous in this context). Now you're saying I chose the wrong thing, but still not suggesting what is the right thing (except for the struct, for which you're suggesting RestoreState), . Suggestions still welcome.

during the call we agreed that, in the end, restore state is the right name for this.

Sorry, here it goes:

"skipping restore out of %s: unrecognized name"

"Skipping restore of %q: unrecognised filename."

Mostly small changes and improvements to the API: * the directory is now /var/lib/snapd/snapshots * `backend.Trash` is now `backend.RestoreState`. * `backend.Iter` is now told to stop early with `backend.Stop` instead of `io.EOF`; the passed-in function returning `io.EOF` is an error. * Snapshot filenames now include the revision * When creating or restoring a user's data, tar is now run as the user. * Errors about hashes now include a small hash prefix (abc1234…)

chipaca force-pushed the snapshot-backend branch from ad07cbf to 59603ad Compare April 18, 2018 09:18

overlord/snapshotstate/backend: introducing the snapshot backend

81ae315

These are the low-level snapshot operations, that will get used by snapshotstate to implement snapshot operations.

chipaca force-pushed the snapshot-backend branch from 59603ad to 81ae315 Compare April 18, 2018 10:09

bboozzoo reviewed Apr 18, 2018

View reviewed changes

chipaca force-pushed the snapshot-backend branch 2 times, most recently from 7014843 to 0832d82 Compare April 18, 2018 23:56

bboozzoo reviewed Apr 19, 2018

View reviewed changes

overlord/snapshotstate/backend: address review feedback (thanks bbooz…

d400538

…zoo)

chipaca force-pushed the snapshot-backend branch from 0832d82 to d400538 Compare April 19, 2018 10:55

bboozzoo approved these changes Apr 23, 2018

View reviewed changes

niemeyer requested changes Apr 23, 2018

View reviewed changes

chipaca added the Simple 😃 A small PR which can be reviewed quickly label Apr 24, 2018

zyga added Complex and removed Simple 😃 A small PR which can be reviewed quickly labels Apr 24, 2018

chipaca added 3 commits April 26, 2018 11:46

overlord/snapshotstate/backend: missed a few points made by gustavo

bb821f5

Merge remote-tracking branch 'upstream/master' into snapshot-backend

e43d1af

niemeyer approved these changes May 22, 2018

View reviewed changes

niemeyer merged commit 48cbfb9 into canonical:master May 22, 2018

chipaca deleted the snapshot-backend branch June 18, 2018 10:17

overlord/snapshotstate/backend: introducing the snapshot backend #5066

overlord/snapshotstate/backend: introducing the snapshot backend #5066

Conversation

chipaca commented Apr 18, 2018

codecov-io commented Apr 18, 2018 • edited Loading

Codecov Report

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 18, 2018 •

edited

Loading

chipaca Apr 19, 2018 •

edited

Loading