Skip to content

fix(objstore): Ruler not starting and incorrect mkdir path when Thanos Store is enabled - #16555

Merged
ashwanthgoli merged 8 commits into
grafana:mainfrom
ringerc:fix-thanos-store-ruler-config
May 2, 2025
Merged

fix(objstore): Ruler not starting and incorrect mkdir path when Thanos Store is enabled#16555
ashwanthgoli merged 8 commits into
grafana:mainfrom
ringerc:fix-thanos-store-ruler-config

Conversation

@ringerc

@ringerc ringerc commented Mar 5, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

As discussed in #16543, loki does not completely switch from using the ruler_storage configuration block when -use-thanos-objstore=true or storage_config.use_thanos_objstore: true is set.

It still uses the non-thanos-store config block ruler.storage to:

  • find the path to mkdir for local rules storage during startup; and
  • decide whether the ruler is configured and should be started in the all and backend targets

Fix both issues, so ruler.storage is completely ignored when in thanos mode.

Also add a warning when one of the config blocks is being overridden, but is non-empty, like:

level=warn ts=2025-03-05T03:42:56.185427635Z caller=modules.go:1320 msg="ruler.storage exists and is not empty, but will be ignored in favour of ruler_storage because storage_config.use_thanos_objstore is true."

and the inverse.

See also related docs change #16553

Fixes bug introduced in 8bca2e7 (#11713) per #16543 (comment)

This should be backported to v3.3.x and v3.4.x, possibly except the warning, since it's present in all releases containing the above commit.

Which issue(s) this PR fixes:

Fixes #16543

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@ringerc

ringerc commented Mar 12, 2025

Copy link
Copy Markdown
Contributor Author

Anyone?

@ashwanthgoli

Copy link
Copy Markdown
Contributor

@ringerc i'll have a look at this today. ty!

ringerc added 3 commits April 16, 2025 11:25
The ruler will fail to start in "all" or "backend" startup target
with message "Ruler storage is not configured; ruler will not be
started" if Loki is configured to use the Thanos object store.

The ruler configuration should be read from the 'ruler_storage' top
level config key when thanos object store is enabled (-use-thanos-objstore=true
or storage_config.use_thanos_objstore: true in the config file).

A bug in the ruler-storage module startup check meant that the
ruler.storage block that's supposed to be ignored in thanos storage
mode was still being used to check whether the ruler was configured.
So a placeholder ruler.storage block had to be provided to get the ruler
to start, even though it would be ignored by ruler itself.

Fix, so the ruler.storage block is completely ignored and can be
entirely removed when thanos client is in use.
When the ruler-storage module starts, it tries to mkdir the local
storage directory. This was not updated to use the path from
the ruler_storage block when the thanos object store is enabled,
and was still using the otherwise-ignored ruler.storage block instead.

Fix so it properly uses ruler_storage.filesystem.dir or
ruler_storage.local.directory when in thanos storage mode, ignoring
the non-thanos-store ruler.storage.local.directory.
Emit a warning if ruler.storage is defined but ignored because
object_store.use_thanos_client: true causes the ruler_storage block
to be used instead. And vice versa when the ruler_storage config only
used with the thanos client is present, but the thanos client is not
enabled.

This is particularly important for ruler, because the thanos object
store client setting affects which ruler storage block is used even
when ruler is just using the local file system as a store.

The new warning looks like this:

    level=warn msg="ruler.storage exists and is not empty, but will be ignored in favour of ruler_storage because storage_config.use_thanos_objstore is true."
@ringerc
ringerc force-pushed the fix-thanos-store-ruler-config branch from a159757 to dd4ef05 Compare April 15, 2025 23:25
@ringerc

ringerc commented Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @ashwanthgoli . I've rebased it. This was incredibly confusing when I was converting to the Thanos object store config so it'd be good to have the bug fix landed.

@ringerc

ringerc commented Apr 21, 2025

Copy link
Copy Markdown
Contributor Author

Anybody willing to review and merge this bug fix?

@ashwanthgoli ashwanthgoli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ashwanthgoli

ashwanthgoli commented May 2, 2025

Copy link
Copy Markdown
Contributor

@ringerc extremely sorry for letting this go unattended for so long. Thank you for the thorough investigation and fixing it! ❤️

@ashwanthgoli ashwanthgoli changed the title Fix Ruler not starting and incorrect mkdir path when Thanos Store is enabled May 2, 2025
@ashwanthgoli

Copy link
Copy Markdown
Contributor

@ringerc can you pull in the latest changes from main? its missing some recent CI changes. Happy to handle it if you don't mind

@ashwanthgoli
ashwanthgoli merged commit 3da9f2c into grafana:main May 2, 2025
@grobinson-grafana

Copy link
Copy Markdown
Contributor

This seems to be failing lint in main.

@ringerc

ringerc commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

Thanks very much

@jameshartig

Copy link
Copy Markdown

Will this get backported to 3.5? I'm not seeing this code in the v3.5.8 release.

@JStickler

Copy link
Copy Markdown
Contributor

@jameshartig This PR will be part of the Loki 3.6 release.

felix0102 pushed a commit to felix0102/loki that referenced this pull request Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport release-3.4.x size/M type/bug Somehing is not working as expected

5 participants