Skip to content

Add support for zstd compression#249

Merged
m90 merged 1 commit intooffen:mainfrom
michalmiddleton:add_zstd_support
Aug 19, 2023
Merged

Add support for zstd compression#249
m90 merged 1 commit intooffen:mainfrom
michalmiddleton:add_zstd_support

Conversation

@michalmiddleton
Copy link
Contributor

Hi,
I took a stab at adding zstd support in a way that keeps gzip as default to maintain backward compatibility:

  • added BACKUP_COMPRESSION parameter (gzip/zstd)
  • updated BACKUP_FILENAME to accept '' for extension that will dynamically change to tar.gz or tar.zst depending on compression
  • added a build test cli-zstd

Please let me know if you require any additional changes.

Real world test:

backups  | time=2023-08-17T19:29:00.451-05:00 level=INFO msg="Stopping 3 container(s) labeled `docker-volume-backup.stop-during-backup=true` out of 16 running container(s)."
backups  | time=2023-08-17T19:30:02.818-05:00 level=INFO msg="Created backup of `/backup` at `/tmp/backup-2023-08-17T19-29-00.tar.zst`."
backups  | time=2023-08-17T19:30:04.637-05:00 level=INFO msg="Restarted 3 container(s) and the matching service(s)."
backups  | time=2023-08-17T19:31:48.106-05:00 level=INFO msg="Removed tar file `/tmp/backup-2023-08-17T19-29-00.tar.zst`."
backups  | time=2023-08-17T19:31:48.106-05:00 level=INFO msg="Finished running backup tasks."

Extract to validate consistency:

root@archer:/data# unzstd backup-2023-08-17T19-29-00.tar.zst
backup-2023-08-17T19-29-00.tar.zst: 17936120320 bytes

root@archer:/data# tar tf backup-2023-08-17T19-29-00.tar > /dev/null
tar: Removing leading `/' from member names
root@archer:/data#

(The uncompressed tar was about 17GB in size and zstd compressed archive was about 7GB!)

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this feature 🎩 I left a few comments inline, nothing major. Let me know what you think about my suggestions.

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this done so fast again. I left another round of nit picky comments, the only one that is really interesting is the one about maybe having a custom type in the config that limits the number of possible compression algorithms. Let me know what you think.

return nil, fmt.Errorf("newScript: unable to parse backup file extension template: %w", tErr)
}
var bf bytes.Buffer
if tErr := tmplFileName.Execute(&bf, extMap); tErr != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be possible to anonymously define the data being passed here

Suggested change
if tErr := tmplFileName.Execute(&bf, extMap); tErr != nil {
if tErr := tmplFileName.Execute(&bf, map[string]string{
"Extension": "tar."+s.c.BackupCompression
}); tErr != nil {

@michalmiddleton
Copy link
Contributor Author

@m90 Thanks for being patient with me :). I pushed amended commit with the changes.

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more tiny remark, everything else looks fabulous and ready for the next release now.

@m90 m90 merged commit 67e7288 into offen:main Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants