Skip to content

Commit

Permalink
MM-38701: Automatically retry on a failed export download (mattermost…
Browse files Browse the repository at this point in the history 
…#524)

We had a resume functionality which would download the file
again from a given offset. But clients would create wrapper
scripts which would auto-retry.

Therefore, we add that functionality in-built and deprecate
the resume flag. The reasoning behind removing the flag is that
if it doesn't succeed within 5 tries, then probably it's better
to attempt to download the file from scratch again sometime later.

https://mattermost.atlassian.net/browse/MM-38701
  • Loading branch information
@agnivade
agnivade authored Jun 9, 2022
1 parent 52f4fbc commit b7b23c0
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 64 deletions.
32 changes: 22 additions & 10 deletions commands/export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ func init() {
ExportCreateCmd.Flags().Bool("attachments", false, "Set to true to include file attachments in the export file.")

ExportDownloadCmd.Flags().Bool("resume", false, "Set to true to resume an export download.")
_ = ExportDownloadCmd.Flags().MarkHidden("resume")
// Intentionally the message does not start with a capital letter because
// cobra prepends "Flag --resume has been deprecated,"
_ = ExportDownloadCmd.Flags().MarkDeprecated("resume", "the tool now resumes a download automatically. The flag will be removed in a future version.")
ExportDownloadCmd.Flags().Int("num-retries", 5, "Number of retries to do to resume a download.")

ExportJobListCmd.Flags().Int("page", 0, "Page number to fetch for the list of export jobs")
ExportJobListCmd.Flags().Int("per-page", 200, "Number of export jobs to be fetched")
Expand Down Expand Up @@ -163,20 +168,17 @@ func exportDownloadCmdF(c client.Client, command *cobra.Command, args []string)
path = name
}

resume, _ := command.Flags().GetBool("resume")
retries, _ := command.Flags().GetInt("num-retries")

var outFile *os.File
info, err := os.Stat(path)
switch {
case err != nil && !os.IsNotExist(err):
// some error occurred and not because file doesn't exist
return fmt.Errorf("failed to stat export file: %w", err)
case err == nil && info.Size() > 0 && !resume:
case err == nil && info.Size() > 0:
// we exit to avoid overwriting an existing non-empty file
return fmt.Errorf("export file already exists")
case os.IsNotExist(err) && resume:
// cannot resume if the file does not exist
return fmt.Errorf("cannot resume download: export file does not exist")
case err != nil:
// file does not exist, we create it
outFile, err = os.Create(path)
Expand All @@ -190,13 +192,23 @@ func exportDownloadCmdF(c client.Client, command *cobra.Command, args []string)
}
defer outFile.Close()

off, err := outFile.Seek(0, io.SeekEnd)
if err != nil {
return fmt.Errorf("failed to seek export file: %w", err)
i := 0
for i < retries+1 {
off, err := outFile.Seek(0, io.SeekEnd)
if err != nil {
return fmt.Errorf("failed to seek export file: %w", err)
}

if _, _, err := c.DownloadExport(name, outFile, off); err != nil {
printer.PrintWarning(fmt.Sprintf("failed to download export file: %v. Retrying...", err))
i++
continue
}
break
}

if _, _, err := c.DownloadExport(name, outFile, off); err != nil {
return fmt.Errorf("failed to download export file: %w", err)
if retries != 0 && i == retries+1 {
return fmt.Errorf("failed to download export after %d retries", retries)
}

return nil
Expand Down
62 changes: 10 additions & 52 deletions commands/export_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,11 @@ func (s *MmctlE2ETestSuite) TestExportDownloadCmdF() {
s.Run("MM-T3879 - no permissions", func() {
printer.Clean()

err := exportDownloadCmdF(s.th.Client, &cobra.Command{}, []string{exportName})
s.Require().EqualError(err, "failed to download export file: : You do not have the appropriate permissions., ")
cmd := &cobra.Command{}
cmd.Flags().Int("num-retries", 5, "")

err := exportDownloadCmdF(s.th.Client, cmd, []string{exportName})
s.Require().EqualError(err, "failed to download export after 5 retries")
s.Require().Empty(printer.GetLines())
s.Require().Empty(printer.GetErrorLines())
})
Expand All @@ -187,6 +190,7 @@ func (s *MmctlE2ETestSuite) TestExportDownloadCmdF() {
printer.Clean()

cmd := &cobra.Command{}
cmd.Flags().Int("num-retries", 5, "")

downloadPath, err := filepath.Abs(exportName)
s.Require().Nil(err)
Expand All @@ -200,32 +204,18 @@ func (s *MmctlE2ETestSuite) TestExportDownloadCmdF() {
s.Require().Empty(printer.GetErrorLines())
})

s.RunForSystemAdminAndLocal("MM-T3381 - resuming non-existent file", func(c client.Client) {
printer.Clean()

cmd := &cobra.Command{}
cmd.Flags().Bool("resume", true, "")

downloadPath, err := filepath.Abs(exportName)
s.Require().Nil(err)

err = exportDownloadCmdF(c, cmd, []string{exportName, downloadPath})
s.Require().EqualError(err, "cannot resume download: export file does not exist")
s.Require().Empty(printer.GetLines())
s.Require().Empty(printer.GetErrorLines())
})

s.RunForSystemAdminAndLocal("MM-T3882 - export does not exist", func(c client.Client) {
printer.Clean()

cmd := &cobra.Command{}
cmd.Flags().Int("num-retries", 5, "")

downloadPath, err := filepath.Abs(exportName)
s.Require().Nil(err)
defer os.Remove(downloadPath)

err = exportDownloadCmdF(c, cmd, []string{exportName, downloadPath})
s.Require().EqualError(err, "failed to download export file: : Unable to find export file., ")
s.Require().EqualError(err, "failed to download export after 5 retries")
s.Require().Empty(printer.GetLines())
s.Require().Empty(printer.GetErrorLines())
})
Expand All @@ -234,6 +224,7 @@ func (s *MmctlE2ETestSuite) TestExportDownloadCmdF() {
printer.Clean()

cmd := &cobra.Command{}
cmd.Flags().Int("num-retries", 5, "")

exportFilePath := filepath.Join(exportPath, exportName)
err := utils.CopyFile(importFilePath, exportFilePath)
Expand All @@ -257,6 +248,7 @@ func (s *MmctlE2ETestSuite) TestExportDownloadCmdF() {
printer.Clean()

cmd := &cobra.Command{}
cmd.Flags().Int("num-retries", 5, "")

exportFilePath := filepath.Join(exportPath, exportName)
err := utils.CopyFile(importFilePath, exportFilePath)
Expand All @@ -279,40 +271,6 @@ func (s *MmctlE2ETestSuite) TestExportDownloadCmdF() {

s.Require().Equal(expected, actual)
})

s.RunForSystemAdminAndLocal("MM-T3884 - resume download", func(c client.Client) {
printer.Clean()

cmd := &cobra.Command{}
cmd.Flags().Bool("resume", true, "")

exportFilePath := filepath.Join(exportPath, exportName)
err := utils.CopyFile(importFilePath, exportFilePath)
s.Require().Nil(err)
defer os.Remove(exportFilePath)

downloadPath, err := filepath.Abs(exportName)
s.Require().Nil(err)
defer os.Remove(downloadPath)
f, err := os.Create(downloadPath)
s.Require().Nil(err)
defer f.Close()

expected, err := ioutil.ReadFile(exportFilePath)
s.Require().Nil(err)
_, err = f.Write(expected[:1024])
s.Require().Nil(err)

err = exportDownloadCmdF(c, cmd, []string{exportName, downloadPath})
s.Require().Nil(err)
s.Require().Empty(printer.GetLines())
s.Require().Empty(printer.GetErrorLines())

actual, err := ioutil.ReadFile(downloadPath)
s.Require().Nil(err)

s.Require().Equal(expected, actual)
})
}

func (s *MmctlE2ETestSuite) TestExportJobShowCmdF() {
Expand Down
4 changes: 2 additions & 2 deletions docs/mmctl_export_download.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ Options

::

-h, --help help for download
--resume Set to true to resume an export download.
-h, --help help for download
--num-retries int Number of retries to do to resume a download. (default 5)

Options inherited from parent commands
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down

0 comments on commit b7b23c0

Please sign in to comment.