Add label to optionally skip container restart after backup

[ChainsLunatic]

Added the restart-after-backup label to handle cases where certain services (e.g. game servers) are started on demand via tools like LazyTainer. These containers don’t need to be automatically restarted after a backup and can remain stopped until they’re launched again when needed.

[m90]

Thanks for the contribution. Even though it's probably a bit exotic, I think the feature makes sense.

I am a bit undecided about the user facing API for this as I would be afraid of people thinking they have to configure it, or get it wrong in any other way. Considering this will rarely be used, I'd like to prevent additional support issues being raised because of this.

I think the proper way of doing this would be adding a aingle docker-volume-backup.stop-behavior=none|stop-restart|stop-only label with distinct values that control all aspects and deprecate the existing label, but then again this is likely the No.1 feature here and it'd be pretty annoying to introduce deprecation warnings for basically everyone.

Long story short, I will need to think about how to do this for a bit, I shall get back to you and we'll get it merged in one way or another in any case.

[ChainsLunatic]

Understandable, both ways have their own advantages. But would'nt deprecating the old label and switch to the new behaviour based approach break the ability so set the labels value via configuration?

[m90]

Yes, you're right about the label situation, so that isn't really an option.

[ChainsLunatic]

So you are looking for another option fitting both concerns now?

[m90]

I think docker-volume-backup.restart-after-backup is probably a good choice, however I am not sure how this would work in conjunction with labels. Can you also set a BACKUP_RESTART_AFTER_BACKUP_LABEL? I think this would be the expected behavior, but I have a hard time imagining how to model a true default value in that case.

[ChainsLunatic]

When I started implementing the feature I had such a config value but soon came to the realization that it made no sense for me, to keep it. Since it's not a standalone feature but just a "flag" for docker-volume-backup.stop-during-backup and can't be used without it, its label configuration is controlling both of them at once. Does this make any sense to you?

[m90]

This is super tricky.

Since it's not a standalone feature but just a "flag"

I think this is why I am afraid of the current state confusing people. stop-during-backup and restart-after-backup sound very "parri passu" when they are not.

I am also thinking maybe it would be a thing to add a distinct new label for the exact behavior you want, but trouble is stop-during-backup is already taken :) and stop-during-backup-and-dont-restart is a little clunky.

[ChainsLunatic]

Hmmm yes.. I guess stop-during-backup.restart would be rather hacky?

[m90]

I start believing adding a stop-during-backup-no-restart label and is - while pretty awkward - the least confusing / most predictable option. I would think since this has zero intersection with stop-during-backup, it should be rather straight forward to add similar behavior. What do you think?

[ChainsLunatic]

What exactly would the configuration look like than? Would I add stop-during-backup-no-restart additionally or instead of stop-during-backup? That's not clear to me to be honest. If I would have to guess I still would say additionally?

[m90]

I would have thought you add it instead, it's either the one or the other.

[ChainsLunatic]

What would happen if I added both? With the configurable label values here, using both labels with different values could be beneficial to the overall feature, I think. We just would need to decide what happens if both labels have the same value.

[m90]

I would have thought both options are mutually exclusive and the command would just error out if a service is labeled with both options. What's the use case you would model using both labels?

[ChainsLunatic]

The benefit I see is more theoretical, I suppose. You could use it similarly to the configurable label feature itself, with different configurations performing different actions. But I think my idea falls apart when I try to come up with a valid scenario. Where exactly would the error occur? If it happens too late, the whole process could fail at an to the user unexpected point.

[m90]

The obvious place to do this would be in stopContainersAndServices (

docker-volume-backup/cmd/backup/stop_restart.go

Lines 94 to 97 in 5223459

	// stopContainersAndServices stops all Docker containers that are marked as to being
	// stopped during the backup and returns a function that can be called to
	// restart everything that has been stopped.
	func (s *script) stopContainersAndServices() (func() error, error) {

) which would just return an error in case conflicting values are found on a service. This is called befored anything else so there is no way the backup will be left in an unexpected state.

It feels tempting to fail even earllier though (which I guess is why you're asking), but that would mean a major change as we'd need to instantiate the Docker client before validating the configuration. I have this on my radar for a long time now, but it's complicated, so I don't think it makes sense to add it as part of this PR.

[ChainsLunatic]

So something like that? I hope it is somehow correct, I am completely new to go.

[ChainsLunatic]

To be honest I am not at all happy with the logging (Stopping %d out of %d running container(s) as they were labeled %s or %s.). It's clunky, but I currently have no better idea at all

[m90]

I would think this also deserves a test case. It can be as simple as copying the "local" test case, changing the label and changing the assertions about running containers here:

docker-volume-backup/test/local/run.sh

Line 21 in 5223459

expect_running_containers "2"

[m90]

Nit: as the above condition already returns there is no need to put this in an else block.

[m90]

This is now out of date. I think I would not mention this feature in the main README, but add a section about it at the end of https://offen.github.io/docker-volume-backup/how-tos/stop-containers-during-backup.html

[ChainsLunatic]

Should be all done now :)

[m90]

Thank you 🎩, let me get this released as well now.