Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rclone backup writes to root directory instead of mounted emptyDir #104

Closed
wlritchi opened this issue Feb 2, 2022 · 4 comments · Fixed by #105
Closed

rclone backup writes to root directory instead of mounted emptyDir #104

wlritchi opened this issue Feb 2, 2022 · 4 comments · Fixed by #105

Comments

@wlritchi
Copy link
Contributor

wlritchi commented Feb 2, 2022

Context

This chart creates an emptyDir (or, if backup persistence is enabled, a PVC) mount at the location given by destDir. Kubernetes sets the filesystem permissions automatically so that tar backups can be written to this directory.

The destDir option is doing double duty; for rclone backups, it also specifies the location within the rclone bucket where backups will be uploaded.

Issue

When rclone is selected as the backup method, the backup tar file is temporarily written to disk before being uploaded. However, the file is written in the current working directory, which defaults to /. On some (all?) Kubernetes providers, the root directory is not writable to containers that have dropped privileges, so unless the container has been set to run as root, the temporary file cannot be written, and the backup fails.

There's a few options for fixes:

  1. Either this chart or the mc-backup Dockerfile should be updated to set the working directory to destDir; or
  2. The script should be updated to write rclone temporary files to destDir; and/or
  3. A new parameter should be added to disambiguate "backup working directory" from "rclone destination path".

It feels cleanest to decouple the two uses of destDir, but doing so could also be a breaking change for people who have rclone backup already working via a root-privileged container.

@wlritchi
Copy link
Contributor Author

wlritchi commented Feb 3, 2022

Hacky workaround: the BACKUP_NAME environment variable (default: minecraftServer.worldSaveName or "world") is normally used to set the prefix for the backup filename - but you can put slashes in it, which allows you to also control which directory it's in. Nothing else in the backup script has any problems if you do this (at least for rclone backups), so you can just set that environment variable to the same path as destDir to put it into the emptyDir/PVC mount that the chart already creates.

    mcbackup:
      destDir: same/destdir/as/before
      extraEnv:
        BACKUP_NAME: /same/destdir/as/before/world

@itzg
Copy link
Owner

itzg commented Feb 3, 2022

The latest image for itzg/mc-backup now sets the working directory to /backups, so perhaps that will help with this scenario.

@wlritchi
Copy link
Contributor Author

wlritchi commented Feb 3, 2022

That makes things less hacky, but a workaround is still required if destDir is set non-default (which it will be if users need to put backups in a particular folder on the rclone remote). That workaround is now (untested):

extraVolumes:
- volumeMounts:
  - name: realBackupDir
    mountPath: /backups
  volumes:
  - name: realBackupDir
    emptyDir: {}  # or some other volume config, if you prefer

i.e. users still have to manually bind their backup volume, because this chart currently puts it at destDir (which makes sense for how tar backups use that parameter, but not for how rclone uses it).

Not sure what our tolerance is for breaking changes to the rclone integration (it's pretty new), but I suspect that if we continue using destDir to mean different things for different backup schemes, it's going to result in some confusing logic in this chart.

@itzg
Copy link
Owner

itzg commented Feb 3, 2022

The backup sidecar support was only added 25 days ago, so I think it's reasonable to make a breaking change now rather than perpetuate an awkward configuration.

If you could PR something, then that would be great.

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 a pull request may close this issue.

2 participants