Add k8s manifest leveraging leaderelection #20512

ChrsMark · 2020-08-10T12:18:38Z

What does this PR do?

This PR proposes new Kubernetes manifests for shake of #19731, leveraging unique Autodiscover provider implemented at #20281.

With these manifests, only Metricbeat's Deamonset will be able to monitor whole k8s cluster since one Deamonset Pod each time will hold the leadership being responsible to coordinate metricsets that collect cluster wide metrics.

We will might need a meta issue to keep track of deprecating Deployment manifests (if needed).

Add an extra section within https://www.elastic.co/guide/en/beats/metricbeat/7.x/running-on-kubernetes.html

Why is it important?

To get rid of the requirement to maintain/handle two different deployment strategies.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Test the manifests similarly to the testing steps of #20281 (comment).

Prepare a multinode cluster (ie on GKE)
Edit metricbeat-leaderelection-kubernetes.yml properly to set the proper image (ie docker.elastic.co/beats/metricbeat:7.10.0-SNAPSHOT) and the proper ES output (ie on Elastic Cloud)
Deploy the metricbeat-leaderelection-kubernetes.yml manifest and make sure that all the desired metricsets are shipping events and that k8s related Dashboards are populated with data correctly.
kubectl delete the leader pod and make sure that the leadership is either transfered to another Pod or it is gained again by the new replacement Pod.
Add static or hints-based autodiscovery configs/providers and make sure that work all together.
Example:

metricbeat.autodiscover:
      providers:
        # To enable hints based autodiscover uncomment this:
        #- type: kubernetes
        #  node: ${NODE_NAME}
        #  hints.enabled: true
        - type: kubernetes
          node: ${NODE_NAME}
          templates:
            - condition:
                contains:
                  kubernetes.pod.name: "nats"
              config:
                - module: nats
                  hosts: "${data.host}:${data.port}"
        - type: kubernetes
          scope: cluster
          node: ${NODE_NAME}
          unique: true
          identifier: gke-lease
          templates:
            - config:
                - module: kubernetes
                  hosts: ["kube-state-metrics:8080"]
                  period: 10s
                  add_metadata: true
                  metricsets:
                    - state_node
                    - state_deployment
                    - state_replicaset

Related issues

Closes Implement kubernetes agent cluster scope leader election #19731

Signed-off-by: chrismark <[email protected]>

elasticmachine · 2020-08-10T12:18:48Z

Pinging @elastic/integrations-platforms (Team:Platforms)

elasticmachine · 2020-08-10T12:43:09Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #20512 updated]
Start Time: 2020-08-14T08:21:21.993+0000
Duration: 60 min 6 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3252
Skipped	727
Total	3979

ChrsMark · 2020-08-13T09:48:46Z

Tested on a 3-node cluster on GKE:

Unique provider does the job for us and starts state_* metricsets to monitor cluster-wide services
Hints-based autodiscover provider works along with unique provider
Template-based autodiscover provider works along with unique provider

Reviews more than welcome at this point.

Signed-off-by: chrismark <[email protected]>

jsoriano · 2020-08-13T10:50:34Z

I wonder if we want to maintain the two sets of manifests for Metricbeat. Having a provider with leader election or a deployment to do the same thing is an implementation detail. I think that having an only strategy is more clear, and having leader election by default seems a better option as it only requires the DaemonSet.
Also having a mostly duplicated configuration can be more problematic for us, we may forget to apply some change in one of the configurations, it already happens to us when we add something to the manifest of some beat but we don't add it to the ones of other beats that could benefit from it.

One way to help on the migration while keeping the current set of manifests would be:

Add the leadership configuration to current DaemonSet.
Add replicas: 0 to current Deployment.
Add a comment in the leadership configuration explaining to comment out this provider and the replicas: 0 line in the Deployment if old behaviour is wanted.
Update docs that mention the Deployment.

Having the deployment with replicas: 0 sould allow to use the new manifest with existing deployments that use the manifests as we provide them. It will keep the Deployment, but it won't start any pod.

jsoriano · 2020-08-13T10:53:44Z

Or we can also add the leadership configuration to current manifests but commented out, and add a comment about deleting the deployment if this configuration is wanted.

jsoriano · 2020-08-13T11:00:14Z

Oh, another thought come to my mind that could affect the decision of deprecating the deployment manifest. How does leader election work when there are many candidates? In a cluster with thousands of nodes there will be thousands of candidates competing for the lease.

ChrsMark · 2020-08-13T12:24:33Z

Oh, another thought come to my mind that could affect the decision of deprecating the deployment manifest. How does leader election work when there are many candidates? In a cluster with thousands of nodes there will be thousands of candidates competing for the lease.

Hmm, good question but I don't think we can know about it if we don't try 🙂 . The thousands of leader candidates will try to get the lock and we rely on the locking mechanism of the leaderelection library for this. I don't see any difference if there are 3 or 3K candidates since all of them will try to acquire the lock from the API server. Having said this, I see an "overhead" on each candidate since it will have one extra routine to keep trying to obtain the lock and I guess that this will also add extra load to the k8s API server (spinlock behaviour requesting for the lock). But all these do not look really bad to me on cluster that carries a lot of workloads/operators-controllers etc.

Is this what you had in mind?

Signed-off-by: chrismark <[email protected]>

jsoriano · 2020-08-13T13:44:43Z

Oh, another thought come to my mind that could affect the decision of deprecating the deployment manifest. How does leader election work when there are many candidates? In a cluster with thousands of nodes there will be thousands of candidates competing for the lease.

Hmm, good question but I don't think we can know about it if we don't try slightly_smiling_face . The thousands of leader candidates will try to get the lock and we rely on the locking mechanism of the leaderelection library for this. I don't see any difference if there are 3 or 3K candidates since all of them will try to acquire the lock from the API server. Having said this, I see an "overhead" on each candidate since it will have one extra routine to keep trying to obtain the lock and I guess that this will also add extra load to the k8s API server (spinlock behaviour requesting for the lock). But all these do not look really bad to me on cluster that carries a lot of workloads/operators-controllers etc.

Is this what you had in mind?

Yes, this is what I was thinking, with 3K candidates, if a leader change is needed, 3K interactions with the API will be done more or less suddenly. But as you say in a cluster this size the API is probably sized accordingly.

jsoriano

Config LGTM, added some suggestions about the docs.

metricbeat/docs/running-on-kubernetes.asciidoc

Signed-off-by: chrismark <[email protected]>

…n_k8s_manifests

(cherry picked from commit 7b7fb3b)

masci

I'm sorry for the posthumous review here but I didn't expect we would merge right ahead.

I'm not sure I fully understand why we're keeping around the single-pod deployment. I know we need to keep backward compatibility but the setup for new users onboarding with 7.10 seems a bit clumsy, having to manually remove the deployment definition or (worse) set replicas to 0.

Can we discuss what's the use case that would impact backward compatibility if we remove the deployment?

masci · 2020-08-14T10:09:16Z

metricbeat/docs/running-on-kubernetes.asciidoc

+Users can enable the respective parts the Daemonset ConfigMap and
+set the `replicas` of the Deployment to `0` in order to only deploy
+the Daemonset on the cluster with the leader election provider enabled
+in order to collect cluster-wide metrics:


@ChrsMark Something doesn't work with the wording here, we should make this statement a bit clearer

masci · 2020-08-14T10:17:53Z

deploy/kubernetes/metricbeat-kubernetes.yaml

+        # Uncomment the following to enable leader election provider that handles
+        # singleton instance configuration across the Daemonset Pods of the whole cluster
+        # in order to monitor some unique data sources, like kube-state-metrics.
+        # When enabling this remember to also delete the Deployment or just set the replicas of the
+        # Deployment to 0.


This is too much wording IMO, if we do the docs right this should only be

# uncomment the following to enable collection from kube-state-metrics

👍 I will try to improve it

masci · 2020-08-14T10:19:05Z

deploy/kubernetes/metricbeat-kubernetes.yaml

+        #  scope: cluster
+        #  node: ${NODE_NAME}
+        #  unique: true
+        #  # identifier: <lease_lock_name>


This might be confusing: under which circumstances should I uncomment? What the value should be in that case?
I'd remove this line altogether.

👍 I will remove it

masci · 2020-08-14T10:20:13Z

deploy/kubernetes/metricbeat-kubernetes.yaml

@@ -258,6 +293,8 @@ metadata:
  labels:
    k8s-app: metricbeat
 spec:
+  # Set to 0 if using leader election provider with the Daemonset


What happens if we remove the Deployment instead? I don't think the UX is great here...

🤔 Removing the deployment completely will have the same result -> 0 Deployment Pods.

ChrsMark · 2020-08-14T10:41:32Z

I'm sorry for the posthumous review here but I didn't expect we would merge right ahead.

I'm not sure I fully understand why we're keeping around the single-pod deployment. I know we need to keep backward compatibility but the setup for new users onboarding with 7.10 seems a bit clumsy, having to manually remove the deployment definition or (worse) set replicas to 0.

Can we discuss what's the use case that would impact backward compatibility if we remove the deployment?

Thanks for the comments @masci! I would say that the current approach is smoother for every case. I don't think we should remove the Deployment completely by now since in many cases it can be really helpful when it comes to scaling where someone may want to collect cluster-wide metrics from big clusters.

In this regard, introducing the new unique provider as an optional-additional approach within the commented out section gives us the option to see it in action optionally by the users that actually want it.

ChrsMark · 2020-08-14T10:52:38Z

Let's continue the discussion at #20601

(cherry picked from commit 7b7fb3b)

Add k8s manifest leveraging leaderelection

3320d21

Signed-off-by: chrismark <[email protected]>

ChrsMark added enhancement in progress Pull request is currently in progress. needs_backport PR is waiting to be backported to other branches. [zube]: In Progress autodiscovery v7.10.0 labels Aug 10, 2020

ChrsMark self-assigned this Aug 10, 2020

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 10, 2020

ChrsMark added the Team:Platforms Label for the Integrations - Platforms team label Aug 10, 2020

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 10, 2020

ChrsMark mentioned this pull request Aug 10, 2020

Add leader election for autodiscover #20281

Merged

6 tasks

ChrsMark added [zube]: In Review needs_reviewer PR needs to be assigned a reviewer review labels Aug 13, 2020

ChrsMark added 2 commits August 13, 2020 12:51

fix config

4eb9519

Signed-off-by: chrismark <[email protected]>

Add docs

9b10a6c

Signed-off-by: chrismark <[email protected]>

ChrsMark requested review from exekias, masci and jsoriano August 13, 2020 10:13

ChrsMark added 2 commits August 13, 2020 15:35

Unify manifests

08fb001

Signed-off-by: chrismark <[email protected]>

Fix docs

1c1e97a

Signed-off-by: chrismark <[email protected]>

jsoriano reviewed Aug 13, 2020

View reviewed changes

metricbeat/docs/running-on-kubernetes.asciidoc Outdated Show resolved Hide resolved

metricbeat/docs/running-on-kubernetes.asciidoc Outdated Show resolved Hide resolved

review changes

fe70e8f

Signed-off-by: chrismark <[email protected]>

jsoriano approved these changes Aug 13, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into add_leaderelectio…

af70273

…n_k8s_manifests

ChrsMark merged commit 7b7fb3b into elastic:master Aug 14, 2020

ChrsMark added a commit to ChrsMark/beats that referenced this pull request Aug 14, 2020

Add k8s manifest leveraging leaderelection (elastic#20512)

5e83c53

(cherry picked from commit 7b7fb3b)

ChrsMark mentioned this pull request Aug 14, 2020

Cherry-pick #20512 to 7.x: Add k8s manifest leveraging leaderelection #20600

Merged

7 tasks

ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Aug 14, 2020

masci reviewed Aug 14, 2020

View reviewed changes

ChrsMark mentioned this pull request Aug 14, 2020

Improve docs of leaderelection configuration #20601

Merged

ChrsMark added a commit that referenced this pull request Aug 14, 2020

Add k8s manifest leveraging leaderelection (#20512) (#20600)

4710660

(cherry picked from commit 7b7fb3b)

ChrsMark mentioned this pull request Sep 2, 2020

Cherry-pick #20601 to 7.x: Improve docs of leaderelection configuration #20916

Merged

melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020

Add k8s manifest leveraging leaderelection (elastic#20512)

6d1ee21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add k8s manifest leveraging leaderelection #20512

Add k8s manifest leveraging leaderelection #20512

ChrsMark commented Aug 10, 2020 •

edited

Loading

elasticmachine commented Aug 10, 2020

elasticmachine commented Aug 10, 2020 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

ChrsMark commented Aug 13, 2020 •

edited

Loading

jsoriano commented Aug 13, 2020

jsoriano commented Aug 13, 2020

jsoriano commented Aug 13, 2020 •

edited

Loading

ChrsMark commented Aug 13, 2020

jsoriano commented Aug 13, 2020

jsoriano left a comment

masci left a comment

masci Aug 14, 2020

ChrsMark Aug 14, 2020

masci Aug 14, 2020

ChrsMark Aug 14, 2020

masci Aug 14, 2020

ChrsMark Aug 14, 2020

masci Aug 14, 2020

ChrsMark Aug 14, 2020

ChrsMark commented Aug 14, 2020

ChrsMark commented Aug 14, 2020

Add k8s manifest leveraging leaderelection #20512

Add k8s manifest leveraging leaderelection #20512

Conversation

ChrsMark commented Aug 10, 2020 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

elasticmachine commented Aug 10, 2020

elasticmachine commented Aug 10, 2020 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

ChrsMark commented Aug 13, 2020 • edited Loading

jsoriano commented Aug 13, 2020

jsoriano commented Aug 13, 2020

jsoriano commented Aug 13, 2020 • edited Loading

ChrsMark commented Aug 13, 2020

jsoriano commented Aug 13, 2020

jsoriano left a comment

Choose a reason for hiding this comment

masci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrsMark commented Aug 14, 2020

ChrsMark commented Aug 14, 2020

ChrsMark commented Aug 10, 2020 •

edited

Loading

elasticmachine commented Aug 10, 2020 •

edited by jenkins-beats-ci bot

Loading

ChrsMark commented Aug 13, 2020 •

edited

Loading

jsoriano commented Aug 13, 2020 •

edited

Loading