Skip to content

compact: Add compactor time validation #7293

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.

Sign up for GitHub

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

Jump to bottom
Open
nashluffy wants to merge 6 commits into
base: main
Choose a base branch
from
Open

compact: Add compactor time validation #7293

nashluffy wants to merge 6 commits into from

Conversation

@nashluffy
Copy link

@nashluffy nashluffy commented Apr 20, 2024 •
edited
Loading

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Adds validation to --min-time and --max-time flags. The documentation isn't super clear and caused me confusion, along with another report in this issue

As far as I'm able to reason about

  • --min-time durations in the future are nonsensical and invalid, but future absolute times are valid.
  • --min-time should never be a more recent date than --max-time.

Verification

Invalid use cases caught by new validation

Future time for --min-time

mluffman:~/code/thanos$ /home/mluffman/go/bin/thanos compact --objstore.config-file=fs-conf.yaml --min-time=5d
ts=2024-04-20T12:47:41.589010523Z caller=factory.go:53 level=info msg="loading bucket configuration"
ts=2024-04-20T12:47:41.589226572Z caller=main.go:135 level=error err="invalid time filters: min time: duration cannot be in the future\npreparing compact command failed\nmain.main\n\t/home/mluffman/code/thanos/cmd/thanos/main.go:135\nruntime.main\n\t/opt/tm/tools/go/1.22.2-20240403/usr/go/src/runtime/proc.go:271\nruntime.goexit\n\t/opt/tm/tools/go/1.22.2-20240403/usr/go/src/runtime/asm_amd64.s:1695"

--max-time greater than --min-time

mluffman:~/code/thanos$ /home/mluffman/go/bin/thanos compact --objstore.config-file=fs-conf.yaml --max-time=-12h --min-time=-4h
ts=2024-04-20T12:48:44.929847137Z caller=factory.go:53 level=info msg="loading bucket configuration"
ts=2024-04-20T12:48:44.930003855Z caller=main.go:135 level=error err="invalid time filters: max time must be a time or duration after min time\npreparing compact command failed\nmain.main\n\t/home/mluffman/code/thanos/cmd/thanos/main.go:135\nruntime.main\n\t/opt/tm/tools/go/1.22.2-20240403/usr/go/src/runtime/proc.go:271\nruntime.goexit\n\t/opt/tm/tools/go/1.22.2-20240403/usr/go/src/runtime/asm_amd64.s:1695"

Existing, valid use cases still passing

No time ranges

mluffman:~/code/thanos$ /home/mluffman/go/bin/thanos compact --objstore.config-file=fs-conf.yaml
ts=2024-04-20T12:49:38.138409312Z caller=factory.go:53 level=info msg="loading bucket configuration"
ts=2024-04-20T12:49:38.138809502Z caller=compact.go:683 level=info msg="starting compact node"

Only --min-time specified

mluffman:~/code/thanos$ /home/mluffman/go/bin/thanos compact --objstore.config-file=fs-conf.yaml --min-time=-4h
ts=2024-04-20T12:49:51.839429392Z caller=factory.go:53 level=info msg="loading bucket configuration"
ts=2024-04-20T12:49:51.8400529Z caller=compact.go:683 level=info msg="starting compact node"

Future absolute time specified for --min-time

mluffman:~/code/thanos$ /home/mluffman/go/bin/thanos compact --objstore.config-file=fs-conf.yaml --min-time=2025-01-01T00:00:00Z
ts=2024-04-20T12:51:46.522495074Z caller=factory.go:53 level=info msg="loading bucket configuration"
ts=2024-04-20T12:51:46.522700514Z caller=compact.go:683 level=info msg="starting compact node"

@nashluffy nashluffy force-pushed the add-compactor-time-validation branch from b5ed8c5 to 334c761 Compare April 20, 2024 13:20
@nashluffy nashluffy changed the title Add compactor time validation compact: Add compactor time validation Apr 20, 2024
moadz
moadz reviewed Nov 11, 2024
View reviewed changes
cmd/thanos/compact.go
}

func validateFilterConf(filterConf *store.FilterConfig) error {
if filterConf.MinTime.PrometheusTimestamp() > filterConf.MaxTime.PrometheusTimestamp() {
Copy link
Contributor

@moadz moadz Nov 11, 2024

Choose a reason for hiding this comment

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

Is it valid to have them equal? If the compactor MinTime and MaxTime are equal it will just be non-functional iirc.

cmd/thanos/compact.go

cc.filterConf = &store.FilterConfig{}
cmd.Flag("min-time", "Start of time range limit to compact. Thanos Compactor will compact only blocks, which happened later than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y.").
cmd.Flag("min-time", "Start of time range limit to compact. Thanos Compactor will only compact blocks whose max time is at a date more recent than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or -2h45m. Durations must be negative, and valid units are ms, s, m, h, d, w, y.").
Copy link
Contributor

@moadz moadz Nov 11, 2024

Choose a reason for hiding this comment

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

Suggested change
cmd.Flag("min-time", "Start of time range limit to compact. Thanos Compactor will only compact blocks whose max time is at a date more recent than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or -2h45m. Durations must be negative, and valid units are ms, s, m, h, d, w, y.").
cmd.Flag("min-time", "Start of time range limit to compact. Thanos Compactor will compact only blocks with a maximum time more recent than this specified value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or -2h45m. Durations must be negative, and valid units are ms, s, m, h, d, w, y.").

Whose is an unnatural possessive for an inanimate object (a block).

cmd/thanos/compact.go
cmd.Flag("min-time", "Start of time range limit to compact. Thanos Compactor will only compact blocks whose max time is at a date more recent than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or -2h45m. Durations must be negative, and valid units are ms, s, m, h, d, w, y.").
Default("0000-01-01T00:00:00Z").SetValue(&cc.filterConf.MinTime)
cmd.Flag("max-time", "End of time range limit to compact. Thanos Compactor will compact only blocks, which happened earlier than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y.").
cmd.Flag("max-time", "End of time range limit to compact. Thanos Compactor will only compact blocks whose min time is at a date less recent this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or -2h45m. Valid units for duration are ms, s, m, h, d, w, y.").
Copy link
Contributor

@moadz moadz Nov 11, 2024

Choose a reason for hiding this comment

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

Suggested change
cmd.Flag("max-time", "End of time range limit to compact. Thanos Compactor will only compact blocks whose min time is at a date less recent this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or -2h45m. Valid units for duration are ms, s, m, h, d, w, y.").
cmd.Flag("max-time", "End of time range limit to compact. Thanos Compactor will compact only blocks with a maximum time older than this specified value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or -2h45m. Valid units for duration are ms, s, m, h, d, w, y.").

Same as above, and I also believe it has to be older than max time, if it's older than min-time, block might be beyond this value and thus you'd actually be compacting max-time + block size (which could be up to multiple days/weeks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@moadz moadz moadz left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

component: compact feature request/improvement size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nashluffy @moadz @douglascamata