Skip to content

Skip jitter for backups that are not triggered by a schedule - #888

Open
kyoresuas wants to merge 1 commit into
offen:mainfrom
kyoresuas:fix/886-skip-jitter-manual-run
Open

kyoresuas wants to merge 1 commit into
offen:mainfrom
kyoresuas:fix/886-skip-jitter-manual-run

Conversation

@kyoresuas

Copy link
Copy Markdown

Closes #886.

BACKUP_JITTER was applied unconditionally in runScript, so a one-off run like docker exec <container> backup was delayed by a random amount of time, too. As discussed in the issue, jitter now only applies to runs that are triggered by a cron schedule.

  • runScript takes a new runScriptOpts argument (same pattern as foregroundOpts) with a scheduled flag.
  • The cron callback in command.schedule passes scheduled: true, runAsCommand (one-off run) leaves it unset.
  • The delay calculation moved into a small jitterDelay helper so it can be unit tested without a Docker daemon or a file lock. The behavior for scheduled runs is unchanged.
  • The reference docs now say that jitter only applies to scheduled runs. I also replaced the dangling comma at the end of the first sentence of that comment block with a period.

Setting BACKUP_JITTER=0m on invocation as a workaround is no longer necessary, but still works.

Tested:

  • go test ./cmd/... ./internal/..., go build ./... and go vet ./cmd/... ./internal/... pass, gofmt -l reports nothing.
  • Added TestJitterDelay covering a manual run with jitter configured (no delay), a scheduled run with zero and negative jitter (no delay), and a scheduled run with jitter configured (delay always within [0, max]).
  • I did not run the integration tests in test/ as they need Docker.

@m90 m90 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good, thank you! There's a couple of things in flight right now, so I'll merge this whenever all of this is ready for release.

This branch has not been deployed

No deployments
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.

Shouldn't honor jitter when manually running a backup

2 participants