From 3c6c8cce12adf240935ed5e4c9b9bdc5321ddf1e Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 28 Sep 2016 05:52:01 +0000 Subject: [PATCH 1/6] Download deltas (but do nothing with them yet) when env var is set. --- store/store.go | 51 ++++++++++++ store/store_test.go | 196 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 247 insertions(+) diff --git a/store/store.go b/store/store.go index e8cbb8fca06..40f48231e0d 100644 --- a/store/store.go +++ b/store/store.go @@ -1132,6 +1132,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)) + if err == nil { + defer os.RemoveAll(downloadDir) + + _, err = s.downloadDeltas(name, downloadDir, downloadInfo, pbar, user) + // We revert to normal downloads if there is any error + 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) + } + + // Currently even on successful delta downloads, continue with the + // normal full download. + logger.Debugf("successfully downloaded deltas for %s", name) + } + } + url := downloadInfo.AnonDownloadURL if url == "" || user != nil { url = downloadInfo.DownloadURL @@ -1195,6 +1214,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{} + + // Initially we only download our first supported format (xdelta). + deltaFormat := s.deltaFormats[0] + + for _, deltaInfo := range downloadInfo.Deltas { + if deltaInfo.Format != deltaFormat { + continue + } + 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 { + return nil, err + } + + url := deltaInfo.AnonDownloadURL + if url == "" || user != nil { + url = deltaInfo.DownloadURL + } + + err = download(deltaName, url, user, s, w, pbar) + if err != nil { + return nil, err + } + deltaPaths = append(deltaPaths, w.Name()) + } + return deltaPaths, nil +} + type assertionSvcError struct { Status int `json:"status"` Type string `json:"type"` diff --git a/store/store_test.go b/store/store_test.go index c51b5cef335..6abb921c3a5 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -22,6 +22,7 @@ package store import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -29,6 +30,7 @@ import ( "net/http/httptest" "net/url" "os" + "path" "strings" "testing" @@ -389,6 +391,200 @@ func (t *remoteRepoTestSuite) TestActualDownload500(c *C) { c.Assert(err, FitsTypeOf, &ErrDownload{}) c.Check(err.(*ErrDownload).Code, Equals, http.StatusInternalServerError) c.Check(n, Equals, len(downloadBackoffs)) // woo!! + +func (t *remoteRepoTestSuite) TestDownloadWithDeltasOK(c *C) { + orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") + defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + + expectedDownloads := []struct { + URL string + Content string + }{ + {URL: "anon-delta-url-1", Content: "delta content 1"}, + {URL: "anon-delta-url-2", Content: "delta content 2"}, + {URL: "full-snap-anon-url", Content: "full snap content"}, + } + deltaIndex := 0 + download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { + c.Check(url, Equals, expectedDownloads[deltaIndex].URL) + w.Write([]byte(expectedDownloads[deltaIndex].Content)) + deltaIndex++ + return nil + } + + downloadInfo := &snap.DownloadInfo{ + AnonDownloadURL: "full-snap-anon-url", + DownloadURL: "full-snap-auth-url", + Deltas: []snap.DeltaInfo{ + {AnonDownloadURL: "anon-delta-url-1", Format: "xdelta"}, + {AnonDownloadURL: "anon-delta-url-2", Format: "xdelta"}, + }, + } + + path, err := t.store.Download("foo", downloadInfo, nil, nil) + + c.Assert(err, IsNil) + defer os.Remove(path) + content, err := ioutil.ReadFile(path) + c.Assert(err, IsNil) + // Currently we still download the complete package and return its content. + c.Assert(string(content), Equals, "full snap content") +} + +func (t *remoteRepoTestSuite) TestDownloadWithDeltasErrorDefaultsToNonDeltaDownload(c *C) { + // If the delta downloads cause any error, the Download function defaults + // back to its normal behaviour and downloads the full snap. + orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") + defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + + firstTime := true + download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { + if firstTime { + firstTime = false + c.Check(url, Equals, "anon-delta-url-1") + return errors.New("Bang") + } + c.Check(url, Equals, "full-snap-anon-url") + w.Write([]byte("full snap content")) + return nil + } + + downloadInfo := &snap.DownloadInfo{ + AnonDownloadURL: "full-snap-anon-url", + DownloadURL: "full-snap-auth-url", + Deltas: []snap.DeltaInfo{ + {AnonDownloadURL: "anon-delta-url-1", Format: "xdelta"}, + }, + } + + path, err := t.store.Download("foo", downloadInfo, nil, nil) + c.Assert(err, IsNil) + defer os.Remove(path) + + content, err := ioutil.ReadFile(path) + c.Assert(err, IsNil) + c.Assert(string(content), Equals, "full snap content") +} + +func (t *remoteRepoTestSuite) TestAuthDownloadDeltasDoesNotUseAnonURL(c *C) { + orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") + defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + + expectedDownloads := []struct { + URL string + Content string + }{ + {URL: "auth-delta-url-1", Content: "delta content 1"}, + {URL: "auth-delta-url-2", Content: "delta content 2"}, + } + deltaIndex := 0 + download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { + c.Check(user, Equals, t.user) + c.Check(url, Equals, expectedDownloads[deltaIndex].URL) + w.Write([]byte(expectedDownloads[deltaIndex].Content)) + deltaIndex++ + return nil + } + + downloadInfo := &snap.DownloadInfo{ + AnonDownloadURL: "full-snap-anon-url", + DownloadURL: "full-snap-auth-url", + Deltas: []snap.DeltaInfo{ + {AnonDownloadURL: "anon-delta-url-1", DownloadURL: "auth-delta-url-1", Format: "xdelta"}, + {AnonDownloadURL: "anon-delta-url-2", DownloadURL: "auth-delta-url-2", Format: "xdelta"}, + }, + } + + downloadDir, err := ioutil.TempDir("", "") + defer os.RemoveAll(downloadDir) + + deltaPaths, err := t.store.downloadDeltas("foo", downloadDir, downloadInfo, nil, t.user) + + c.Assert(err, IsNil) + c.Assert(len(deltaPaths), Equals, 2) +} + +func (t *remoteRepoTestSuite) TestDownloadDeltasFilePathsAndContents(c *C) { + orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") + defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + + expectedDownloads := []struct { + URL string + Content string + Path string + }{ + {URL: "anon-delta-url-1", Content: "delta content 1", Path: "foo_1_2_delta.xdelta"}, + {URL: "anon-delta-url-2", Content: "delta content 2", Path: "foo_2_3_delta.xdelta"}, + } + deltaIndex := 0 + download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { + c.Check(url, Equals, expectedDownloads[deltaIndex].URL) + w.Write([]byte(expectedDownloads[deltaIndex].Content)) + deltaIndex++ + return nil + } + + downloadInfo := &snap.DownloadInfo{ + AnonDownloadURL: "full-snap-anon-url", + DownloadURL: "full-snap-auth-url", + Deltas: []snap.DeltaInfo{ + {AnonDownloadURL: "anon-delta-url-1", DownloadURL: "auth-delta-url-1", Format: "xdelta", + FromRevision: 1, ToRevision: 2}, + {AnonDownloadURL: "anon-delta-url-2", DownloadURL: "auth-delta-url-2", Format: "xdelta", + FromRevision: 2, ToRevision: 3}, + }, + } + + downloadDir, err := ioutil.TempDir("", "") + defer os.RemoveAll(downloadDir) + + deltaPaths, err := t.store.downloadDeltas("foo", downloadDir, downloadInfo, nil, nil) + + c.Assert(err, IsNil) + c.Assert(len(deltaPaths), Equals, 2) + for i, deltaPath := range deltaPaths { + c.Assert(deltaPath, Equals, path.Join(downloadDir, expectedDownloads[i].Path)) + content, err := ioutil.ReadFile(deltaPath) + c.Assert(err, IsNil) + c.Assert(string(content), Equals, expectedDownloads[i].Content) + } +} + +func (t *remoteRepoTestSuite) TestDownloadDeltasDownloadsOneFormatOnly(c *C) { + orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") + defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + + expectedURLs := []string{"anon-xdelta-url-1", "anon-xdelta-url-2"} + deltaIndex := 0 + download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { + c.Check(url, Equals, expectedURLs[deltaIndex]) + w.Write([]byte("content")) + deltaIndex++ + return nil + } + + downloadInfo := &snap.DownloadInfo{ + AnonDownloadURL: "full-snap-anon-url", + DownloadURL: "full-snap-auth-url", + Deltas: []snap.DeltaInfo{ + {AnonDownloadURL: "anon-xdelta-url-1", Format: "xdelta"}, + {AnonDownloadURL: "anon-xdelta-url-2", Format: "xdelta"}, + {AnonDownloadURL: "anon-bsdiff-url-3", Format: "bsdiff"}, + }, + } + + downloadDir, err := ioutil.TempDir("", "") + defer os.RemoveAll(downloadDir) + + deltaPaths, err := t.store.downloadDeltas("foo", downloadDir, downloadInfo, nil, nil) + + c.Assert(err, IsNil) + c.Assert(len(deltaPaths), Equals, 2) } func (t *remoteRepoTestSuite) TestDoRequestSetsAuth(c *C) { From 47fcc030ec8b232893a8879d86de9505f980bac7 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 28 Sep 2016 06:35:12 +0000 Subject: [PATCH 2/6] Ensure either the success or failure log message is executed, but not both. --- store/store.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/store/store.go b/store/store.go index 40f48231e0d..f92035fc36d 100644 --- a/store/store.go +++ b/store/store.go @@ -1143,11 +1143,11 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog // Just log the error and continue with the normal non-delta // download. logger.Noticef("cannot download deltas for %s: %v", name, err) + } else { + // Currently even on successful delta downloads, continue with the + // normal full download. + logger.Debugf("successfully downloaded deltas for %s", name) } - - // Currently even on successful delta downloads, continue with the - // normal full download. - logger.Debugf("successfully downloaded deltas for %s", name) } } From b1d5adbfd34db6bcf59affe6159d0073148003b1 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 28 Sep 2016 14:13:37 +0000 Subject: [PATCH 3/6] Review feedback: * 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. --- snap/info.go | 4 + store/store.go | 19 ++- store/store_test.go | 304 ++++++++++++++++++-------------------------- 3 files changed, 143 insertions(+), 184 deletions(-) diff --git a/snap/info.go b/snap/info.go index 059a4438221..5b2d42e355a 100644 --- a/snap/info.go +++ b/snap/info.go @@ -234,6 +234,10 @@ type DownloadInfo struct { Size int64 `json:"size,omitempty"` Sha3_384 string `json:"sha3-384,omitempty"` + // The server can include information about available deltas for a given + // snap at a specific revision during refresh. Currently during refresh the + // server will provide single matching deltas only, from the clients + // revision to the target revision when available, per requested format. Deltas []DeltaInfo `json:"deltas,omitempty"` } diff --git a/store/store.go b/store/store.go index f92035fc36d..1b46d126f21 100644 --- a/store/store.go +++ b/store/store.go @@ -1060,7 +1060,7 @@ 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" { reqOptions.ExtraHeaders = map[string]string{ "X-Ubuntu-Delta-Formats": strings.Join(s.deltaFormats, ","), } @@ -1132,8 +1132,8 @@ 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)) + if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" && len(downloadInfo.Deltas) > 0 { + downloadDir, err := ioutil.TempDir("", name+"-deltas") if err == nil { defer os.RemoveAll(downloadDir) @@ -1142,11 +1142,11 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog 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) + logger.Noticef("Cannot download deltas for %s: %v", name, err) } else { // Currently even on successful delta downloads, continue with the // normal full download. - logger.Debugf("successfully downloaded deltas for %s", name) + logger.Debugf("Successfully downloaded deltas for %s", name) } } } @@ -1216,9 +1216,9 @@ var download = func(name, downloadURL string, user *auth.UserState, s *Store, w // 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)) - // Initially we only download our first supported format (xdelta). + // We only download our preferred delta format. deltaFormat := s.deltaFormats[0] for _, deltaInfo := range downloadInfo.Deltas { @@ -1241,6 +1241,11 @@ func (s *Store) downloadDeltas(name string, downloadDir string, downloadInfo *sn if err != nil { return nil, err } + err = w.Close() + if err != nil { + return nil, err + } + deltaPaths = append(deltaPaths, w.Name()) } return deltaPaths, nil diff --git a/store/store_test.go b/store/store_test.go index 6abb921c3a5..0ee6781c50b 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -391,200 +391,150 @@ func (t *remoteRepoTestSuite) TestActualDownload500(c *C) { c.Assert(err, FitsTypeOf, &ErrDownload{}) c.Check(err.(*ErrDownload).Code, Equals, http.StatusInternalServerError) c.Check(n, Equals, len(downloadBackoffs)) // woo!! +} -func (t *remoteRepoTestSuite) TestDownloadWithDeltasOK(c *C) { - orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") - defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) - - expectedDownloads := []struct { - URL string - Content string - }{ - {URL: "anon-delta-url-1", Content: "delta content 1"}, - {URL: "anon-delta-url-2", Content: "delta content 2"}, - {URL: "full-snap-anon-url", Content: "full snap content"}, - } - deltaIndex := 0 - download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { - c.Check(url, Equals, expectedDownloads[deltaIndex].URL) - w.Write([]byte(expectedDownloads[deltaIndex].Content)) - deltaIndex++ - return nil - } +type downloadBehaviour []struct { + URL string + Error bool +} - downloadInfo := &snap.DownloadInfo{ - AnonDownloadURL: "full-snap-anon-url", - DownloadURL: "full-snap-auth-url", +var deltaTests = []struct{ + Downloads downloadBehaviour + Info snap.DownloadInfo + ExpectedContent string +}{{ + // The full snap is downloaded after the delta (currently, + // until the code applying the delta lands). + Downloads: downloadBehaviour{ + {URL: "delta-url"}, + {URL: "full-snap-url"}, + }, + Info: snap.DownloadInfo{ + AnonDownloadURL: "full-snap-url", Deltas: []snap.DeltaInfo{ - {AnonDownloadURL: "anon-delta-url-1", Format: "xdelta"}, - {AnonDownloadURL: "anon-delta-url-2", Format: "xdelta"}, + {AnonDownloadURL: "delta-url", Format: "xdelta"}, }, - } - - path, err := t.store.Download("foo", downloadInfo, nil, nil) + }, + ExpectedContent: "full-snap-url-content", +}, { + // If there is an error during the delta download, the + // full snap is downloaded as per normal. + Downloads: downloadBehaviour{ + {Error: true}, + {URL: "full-snap-url"}, + }, + Info: snap.DownloadInfo{ + AnonDownloadURL: "full-snap-url", + Deltas: []snap.DeltaInfo{ + {AnonDownloadURL: "delta-url", Format: "xdelta"}, + }, + }, + ExpectedContent: "full-snap-url-content", +}} - c.Assert(err, IsNil) - defer os.Remove(path) - content, err := ioutil.ReadFile(path) - c.Assert(err, IsNil) - // Currently we still download the complete package and return its content. - c.Assert(string(content), Equals, "full snap content") -} +func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { + origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) -func (t *remoteRepoTestSuite) TestDownloadWithDeltasErrorDefaultsToNonDeltaDownload(c *C) { - // If the delta downloads cause any error, the Download function defaults - // back to its normal behaviour and downloads the full snap. - orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") - defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + for _, testCase := range deltaTests { - firstTime := true - download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { - if firstTime { - firstTime = false - c.Check(url, Equals, "anon-delta-url-1") - return errors.New("Bang") + downloadIndex := 0 + download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { + if testCase.Downloads[downloadIndex].Error { + downloadIndex++ + return errors.New("Bang") + } + c.Check(url, Equals, testCase.Downloads[downloadIndex].URL) + w.Write([]byte(testCase.Downloads[downloadIndex].URL+"-content")) + downloadIndex++ + return nil } - c.Check(url, Equals, "full-snap-anon-url") - w.Write([]byte("full snap content")) - return nil - } - downloadInfo := &snap.DownloadInfo{ - AnonDownloadURL: "full-snap-anon-url", - DownloadURL: "full-snap-auth-url", - Deltas: []snap.DeltaInfo{ - {AnonDownloadURL: "anon-delta-url-1", Format: "xdelta"}, - }, - } - path, err := t.store.Download("foo", downloadInfo, nil, nil) - c.Assert(err, IsNil) - defer os.Remove(path) + path, err := t.store.Download("foo", &testCase.Info, nil, nil) - content, err := ioutil.ReadFile(path) - c.Assert(err, IsNil) - c.Assert(string(content), Equals, "full snap content") -} - -func (t *remoteRepoTestSuite) TestAuthDownloadDeltasDoesNotUseAnonURL(c *C) { - orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") - defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) - - expectedDownloads := []struct { - URL string - Content string - }{ - {URL: "auth-delta-url-1", Content: "delta content 1"}, - {URL: "auth-delta-url-2", Content: "delta content 2"}, - } - deltaIndex := 0 - download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { - c.Check(user, Equals, t.user) - c.Check(url, Equals, expectedDownloads[deltaIndex].URL) - w.Write([]byte(expectedDownloads[deltaIndex].Content)) - deltaIndex++ - return nil + c.Assert(err, IsNil) + defer os.Remove(path) + content, err := ioutil.ReadFile(path) + c.Assert(err, IsNil) + c.Assert(string(content), Equals, testCase.ExpectedContent) } +} - downloadInfo := &snap.DownloadInfo{ - AnonDownloadURL: "full-snap-anon-url", - DownloadURL: "full-snap-auth-url", +var downloadDeltaTests = []struct{ + Info snap.DownloadInfo + Authenticated bool + Formats []string + ExpectedURL string + ExpectedPath string +}{{ + // An unauthenticated request downloads the anonymous delta url. + Info: snap.DownloadInfo{ Deltas: []snap.DeltaInfo{ - {AnonDownloadURL: "anon-delta-url-1", DownloadURL: "auth-delta-url-1", Format: "xdelta"}, - {AnonDownloadURL: "anon-delta-url-2", DownloadURL: "auth-delta-url-2", Format: "xdelta"}, + {AnonDownloadURL: "anon-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26}, }, - } - - downloadDir, err := ioutil.TempDir("", "") - defer os.RemoveAll(downloadDir) - - deltaPaths, err := t.store.downloadDeltas("foo", downloadDir, downloadInfo, nil, t.user) - - c.Assert(err, IsNil) - c.Assert(len(deltaPaths), Equals, 2) -} - -func (t *remoteRepoTestSuite) TestDownloadDeltasFilePathsAndContents(c *C) { - orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") - defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) - - expectedDownloads := []struct { - URL string - Content string - Path string - }{ - {URL: "anon-delta-url-1", Content: "delta content 1", Path: "foo_1_2_delta.xdelta"}, - {URL: "anon-delta-url-2", Content: "delta content 2", Path: "foo_2_3_delta.xdelta"}, - } - deltaIndex := 0 - download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { - c.Check(url, Equals, expectedDownloads[deltaIndex].URL) - w.Write([]byte(expectedDownloads[deltaIndex].Content)) - deltaIndex++ - return nil - } - - downloadInfo := &snap.DownloadInfo{ - AnonDownloadURL: "full-snap-anon-url", - DownloadURL: "full-snap-auth-url", + }, + Authenticated: false, + Formats: []string{"xdelta"}, + ExpectedURL: "anon-delta-url", + ExpectedPath: "snapname_24_26_delta.xdelta", +}, { + // An authenticated request downloads the authenticated delta url. + Info: snap.DownloadInfo{ Deltas: []snap.DeltaInfo{ - {AnonDownloadURL: "anon-delta-url-1", DownloadURL: "auth-delta-url-1", Format: "xdelta", - FromRevision: 1, ToRevision: 2}, - {AnonDownloadURL: "anon-delta-url-2", DownloadURL: "auth-delta-url-2", Format: "xdelta", - FromRevision: 2, ToRevision: 3}, + {DownloadURL: "auth-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26}, }, - } - - downloadDir, err := ioutil.TempDir("", "") - defer os.RemoveAll(downloadDir) - - deltaPaths, err := t.store.downloadDeltas("foo", downloadDir, downloadInfo, nil, nil) + }, + Authenticated: true, + Formats: []string{"xdelta"}, + ExpectedURL: "auth-delta-url", + ExpectedPath: "snapname_24_26_delta.xdelta", +}, { + // The preferred delta format is downloaded. + Info: snap.DownloadInfo{ + Deltas: []snap.DeltaInfo{ + {DownloadURL: "delta-url", Format: "format1", FromRevision: 24, ToRevision: 26}, + {DownloadURL: "preferred-delta-url", Format: "format2", FromRevision: 24, ToRevision: 26}, + }, + }, + Authenticated: false, + Formats: []string{"format2", "format1"}, + ExpectedURL: "preferred-delta-url", + ExpectedPath: "snapname_24_26_delta.format2", +}} +func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) { + origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) + + for _, testCase := range downloadDeltaTests { + t.store.deltaFormats = testCase.Formats + download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { + expectedUser := t.user + if !testCase.Authenticated { + expectedUser = nil + } + c.Check(user, Equals, expectedUser) + c.Check(url, Equals, testCase.ExpectedURL) + return nil + } - c.Assert(err, IsNil) - c.Assert(len(deltaPaths), Equals, 2) - for i, deltaPath := range deltaPaths { - c.Assert(deltaPath, Equals, path.Join(downloadDir, expectedDownloads[i].Path)) - content, err := ioutil.ReadFile(deltaPath) + downloadDir, err := ioutil.TempDir("", "") c.Assert(err, IsNil) - c.Assert(string(content), Equals, expectedDownloads[i].Content) - } -} + defer os.RemoveAll(downloadDir) -func (t *remoteRepoTestSuite) TestDownloadDeltasDownloadsOneFormatOnly(c *C) { - orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") - defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + authedUser := t.user + if !testCase.Authenticated { + authedUser = nil + } - expectedURLs := []string{"anon-xdelta-url-1", "anon-xdelta-url-2"} - deltaIndex := 0 - download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { - c.Check(url, Equals, expectedURLs[deltaIndex]) - w.Write([]byte("content")) - deltaIndex++ - return nil - } + deltaPaths, err := t.store.downloadDeltas("snapname", downloadDir, &testCase.Info, nil, authedUser) - downloadInfo := &snap.DownloadInfo{ - AnonDownloadURL: "full-snap-anon-url", - DownloadURL: "full-snap-auth-url", - Deltas: []snap.DeltaInfo{ - {AnonDownloadURL: "anon-xdelta-url-1", Format: "xdelta"}, - {AnonDownloadURL: "anon-xdelta-url-2", Format: "xdelta"}, - {AnonDownloadURL: "anon-bsdiff-url-3", Format: "bsdiff"}, - }, + c.Assert(err, IsNil) + c.Assert(len(deltaPaths), Equals, 1) + c.Assert(deltaPaths[0], Equals, path.Join(downloadDir, testCase.ExpectedPath)) } - - downloadDir, err := ioutil.TempDir("", "") - defer os.RemoveAll(downloadDir) - - deltaPaths, err := t.store.downloadDeltas("foo", downloadDir, downloadInfo, nil, nil) - - c.Assert(err, IsNil) - c.Assert(len(deltaPaths), Equals, 2) } func (t *remoteRepoTestSuite) TestDoRequestSetsAuth(c *C) { @@ -1864,9 +1814,9 @@ var MockUpdatesWithDeltasJSON = ` ` func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithDeltas(c *C) { - orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") - defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil) + origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c.Check(r.Header.Get("X-Ubuntu-Delta-Formats"), Equals, `xdelta`) @@ -1939,9 +1889,9 @@ func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithDeltas(c * func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithoutDeltas(c *C) { // Verify the X-Delta-Format header is not set. - orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS") - defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "0"), IsNil) + origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "0"), IsNil) mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c.Check(r.Header.Get("X-Ubuntu-Delta-Formats"), Equals, ``) From 4b6d3f040f9f6d544d6076f3f72a4cc99ba58941 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 28 Sep 2016 15:23:37 +0000 Subject: [PATCH 4/6] Simplify client to request a single delta format only and therefore only expect a single delta download in response. --- store/store.go | 81 ++++++++++++++++++++--------------------- store/store_test.go | 87 ++++++++++++++++++++++++++++++--------------- 2 files changed, 100 insertions(+), 68 deletions(-) diff --git a/store/store.go b/store/store.go index 1b46d126f21..cc21da12d6a 100644 --- a/store/store.go +++ b/store/store.go @@ -23,6 +23,7 @@ package store import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -141,7 +142,7 @@ type Config struct { Series string DetailFields []string - DeltaFormats []string + DeltaFormat string } // Store represents the ubuntu snap store @@ -160,7 +161,7 @@ type Store struct { fallbackStoreID string detailFields []string - deltaFormats []string + deltaFormat string // reused http client client *http.Client @@ -312,8 +313,8 @@ type searchResults struct { // The fields we are interested in var detailFields = getStructFields(snapDetails{}) -// The default delta formats if none are configured. -var defaultSupportedDeltaFormats = []string{"xdelta"} +// The default delta format if not configured. +var defaultSupportedDeltaFormat = "xdelta" // New creates a new Store with the given access configuration and for given the store id. func New(cfg *Config, authContext auth.AuthContext) *Store { @@ -357,9 +358,9 @@ func New(cfg *Config, authContext auth.AuthContext) *Store { series = cfg.Series } - deltaFormats := cfg.DeltaFormats - if deltaFormats == nil { - deltaFormats = defaultSupportedDeltaFormats + deltaFormat := cfg.DeltaFormat + if deltaFormat == "" { + deltaFormat = defaultSupportedDeltaFormat } // see https://wiki.ubuntu.com/AppStore/Interfaces/ClickPackageIndex @@ -377,7 +378,7 @@ func New(cfg *Config, authContext auth.AuthContext) *Store { detailFields: fields, client: newHTTPClient(), authContext: authContext, - deltaFormats: deltaFormats, + deltaFormat: deltaFormat, } } @@ -1062,7 +1063,7 @@ func (s *Store) ListRefresh(installed []*RefreshCandidate, user *auth.UserState) if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" { reqOptions.ExtraHeaders = map[string]string{ - "X-Ubuntu-Delta-Formats": strings.Join(s.deltaFormats, ","), + "X-Ubuntu-Delta-Formats": s.deltaFormat, } } @@ -1132,12 +1133,12 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog } }() - if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" && len(downloadInfo.Deltas) > 0 { + if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" && len(downloadInfo.Deltas) == 1 { downloadDir, err := ioutil.TempDir("", name+"-deltas") if err == nil { defer os.RemoveAll(downloadDir) - _, err = s.downloadDeltas(name, downloadDir, downloadInfo, pbar, user) + deltaPath, err := s.downloadDelta(name, downloadDir, downloadInfo, pbar, user) // We revert to normal downloads if there is any error if err != nil { // Just log the error and continue with the normal non-delta @@ -1146,7 +1147,7 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog } else { // Currently even on successful delta downloads, continue with the // normal full download. - logger.Debugf("Successfully downloaded deltas for %s", name) + logger.Debugf("Successfully downloaded deltas for %s at %s", name, deltaPath) } } } @@ -1214,41 +1215,41 @@ 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 := make([]string, 0, len(downloadInfo.Deltas)) +// downloadDelta downloads the delta for the preferred format, returning the path. +func (s *Store) downloadDelta(name string, downloadDir string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) (string, error) { - // We only download our preferred delta format. - deltaFormat := s.deltaFormats[0] + if len(downloadInfo.Deltas) != 1 { + return "", errors.New("Store returned more than one delta") + } - for _, deltaInfo := range downloadInfo.Deltas { - if deltaInfo.Format != deltaFormat { - continue - } - deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format) + deltaInfo := downloadInfo.Deltas[0] - w, err := os.Create(path.Join(downloadDir, deltaName)) - if err != nil { - return nil, err - } + if deltaInfo.Format != s.deltaFormat { + return "", errors.New("Store returned delta with format " + deltaInfo.Format) + } - url := deltaInfo.AnonDownloadURL - if url == "" || user != nil { - url = deltaInfo.DownloadURL - } + deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format) - err = download(deltaName, url, user, s, w, pbar) - if err != nil { - return nil, err - } - err = w.Close() - if err != nil { - return nil, err - } + w, err := os.Create(path.Join(downloadDir, deltaName)) + if err != nil { + return "", err + } - deltaPaths = append(deltaPaths, w.Name()) + url := deltaInfo.AnonDownloadURL + if url == "" || user != nil { + url = deltaInfo.DownloadURL } - return deltaPaths, nil + + err = download(deltaName, url, user, s, w, pbar) + if err != nil { + return "", err + } + err = w.Close() + if err != nil { + return "", err + } + + return w.Name(), nil } type assertionSvcError struct { diff --git a/store/store_test.go b/store/store_test.go index 0ee6781c50b..e4ce9dfce2e 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -394,11 +394,11 @@ func (t *remoteRepoTestSuite) TestActualDownload500(c *C) { } type downloadBehaviour []struct { - URL string - Error bool + URL string + Error bool } -var deltaTests = []struct{ +var deltaTests = []struct { Downloads downloadBehaviour Info snap.DownloadInfo ExpectedContent string @@ -430,6 +430,20 @@ var deltaTests = []struct{ }, }, ExpectedContent: "full-snap-url-content", +}, { + // If more than one matching delta is returned by the store + // we ignore deltas and do the full download. + Downloads: downloadBehaviour{ + {URL: "full-snap-url"}, + }, + Info: snap.DownloadInfo{ + AnonDownloadURL: "full-snap-url", + Deltas: []snap.DeltaInfo{ + {AnonDownloadURL: "delta-url", Format: "xdelta"}, + {AnonDownloadURL: "delta-url-2", Format: "xdelta"}, + }, + }, + ExpectedContent: "full-snap-url-content", }} func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { @@ -446,12 +460,11 @@ func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { return errors.New("Bang") } c.Check(url, Equals, testCase.Downloads[downloadIndex].URL) - w.Write([]byte(testCase.Downloads[downloadIndex].URL+"-content")) + w.Write([]byte(testCase.Downloads[downloadIndex].URL + "-content")) downloadIndex++ return nil } - path, err := t.store.Download("foo", &testCase.Info, nil, nil) c.Assert(err, IsNil) @@ -462,12 +475,12 @@ func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { } } -var downloadDeltaTests = []struct{ - Info snap.DownloadInfo - Authenticated bool - Formats []string - ExpectedURL string - ExpectedPath string +var downloadDeltaTests = []struct { + Info snap.DownloadInfo + Authenticated bool + Format string + ExpectedURL string + ExpectedPath string }{{ // An unauthenticated request downloads the anonymous delta url. Info: snap.DownloadInfo{ @@ -476,9 +489,9 @@ var downloadDeltaTests = []struct{ }, }, Authenticated: false, - Formats: []string{"xdelta"}, - ExpectedURL: "anon-delta-url", - ExpectedPath: "snapname_24_26_delta.xdelta", + Format: "xdelta", + ExpectedURL: "anon-delta-url", + ExpectedPath: "snapname_24_26_delta.xdelta", }, { // An authenticated request downloads the authenticated delta url. Info: snap.DownloadInfo{ @@ -487,29 +500,43 @@ var downloadDeltaTests = []struct{ }, }, Authenticated: true, - Formats: []string{"xdelta"}, - ExpectedURL: "auth-delta-url", - ExpectedPath: "snapname_24_26_delta.xdelta", + Format: "xdelta", + ExpectedURL: "auth-delta-url", + ExpectedPath: "snapname_24_26_delta.xdelta", }, { - // The preferred delta format is downloaded. + // An error is returned if more than one matching delta is returned by the store, + // though this may be handled in the future. Info: snap.DownloadInfo{ Deltas: []snap.DeltaInfo{ - {DownloadURL: "delta-url", Format: "format1", FromRevision: 24, ToRevision: 26}, - {DownloadURL: "preferred-delta-url", Format: "format2", FromRevision: 24, ToRevision: 26}, + {DownloadURL: "xdelta-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 25}, + {DownloadURL: "bsdiff-delta-url", Format: "xdelta", FromRevision: 25, ToRevision: 26}, }, }, Authenticated: false, - Formats: []string{"format2", "format1"}, - ExpectedURL: "preferred-delta-url", - ExpectedPath: "snapname_24_26_delta.format2", + Format: "xdelta", + ExpectedURL: "", + ExpectedPath: "", +}, { + // If the supported format isn't available, an error is returned. + Info: snap.DownloadInfo{ + Deltas: []snap.DeltaInfo{ + {DownloadURL: "xdelta-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26}, + {DownloadURL: "ydelta-delta-url", Format: "ydelta", FromRevision: 24, ToRevision: 26}, + }, + }, + Authenticated: false, + Format: "bsdiff", + ExpectedURL: "", + ExpectedPath: "", }} + func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) { origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) for _, testCase := range downloadDeltaTests { - t.store.deltaFormats = testCase.Formats + t.store.deltaFormat = testCase.Format download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { expectedUser := t.user if !testCase.Authenticated { @@ -529,11 +556,15 @@ func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) { authedUser = nil } - deltaPaths, err := t.store.downloadDeltas("snapname", downloadDir, &testCase.Info, nil, authedUser) + deltaPath, err := t.store.downloadDelta("snapname", downloadDir, &testCase.Info, nil, authedUser) - c.Assert(err, IsNil) - c.Assert(len(deltaPaths), Equals, 1) - c.Assert(deltaPaths[0], Equals, path.Join(downloadDir, testCase.ExpectedPath)) + if testCase.ExpectedPath == "" { + c.Assert(deltaPath, Equals, "") + c.Assert(err, NotNil) + } else { + c.Assert(err, IsNil) + c.Assert(deltaPath, Equals, path.Join(downloadDir, testCase.ExpectedPath)) + } } } From 975a4779d520a8cbda1e4c622bf9a2fe7186615c Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 28 Sep 2016 21:54:00 +0000 Subject: [PATCH 5/6] Fix the error strings (lower-case). Defer the close of the delta download to ensure it always happens. Test structs don't need public attributes. --- store/store.go | 20 +++++--- store/store_test.go | 112 ++++++++++++++++++++++---------------------- 2 files changed, 69 insertions(+), 63 deletions(-) diff --git a/store/store.go b/store/store.go index cc21da12d6a..3f1fe224318 100644 --- a/store/store.go +++ b/store/store.go @@ -1219,13 +1219,13 @@ var download = func(name, downloadURL string, user *auth.UserState, s *Store, w 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") + return "", errors.New("store returned more than one delta") } 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) } deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format) @@ -1234,6 +1234,16 @@ func (s *Store) downloadDelta(name string, downloadDir string, downloadInfo *sna if err != nil { return "", err } + deltaPath := w.Name() + defer func() { + if cerr := w.Close(); cerr != nil && err == nil { + err = cerr + } + if err != nil { + os.Remove(w.Name()) + deltaPath = "" + } + }() url := deltaInfo.AnonDownloadURL if url == "" || user != nil { @@ -1244,12 +1254,8 @@ func (s *Store) downloadDelta(name string, downloadDir string, downloadInfo *sna if err != nil { return "", err } - err = w.Close() - if err != nil { - return "", err - } - return w.Name(), nil + return deltaPath, nil } type assertionSvcError struct { diff --git a/store/store_test.go b/store/store_test.go index e4ce9dfce2e..4cdd2aa0d39 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -394,56 +394,56 @@ func (t *remoteRepoTestSuite) TestActualDownload500(c *C) { } type downloadBehaviour []struct { - URL string - Error bool + url string + error bool } var deltaTests = []struct { - Downloads downloadBehaviour - Info snap.DownloadInfo - ExpectedContent string + downloads downloadBehaviour + info snap.DownloadInfo + expectedContent string }{{ // The full snap is downloaded after the delta (currently, // until the code applying the delta lands). - Downloads: downloadBehaviour{ - {URL: "delta-url"}, - {URL: "full-snap-url"}, + downloads: downloadBehaviour{ + {url: "delta-url"}, + {url: "full-snap-url"}, }, - Info: snap.DownloadInfo{ + info: snap.DownloadInfo{ AnonDownloadURL: "full-snap-url", Deltas: []snap.DeltaInfo{ {AnonDownloadURL: "delta-url", Format: "xdelta"}, }, }, - ExpectedContent: "full-snap-url-content", + expectedContent: "full-snap-url-content", }, { // If there is an error during the delta download, the // full snap is downloaded as per normal. - Downloads: downloadBehaviour{ - {Error: true}, - {URL: "full-snap-url"}, + downloads: downloadBehaviour{ + {error: true}, + {url: "full-snap-url"}, }, - Info: snap.DownloadInfo{ + info: snap.DownloadInfo{ AnonDownloadURL: "full-snap-url", Deltas: []snap.DeltaInfo{ {AnonDownloadURL: "delta-url", Format: "xdelta"}, }, }, - ExpectedContent: "full-snap-url-content", + expectedContent: "full-snap-url-content", }, { // If more than one matching delta is returned by the store // we ignore deltas and do the full download. - Downloads: downloadBehaviour{ - {URL: "full-snap-url"}, + downloads: downloadBehaviour{ + {url: "full-snap-url"}, }, - Info: snap.DownloadInfo{ + info: snap.DownloadInfo{ AnonDownloadURL: "full-snap-url", Deltas: []snap.DeltaInfo{ {AnonDownloadURL: "delta-url", Format: "xdelta"}, {AnonDownloadURL: "delta-url-2", Format: "xdelta"}, }, }, - ExpectedContent: "full-snap-url-content", + expectedContent: "full-snap-url-content", }} func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { @@ -455,79 +455,79 @@ func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { downloadIndex := 0 download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { - if testCase.Downloads[downloadIndex].Error { + if testCase.downloads[downloadIndex].error { downloadIndex++ return errors.New("Bang") } - c.Check(url, Equals, testCase.Downloads[downloadIndex].URL) - w.Write([]byte(testCase.Downloads[downloadIndex].URL + "-content")) + c.Check(url, Equals, testCase.downloads[downloadIndex].url) + w.Write([]byte(testCase.downloads[downloadIndex].url + "-content")) downloadIndex++ return nil } - path, err := t.store.Download("foo", &testCase.Info, nil, nil) + path, err := t.store.Download("foo", &testCase.info, nil, nil) c.Assert(err, IsNil) defer os.Remove(path) content, err := ioutil.ReadFile(path) c.Assert(err, IsNil) - c.Assert(string(content), Equals, testCase.ExpectedContent) + c.Assert(string(content), Equals, testCase.expectedContent) } } var downloadDeltaTests = []struct { - Info snap.DownloadInfo - Authenticated bool - Format string - ExpectedURL string - ExpectedPath string + info snap.DownloadInfo + authenticated bool + format string + expectedURL string + expectedPath string }{{ // An unauthenticated request downloads the anonymous delta url. - Info: snap.DownloadInfo{ + info: snap.DownloadInfo{ Deltas: []snap.DeltaInfo{ {AnonDownloadURL: "anon-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26}, }, }, - Authenticated: false, - Format: "xdelta", - ExpectedURL: "anon-delta-url", - ExpectedPath: "snapname_24_26_delta.xdelta", + authenticated: false, + format: "xdelta", + expectedURL: "anon-delta-url", + expectedPath: "snapname_24_26_delta.xdelta", }, { // An authenticated request downloads the authenticated delta url. - Info: snap.DownloadInfo{ + info: snap.DownloadInfo{ Deltas: []snap.DeltaInfo{ {DownloadURL: "auth-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26}, }, }, - Authenticated: true, - Format: "xdelta", - ExpectedURL: "auth-delta-url", - ExpectedPath: "snapname_24_26_delta.xdelta", + authenticated: true, + format: "xdelta", + expectedURL: "auth-delta-url", + expectedPath: "snapname_24_26_delta.xdelta", }, { // An error is returned if more than one matching delta is returned by the store, // though this may be handled in the future. - Info: snap.DownloadInfo{ + info: snap.DownloadInfo{ Deltas: []snap.DeltaInfo{ {DownloadURL: "xdelta-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 25}, {DownloadURL: "bsdiff-delta-url", Format: "xdelta", FromRevision: 25, ToRevision: 26}, }, }, - Authenticated: false, - Format: "xdelta", - ExpectedURL: "", - ExpectedPath: "", + authenticated: false, + format: "xdelta", + expectedURL: "", + expectedPath: "", }, { // If the supported format isn't available, an error is returned. - Info: snap.DownloadInfo{ + info: snap.DownloadInfo{ Deltas: []snap.DeltaInfo{ {DownloadURL: "xdelta-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26}, {DownloadURL: "ydelta-delta-url", Format: "ydelta", FromRevision: 24, ToRevision: 26}, }, }, - Authenticated: false, - Format: "bsdiff", - ExpectedURL: "", - ExpectedPath: "", + authenticated: false, + format: "bsdiff", + expectedURL: "", + expectedPath: "", }} func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) { @@ -536,14 +536,14 @@ func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) { c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) for _, testCase := range downloadDeltaTests { - t.store.deltaFormat = testCase.Format + t.store.deltaFormat = testCase.format download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error { expectedUser := t.user - if !testCase.Authenticated { + if !testCase.authenticated { expectedUser = nil } c.Check(user, Equals, expectedUser) - c.Check(url, Equals, testCase.ExpectedURL) + c.Check(url, Equals, testCase.expectedURL) return nil } @@ -552,18 +552,18 @@ func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) { defer os.RemoveAll(downloadDir) authedUser := t.user - if !testCase.Authenticated { + if !testCase.authenticated { authedUser = nil } - deltaPath, err := t.store.downloadDelta("snapname", downloadDir, &testCase.Info, nil, authedUser) + deltaPath, err := t.store.downloadDelta("snapname", downloadDir, &testCase.info, nil, authedUser) - if testCase.ExpectedPath == "" { + if testCase.expectedPath == "" { c.Assert(deltaPath, Equals, "") c.Assert(err, NotNil) } else { c.Assert(err, IsNil) - c.Assert(deltaPath, Equals, path.Join(downloadDir, testCase.ExpectedPath)) + c.Assert(deltaPath, Equals, path.Join(downloadDir, testCase.expectedPath)) } } } From 1419b6c5ca04f78e336c4f70a5054dd9a9eb7bc7 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Thu, 29 Sep 2016 09:53:30 +0000 Subject: [PATCH 6/6] Rename the env var switch for deltas to use SNAPD_ prefix. Add more context to delta-related error message. --- store/store.go | 8 ++++---- store/store_test.go | 24 ++++++++++++------------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/store/store.go b/store/store.go index 3f1fe224318..a79680b577f 100644 --- a/store/store.go +++ b/store/store.go @@ -1061,7 +1061,7 @@ func (s *Store) ListRefresh(installed []*RefreshCandidate, user *auth.UserState) Data: jsonData, } - if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" { + if os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") == "1" { reqOptions.ExtraHeaders = map[string]string{ "X-Ubuntu-Delta-Formats": s.deltaFormat, } @@ -1133,7 +1133,7 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog } }() - if os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") == "1" && len(downloadInfo.Deltas) == 1 { + if os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") == "1" && len(downloadInfo.Deltas) == 1 { downloadDir, err := ioutil.TempDir("", name+"-deltas") if err == nil { defer os.RemoveAll(downloadDir) @@ -1219,13 +1219,13 @@ var download = func(name, downloadURL string, user *auth.UserState, s *Store, w 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") + return "", errors.New("store returned more than one download delta") } deltaInfo := downloadInfo.Deltas[0] if deltaInfo.Format != s.deltaFormat { - return "", fmt.Errorf("store returned delta with format %q", deltaInfo.Format) + return "", fmt.Errorf("store returned a download delta with the wrong format (%q instead of the configured %s format)", deltaInfo.Format, s.deltaFormat) } deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format) diff --git a/store/store_test.go b/store/store_test.go index 4cdd2aa0d39..486dad5607d 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -447,9 +447,9 @@ var deltaTests = []struct { }} func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) { - origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") - defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) + origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) for _, testCase := range deltaTests { @@ -531,9 +531,9 @@ var downloadDeltaTests = []struct { }} func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) { - origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") - defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) + origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) for _, testCase := range downloadDeltaTests { t.store.deltaFormat = testCase.format @@ -1845,9 +1845,9 @@ var MockUpdatesWithDeltasJSON = ` ` func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithDeltas(c *C) { - origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") - defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) + origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "1"), IsNil) mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c.Check(r.Header.Get("X-Ubuntu-Delta-Formats"), Equals, `xdelta`) @@ -1920,9 +1920,9 @@ func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithDeltas(c * func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithoutDeltas(c *C) { // Verify the X-Delta-Format header is not set. - origUseDeltas := os.Getenv("SNAPPY_USE_DELTAS_EXPERIMENTAL") - defer os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", origUseDeltas) - c.Assert(os.Setenv("SNAPPY_USE_DELTAS_EXPERIMENTAL", "0"), IsNil) + origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") + defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas) + c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "0"), IsNil) mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c.Check(r.Header.Get("X-Ubuntu-Delta-Formats"), Equals, ``)