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

Improve Swarm support #333

Merged
merged 26 commits into from
Jan 31, 2024
Merged

Improve Swarm support #333

merged 26 commits into from
Jan 31, 2024

Conversation

m90
Copy link
Member

@m90 m90 commented Jan 25, 2024

Supersedes #328

Closes #327

@m90 m90 changed the title Query for labeled services as well Improve Swarm support Jan 26, 2024
@m90
Copy link
Member Author

m90 commented Jan 26, 2024

@pixxon So I think this should work as discussed in #328 now. I still have to do cleanup and add documentation but if you have feedback or even want to test an image built off this branch yourself, I'd be thankful for any remarks.

The test is currently the only reference for how to configure things, but I think you have an idea of how it's supposed to work anyways.

cmd/backup/script.go Outdated Show resolved Hide resolved
cmd/backup/script.go Outdated Show resolved Hide resolved
cmd/backup/script.go Outdated Show resolved Hide resolved
@pixxon
Copy link
Contributor

pixxon commented Jan 27, 2024

Except the three comments above, the feature looks great to me. I will build an image and later today try it out.

I have one scenario in mind, where the service does not scale down in time. It might leave the service in a state that requires manually intervention from the user.

@m90
Copy link
Member Author

m90 commented Jan 27, 2024

Thanks for your review. I'll address these points and some other tiny bits I was thinking of later today, and I'll also push a pre-release to Docker Hub, so I can test it myself in a setup of mine. I'll let you know when this is out.

@m90
Copy link
Member Author

m90 commented Jan 27, 2024

I pushed a build of ba095ea tagged as v2.36.0-alpha.1 (both to Docker Hub and GHCR) and also already tested it in a low-stakes Swarm setup of mine, where it seems to work as intended. I'll let it run unsupervised for a while now before proceeding.

If anyone wants to test this in their setup as well, I'm happy about any feedback. Documentation is not yet deployed, but can be accessed here https://github.com/offen/docker-volume-backup/blob/swarm-scale/docs/how-tos/use-with-docker-swarm.md

There seems to be an issue where services restart prematurely which I'll need to figure out first.

cmd/backup/script.go Outdated Show resolved Hide resolved
@m90
Copy link
Member Author

m90 commented Jan 27, 2024

I just pushed v2.36.0-alpha.2 which seems to fix the issue and can be tested by others.

cmd/backup/script.go Outdated Show resolved Hide resolved
@m90
Copy link
Member Author

m90 commented Jan 27, 2024

316a7a6 is now pushed as v2.36.0-alpha.3

@m90 m90 marked this pull request as ready for review January 28, 2024 17:02
@pixxon
Copy link
Contributor

pixxon commented Jan 29, 2024

Small note about the configuration. Users can apply the label via BACKUP_STOP_CONTAINER_LABEL environment variable. However in case of services it might be a bit misleading.

I can't really think of a great solution to avoid this. Adding BACKUP_STOP_SERVICE_LABEL would allow the user to set separate labels, but would increase the complexity of the configuration, allowing more user errors to happen. ( User sets wrong label and environment variable combination so the services do not stop... )

@m90
Copy link
Member Author

m90 commented Jan 29, 2024

Users can apply the label via BACKUP_STOP_CONTAINER_LABEL environment variable. However in case of services it might be a bit misleading.

I was already thinking about the same problem, and since you also raise it, it's probably a good idea to do something about it.

I think the proper way would be to deprecate BACKUP_STOP_CONTAINER_LABEL (i.e. using it logs a warning and it shall be removed in any potential future major version) and start using BACKUP_STOP_DURING_BACKUP_LABEL, which is also more in line with the labels themselves.

@m90
Copy link
Member Author

m90 commented Jan 29, 2024

I pushed 87ea8d0 as v2.36.0-alpha.4 and will deploy it to all of my setups next. In case this works as expected, I think this is ready to go now.

@m90 m90 merged commit c3daeac into main Jan 31, 2024
2 checks passed
@m90 m90 deleted the swarm-scale branch January 31, 2024 11:17
@m90
Copy link
Member Author

m90 commented Jan 31, 2024

This is now released as v2.36.0. Thanks for the input @pixxon

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.

Improve Docker Swarm support
2 participants