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

Move cron scheduling inside application #338

Merged
merged 9 commits into from
Feb 6, 2024
Merged

Conversation

pixxon
Copy link
Contributor

@pixxon pixxon commented Jan 31, 2024

Closes #268

Allows the application to take control of the scheduling instead of relying on crond.

  • Tries to read config from environment variables
  • Reads all the files under /etc/dockervolumebackup/conf.d and each creates a new job
  • Without --foreground flag, returns to old behaviour

@pixxon
Copy link
Contributor Author

pixxon commented Jan 31, 2024

Still heavily in progress, but wanted to push the current state.

Since I am not too familiar with golang, all sort of feedback is more than welcome.

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting to work on this. I'll leave comments here and there, but please let me know when you think this is ready for a proper review.

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

m90 commented Feb 3, 2024

Do you think it would make sense to already update the entrypoint.sh script so that tests also run the non-cron scheduler?

@pixxon
Copy link
Contributor Author

pixxon commented Feb 3, 2024

Do you think it would make sense to already update the entrypoint.sh script so that tests also run the non-cron scheduler?

Yesterday I ran the whole test suite with success locally using the new scheduler. I was thinking of switching over to it, but have a question.

Do you want to still support the busybox crond ( aka allow users to switch to that one, by setting an explicit flag ) or remove it completely?

@pixxon
Copy link
Contributor Author

pixxon commented Feb 3, 2024

I switched over to the new scheduler. Users who want to opt into the old version can do so by specifying the entrypoint of the container to the old script.

I would assume that the old version will be deprecated anyway, it is only there for some grace period. So I did not document this alternative, but I can write something about it if you think it's worth.

Also I think this is the final implementation. Once the CI runs successfully, please review the code and give feedback if changes are needed

@m90
Copy link
Member

m90 commented Feb 4, 2024

Users who want to opt into the old version can do so by specifying the entrypoint of the container to the old script.

I'd remove the old entrypoint. This patch should be able to support everything that has been supported previously, so there's no need to give users a choice. If there are things we cannot support that have been supported previously, we'd need to figure this out.

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments, let me know what you think of them.

I'll also have to think about the error handling in func main and how it interacts with the config loading and constructing the script a bit more. Maybe this can be a bit more straightforward.

cmd/backup/configProvider.go Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
cmd/backup/configProvider.go Outdated Show resolved Hide resolved
cmd/backup/configProvider.go Outdated Show resolved Hide resolved
cmd/backup/configProvider.go Outdated Show resolved Hide resolved
cmd/backup/configProvider.go Outdated Show resolved Hide resolved
cmd/backup/main.go Outdated Show resolved Hide resolved
cmd/backup/main.go Outdated Show resolved Hide resolved
cmd/backup/main.go Outdated Show resolved Hide resolved
cmd/backup/main.go Outdated Show resolved Hide resolved
Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very nice already. I left some minor comments, let me know if there are questions around them.

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

m90 commented Feb 6, 2024

This looks great now, thank you very much for all the work you put into this. As it's quite a drastic change, I'd like to do some manual testing in setups of mine before cutting an actual release from this, so I'll merge this to a next branch and release v2.37.0-alpha.1 from it. I'll keep you updated on the progress.

@m90 m90 changed the base branch from main to next February 6, 2024 20:03
@m90 m90 merged commit d642a60 into offen:next Feb 6, 2024
2 checks passed
@m90
Copy link
Member

m90 commented Feb 7, 2024

@pixxon One difference I noticed between the busybox cron and this version is that now, we'll happily register syntactically valid, but "impossible" cron schedules like "run on the 31st of Februrary" (i.e. 0 0 5 31 2 ?).

Previously this would refuse to be added to the crontab with a message like

crond: user root: parse error at 31

In the end, the result is the same as the container runs, idling forever, but I am wondering if we should (if possible) also notify users who provided such an expression?

@m90
Copy link
Member

m90 commented Feb 7, 2024

Ah, I just noticed you mentioned this in the documentation already. In any case, if there is a possibility to somehow make the cron library give us a warning about how a schedule will never run, I think we should log it. If not, I guess it's fine too.

@m90
Copy link
Member

m90 commented Feb 8, 2024

Interesting, I found a terribly hacky way of doing this. If you parse the cron expression and ask for its next execution, the library call will panic in case the cron will never run. Expressions that will run pass this test just fine

		sched, _ := cron.ParseStandard(config.BackupCronExpression)
		now := time.Now()
		next := sched.Next(now) // panics when the cron would never run

Not sure if I want to use this though.

(Stolen from here robfig/cron#392 (comment))

m90 pushed a commit that referenced this pull request Feb 12, 2024
* Move cron scheduling inside application

* Make envvar a fallback and check for errors

* Panic significantly less

* propagate error out of runBackup

* Add structured logging

* FIx error propagation to exit

* Enable the new scheduler by default

* Review fixes

* Added docs and better error propagation
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.

Consider using a dedicated cron package
2 participants