From 6becd5a3aa4c7f23a08b164031a51939e6b9c011 Mon Sep 17 00:00:00 2001 From: Oliver Sauder Date: Mon, 28 Nov 2016 13:13:11 +0100 Subject: [PATCH 1/3] Added max-tries flag for mirror update --- aptly/interfaces.go | 2 +- cmd/mirror_update.go | 6 ++-- deb/remote.go | 4 +-- http/download.go | 73 ++++++++++++++++++++++++++++---------------- http/fake.go | 4 +-- 5 files changed, 55 insertions(+), 34 deletions(-) diff --git a/aptly/interfaces.go b/aptly/interfaces.go index b6375784..f8d72096 100644 --- a/aptly/interfaces.go +++ b/aptly/interfaces.go @@ -82,7 +82,7 @@ type Downloader interface { // Download starts new download task Download(url string, destination string, result chan<- error) // DownloadWithChecksum starts new download task with checksum verification - DownloadWithChecksum(url string, destination string, result chan<- error, expected utils.ChecksumInfo, ignoreMismatch bool) + DownloadWithChecksum(url string, destination string, result chan<- error, expected utils.ChecksumInfo, ignoreMismatch bool, maxTries int) // Pause pauses task processing Pause() // Resume resumes task processing diff --git a/cmd/mirror_update.go b/cmd/mirror_update.go index b2df188e..f0d2f4d5 100644 --- a/cmd/mirror_update.go +++ b/cmd/mirror_update.go @@ -40,6 +40,7 @@ func aptlyMirrorUpdate(cmd *commander.Command, args []string) error { } ignoreMismatch := context.Flags().Lookup("ignore-checksums").Value.Get().(bool) + maxTries := context.Flags().Lookup("max-tries").Value.Get().(int) verifier, err := getVerifier(context.Flags()) if err != nil { @@ -52,7 +53,7 @@ func aptlyMirrorUpdate(cmd *commander.Command, args []string) error { } context.Progress().Printf("Downloading & parsing package files...\n") - err = repo.DownloadPackageIndexes(context.Progress(), context.Downloader(), context.CollectionFactory(), ignoreMismatch) + err = repo.DownloadPackageIndexes(context.Progress(), context.Downloader(), context.CollectionFactory(), ignoreMismatch, maxTries) if err != nil { return fmt.Errorf("unable to update: %s", err) } @@ -121,7 +122,7 @@ func aptlyMirrorUpdate(cmd *commander.Command, args []string) error { // In separate goroutine (to avoid blocking main), push queue to downloader go func() { for _, task := range queue { - context.Downloader().DownloadWithChecksum(repo.PackageURL(task.RepoURI).String(), task.DestinationPath, ch, task.Checksums, ignoreMismatch) + context.Downloader().DownloadWithChecksum(repo.PackageURL(task.RepoURI).String(), task.DestinationPath, ch, task.Checksums, ignoreMismatch, maxTries) } // We don't need queue after this point @@ -187,6 +188,7 @@ Example: cmd.Flag.Bool("ignore-checksums", false, "ignore checksum mismatches while downloading package files and metadata") cmd.Flag.Bool("ignore-signatures", false, "disable verification of Release file signatures") cmd.Flag.Int64("download-limit", 0, "limit download speed (kbytes/sec)") + cmd.Flag.Int("max-tries", 1, "max download tries till process fails with download error") cmd.Flag.Var(&keyRingsFlag{}, "keyring", "gpg keyring to use when verifying Release file (could be specified multiple times)") return cmd diff --git a/deb/remote.go b/deb/remote.go index 9610e3c3..7dd15aca 100644 --- a/deb/remote.go +++ b/deb/remote.go @@ -403,7 +403,7 @@ ok: // DownloadPackageIndexes downloads & parses package index files func (repo *RemoteRepo) DownloadPackageIndexes(progress aptly.Progress, d aptly.Downloader, collectionFactory *CollectionFactory, - ignoreMismatch bool) error { + ignoreMismatch bool, maxTries int) error { if repo.packageList != nil { panic("packageList != nil") } @@ -433,7 +433,7 @@ func (repo *RemoteRepo) DownloadPackageIndexes(progress aptly.Progress, d aptly. for _, info := range packagesURLs { url, kind := info[0], info[1] - packagesReader, packagesFile, err := http.DownloadTryCompression(d, url, repo.ReleaseFiles, ignoreMismatch) + packagesReader, packagesFile, err := http.DownloadTryCompression(d, url, repo.ReleaseFiles, ignoreMismatch, maxTries) if err != nil { return err } diff --git a/http/download.go b/http/download.go index acf605bc..aa635863 100644 --- a/http/download.go +++ b/http/download.go @@ -52,6 +52,7 @@ type downloadTask struct { result chan<- error expected utils.ChecksumInfo ignoreMismatch bool + triesLeft int } // NewDownloader creates new instance of Downloader which specified number @@ -127,13 +128,13 @@ func (downloader *downloaderImpl) GetProgress() aptly.Progress { // Download starts new download task func (downloader *downloaderImpl) Download(url string, destination string, result chan<- error) { - downloader.DownloadWithChecksum(url, destination, result, utils.ChecksumInfo{Size: -1}, false) + downloader.DownloadWithChecksum(url, destination, result, utils.ChecksumInfo{Size: -1}, false, 1) } // DownloadWithChecksum starts new download task with checksum verification func (downloader *downloaderImpl) DownloadWithChecksum(url string, destination string, result chan<- error, - expected utils.ChecksumInfo, ignoreMismatch bool) { - downloader.queue <- &downloadTask{url: url, destination: destination, result: result, expected: expected, ignoreMismatch: ignoreMismatch} + expected utils.ChecksumInfo, ignoreMismatch bool, maxTries int) { + downloader.queue <- &downloadTask{url: url, destination: destination, result: result, expected: expected, ignoreMismatch: ignoreMismatch, triesLeft: maxTries} } // handleTask processes single download task @@ -153,32 +154,59 @@ func (downloader *downloaderImpl) handleTask(task *downloadTask) { req.URL.RawQuery = "" } - resp, err := downloader.client.Do(req) + + var temppath string + for task.triesLeft > 0 { + + temppath, err = downloader.downloadTask(req, task) + + if err != nil { + task.triesLeft-- + } else { + // successful download + break + } + } + + // still an error after retring, giving up if err != nil { + task.result <- err + return + } + + err = os.Rename(temppath, task.destination) + if err != nil { + os.Remove(temppath) task.result <- fmt.Errorf("%s: %s", task.url, err) return } + + task.result <- nil +} + +func (downloader *downloaderImpl) downloadTask(req *http.Request, task *downloadTask) (string, error) { + resp, err := downloader.client.Do(req) + if err != nil { + return "", fmt.Errorf("%s: %s", task.url, err) + } if resp.Body != nil { defer resp.Body.Close() } if resp.StatusCode < 200 || resp.StatusCode > 299 { - task.result <- &HTTPError{Code: resp.StatusCode, URL: task.url} - return + return "", &HTTPError{Code: resp.StatusCode, URL: task.url} } err = os.MkdirAll(filepath.Dir(task.destination), 0777) if err != nil { - task.result <- fmt.Errorf("%s: %s", task.url, err) - return + return "", fmt.Errorf("%s: %s", task.url, err) } temppath := task.destination + ".down" outfile, err := os.Create(temppath) if err != nil { - task.result <- fmt.Errorf("%s: %s", task.url, err) - return + return "", fmt.Errorf("%s: %s", task.url, err) } defer outfile.Close() @@ -194,8 +222,7 @@ func (downloader *downloaderImpl) handleTask(task *downloadTask) { _, err = io.Copy(w, resp.Body) if err != nil { os.Remove(temppath) - task.result <- fmt.Errorf("%s: %s", task.url, err) - return + return "", fmt.Errorf("%s: %s", task.url, err) } if task.expected.Size != -1 { @@ -218,20 +245,12 @@ func (downloader *downloaderImpl) handleTask(task *downloadTask) { downloader.progress.Printf("WARNING: %s\n", err.Error()) } else { os.Remove(temppath) - task.result <- err - return + return "", err } } } - err = os.Rename(temppath, task.destination) - if err != nil { - os.Remove(temppath) - task.result <- fmt.Errorf("%s: %s", task.url, err) - return - } - - task.result <- nil + return temppath, nil } // process implements download thread in goroutine @@ -253,13 +272,13 @@ func (downloader *downloaderImpl) process() { // // Temporary file would be already removed, so no need to cleanup func DownloadTemp(downloader aptly.Downloader, url string) (*os.File, error) { - return DownloadTempWithChecksum(downloader, url, utils.ChecksumInfo{Size: -1}, false) + return DownloadTempWithChecksum(downloader, url, utils.ChecksumInfo{Size: -1}, false, 1) } // DownloadTempWithChecksum is a DownloadTemp with checksum verification // // Temporary file would be already removed, so no need to cleanup -func DownloadTempWithChecksum(downloader aptly.Downloader, url string, expected utils.ChecksumInfo, ignoreMismatch bool) (*os.File, error) { +func DownloadTempWithChecksum(downloader aptly.Downloader, url string, expected utils.ChecksumInfo, ignoreMismatch bool, maxTries int) (*os.File, error) { tempdir, err := ioutil.TempDir(os.TempDir(), "aptly") if err != nil { return nil, err @@ -274,7 +293,7 @@ func DownloadTempWithChecksum(downloader aptly.Downloader, url string, expected } ch := make(chan error, 1) - downloader.DownloadWithChecksum(url, tempfile, ch, expected, ignoreMismatch) + downloader.DownloadWithChecksum(url, tempfile, ch, expected, ignoreMismatch, maxTries) err = <-ch @@ -311,7 +330,7 @@ var compressionMethods = []struct { // DownloadTryCompression tries to download from URL .bz2, .gz and raw extension until // it finds existing file. -func DownloadTryCompression(downloader aptly.Downloader, url string, expectedChecksums map[string]utils.ChecksumInfo, ignoreMismatch bool) (io.Reader, *os.File, error) { +func DownloadTryCompression(downloader aptly.Downloader, url string, expectedChecksums map[string]utils.ChecksumInfo, ignoreMismatch bool, maxTries int) (io.Reader, *os.File, error) { var err error for _, method := range compressionMethods { @@ -322,7 +341,7 @@ func DownloadTryCompression(downloader aptly.Downloader, url string, expectedChe for suffix, expected := range expectedChecksums { if strings.HasSuffix(tryURL, suffix) { - file, err = DownloadTempWithChecksum(downloader, tryURL, expected, ignoreMismatch) + file, err = DownloadTempWithChecksum(downloader, tryURL, expected, ignoreMismatch, maxTries) foundChecksum = true break } diff --git a/http/fake.go b/http/fake.go index c2eda915..d564edc2 100644 --- a/http/fake.go +++ b/http/fake.go @@ -59,7 +59,7 @@ func (f *FakeDownloader) Empty() bool { } // DownloadWithChecksum performs fake download by matching against first expectation in the queue or any expectation, with cheksum verification -func (f *FakeDownloader) DownloadWithChecksum(url string, filename string, result chan<- error, expected utils.ChecksumInfo, ignoreMismatch bool) { +func (f *FakeDownloader) DownloadWithChecksum(url string, filename string, result chan<- error, expected utils.ChecksumInfo, ignoreMismatch bool, maxTries int) { var expectation expectedRequest if len(f.expected) > 0 && f.expected[0].URL == url { expectation, f.expected = f.expected[0], f.expected[1:] @@ -116,7 +116,7 @@ func (f *FakeDownloader) DownloadWithChecksum(url string, filename string, resul // Download performs fake download by matching against first expectation in the queue func (f *FakeDownloader) Download(url string, filename string, result chan<- error) { - f.DownloadWithChecksum(url, filename, result, utils.ChecksumInfo{Size: -1}, false) + f.DownloadWithChecksum(url, filename, result, utils.ChecksumInfo{Size: -1}, false, 1) } // Shutdown does nothing From f31b5ec3f8728fa4b2a29e34a837f90a35531bad Mon Sep 17 00:00:00 2001 From: Oliver Sauder Date: Mon, 28 Nov 2016 14:02:17 +0100 Subject: [PATCH 2/3] Adjusted test with new maxTries param for download --- deb/remote_test.go | 8 ++++---- http/download_test.go | 34 +++++++++++++++++----------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/deb/remote_test.go b/deb/remote_test.go index 7fdb9b24..deb32dd3 100644 --- a/deb/remote_test.go +++ b/deb/remote_test.go @@ -261,7 +261,7 @@ func (s *RemoteRepoSuite) TestDownload(c *C) { s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.gz", &http.HTTPError{Code: 404}) s.downloader.ExpectResponse("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages", examplePackagesFile) - err = s.repo.DownloadPackageIndexes(s.progress, s.downloader, s.collectionFactory, false) + err = s.repo.DownloadPackageIndexes(s.progress, s.downloader, s.collectionFactory, false, 1) c.Assert(err, IsNil) c.Assert(s.downloader.Empty(), Equals, true) @@ -293,7 +293,7 @@ func (s *RemoteRepoSuite) TestDownloadWithSources(c *C) { s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/source/Sources.gz", &http.HTTPError{Code: 404}) s.downloader.ExpectResponse("http://mirror.yandex.ru/debian/dists/squeeze/main/source/Sources", exampleSourcesFile) - err = s.repo.DownloadPackageIndexes(s.progress, s.downloader, s.collectionFactory, false) + err = s.repo.DownloadPackageIndexes(s.progress, s.downloader, s.collectionFactory, false, 1) c.Assert(err, IsNil) c.Assert(s.downloader.Empty(), Equals, true) @@ -334,7 +334,7 @@ func (s *RemoteRepoSuite) TestDownloadFlat(c *C) { err := s.flat.Fetch(downloader, nil) c.Assert(err, IsNil) - err = s.flat.DownloadPackageIndexes(s.progress, downloader, s.collectionFactory, true) + err = s.flat.DownloadPackageIndexes(s.progress, downloader, s.collectionFactory, true, 1) c.Assert(err, IsNil) c.Assert(downloader.Empty(), Equals, true) @@ -367,7 +367,7 @@ func (s *RemoteRepoSuite) TestDownloadWithSourcesFlat(c *C) { err := s.flat.Fetch(downloader, nil) c.Assert(err, IsNil) - err = s.flat.DownloadPackageIndexes(s.progress, downloader, s.collectionFactory, true) + err = s.flat.DownloadPackageIndexes(s.progress, downloader, s.collectionFactory, true, 1) c.Assert(err, IsNil) c.Assert(downloader.Empty(), Equals, true) diff --git a/http/download_test.go b/http/download_test.go index b63fd665..f198449d 100644 --- a/http/download_test.go +++ b/http/download_test.go @@ -95,38 +95,38 @@ func (s *DownloaderSuite) TestDownloadWithChecksum(c *C) { defer d.Shutdown() ch := make(chan error) - d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{}, false) + d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{}, false, 1) res := <-ch c.Assert(res, ErrorMatches, ".*size check mismatch 12 != 0") - d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "abcdef"}, false) + d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "abcdef"}, false, 1) res = <-ch c.Assert(res, ErrorMatches, ".*md5 hash mismatch \"a1acb0fe91c7db45ec4d775192ec5738\" != \"abcdef\"") - d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "abcdef"}, true) + d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "abcdef"}, true, 1) res = <-ch c.Assert(res, IsNil) - d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738"}, false) + d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738"}, false, 1) res = <-ch c.Assert(res, IsNil) - d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738", SHA1: "abcdef"}, false) + d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738", SHA1: "abcdef"}, false, 1) res = <-ch c.Assert(res, ErrorMatches, ".*sha1 hash mismatch \"921893bae6ad6fd818401875d6779254ef0ff0ec\" != \"abcdef\"") d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738", - SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec"}, false) + SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec"}, false, 1) res = <-ch c.Assert(res, IsNil) d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738", - SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec", SHA256: "abcdef"}, false) + SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec", SHA256: "abcdef"}, false, 1) res = <-ch c.Assert(res, ErrorMatches, ".*sha256 hash mismatch \"b3c92ee1246176ed35f6e8463cd49074f29442f5bbffc3f8591cde1dcc849dac\" != \"abcdef\"") d.DownloadWithChecksum(s.url+"/test", s.tempfile.Name(), ch, utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738", - SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec", SHA256: "b3c92ee1246176ed35f6e8463cd49074f29442f5bbffc3f8591cde1dcc849dac"}, false) + SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec", SHA256: "b3c92ee1246176ed35f6e8463cd49074f29442f5bbffc3f8591cde1dcc849dac"}, false, 1) res = <-ch c.Assert(res, IsNil) } @@ -183,11 +183,11 @@ func (s *DownloaderSuite) TestDownloadTempWithChecksum(c *C) { defer d.Shutdown() f, err := DownloadTempWithChecksum(d, s.url+"/test", utils.ChecksumInfo{Size: 12, MD5: "a1acb0fe91c7db45ec4d775192ec5738", - SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec", SHA256: "b3c92ee1246176ed35f6e8463cd49074f29442f5bbffc3f8591cde1dcc849dac"}, false) + SHA1: "921893bae6ad6fd818401875d6779254ef0ff0ec", SHA256: "b3c92ee1246176ed35f6e8463cd49074f29442f5bbffc3f8591cde1dcc849dac"}, false, 1) defer f.Close() c.Assert(err, IsNil) - _, err = DownloadTempWithChecksum(d, s.url+"/test", utils.ChecksumInfo{Size: 13}, false) + _, err = DownloadTempWithChecksum(d, s.url+"/test", utils.ChecksumInfo{Size: 13}, false, 1) c.Assert(err, ErrorMatches, ".*size check mismatch 12 != 13") } @@ -220,7 +220,7 @@ func (s *DownloaderSuite) TestDownloadTryCompression(c *C) { buf = make([]byte, 4) d := NewFakeDownloader() d.ExpectResponse("http://example.com/file.bz2", bzipData) - r, file, err := DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false) + r, file, err := DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false, 1) c.Assert(err, IsNil) defer file.Close() io.ReadFull(r, buf) @@ -232,7 +232,7 @@ func (s *DownloaderSuite) TestDownloadTryCompression(c *C) { d = NewFakeDownloader() d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) d.ExpectResponse("http://example.com/file.gz", gzipData) - r, file, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false) + r, file, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false, 1) c.Assert(err, IsNil) defer file.Close() io.ReadFull(r, buf) @@ -245,7 +245,7 @@ func (s *DownloaderSuite) TestDownloadTryCompression(c *C) { d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) d.ExpectError("http://example.com/file.gz", &HTTPError{Code: 404}) d.ExpectResponse("http://example.com/file", rawData) - r, file, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false) + r, file, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false, 1) c.Assert(err, IsNil) defer file.Close() io.ReadFull(r, buf) @@ -257,21 +257,21 @@ func (s *DownloaderSuite) TestDownloadTryCompression(c *C) { d = NewFakeDownloader() d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) d.ExpectResponse("http://example.com/file.gz", "x") - r, file, err = DownloadTryCompression(d, "http://example.com/file", nil, true) + r, file, err = DownloadTryCompression(d, "http://example.com/file", nil, true, 1) c.Assert(err, ErrorMatches, "unexpected EOF") c.Assert(d.Empty(), Equals, true) } func (s *DownloaderSuite) TestDownloadTryCompressionErrors(c *C) { d := NewFakeDownloader() - _, _, err := DownloadTryCompression(d, "http://example.com/file", nil, true) + _, _, err := DownloadTryCompression(d, "http://example.com/file", nil, true, 1) c.Assert(err, ErrorMatches, "unexpected request.*") d = NewFakeDownloader() d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) d.ExpectError("http://example.com/file.gz", &HTTPError{Code: 404}) d.ExpectError("http://example.com/file", errors.New("403")) - _, _, err = DownloadTryCompression(d, "http://example.com/file", nil, true) + _, _, err = DownloadTryCompression(d, "http://example.com/file", nil, true, 1) c.Assert(err, ErrorMatches, "403") d = NewFakeDownloader() @@ -283,6 +283,6 @@ func (s *DownloaderSuite) TestDownloadTryCompressionErrors(c *C) { "file.gz": {Size: 7}, "file": {Size: 7}, } - _, _, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false) + _, _, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false, 1) c.Assert(err, ErrorMatches, "checksums don't match.*") } From af71b9541cd1a9d1a1acc0307e37c9bebee5adf0 Mon Sep 17 00:00:00 2001 From: Oliver Sauder Date: Tue, 29 Nov 2016 09:00:16 +0100 Subject: [PATCH 3/3] Fixing typo --- http/download.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/download.go b/http/download.go index aa635863..77af8703 100644 --- a/http/download.go +++ b/http/download.go @@ -168,7 +168,7 @@ func (downloader *downloaderImpl) handleTask(task *downloadTask) { } } - // still an error after retring, giving up + // still an error after retrying, giving up if err != nil { task.result <- err return