o/snapstate: check disk space w/o store if possible #11159
Conversation
The disk space checks were obtaining the size of the snaps' prerequisites and base via a call to the store. However, an InstallPathMany should allow the user to install without calls to the store, if all requirements are available locally. This change makes it so that, if the required bases and prereqs are included in the install, those are used for the disk check instead of gotten through the store. Note that for Install/UpdateMany with prerequisites the already downloaded prereqs are also used, instead of redownloaded.
Replaced the snapstate tests that assert the install size can work locally with more focused storehelpers tests. Changed the snapstate tests that assert that the InstallPathMany works w/o the store to include the disk space checks as well.
|
I refactored the long snapstate tests into more focused storehelper tests for the installSize code and one snapstate test that asserts that InstallPathMany (including disk space checks) doesn't call the store. I think in terms of testing this achieves the same goal while cutting the PR diff almost in half. |
|
|
||
| // core is already installed | ||
| s.mockCoreSnap(c) | ||
| coreSize := getInstalledSnapSize(c, st, "core") |
There was a problem hiding this comment.
This could use a c.Assert(coreSize, Not(Equals), int64(0)) check (or a precise check for expected size), and it will currently fail, but see my other comment.
| sz, err := snapstate.InstallSize(st, []snapstate.MinimalInstallInfo{ | ||
| snapstate.InstallSnapInfo{Info: snap1}, snapstate.InstallSnapInfo{Info: snap2}}, 0) | ||
| c.Assert(err, IsNil) | ||
| c.Check(sz, Equals, uint64(snap1.Size+snap2.Size+coreSize)) |
There was a problem hiding this comment.
Existing tests in this suite use predefined const for expected sizes (see snap1Size, snap2Size etc.) so that any unexpected size would fail; can we do the same here?
There was a problem hiding this comment.
Actually I think all snaps are locally installed as well in this test. Maybe we should have one more test where one snap is available locally and the other isn't, and we hit the store.
| i, err := snapst.CurrentInfo() | ||
| c.Assert(err, IsNil) | ||
|
|
||
| return i.DownloadInfo.Size |
There was a problem hiding this comment.
Download size inside info is ephemeral and only available when we have snap.Info coming right from the store - see snap.Info struct, the comment above embedded DownloadInfo, so I think we will get 0 here.
I think for local snaps installed via InstallPathMany, to avoid store calls, we should check the size of the snap blobs.
There was a problem hiding this comment.
as we discussed elsewhere:
I was checking the same, it's set in ReadInfoFromSnapFile
maybe though for symmetry DownloadSize of pathInfo should do simply: return Info.Size
overlord/snapstate/storehelpers.go
Outdated
| @@ -113,6 +113,11 @@ var installSize = func(st *state.State, snaps []minimalInstallInfo, userID int) | |||
| accountedSnaps[snap.InstanceName] = true | |||
| } | |||
|
|
|||
| // if prerequisites are included in the install, don't download them again | |||
There was a problem hiding this comment.
We aren't technically downloading them, only querying for size, the comment should probably be tweaked.
|
@stolowski thanks for the feedback. The last commits add the test you mention as well as the other improvements |
Codecov Report
@@ Coverage Diff @@
## master #11159 +/- ##
==========================================
+ Coverage 78.26% 78.32% +0.06%
==========================================
Files 918 920 +2
Lines 104403 104874 +471
==========================================
+ Hits 81706 82141 +435
- Misses 17580 17604 +24
- Partials 5117 5129 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The disk space checks were obtaining the size of the snaps' prerequisites and
base via a call to the store. However, an InstallPathMany should allow the
user to install without calls to the store, if all requirements are
available locally. This change makes it so that, if the required bases and
prereqs are included in the install, those are used for the disk check instead
of gotten through the store. Note that for Install/UpdateMany with
prerequisites the already downloaded prereqs are also used, instead of
redownloaded.