many: implement fetching sections and package names periodically. #3866

chipaca · 2017-09-06T16:36:22Z

These are then used for quicker tab completion.

zyga

+1 with some suggestions.

zyga · 2017-09-07T07:18:39Z

dirs/dirs.go

@@ -187,6 +191,10 @@ func SetRootDir(rootdir string) {

 	SnapStateFile = filepath.Join(rootdir, snappyDir, "state.json")

+	SnapCacheDir = filepath.Join(rootdir, snappyDir, "cache")


Why not /var/cache/snapd?

I'd like it to be /var/cache/snapd as well, but that would mean adding that to writable-paths, and I don't know the impact on packaging elsewhere (is /var/cache/{package} universal?); keeping it under snappyDir makes it a no-brainer.

zyga · 2017-09-07T07:20:09Z

overlord/snapstate/snapmgr.go

+// ensureCatalogRefresh ensures that we refresh the cataloge
+// data periodically
+func (m *SnapManager) ensureCatalogRefresh() error {
+	// sneakily don't do anything if in testing


We could move the mkdir here so that it stays operational even if someone just goes and does rm -rf on the thing after snapd started.

zyga · 2017-09-07T07:20:19Z

overlord/snapstate/snapmgr.go

@@ -278,6 +282,9 @@ func Manager(st *state.State) (*SnapManager, error) {
 	if err := os.MkdirAll(dirs.SnapCookieDir, 0700); err != nil {
 		return nil, fmt.Errorf("cannot create directory %q: %v", dirs.SnapCookieDir, err)
 	}
+	if err := os.MkdirAll(dirs.SnapCacheDir, 0755); err != nil {


Please consider moving this below.

Below what?

below, where his other comment is (or was). I know what he means (and i forgot to do it in my last round)

zyga · 2017-09-07T07:22:09Z

overlord/snapstate/snapstate.go

+		return err
+	}
+
+	namesFd, err := osutil.NewAtomicFile(dirs.SnapNamesFile, 0644, 0, -1, -1)


Thank you, much less gray hair today :-)

Nitpick: namesFile? namesf? "fd" is generally a file descriptor in this context.

chipaca · 2017-09-07T07:38:20Z

Marking this blocked so it isn't merged in error (agreed with server-side about sorting the names endpoint, so we'll wait for that)

- canonical/snapd#3807 - canonical/snapd#3866

codecov-io · 2017-09-12T11:21:28Z

Codecov Report

Merging #3866 into master will decrease coverage by 0.07%.
The diff coverage is 55.39%.

@@            Coverage Diff             @@
##           master    #3866      +/-   ##
==========================================
- Coverage   75.93%   75.85%   -0.08%     
==========================================
  Files         416      416              
  Lines       35983    36122     +139     
==========================================
+ Hits        27325    27402      +77     
- Misses       6744     6794      +50     
- Partials     1914     1926      +12

Impacted Files	Coverage Δ
overlord/storestate/storestate.go	`100% <ø> (ø)`	⬆️
cmd/snap/cmd_find.go	`62.5% <0%> (-2.72%)`	⬇️
dirs/dirs.go	`98% <100%> (+0.08%)`	⬆️
cmd/snap/complete.go	`57.2% <17.24%> (-5.25%)`	⬇️
overlord/snapstate/snapstate.go	`80% <37.5%> (-0.87%)`	⬇️
store/store.go	`79.57% <66.66%> (-0.58%)`	⬇️
overlord/snapstate/snapmgr.go	`82.29% <92%> (+0.59%)`	⬆️
interfaces/builtin/network.go	`100% <0%> (ø)`	⬆️

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 50b555e...a1e630b. Read the comment docs.

niemeyer

Thanks for the new logic, John.

Several trivial nitpicks, and one point we need to address about the new directory.

niemeyer · 2017-09-12T23:12:26Z

overlord/snapstate/snapmgr.go

@@ -456,14 +463,26 @@ func (m *SnapManager) LastRefresh() (time.Time, error) {
 	return lastRefresh, nil
 }

+// NextRefresh returns the time the next update of the system's snaps
+// will be attempted.
+// The caller should be holding the state lock.


Thanks for the comments.

niemeyer · 2017-09-12T23:17:53Z

overlord/snapstate/snapmgr.go

+	if CanAutoRefresh == nil {
+		return nil
+	}
+	m.state.Lock()


defer m.state.Unlock()

If there's a reason to unlock at the end, the logic may be reversed for predictable correct termination:

m.state.Unlock()
defer m.state.Lock()

I need to hold the lock to get the cached store object, and to set the next timeout; I need it unlocked to call refreshCatalogs. So it'd have to be

m.state.Lock() defer m.state.Unlock() // do stuff m.state.Unlock() defer m.state.Lock() // do other stuff

I thought it best to avoid the resulting spurious lock/unlock defers

It'll be worse if we get a lock out because something in the middle panics. I'm not quite sure about what's the call chain leading here by now, but if we have a Lock on a restore up chain, this would prevent even the systemd restart that would usually happen, and potentially render a device dead.

Note that the Lock/Unlock is pretty fast if there are no writes inside it.

Ah, there's another alternative as well: splitting the logic out into a function.

niemeyer · 2017-09-12T23:21:41Z

overlord/snapstate/snapmgr.go

+	// catalog refresh does not carry on trying on error
+	m.nextCatalogRefresh = next
+	m.state.Unlock()
+	logger.Debugf("Next catalog refresh scheduled for %s.", next)


It may be confusing to see that when the next catalog refresh will actually happen right now. Perhaps:

"Catalog refresh starting now and will try again at %s."

niemeyer · 2017-09-12T23:26:43Z

overlord/snapstate/snapmgr.go

+		return nil
+	}
+	m.state.Lock()
+	aStore := storestate.Store(m.state)


I think we generally call it theStore inside this package. I don't mind which way we go, but would be nice to eventually have a global convention for this.

grep agrees with you

niemeyer · 2017-09-12T23:29:07Z

overlord/snapstate/snapstate.go

+		return err
+	}
+
+	namesFd, err := osutil.NewAtomicFile(dirs.SnapNamesFile, 0644, 0, -1, -1)


Nitpick: namesFile? namesf? "fd" is generally a file descriptor in this context.

niemeyer · 2017-09-13T00:15:18Z

cmd/snap/complete.go

+			} else {
+				break
+			}
+		}


Slightly easier to read:

if line < match { continue } if strings.HasPrefix(line, match) { ret = append(ret, ...) continue } break

I'll believe you on this one :-)

niemeyer · 2017-09-13T00:34:54Z

daemon/api_test.go

@@ -288,6 +286,8 @@ func (s *apiBaseSuite) daemon(c *check.C) *Daemon {
 		Serial: "serialserial",
 	})

+	snapstate.CanAutoRefresh = nil


Why was this moved? Seems much safer to have it very early on while the suite is being setup rather than on a function after much logic has been run, and even the daemon was created.

because the overlord manager's init now writes that variable

Can we please have a comment? Seems tempting to move it elsewhere.

niemeyer · 2017-09-13T00:37:18Z

daemon/daemon_test.go

@@ -63,10 +62,6 @@ func (s *daemonSuite) checkAuthorizationForPid(pid uint32, actionId string, deta
 	return s.authorized, s.err
 }

-func (s *daemonSuite) SetUpSuite(c *check.C) {
-	snapstate.CanAutoRefresh = nil


niemeyer · 2017-09-13T00:45:26Z

tests/main/completion/task.yaml

@@ -15,6 +23,7 @@ prepare: |
    expect -d -f key.exp0

 restore: |
+    chattr -i /var/cache/snapd/names


Shouldn't it be removed since prepare created it?

starting snapd created it. Maybe the test should stop snapd, move the old names aside, then do this, and restore it on restore?

Or just remove it again, which sounds easier.. the test just removed it anyway (by overwriting it), and this is the one test that cares the most about this file being right.

niemeyer · 2017-09-13T00:47:18Z

dirs/dirs.go

@@ -187,6 +191,10 @@ func SetRootDir(rootdir string) {

 	SnapStateFile = filepath.Join(rootdir, snappyDir, "state.json")

+	SnapCacheDir = filepath.Join(rootdir, "/var/cache/snapd")


We need to fix packaging so there's proper ownership on this directory on all distributions, and the packaging purging logic for all distros also need to account for this new directory.

❤️ - indeed

gah. you are correct. See, this is (one of the reasons) why I'd originally had it under /var/lib/snapd -- laziness.

Yeah, to be honest I wouldn't mind having these files inside /var/lib/snapd either.

niemeyer

Responded to your comments. LGTM once you're satisfied with all points.

The last master merge broke some tests; fixed them. Addressed the last few bits of review feedback. Added packaging bits for suse, fedora to clean up /var/cache/snapd on remove.

many: implement fetching sections and package names periodically.

8416689

These are then used for quicker tab completion.

zyga approved these changes Sep 7, 2017

View reviewed changes

chipaca added the ⛔ Blocked label Sep 7, 2017

chipaca added this to the 2.29 milestone Sep 7, 2017

ogra1 added a commit to canonical/core-build that referenced this pull request Sep 7, 2017

make /var/{cache,lib}/snapd synced

ebb3d2b

- canonical/snapd#3807 - canonical/snapd#3866

ogra1 added a commit to canonical/core-build that referenced this pull request Sep 7, 2017

make /var/{cache,lib}/snapd synced

25f72a2

- canonical/snapd#3807 - canonical/snapd#3866

chipaca added 3 commits September 7, 2017 10:06

fixed typo

74be33f

Merge remote-tracking branch 'upstream/master' into snap-commands-ng

3013ea4

CanAutoRefresh needs stomping after setting up the overlord, now

fb5d914

chipaca force-pushed the snap-commands-ng branch from 30a382c to fb5d914 Compare September 11, 2017 17:41

chipaca added 2 commits September 12, 2017 10:37

Merge branch 'master' into snap-commands-ng

2e2a0a3

make the tests happy by faking a sorted "names"

6a32db0

niemeyer requested changes Sep 13, 2017

View reviewed changes

chipaca added 2 commits September 14, 2017 00:03

address review feedback

5ffc463

Merge branch 'master' into snap-commands-ng

6234c5e

niemeyer approved these changes Sep 13, 2017

View reviewed changes

fixups and cleanups and addressing review feedback.

f7e1493

The last master merge broke some tests; fixed them. Addressed the last few bits of review feedback. Added packaging bits for suse, fedora to clean up /var/cache/snapd on remove.

chipaca force-pushed the snap-commands-ng branch from acccc7d to f7e1493 Compare September 14, 2017 09:33

chipaca added 2 commits September 14, 2017 16:35

fix spec files

58e0bea

forgot one last change requested in review

a1e630b

chipaca removed the ⛔ Blocked label Sep 15, 2017

chipaca merged commit 79b6e3b into canonical:master Sep 15, 2017

chipaca deleted the snap-commands-ng branch September 15, 2017 11:08

		@@ -187,6 +191,10 @@ func SetRootDir(rootdir string) {

		SnapStateFile = filepath.Join(rootdir, snappyDir, "state.json")

		SnapCacheDir = filepath.Join(rootdir, snappyDir, "cache")

		@@ -187,6 +191,10 @@ func SetRootDir(rootdir string) {

		SnapStateFile = filepath.Join(rootdir, snappyDir, "state.json")

		SnapCacheDir = filepath.Join(rootdir, "/var/cache/snapd")

many: implement fetching sections and package names periodically. #3866

many: implement fetching sections and package names periodically. #3866

Conversation

chipaca commented Sep 6, 2017

zyga 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

chipaca commented Sep 7, 2017

codecov-io commented Sep 12, 2017 • edited Loading

Codecov Report

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

niemeyer Sep 13, 2017 • 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

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

niemeyer Sep 13, 2017 • 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

niemeyer left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 12, 2017 •

edited

Loading

niemeyer Sep 13, 2017 •

edited

Loading

niemeyer Sep 13, 2017 •

edited

Loading