-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add restic support #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking fantastic. (I haven't tested the restic aspect yet myself, but changes look great.)
For TODO item 2, this new tool I wrote would help since it will auto-align the container's uid/gid with an attached volume:
## specific method ## | ||
## functions ## | ||
##################### | ||
# Each function that corresponds to a name of a backup method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice structuring
Thanks :)
I've took a look at it and tried to use it. Unfortunately, I'm not entirely happy with it. I'm thinking of using a small entrypoint that'll decide which volume to use for the user (i.e. if using restic, there might be no Also, regardless of it all, if we start from scratch (there's no backups yet) and the source directory is world-readable, I think we shouldn't use root account, even if source was created as root. Just my opinion. I'll be testing it further, but roughly looks good for release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I checked my current deployment and actually run the mc-backup as non-root explicitly
https://gist.github.com/itzg/13d60211213489d138a07b785d2697af#file-mc-deployment-yaml-L78
Other than the potential --match
change, is this ready to merge?
Dockerfile
Outdated
|
||
COPY backup-loop.sh /opt/ | ||
|
||
VOLUME ["/data", "/backups"] | ||
|
||
ENTRYPOINT ["/opt/entrypoint-demoter", "--match", "/data"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this should match /backups
since that's the one that needs to be writable by the container's user, if that option is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that /backups
might not always exist / be mounted (i.e. using external bucket, using different path etc). This is why I think this should be more dynamically decided what arguments to run.
This is an example, where /backups isn't mounted:
bash-5.0# ps aux
PID USER TIME COMMAND
1 root 0:00 /opt/entrypoint-demoter --match /backups /opt/backup-loop.sh
10 root 0:00 {backup-loop.sh} /bin/bash /opt/backup-loop.sh
157 root 0:00 sleep 1h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, /data
would always be attached since that's the absolute requirement of the backup operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually yes and no. Since in the Dockerfile /data
and /backups
are mentioned, it'll always create custom volumes for them, if no volumes override them. Example:
docker inspect deploy-backup | jq '.[0].Mounts | [ .[] | {Type, Name, Destination} ]'
[
{
"Type": "volume",
"Name": "e719c8cf78817b70961c83760dd3f7146fc1fc0e7e7acbe3fc73fcd701b2b7fe",
"Destination": "/data"
},
{
"Type": "volume",
"Name": "f1169471a1175a27ecb475aebae84b9e9d2d0ffbfd3b8af945c2ae0971f85b6d",
"Destination": "/backups"
},
{
"Type": "volume",
"Name": "deploy-data",
"Destination": "/something/data"
}
]
This means that these directories will be mounted as root iirc.
Also, remember that one can use SRC_DIR
to change the location, so it might be something else. I'd be for removing volumes from Dockerfile (or leaving at most /backups
) and creating tinny entrypoint wrapper around the demoter to dynamically decide what arguments to pass (i.e. use environment variables passed to it - but then we'd need to duplicate location of some environment variable defaults :/).
I'd like to test it a little bit more, sorry. Restic has some unexpected behaviours, like restic/restic#1466 . While I think I've managed to work around this quirk, I don't know if I handled all different use cases. Out of curiosity, where do you host your k8s cluster for mc? I feel like I'm not personally ready to migrate everything to it quite yet, and feel like MC isn't really ready for k8s, but considering it as it'd simplify quite a few things. |
I have been quite pleased with running MC with kubernetes especially with this specialized ingress proxy I developed https://github.com/itzg/mc-router When the kids change their minds about MC flavor I can replicate a kubernetes deployment config, tweak the names, and it gets newly routed dynamically. As for hosting, I'm just running on an Intel NUC under my desk at home :) It's running Ubuntu and I'm using their microk8s distro of kubernetes. |
Oh, this looks nice. Just wanted to code it to play around k8s and ingress? Just wondering why not use something ready, i.e. https://kubernetes.io/docs/concepts/services-networking/ingress/#name-based-virtual-hosting :)
That makes sense then. I'm in a process of developing the infrastructure of a bigger server, to migrate from our current solution, which is a pain to work with. Hence automating everything :) |
Ah yes, Kubernetes ingress is super cool, but only works for HTTP routing at this time. Since MC has its own proprietary TCP protocol, I had to intercept its handshake messages to do routing of it. |
I'm sorry, I just noticed the other PR I merged recently has introduced conflicts. Let me know if you would like me to help with that. |
Hey,
No problem. I can take a look and resolve the conflict later. I don't know if I'll find time today / tomorrow. I'd like to test it over the weekend and if I won't find any issues, it'll be good to merge from my side.
…On Jul 31, 2019, 05:29, at 05:29, Geoff Bourne ***@***.***> wrote:
I'm sorry, I just noticed the other PR I merged recently has introduced
conflicts. Let me know if you would like me to help with that.
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#3 (comment)
|
9400a59
to
059a440
Compare
@itzg From my side it looks good - except for the entrypoint demoter. I think that since the volumes are mounted inside Dockerfile, they'll always be mounted as root. I don't know what the best solution would be though. |
Also - could you please verify that the README etc. can be edited on Windows (I assume you use Windows, due to the previous endings). I rarely use .gitattributes - it technically should convert the endings, but possible I did something wrong. |
Yep, I'm still able to edit the README. I tend to use IntelliJ, so it's fine with any flavor of line ending :) The entrypoint-demoter match looks good. Even though the |
This PR adds restic backup method, providing fast, encrypted, snapshot-based backups, supporting many different repository types:
... and more.
This is still to be tested thoroughly.
Also, I don't like how the tests are being done right now -any ideas for improvements welcome!
TODO: