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: fetch & cache remote snaps and sections; complete from there #3748
Conversation
|
(sneakily rebased as nobody had looked at it yet) |
|
Sigh. Tests in overlord/snapstate don't |
chipaca
added some commits
Aug 23, 2017
|
completion spread test failure is real. Need to dig. |
chipaca
John Lenton
Some checks were not successful
codecov-io
commented
Aug 24, 2017
•
Edited 1 time
-
codecov-io
Aug 31, 2017
-
Aug 31, 2017
Codecov Report
@@ Coverage Diff @@
## master #3748 +/- ##
==========================================
- Coverage 75.74% 75.65% -0.09%
==========================================
Files 409 409
Lines 35388 35513 +125
==========================================
+ Hits 26804 26869 +65
- Misses 6688 6735 +47
- Partials 1896 1909 +13
Continue to review full report at Codecov.
|
pedronis
added
the
Blocked
label
Aug 29, 2017
|
I've got the conflict resolved locally, as well as dropping refresh from 1/hour to 1/day. I'll hold off on pushing these until I've had a review, to not spam travis too much. |
zyga
reviewed
Aug 30, 2017
Some comments inline.
I think my main worry is that wget a better solution because it writes stuff to disk while we keep the result in memory. As number of snaps grow this will eventually prevent snapd from working on memory-constrained devices.
-
Could the store send stuff sorted in the right order? This saves O(N) sort calls on the scale of the planet.
-
Could we just write it to disk and do an atomic rename?
| + | ||
| + var ret []flags.Completion | ||
| + | ||
| + // TODO: the file is sorted; we should bisect it |
zyga
Aug 30, 2017
Contributor
How much data are you expecting to see in the particular use case that you wrote this for?
chipaca
Aug 30, 2017
Member
I hope it'll grow to be in the millions. I expect it to be in the hundreds of thousands at least.
| + } | ||
| + } | ||
| + } | ||
| + |
| @@ -170,6 +174,10 @@ func SetRootDir(rootdir string) { | ||
| SnapStateFile = filepath.Join(rootdir, snappyDir, "state.json") | ||
| + SnapCacheDir = filepath.Join(rootdir, snappyDir, "cache") |
| @@ -537,6 +555,32 @@ func (m *SnapManager) ensureRefreshes() error { | ||
| return err | ||
| } | ||
| +// ensureMiscRefresh ensures that we refresh the miscellaneous static | ||
| +// data periodically |
| +} | ||
| + | ||
| +func refreshNames(aStore StoreService) error { | ||
| + snapCmds, err := aStore.SnapCommands() |
zyga
Aug 30, 2017
Contributor
Should we perhaps stream this instead of buffering it in memory. It may have essentially any size.
| + fmt.Fprintln(buf, name) | ||
| + } | ||
| + | ||
| + return osutil.AtomicWriteFile(dirs.SnapNamesFile, buf.Bytes(), 0644, 0) |
zyga
Aug 30, 2017
Contributor
How about having an atomic buffered disk file write API that doesn't need in-memory store of arbitrarily large buffer?
chipaca
Aug 30, 2017
Member
I think we do want to move to streaming apis everywhere, but it's a chunk of work
|
@zyga, to address your overall concerns, yes we can ask the store to send them sorted (and I'm in the process of doing so), and we can drop the sorting once that is deployed. And yes we should have streaming versions of these things (in particular @mvo5 has needed an atomic rename of a io.writer before, i think). Do you consider these to be blockers? |
|
@zyga also, note that the file we write is rather different from what we get from the store (it doesn't stop it being streamable, but it isn't just a "hand over |
chipaca
removed
the
Blocked
label
Aug 31, 2017
chipaca
John Lenton
Some checks were not successful
pedronis
reviewed
Sep 1, 2017
looks reasonable, some comments, I wish the SetRootDir where separate :/
| @@ -170,6 +174,10 @@ func SetRootDir(rootdir string) { | ||
| SnapStateFile = filepath.Join(rootdir, snappyDir, "state.json") | ||
| + SnapCacheDir = filepath.Join(rootdir, snappyDir, "cache") |
| + n++ | ||
| + } | ||
| + bufsize += n | ||
| + sort.Strings(names) |
| + return nil, fmt.Errorf("received an unexpected content type (%q) when trying to retrieve the commands via %q", ct, resp.Request.URL) | ||
| + } | ||
| + | ||
| + cmds := make(map[string][]string, len(cmdsData.Payload.Packages)) |
pedronis
Sep 1, 2017
Contributor
do we need a TODO here about the fact that at moment we get only snap names?
|
yes, refreshMisc is not a great name |
| @@ -69,6 +69,7 @@ const defaultRefreshSchedule = "00:00-04:59/5:00-10:59/11:00-16:59/17:00-23:59" | ||
| // overridden in the tests | ||
| var errtrackerReport = errtracker.Report | ||
| +var miscRefreshDelay = 24 * time.Hour |
pedronis
Sep 1, 2017
Contributor
this probably merits its own comment/explanation, also perhaps s/misc/catalogs/ or something along those lines?
chipaca commentedAug 16, 2017
While the sections work wasn't strictly needed, it helped to
understand the pattern I was going for as to how these things are
handled (e.g. why a 'cache' directory and not dropping these things
directly in /snapd/).
The worst part of this is probably the names of the ensure bits. I'll
leave you all to decide that.