-
Notifications
You must be signed in to change notification settings - Fork 614
[opentelemetry-kube-stack]: Merge both collectors in a single one with leader election #1735
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
base: main
Are you sure you want to change the base?
Conversation
I think that waiting for #1728 and bumping the operator to the new version can be worth |
Could you retrigger the CI? INFO: Downloading bootstrap version 'v2.2.0' of cosign to verify version to be installed... |
I think that the PR is ready to review now, everything is up to date and tests are fixed 😄 |
extensions: | ||
k8s_leader_elector/k8s_cluster: | ||
auth_type: serviceAccount | ||
lease_name: k8s.cluster.receiver.opentelemetry.io | ||
lease_namespace: {{ .namespace }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it will merge cleanly, but if the leader elector extension ever needs changed we'll have to make sure to update all the instances. Can you move this to its own template and reuse it in the other templates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, nice idea! Tomorrow I'll do it and I'll rebase my branch too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL @TylerHelmuth , I've extracted the generation to another block and reused it for both cases
… leader election Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought as to whether we want to opt users in. Also, helm deletes can be a bit odd, so i think it would be worth writing some type of changelog entry in the README that encourages Gitops users to manually prune the resources
@@ -527,48 +531,6 @@ collectors: | |||
- batch | |||
exporters: | |||
- debug | |||
cluster: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think users would want this to be configurable? i.e. we give the user the ability to choose if they want the standalone or use leader election? I think there's value in having the option, but this is meant to be an opinionated chart... my only hesitation is that using leader election can be a bit frustrating for some users architectures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that this is opinionated, I'd get rid of it because the current cluster
collector doesn't work in HA, so as default it's not "safe to use". I thought about keeping both collectors as they were, but adding leader election to cluster
one. The problem with that approach was the more resources than needed were used when the daemonset can do that taks quite better.
Maybe, we can create an example with the previous configuration with both collectors and clarify in the readme that for those cases where leader election isn't an option, they can just check the example to use this approach of 2 collectors without leader election, but emphasizing the fact of not being in HA the cluster
one.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on getting rid of the cluster collector but still making it configurable (probably some docs for the following):
- Override daemonset collector to remove the
k8s_leader_elector
extension from the configuration -> some Otel collector distributions might not include this extension - Being able to manually define back the
cluster
collector.
That will allow current configurations that rely on the cluster
collector to use future Chart versions with minimum changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an example about how to define back the previous approach as well as some notes in readme.md to explain how to upgrade and to announce the "new requirement" of leader election extension and linking the example that defines back the previous approach.
To disable the leader election I have had to add a new field to the preset because I haven't found any other good way to disable it without adding a lot of code (but if someone has a good idea that I've missed just say them to me, I'm quite newbie with advanced helm templating 😄 )
PTAL
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
This PR bumps the operator dependency and merges both collectors in a single one running as daemonset with support to leader election for k8s api receivers, getting rid of the collector deployed as single instance deployment
Fixes #1726