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.

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

operator: Ruler enhancement proposal #5985

Merged
merged 8 commits into from
May 6, 2022
Merged

operator: Ruler enhancement proposal #5985

merged 8 commits into from
May 6, 2022

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented Apr 21, 2022 •
edited
Loading

What this PR does / why we need it:
The following PR adds a enhancement proposal for adding Ruler support into the Loki Operator.

Which issue(s) this PR fixes:
Addresses #5211 and #5843

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@periklis periklis added type/feature Something new we should do proposal A design document to propose a large change to Promtail sig/operator labels Apr 21, 2022
@periklis periklis requested a review from a team as a code owner April 21, 2022 12:59
@periklis periklis self-assigned this Apr 21, 2022
@periklis periklis mentioned this pull request Apr 21, 2022
Merged
4 tasks
@periklis periklis force-pushed the ruler-enhancement-proposal branch 7 times, most recently from a67c095 to 252d62f Compare April 22, 2022 09:46
simonswine
simonswine approved these changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@simonswine simonswine 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 putting the design document together. I think this is mostly LGTM.

I would like to see a more flexible mapping of Rules to Stacks, but that is not a hard requirement for merging.

operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
xperimental
xperimental reviewed Apr 26, 2022
View reviewed changes
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
bwplotka
bwplotka approved these changes Apr 29, 2022
View reviewed changes
Copy link

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM! Great work. For my taste, there are too many CR details discussed here that might be discussed in PR.

I think I would vote for fewer API components and have LokiRule for a start - no need to confuse people and break consistency here. Similar as it was done in PromRule.

operator/docs/enhancements/enhancement_template.md Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
owen-d
owen-d reviewed May 2, 2022
View reviewed changes
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Left a few small suggestions, but overall looks reasonable to me.

operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md

## Drawbacks

The above proposed design and implementation for LokiStack Ruler support adds a significant amount of new APIs as well as complexity into exposing a declarative approach **only** for the ruler component. Regardless the hard effort to minimize the amount of configuration settings in the proposed CRDs it remains still a huge addition to the operator code base. In contrast to the existing `LokiStack` CRD that is a very slim set of Loki settings (Note: without considering the gateway tenant configuration), the `RulerConfig` is almost one-to-one identical to the [ruler config](https://grafana.com/docs/loki/latest/configuration/#ruler).
Copy link
Member

@owen-d owen-d May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would consider slimming down some of these specs and instead use reasonable defaults until the need for such complexity is proven. This would reduce the new surface area by a large margin and options could be added incrementally as needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@owen-d From a user experience perspective the amount of non-defaulted fields the user needs to declare thanks to +kubebuilder:default annotation is limited to a minimum. Do you foresee any upcoming changes for remote_write or alertmanager (e.g. fields that will be deprecated, removed or replaced)?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any changes coming, but @dannykopping may have a better sense than I.

xperimental
xperimental reviewed May 3, 2022
View reviewed changes
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

A few more comments. Sorry for forgetting to click the send button last week.

operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Show resolved Hide resolved
operator/docs/enhancements/ruler_support.md Outdated Show resolved Hide resolved
@periklis periklis force-pushed the ruler-enhancement-proposal branch from c6a0fbc to b355057 Compare May 4, 2022 07:27
@periklis periklis merged commit 41647d1 into grafana:main May 6, 2022
@periklis periklis mentioned this pull request May 19, 2022
Merged
4 tasks
This was referenced Jun 28, 2022
Closed
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owen-d owen-d owen-d approved these changes

@cyriltovena cyriltovena cyriltovena approved these changes

@jan--f jan--f jan--f left review comments

@xperimental xperimental xperimental left review comments

@bwplotka bwplotka bwplotka approved these changes

@simonswine simonswine simonswine approved these changes

Assignees

@periklis periklis

Labels
proposal A design document to propose a large change to Promtail sig/operator size/XL type/feature Something new we should do
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@periklis @xperimental @simonswine @jan--f @cyriltovena @bwplotka @owen-d