store: download deltas if explicitly enabled #2017

absoludity · 2016-09-28T06:26:23Z

This branch enables (via the SNAPPY_USE_DELTAS env var) the download of deltas associated with a store downloadInfo.

It is written to default back to the normal full download if there's an error, and in this branch, even when there is no error it doesn't actually do anything with the downloaded deltas anyway (other than log success and delete them again).

I'll have another branch soon which will actually use the downloaded deltas.

(Note: when running ./run-checks I see one unrelated test failure which passes if I run that test on its own - http://paste.ubuntu.com/23245672/ - is that a known flaky test? EDIT: confirmed that re-running ./run-checks and everything passed, so I'm assuming it's flaky).

niemeyer

Looks like a good start. We discussed it online too.

Some initial points:

niemeyer · 2016-09-28T07:40:36Z

store/store_test.go

@@ -335,6 +337,201 @@ func (t *remoteRepoTestSuite) TestDownloadSyncFails(c *C) {
 	c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false)
 }

+func (t *remoteRepoTestSuite) TestDownloadWithDeltasOK(c *C) {


These tests all look almost exactly the same. Can we represent them as a table with the variants we're after and a single test function that loops over the table, instead of duplicating all the logic on every test?

Something like:

var deltaTests = []deltaTest{{ foo: 1, bar: 2, }, { baz: []string{"foo", "bar"}, }} func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { for _, test := range deltaTests { ... } }

Heh - yes. I actually prefer that style (which seems more popular in golang than in python, where I'm usually asked to split it into separate tests :) ).

Note that there are two types of tests... the first two are testing the store.Download function - and I've combined those into one with two test cases.

The other three are testing the private store.downloadDeltas function, which I've grouped together also. Much neater (as you can just compare the input and expected output).

niemeyer · 2016-09-28T08:06:30Z

store/store.go

@@ -1131,6 +1131,25 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog
 		}
 	}()

+	if os.Getenv("SNAPPY_USE_DELTAS") == "1" && len(downloadInfo.Deltas) >= 0 {


Yes. I also updated the env var to be sure it's experimental.

niemeyer · 2016-09-28T08:08:49Z

store/store.go

+			if err != nil {
+				// Just log the error and continue with the normal non-delta
+				// download.
+				logger.Noticef("cannot download deltas for %s: %v", name, err)


s/cannot/Cannot/, and s/succ/Succ/ below.

(lowercase on errors, uppercase on logs)

Ah - good to know. Done.

niemeyer · 2016-09-28T08:09:45Z

store/store.go

@@ -1131,6 +1131,25 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog
 		}
 	}()

+	if os.Getenv("SNAPPY_USE_DELTAS") == "1" && len(downloadInfo.Deltas) >= 0 {
+		downloadDir, err := ioutil.TempDir("", fmt.Sprintf("%s-deltas", name))


name+"-deltas" is nicer for these trivial cases.

niemeyer · 2016-09-28T08:11:04Z

store/store.go

@@ -1178,6 +1197,38 @@ var download = func(name, downloadURL string, user *auth.UserState, s *Store, w
 	return err
 }

+// downloadDeltas downloads the deltas associated with a downloadInfo, returning the paths.
+func (s *Store) downloadDeltas(name string, downloadDir string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) ([]string, error) {
+	deltaPaths := []string{}


deltaPaths := make([]string, 0, len(downloadInfo.Deltas)

Done.... and then removed when updating to only ever return a single delta path.

niemeyer · 2016-09-28T08:59:50Z

store/store.go

+		}
+		deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format)
+
+		w, err := os.Create(path.Join(downloadDir, deltaName))


This needs to be closed somewhen.

Right, they can be closed as soon as each delta download completes, which I've added.

niemeyer · 2016-09-28T09:02:31Z

store/store.go

+		deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format)
+
+		w, err := os.Create(path.Join(downloadDir, deltaName))
+		if err != nil {


Also needs to collect after itself on errors. This function may create multiple files, and these need to be removed before returning if something bad happened.

The call-site (store.Download) provides a working directory within which the delta is downloaded (and on the next branch, the result of applying the delta will be constructed), and cleans the directory up. I initially had downloadDelta creating the directory itself, but it seemed less clean to have the call-site having to delete the directory created by downloadDelta.

Let me know what you prefer.

absoludity

Thanks Gustavo - for the feedback here and on chat.

All the suggestions have been done below - details below.

Also, based on the chat, I've realised I got ahead of myself supporting future possibilities which we haven't set in stone. So I've simplified to implement only the initial delta download support (carefully so that we can move to, for example, support multiple deltas later, without breaking old clients).

The metadata endpoint of the store returns a list of deltas as it can be queried currently for multiple formats, and in the future we'd like to consider sending consecutive deltas when a direct one is not available. Nonetheless, I've simplified this initial snapd implementation so that the client only requests a single format (defaults to xdelta), and therefore only expects a single delta to download. If it receives more than one, it ignores deltas and does the normal full download (ie. if we change both the store and snapd in the future to send/receive multiple deltas, the worst-case for an out-of-date client is that it downloads full snaps).

Thanks

absoludity · 2016-09-28T11:54:44Z

store/store.go

@@ -1131,6 +1131,25 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog
 		}
 	}()

+	if os.Getenv("SNAPPY_USE_DELTAS") == "1" && len(downloadInfo.Deltas) >= 0 {


Yes. I also updated the env var to be sure it's experimental.

absoludity · 2016-09-28T11:56:36Z

store/store.go

@@ -1131,6 +1131,25 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog
 		}
 	}()

+	if os.Getenv("SNAPPY_USE_DELTAS") == "1" && len(downloadInfo.Deltas) >= 0 {
+		downloadDir, err := ioutil.TempDir("", fmt.Sprintf("%s-deltas", name))


absoludity · 2016-09-28T11:57:24Z

store/store.go

+			if err != nil {
+				// Just log the error and continue with the normal non-delta
+				// download.
+				logger.Noticef("cannot download deltas for %s: %v", name, err)


Ah - good to know. Done.

absoludity · 2016-09-28T11:58:49Z

store/store.go

@@ -1178,6 +1197,38 @@ var download = func(name, downloadURL string, user *auth.UserState, s *Store, w
 	return err
 }

+// downloadDeltas downloads the deltas associated with a downloadInfo, returning the paths.
+func (s *Store) downloadDeltas(name string, downloadDir string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) ([]string, error) {
+	deltaPaths := []string{}


Done.... and then removed when updating to only ever return a single delta path.

absoludity · 2016-09-28T12:03:07Z

store/store.go

+		}
+		deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format)
+
+		w, err := os.Create(path.Join(downloadDir, deltaName))


Right, they can be closed as soon as each delta download completes, which I've added.

absoludity · 2016-09-28T12:05:55Z

store/store.go

+		deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format)
+
+		w, err := os.Create(path.Join(downloadDir, deltaName))
+		if err != nil {


The call-site (store.Download) provides a working directory within which the delta is downloaded (and on the next branch, the result of applying the delta will be constructed), and cleans the directory up. I initially had downloadDelta creating the directory itself, but it seemed less clean to have the call-site having to delete the directory created by downloadDelta.

Let me know what you prefer.

absoludity · 2016-09-28T14:10:02Z

store/store_test.go

@@ -335,6 +337,201 @@ func (t *remoteRepoTestSuite) TestDownloadSyncFails(c *C) {
 	c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false)
 }

+func (t *remoteRepoTestSuite) TestDownloadWithDeltasOK(c *C) {


Heh - yes. I actually prefer that style (which seems more popular in golang than in python, where I'm usually asked to split it into separate tests :) ).

Note that there are two types of tests... the first two are testing the store.Download function - and I've combined those into one with two test cases.

The other three are testing the private store.downloadDeltas function, which I've grouped together also. Much neater (as you can just compare the input and expected output).

niemeyer

A few more points, and it's perhaps good to go in IMO.

niemeyer · 2016-09-28T18:34:38Z

store/store.go

+func (s *Store) downloadDelta(name string, downloadDir string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) (string, error) {
+
+	if len(downloadInfo.Deltas) != 1 {
+		return "", errors.New("Store returned more than one delta")


s/S/s/ (errors lowercase, logs uppercase)

Same below.

Sorry - you did mention that earlier for logs, but managed to forget it when creating those errors. Done.

niemeyer · 2016-09-28T18:35:38Z

store/store.go

+	deltaInfo := downloadInfo.Deltas[0]
+
+	if deltaInfo.Format != s.deltaFormat {
+		return "", errors.New("Store returned delta with format " + deltaInfo.Format)


return "", fmt.Errorf("store returned delta with format %q", deltaInfo.Format)

so it's quoted.

niemeyer · 2016-09-28T18:36:40Z

store/store.go

+	w, err := os.Create(path.Join(downloadDir, deltaName))
+	if err != nil {
+		return "", err
+	}


defer w.Close()

Done... although I did the similar defer func(){} to ensure we check the error on w.Close() and return that error unless there's an error being returned already (and either way deleting the file in that case). Let me know if that's not what you wanted.

niemeyer · 2016-09-28T18:37:20Z

store/store.go

+
+	err = download(deltaName, url, user, s, w, pbar)
+	if err != nil {
+		return "", err


Leaking an open file here, without the defer above.

niemeyer · 2016-09-28T18:39:23Z

store/store_test.go

@@ -335,6 +337,181 @@ func (t *remoteRepoTestSuite) TestDownloadSyncFails(c *C) {
 	c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false)
 }

+type downloadBehaviour []struct {
+	URL   string
+	Error bool


These can all lowercase. Ideally we should use uppercase specifically when we intend things to be exported somehow, either from the package or via marshaling.

niemeyer · 2016-09-28T18:40:10Z

store/store_test.go

+var deltaTests = []struct {
+	Downloads       downloadBehaviour
+	Info            snap.DownloadInfo
+	ExpectedContent string


Same.. expectedContent, etc.

...and Done.

niemeyer · 2016-09-28T18:41:32Z

store/store_test.go

+	Authenticated bool
+	Format        string
+	ExpectedURL   string
+	ExpectedPath  string


absoludity

Thanks again @niemeyer . All updated, details below (one slight difference from what you suggested for the defer w.Close())

absoludity · 2016-09-28T21:16:25Z

store/store.go

+func (s *Store) downloadDelta(name string, downloadDir string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) (string, error) {
+
+	if len(downloadInfo.Deltas) != 1 {
+		return "", errors.New("Store returned more than one delta")


Sorry - you did mention that earlier for logs, but managed to forget it when creating those errors. Done.

absoludity · 2016-09-28T21:16:42Z

store/store.go

+	deltaInfo := downloadInfo.Deltas[0]
+
+	if deltaInfo.Format != s.deltaFormat {
+		return "", errors.New("Store returned delta with format " + deltaInfo.Format)


absoludity · 2016-09-28T21:16:53Z

store/store.go

+	w, err := os.Create(path.Join(downloadDir, deltaName))
+	if err != nil {
+		return "", err
+	}


Done... although I did the similar defer func(){} to ensure we check the error on w.Close() and return that error unless there's an error being returned already (and either way deleting the file in that case). Let me know if that's not what you wanted.

absoludity · 2016-09-28T21:21:00Z

store/store_test.go

@@ -335,6 +337,181 @@ func (t *remoteRepoTestSuite) TestDownloadSyncFails(c *C) {
 	c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false)
 }

+type downloadBehaviour []struct {
+	URL   string
+	Error bool


absoludity · 2016-09-28T21:23:47Z

store/store_test.go

+var deltaTests = []struct {
+	Downloads       downloadBehaviour
+	Info            snap.DownloadInfo
+	ExpectedContent string


...and Done.

absoludity · 2016-09-28T21:27:15Z

store/store_test.go

+	Authenticated bool
+	Format        string
+	ExpectedURL   string
+	ExpectedPath  string


chipaca · 2016-09-29T08:30:13Z

store/store.go

+	deltaInfo := downloadInfo.Deltas[0]
+
+	if deltaInfo.Format != s.deltaFormat {
+		return "", fmt.Errorf("store returned delta with format %q", deltaInfo.Format)


if I saw this error I wouldn't know what the expected delta format was without digging a lot more

I've updated it to:

fmt.Errorf("store returned a download delta with the wrong format (%q instead of the configured %s format)", deltaInfo.Format, s.deltaFormat)

let me know if this is more obvious.

yep! thanks

chipaca · 2016-09-29T08:33:44Z

store/store.go

+	if err != nil {
+		return "", err
+	}
+


so at this point the file is downloaded and stored somewhere, but you have no guarantees that it ever made it to disc. This treatment is fine for tempfiles (such as what Download does) but not at all fine for things you expect to be on permanent storage. You need to download to a temporary filename (in the same directory you want it to then live), and on success sync the file, rename it, and sync the directory. Take a look at AtomicWriteFileChown in osutil/io.go.

We chatted about this - and you mentioned that it is fine without a w.Sync() even, as xdelta doesn't go through raw io, but goes through the vfs.

chipaca · 2016-09-29T08:36:21Z

store/store.go

@@ -1059,9 +1060,9 @@ func (s *Store) ListRefresh(installed []*RefreshCandidate, user *auth.UserState)
 		Data:        jsonData,
 	}

-	if os.Getenv("SNAPPY_USE_DELTAS") == "1" {
+	if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" {


can you make it SNAPD_ while you're at it? sorry to be nitpicky, and I know there are some env vars that are SNAPPY_, but I'm trying to have things that only affect snapd be SNAPD_

absoludity

Thanks @chipaca . Updated except for the delta-download-to-permanent-storage which we chatted about, as per below.

absoludity · 2016-09-29T08:42:46Z

store/store.go

@@ -1059,9 +1060,9 @@ func (s *Store) ListRefresh(installed []*RefreshCandidate, user *auth.UserState)
 		Data:        jsonData,
 	}

-	if os.Getenv("SNAPPY_USE_DELTAS") == "1" {
+	if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" {


absoludity · 2016-09-29T08:45:48Z

store/store.go

+	deltaInfo := downloadInfo.Deltas[0]
+
+	if deltaInfo.Format != s.deltaFormat {
+		return "", fmt.Errorf("store returned delta with format %q", deltaInfo.Format)


I've updated it to:

fmt.Errorf("store returned a download delta with the wrong format (%q instead of the configured %s format)", deltaInfo.Format, s.deltaFormat)

let me know if this is more obvious.

absoludity · 2016-09-29T09:43:12Z

store/store.go

+	if err != nil {
+		return "", err
+	}
+


We chatted about this - and you mentioned that it is fine without a w.Sync() even, as xdelta doesn't go through raw io, but goes through the vfs.

… both.

* Be explicit about the expected content of info.DownloadInfo.Deltas * Merge similar tests with test cases. * Use a more explicitly experimental env var name * Other minor fixes.

only expect a single delta download in response.

Defer the close of the delta download to ensure it always happens. Test structs don't need public attributes.

Add more context to delta-related error message.

Imported using git-ubuntu import. Changelog parent: d92891f New changelog entries: * New upstream release, LP: #1637215: - release: os-release on core has changed - tests: /dev/ptmx does not work on powerpc, skip here - docs: moved to github.com/snapcore/snapd/wiki (canonical#2258) - debian: golang is not installable on powerpc, use golang-any * New upstream release, LP: #1637215: - overlord/ifacestate: add unit tests for undo of setup-snap- security (canonical#2243) - daemon,overlord,snap,tests: download to .partial in final dir (canonical#2237) - overlord/state: marshaling tests for lanes (canonical#2245) - overlord/state: introduce state lanes (canonical#2241) - overlord/snapstate: fix revert+refresh (canonical#2224) - interfaces/sytemd: enable/disable generated service units (canonical#2229) - many: fix incorrect security files generation on undo - overlord/snapstate: add dynamic snapdX.Y assumes (canonical#2227) - interfaces: network-manager: give slot full read-write access to /run/NetworkManager - docs: update the name of the command for the cross-build - overlord/snapstate: fix missing argument to Noticef - snapstate: ensure gadget/core/kernel can not be disabled (canonical#2218) - asserts: limit to 1y only if len(models) == 0 (canonical#2219) - debian: only install share/locale if available (missing on powerpc) - overlrod/snapstate: fix revert followed by refresh to old-current (canonical#2214) - interfaces/builtin: network-manager and bluez can change hostname (canonical#2204) - snap: switch the auto-import dir to /run/snapd/auto-import - docs: less details about cloud.cfg as requested in trello (canonical#2206) - spread.yaml: Ensure ubuntu user has passwordless sudo for autopkgtests (canonical#2201) - interfaces/builtin: add dcdbas-control interface - boot: do not set boot to try mode if the revision is unchanged - interfaces: add shutdown interface (canonical#2162) - interfaces: add system-power-control interface - many: use the new systemd backend for configuring GPIOs - overlord/ifacestate: setup security for slots before plugs - snap: spool assertion candidates if snapd is not up yet - store,daemon,overlord: download things to a partials dir - asserts,daemon: implement system-user-authority header/concept - interfaces/builtin: home base declaration rule using on-classic for its policy - interfaces/builtin: finish decl based checks - asserts: bump snap-declaration to allow signing with new-style plugs and slots - overlord: checks for kernel installation/refresh based on model assertion and previous kernel - tests/lib/fakestore: fix logic to distinguish assertion not found errors - client: add a few explicit error types (around the request cycle) - tests/lib/fakestore/cmd/fakestore: make it log, and fix a typo - overlord/snapstate: two bugs for one - snappy: disable auto-import of assertions on classic (canonical#2122) - overlord/snapstate: move trash cleanup to a cleanup handler (canonical#2173) - daemon: make create-user --known fail on classic without --force- managed (canonical#2123) - asserts,interfaces/policy: implement on-classic plug/slot constraints - overlord: check that the first installed gadget matches the model assertion - tests: use the snapd-control-consumer snap from the store - cmd/snap: make snap run not talk to snapd for finding the revision - snap/squashfs: try to hard link instead of copying. Also, switch to osutil.CopyFile for cp invocation. - store: send supported max-format when retrieving assertions - snapstate, devicestate: do not remove seed - boot,image,overlord,partition: read/write boot variables in single operation - tests: reenable ubuntu-core tests on qemu - asserts,interfaces/policy: allow OR-ing of subrule constraints in plug/slot rules - many: move from flags as ints to flags as structs-of-bools (canonical#2156) - many: add supports for keeping and finding assertions with different format iterations - snap: stop using ubuntu-core-launcher, use snap-confine - many: introduce an assertion format iteration concept, refuse to add unsupported assertion - interfaces: tweak wording and comment - spread.yaml: dump apparmor denials on spread failure - tests: unflake ubuntu-core-reboot (canonical#2150) - cmd/snap: tweak unknown command error message (canonical#2139) - client,daemon,cmd: add payment-declined error kind (canonical#2107) - cmd/snap: update remove command help (canonical#2145) - many: removed frameworks target and fixed service files (canonical#2138) - asserts,snap: validate attributes to a JSON-compatible type subset (canonical#2140) - asserts: remove unused serial-proof type - tests: skip auto-import tests on systems without test keys (canonical#2142) - overlord/devicestate: don't spam the debug log on classic (canonical#2141) - cmd/snap: simplify auto-import mountinfo parsing (canonical#2135) - tests: run ubuntu-core upgrades on isolated machine (canonical#2137) - overlord/devicestate: recover seeding from old external approach (canonical#2134) - overlord: merge overlord/boot pkg into overlord/devicestate (canonical#2118) - daemon: add postCreateUserSuite test suite (canonical#2124) - tests: abort tests if an update process is scheduled (canonical#2119) - snapstate: avoid reboots if nothing in the boot setup has changed (canonical#2117) - cmd/snap: do not auto-import from loop or non-dev devices (canonical#2121) - tests: add spread test for `snap auto-import` (canonical#2126) - tests: add test for auto-mount assertion import (canonical#2127) - osutil: add missing unit tests for IsMounted (canonical#2133) - tests: check for failure creating user on managed ubuntu-core systems (canonical#2096) - snap: ignore /dev/loop addings from udev (canonical#2111) - tests: remove snapd.boot-ok reference (canonical#2109) - tests: enable tests related to the home interface in all-snaps (canonical#2106) - snapstate: only import defaults from gadget on install (canonical#2105) - many: move firstboot code into the snapd daemon (canonical#2033) - store: send correct JSON type of string for expected payment amount (canonical#2103) - cmd/snap: rename is-managed to managed and tune (canonical#2102) - interfaces,overlord/ifacestate: initial cleaning up of no arg AutoConnect related bits (canonical#2090) - client, cmd: prompt for password when buying (canonical#2086) - snapstate: fix hanging `snap remove` if snap is no longer mounted - image: support gadget specific cloud.conf file (canonical#2101) - cmd/snap,ctlcmd: fix behavior of snap(ctl) get (canonical#2093) - store: local users download from the anonymous url (canonical#2100) - docs/hooks.md: fix typos (canonical#2099) - many: check installation of slots and plugs against declarations - docs: fix missing "=" in the systemd-active docs - store: do not set store auth for local users (canonical#2092) - interfaces,overlord/ifacestate: use declaration-based checking for auto-connect (canonical#2071) - overlord, daemon, snap: support gadget config defaults (canonical#2082)The main semantic changes are: - tests: fix snap-disconnect tests after core rename (canonical#2088) - client,daemon,overlord,cmd: add /v2/users and create-user on auto- import (canonical#2074) - many: abbreviated forms of disconnect (canonical#2066) - asserts: require lowercase model until insensitive matching is ready (canonical#2076) - cmd/snap: add version command, same as --version (canonical#2075) - all: use "core" by default but allow "ubuntu-core" still (canonical#2070) - overlord/devicestate, docs/hooks.md: nest prepare-device configuration options - daemon: fix login API to return local macaroons (canonical#2078) - daemon: do not hardcode UID in userLookup (canonical#2080) - client, cmd: connect fixes (canonical#2026) - many: preparations for switching most of autoconnect to use the declarationsfor now: - overlord/auth: update CheckMacaroon to verify local snapd macaroons (canonical#2069) - cmd/snap: trivial auto-import and download tweaks (canonical#2067) - interfaces: add repo.ResolveConnect that handles name resolution - interfaces/policy: introduce InstallCandidate and its checks - interfaces/policy,overlord: check connection requests against the declarations in ifacestate - many: setup snapd macaroon for local users (canonical#2051)Next step: do snapd macaroons verification. - interfaces/policy: implement snap-id/publisher-id checks - many: change Connect to take ConnRef instead of strings (canonical#2060) - snap: auto mount block devices and import assertions (canonical#2047) - daemon: add `snap create-user --force-managed` support (canonical#2041) - docs: remove references to removed buying features (canonical#2057) - interfaces,docs: allow sharing SNAP{,_DATA,_COMMON} via content iface (canonical#2063) - interfaces: add Plug/Slot/Connection reference helpers (canonical#2056) - client,daemon,cmd/snap: improve create-user APIs (canonical#2054) - many: introduce snap refresh --ignore-validation <snap> to override refresh validation (canonical#2052) - daemon: add support for `snap create-user --known` (canonical#2040) - interfaces/policy: start of interface policy checking code based on declarations (canonical#2050) - overlord/configstate: support nested configuration (canonical#2039) - asserts,interfaces/builtin,overlord/assertstate: introduce base- declaration (canonical#2037) - interfaces: builtin: Allow writing DHCP lease files to /run/NetworkManager/dhcp (canonical#2049) - many: remove all traces of the /v2/buy/methods endpoint (canonical#2045) - tests: add external spread backend (canonical#1918) - asserts: parse the slot rules in snap-declarations (canonical#2035) - interfaces: allow read of /etc/ld.so.preload by default for armhf on series 16 (canonical#2048) - store: change purchase to order and store clean up first pass (canonical#2043) - daemon, store: switch to new store APIs in snapd (canonical#2036) - many: add email to UserState (canonical#2038) - asserts: support parsing the plugs stanza i.e. plug rules in snap- declarations (canonical#2027) - store: apply deltas if explicitly enabled (canonical#2031) - tests: fix create-key/snap-sign test isolation (canonical#2032) - snap/implicit: don't restrict the camera iface to clasic (canonical#2025) - client, cmd: change buy command to match UX document (canonical#2011) - coreconfig: nuke it. Also, ignore po/snappy.pot. (canonical#2030) - store: download deltas if explicitly enabled (canonical#2017) - many: allow use of the system user assertion with create-user (canonical#1990) - asserts,overlord,snap: add prepare-device hook for device registration (canonical#2005) - debian: adjust packaging for trusty/deputy systemd (canonical#2003) - asserts: introduce AttributeConstraints (canonical#2015) - interface/builtin: access system bus on screen-inhibit-control - tests: add firewall-control interface test (canonical#2009) - snapstate: pass errors from ListRefresh in updateInfo (canonical#2018) - README: add links to IRC, mailing list and social media (canonical#2022) - docs: add `configure` hook to hooks list (canonical#2024)LP: #1596629 - cmd/snap,configstate: rename apply-config variables to configure. (canonical#2023) - store: retry download on 500 (canonical#2019) - interfaces/builtin: support time and date settings via 'org.freedesktop.timedate1 (canonical#1832)

niemeyer requested changes Sep 28, 2016

View reviewed changes

niemeyer changed the title ~~Download deltas (but do nothing with them yet) when env var is set.~~ store: download deltas if explicitly enabled Sep 28, 2016

absoludity force-pushed the download-deltas-try-2 branch from 985ccec to a69a1aa Compare September 28, 2016 15:41

absoludity commented Sep 28, 2016

View reviewed changes

niemeyer approved these changes Sep 28, 2016

View reviewed changes

absoludity commented Sep 28, 2016

View reviewed changes

chipaca suggested changes Sep 29, 2016

View reviewed changes

absoludity commented Sep 29, 2016

View reviewed changes

chipaca approved these changes Sep 29, 2016

View reviewed changes

Michael Nelson added 6 commits September 29, 2016 10:11

Download deltas (but do nothing with them yet) when env var is set.

3c6c8cc

Ensure either the success or failure log message is executed, but not…

47fcc03

… both.

Review feedback:

b1d5adb

* Be explicit about the expected content of info.DownloadInfo.Deltas * Merge similar tests with test cases. * Use a more explicitly experimental env var name * Other minor fixes.

Simplify client to request a single delta format only and therefore

4b6d3f0

only expect a single delta download in response.

Fix the error strings (lower-case).

975a477

Defer the close of the delta download to ensure it always happens. Test structs don't need public attributes.

Rename the env var switch for deltas to use SNAPD_ prefix.

1419b6c

Add more context to delta-related error message.

absoludity force-pushed the download-deltas-try-2 branch from 1161612 to 1419b6c Compare September 29, 2016 10:35

chipaca merged commit 58da1b0 into canonical:master Sep 29, 2016

store: download deltas if explicitly enabled #2017

store: download deltas if explicitly enabled #2017

Conversation

absoludity commented Sep 28, 2016 • edited Loading

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

niemeyer Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

absoludity Sep 28, 2016 • 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

absoludity Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

absoludity 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

absoludity Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity Sep 28, 2016 • edited Loading

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

absoludity Sep 28, 2016 • 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

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity Sep 28, 2016 • 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

absoludity Sep 29, 2016 • 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

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity commented Sep 28, 2016 •

edited

Loading

niemeyer Sep 28, 2016 •

edited

Loading

absoludity Sep 28, 2016 •

edited

Loading

absoludity Sep 28, 2016 •

edited

Loading

absoludity Sep 28, 2016 •

edited

Loading

absoludity Sep 28, 2016 •

edited

Loading

absoludity Sep 28, 2016 •

edited

Loading

absoludity Sep 28, 2016 •

edited

Loading

absoludity Sep 29, 2016 •

edited

Loading

absoludity Sep 29, 2016 •

edited

Loading