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.

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

feat(kafka): Add Ingestion from Kafka in Ingesters #14192

Merged
merged 13 commits into from
Sep 24, 2024

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented Sep 19, 2024

What this PR does / why we need it:

This adds a new flag to allow starting ingesting from Kafka. It's heavily inspired by Mimir Kafka ingestion.

This is meant to be used as a new set of replica of ingesters.

The idea is simple, we keep the same ingestion from Ingester but allow to ingest from Kafka.
Ingesters now shares partition ownership through the partition ring.

A new downscale partition endpoint is added for downscaling and keeping partition alive until the ingester query window (2h) is passed. The new endpoint is used by the new rollout operator.

Which issue(s) this PR fixes:
Fixes https://github.com/grafana/loki-private/issues/1115

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
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • 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

Sorry, something went wrong.

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.

LGTM

pkg/kafka/partition/committer.go Show resolved Hide resolved
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Sep 20, 2024
@cyriltovena cyriltovena marked this pull request as ready for review September 20, 2024 13:53
@cyriltovena cyriltovena requested a review from a team as a code owner September 20, 2024 13:53

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
pkg/kafka/config.go Outdated Show resolved Hide resolved
pkg/ingester/downscale.go Outdated Show resolved Hide resolved
pkg/kafka/reader_client.go Outdated Show resolved Hide resolved
cyriltovena and others added 2 commits September 23, 2024 12:20

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Joao Marcal <[email protected]>
pkg/kafka/partition/committer.go Outdated Show resolved Hide resolved
pkg/kafka/partition/reader.go Show resolved Hide resolved

// ExtractIngesterPartitionID returns the partition ID owner the the given ingester.
func ExtractIngesterPartitionID(ingesterID string) (int32, error) {
if strings.Contains(ingesterID, "local") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably fine for now, but I don't like it as a long-term solution. I don't think its that common practice to include .local in the hostname across all major OSes.
It would be better to define an arg for this to set it at runtime. We could use the Downward pod API in kubernetes to inject the pod name, for example.

@@ -616,6 +616,9 @@ func (t *Loki) initIngester() (_ services.Service, err error) {
t.Server.HTTP.Methods("POST", "GET", "DELETE").Path("/ingester/prepare_shutdown").Handler(
httpMiddleware.Wrap(http.HandlerFunc(t.Ingester.PrepareShutdown)),
)
t.Server.HTTP.Methods("POST", "GET", "DELETE").Path("/ingester/prepare-partition-downscale").Handler(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prepare_shutdown uses underscores, prepare-partition-downscale uses hyphens. Ideally, these would be in sync. Except hyphens seems more web-friendly and changing prepare_shutdown to hyphens would be a breaking change.
Not a big deal, but would be nice to align these now before they get some usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like Mimir uses dash everywhere. But It's a bit too late since prepare_shutdown is used.

This is used by the rollout operator and can be configured so I'll use _ everywhere. see https://github.com/grafana/rollout-operator?tab=readme-ov-file#delayed-scaledown

cyriltovena and others added 2 commits September 23, 2024 15:18

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: benclive <[email protected]>
Copy link
Contributor

@benclive benclive left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

lgtm but I haven't manually tested it

@cyriltovena cyriltovena merged commit b6e9945 into grafana:main Sep 24, 2024
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants