-
Notifications
You must be signed in to change notification settings - Fork 0
Prototype #1
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.
Looking good!
src/privateer2/backup.py
Outdated
print("in the directory /privateer/keys") | ||
else: | ||
print(f"Backing up '{volume}' from '{name}' to '{server}'") | ||
run_docker_command("Backup", image, command=command, mounts=mounts) |
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 think run_docker_command
sounds too much like it runs an actual command into an existing docker container, bit like exec_run
run_docker_command("Backup", image, command=command, mounts=mounts) | |
run_container_with_command("Backup", image, command=command, mounts=mounts) |
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.
good shout
src/privateer2/server.py
Outdated
if container.status == "running": | ||
container.stop() | ||
else: | ||
print("Container '{machine.container}' for '{name}' does not exist") |
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.
that needs the f right?
print("Container '{machine.container}' for '{name}' does not exist") | |
print(f"Container '{machine.container}' for '{name}' does not exist") |
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.
yeah, that's right. I've fixed that and added a test as this behaviour was never tested properly
Co-authored-by: M-Kusumgar <[email protected]>
Co-authored-by: M-Kusumgar <[email protected]>
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 looks great, couple of questions comments
- Is it worth namespacing the keys in the vault with the config name too? e.g. for the example with
local.json
with serveralice
and clientbob
. Write these tosecrets/local/alice
andsecrets/local/bob
. I guess otherwise if 2 configs are using the same local names they could overwrite each other? Or you already working around that somehow - If we have a service running which is using the volume we are backing up or restoring do we have to take down that service? Or is rsync smart enough that that can work if some other process is potentially modify the files?
After writing a configuration, on any machine run | ||
|
||
``` | ||
privateer2 keygen --all |
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.
Is this going to replace privateer eventually?
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.
yeah, I'll do some sort of merge into that package source
Co-authored-by: Rob <[email protected]>
In the configuration, the
This depends on the service unfortunately. Most things will cope acceptably well, but there's always a chance that a partly written file will be copied over and we can't stop that. If the system needs taking down before backup, then that system won't be able to use scheduled backups, and we'd need to think about how to do backup in a consistent way. We'd have similar issues with the privateer1 approach of using |
Basic implementation. I'll probably go back and carve out a slightly smaller surface area here for the first real PR...
Before merging:
:prototype
tag