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

Refactor mimir.rules.kubernetes component #6798

Closed
wants to merge 2 commits into from
Closed

Refactor mimir.rules.kubernetes component #6798

wants to merge 2 commits into from

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Mar 29, 2024 •
edited
Loading

PR Description

This change refactors the mimir.rules.kubernetes component to move most mutable state into a separate eventProcessor struct. The existing code already split most behavior between the main rules.go file and events.go file. This takes the next logical step and wraps the state from events.go into its own struct.

This is a prerequisite for a future PR that will add clustering support to the mimir.rules.kubernetes component. In the future PR a particular agent instance will be the "leader" for syncing rules from Mimir or not. When not the leader, the eventProcesor will be a no-op.

Which issue(s) this PR fixes

N/A

Notes to the Reviewer

As discussed offline, this change and the future PR to add clustering support is a stop-gap until there's some sort of universal clustered mode support for all components.

This has been tested with existing unit tests and locally by running Agents and Mimir in a k3s cluster via the Helm chart.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@56quarters 56quarters force-pushed the 56quarters/minimal-events branch 2 times, most recently from c0aefe0 to 2cc0f46 Compare April 2, 2024 15:50
@56quarters 56quarters marked this pull request as ready for review April 2, 2024 15:52
@56quarters 56quarters requested a review from a team April 2, 2024 15:52
@wildum
Copy link
Contributor

wildum commented Apr 4, 2024

loki.rules.kubernetes is very similar to the mimir.rules.kubernetes component. Would it make sense to do the same changes for the loki component in this PR and keep the similarities between the two implementations? Or should it be done via a following PR?

@56quarters
Copy link
Contributor Author

loki.rules.kubernetes is very similar to the mimir.rules.kubernetes component. Would it make sense to do the same changes for the loki component in this PR and keep the similarities between the two implementations? Or should it be done via a following PR?

Good point! My preference would be to:

  • Make this change only to mimir.rules.kubernetes
  • Make the change to mimir.rules.kubernetes to support clustering mode
  • Make the same changes to loki.rules.kubernetes based on the above PRs

@wildum
Copy link
Contributor

wildum commented Apr 4, 2024

Sounds good, would you mind creating an issue to track it and link it to this PR please?

@56quarters 56quarters force-pushed the 56quarters/minimal-events branch from 2cc0f46 to 71d019c Compare April 4, 2024 16:38
@56quarters 56quarters mentioned this pull request Apr 4, 2024
Open
@56quarters
Copy link
Contributor Author

Sounds good, would you mind creating an issue to track it and link it to this PR please?

Done in grafana/alloy#194

wildum
wildum approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@wildum wildum left a comment

Choose a reason for hiding this comment

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

LGTM

internal/component/mimir/rules/kubernetes/events.go Outdated Show resolved Hide resolved
@56quarters
Refactor mimir.rules.kubernetes component
827df5b 
This change refactors the `mimir.rules.kubernetes` component to move most
mutable state into a separate `eventProcessor` struct. The existing code
already split most behavior between the main `rules.go` file and `events.go`
file. This takes the next logical step and wraps the state from `events.go`
into its own struct.

This is a prerequisite for a future PR that will add clustering support to
the `mimir.rules.kubernetes` component. In the future PR a particular agent
instance will be the "leader" for syncing rules from Mimir or not. When not
the leader, the `eventProcesor` will be a no-op.

Signed-off-by: Nick Pillitteri <[email protected]>
@56quarters
Code review
564e057 
Signed-off-by: Nick Pillitteri <[email protected]>
@56quarters 56quarters force-pushed the 56quarters/minimal-events branch from 39d7ee9 to 564e057 Compare April 5, 2024 16:40
@56quarters 56quarters requested a review from wildum April 8, 2024 14:05
@wildum wildum mentioned this pull request Apr 9, 2024
Merged
@wildum wildum force-pushed the 56quarters/minimal-events branch from 1c0b176 to 564e057 Compare April 9, 2024 15:36
@grafana grafana deleted a comment from CLAassistant Apr 9, 2024
@wildum
Copy link
Contributor

wildum commented Apr 9, 2024

I moved this PR to Alloy: grafana/alloy#158

@wildum wildum closed this Apr 9, 2024
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label May 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wildum wildum Awaiting requested review from wildum

@clayton-cornell clayton-cornell Awaiting requested review from clayton-cornell

@jdbaldry jdbaldry Awaiting requested review from jdbaldry

Assignees
No one assigned
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@56quarters @wildum