[Metricbeat][Kubernetes] Share watchers between metricsets

[constanca-m]

Proposed commit message

• WHAT: metricsets share watchers between each other, instead of each having their own.
• WHY: Please check the issue [Metricbeat][Kubernetes] Refactor watchers used in Kubernetes metricsets for metadata enrichment #37243.

Details

The needed watchers for each resource are defined in getExtraWatchers function. You can check which are required in the table of section Expected watchers in this issue.

Note: only state_resourcequota does not have the expected watchers with this change. This is because we need to change the implementation of that metricset first.

We have a global map that saves all the watchers:

type watchers struct {
	watchersMap map[string]*watcherData
	lock        sync.RWMutex
}

The key to this map is the resource name, and the values are defined as:

type watcherData struct {
	metricsetsUsing []string // list of metricsets using this watcher

	watcher kubernetes.Watcher
	started bool // true if watcher has started, false otherwise

	enrichers map[string]*enricher // map of enrichers using this watcher. The key is the metricset name

	metadataObjects map[string]bool // map of ids of each object received by the handler functions
}

• metricsetsUsing contains the list of metricsets that are using this watcher. We need this because when the enricher calls Start() or Stop(), the watchers start/stop. We cannot start a watcher more than once. We only stop a watcher if the list of metricsets using it is empty. We use metricset to avoid conflicts between metricsets that use the same resource, like state_pod and pod.
• watcher is the kubernetes watcher for the resource.
• started just tells us if the watcher is started. This is mainly needed for the enricher.Start() and for testing purposes.
• enrichers is the list of enrichers for this watcher per each metricset
• metadataEvents is the resulted metadata events from the resource event handler. Please see the next list, point 6.2, why this is necessary

The algorithm goes like this when NewResourceMetadataEnricher is called:

1. The configuration is validated. It will return a nil enricher if it fails.
2. We create the configuration needed for the metadata generator. It will return a nil enricher if it fails.
3. We create the K8s client. It will return a nil enricher if it fails.
4. We start all the watchers:
   1. We first check if the resource exists. If it fails, we stop.
   2. We build the kubernetes.WatchOptions{} needed for the watcher. If it fails, we stop.
   3. We start the watcher for this specific resource:
      1. We first check if the watcher is already created.
      2. If it is, then we don't do anything.
      3. Otherwise, we create a new watcher and put it in the map with key = resource name.
      4. We add this metricset to the list of metricsets we have that are using this watcher.
   4. We get all needed extra resources for this resource, and repeat step 3.
5. We create the metadata generators.
6. Lastly, create the enricher.
   Considerations:
   1. Because each watcher only has one function for UpdateFunc / addFunc and DeleteFunc, we need to save which metricsets and respective enrichers need that handler function. For this, we keep track of the enrichers using a map, and iterate over that map when one of the functions is triggered.
   2. It is possible that AddFunc is called for one metricset first, and when the other metricset starts, the AddFunc is no longer triggered. To avoid the loss of metadata, we have the map metadataObjects, that saves the id of the object that triggered the handler function. This way, for each enricher upon creation, we iterate over all this map and using the id saved there, we get the object from the watcher store. Using this object, we call the update function and ensure all enrichers have up to date metadata.

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

1. Clone this branch.
2. Follow the steps of this README file to launch metricbeat with the changes.
3. Check it is working as expected.

Related issues

• Relates to [Metricbeat][Kubernetes] Refactor watchers used in Kubernetes metricsets for metadata enrichment #37243.

Results

Metricbeat

These results come from running metricbeat with this configuration for kubernetes module (all metricsets that launch watchers are enabled, the others are not).

metricbeat.autodiscover:
  providers:
    - type: kubernetes
      scope: cluster
      node: ${NODE_NAME}
      unique: true
      templates:
        - config:
            - module: kubernetes
              hosts: ["kube-state-metrics:8080"]
              period: 10s
            #  #add_metadata: true
              metricsets:
                - state_node
                - state_deployment
                - state_daemonset
                - state_replicaset
                - state_pod
                - state_container
                - state_cronjob
                - state_job
                #- state_resourcequota
                - state_statefulset
                - state_service
                - state_persistentvolume
                - state_persistentvolumeclaim
                - state_storageclass
                - state_namespace
            - module: kubernetes
              metricsets:
                - node
                - pod
                - container
              period: 10s
              host: ${NODE_NAME}
              hosts: ["https://${NODE_NAME}:10250"]
              bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
              ssl.verification_mode: "none"

The logs for the watchers initialization will look like this (only message field displayed for simplicity):

{"log.level":"debug","@timestamp":"2024-02-08T08:03:02.094Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher node successfully, created by node.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:02.116Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher pod successfully, created by pod.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:02.116Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":368},"message":"Created watcher state_namespace successfully, created by pod.","service.name":"metricbeat","ecs.version":"1.6.0"}                <-------------------------------------
{"log.level":"debug","@timestamp":"2024-02-08T08:03:02.191Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher deployment successfully, created by state_deployment.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:02.206Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher daemonset successfully, created by state_daemonset.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:02.516Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher replicaset successfully, created by state_replicaset.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:03.118Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher cronjob successfully, created by state_cronjob.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:03.126Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher statefulset successfully, created by state_statefulset.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:03.132Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher service successfully, created by state_service.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:03.139Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher persistentvolume successfully, created by state_persistentvolume.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:03.146Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher persistentvolumeclaim successfully, created by state_persistentvolumeclaim.","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2024-02-08T08:03:03.152Z","log.logger":"kubernetes","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.createAllWatchers","file.name":"util/kubernetes.go","file.line":354},"message":"Created watcher storageclass successfully, created by state_storageclass.","service.name":"metricbeat","ecs.version":"1.6.0"}

Notice the line with <----------: the pod was the resource that created the watcher for namespace, since it is one of the required resources, and it did not exist yet. This is also the reason why we don't see the line "message":"Created watcher state_namespace successfully, created by state_namespace.", because by the time state_namespace is iterating over the needed watchers, they are already created.

In Discover:

Elastic Agent

These results come from running EA with this standalone manifest, but with the custom image.

Logs:

These are the logs for starting the watchers (working as expected).

{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.051Z","message":"Started watcher statefulset successfully, created by statefulset.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"ecs.version":"1.6.0","log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.051Z","message":"Started watcher state_namespace successfully, created by statefulset.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"log.logger":"kubernetes","log.origin":{"file.line":321,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.257Z","message":"Started watcher node successfully, created by node.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.361Z","message":"Started watcher pod successfully, created by pod.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"ecs.version":"1.6.0","log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.470Z","message":"Started watcher deployment successfully, created by deployment.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.577Z","message":"Started watcher persistentvolume successfully, created by persistentvolume.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"ecs.version":"1.6.0","log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.679Z","message":"Started watcher persistentvolumeclaim successfully, created by persistentvolumeclaim.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.782Z","message":"Started watcher replicaset successfully, created by replicaset.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:48.887Z","message":"Started watcher service successfully, created by service.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"service.name":"metricbeat","ecs.version":"1.6.0","log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:49.017Z","message":"Started watcher storageclass successfully, created by storageclass.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:49.120Z","message":"Started watcher cronjob successfully, created by cronjob.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:49.225Z","message":"Started watcher daemonset successfully, created by daemonset.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"service.name":"metricbeat","ecs.version":"1.6.0","log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"ecs.version":"1.6.0"}
...
{"log.level":"debug","@timestamp":"2023-12-08T10:17:49.329Z","message":"Started watcher job successfully, created by job.","component":{"binary":"metricbeat","dataset":"elastic_agent.metricbeat","id":"kubernetes/metrics-default","type":"kubernetes/metrics"},"log":{"source":"kubernetes/metrics-default"},"ecs.version":"1.6.0","log.logger":"kubernetes","log.origin":{"file.line":307,"file.name":"util/kubernetes.go","function":"github.com/elastic/beats/v7/metricbeat/module/kubernetes/util.startAllWatchers"},"service.name":"metricbeat","ecs.version":"1.6.0"}

The results for the dashboards are (check if it still works):

• [Metrics Kubernetes] Cronjobs
• [Metrics Kubernetes] StatefulSets
• [Metrics Kubernetes] Pods
• [Metrics Kubernetes] Deployments
• [Metrics Kubernetes] DaemonSets
• [Metrics Kubernetes] Jobs
• [Metrics Kubernetes] Nodes
• [Metrics Kubernetes] PV/PVC
• [Metrics Kubernetes] Cluster Overview
• [Metrics Kubernetes] Services - It is broken, but it is not related with the changes on this PR.

Note: only dashboards for resources that launch watchers are considered. There were no changes in the others.

Notes for testing

Important things to consider when testing this PR code changes:

• This PR changes only affect the kubernetes module metricsets that use metadata enrichment. These are state_namespace state_node state_deployment state_daemonset state_replicaset state_pod state_container state_job state_cronjob state_statefulset state_service state_persistentvolume state_persistentvolumeclaim state_storageclass pod container node
• Everything that was working before this PR changes should still be working. The changes only reduce the number of watchers created from the different metricsets, thus reducing the k8s API calls.
• Thorough regression testing is needed. In more details:
  a. All events coming from the affected metricsets in Kibana should be enriched with own resource metadata (labels, annotations) and kubernetes node metadata and kubernetes namespace metadata when applicable.
  b. When a new metadata(like a new label) is added on a resource(i.e. pod) then the new events from the related metricset(pod, container, state_pod, state_container) should contain the new metadata
  c. When a new node or namespace label and annotation is added to a node/namespace, then the events from relevant metricsets(state_node, node or state_namespace) should include the new metadata.
  d. The events of the rest of the metricsets(i.e. state_pod or state_deployment) coming from resources in the updated node/namespace won't get the updated node or namespace metadata out of the box.
  e. In order for those events to be updated, there should be first an update in the metadata of these resources. For example if a node is labeled then the pods of that node won't get the new node label immediately. In order to get it, we should also add a label to these pod to trigger a watcher event. Then the new events will include the new pod label and node label
  f. Test with addition/removal of metadata on pods that run on the leader node and also on the non-leader nodes.

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @constanca-m? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

[elasticmachine]

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-12-07T09:16:30.968+0000

• Duration: 8 min 49 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[elasticmachine]

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Duration: 22 min 52 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[elasticmachine]

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Duration: 18 min 18 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Duration: 50 min 17 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[gizas]

Also as a general comment all this functions I think can move to util folder and keep same structure (as we have util.NewResourceMetadataEnricher), because now we go back and forth

[constanca-m]

You mean in a different file inside util? They are in the util folder right now.

[gizas]

• This PR will also need testing with Agent for sure. We need to build the agent and repeat the same tests and see if we dont break anything.

• Also I would need to run some E2E tests to see that metadata enrichemnt are ok.

• We will need to decide what a configuration like eg add_resource_metadata.namespace.enabled: false will do in our case

[MichaelKatsoulis]

Without having checked the code line by line, I believe that this approach is not aligned with kubeStateMetricsCache and kubeletStatsCache approach where we try to solve a similar issue.

beats/metricbeat/module/kubernetes/kubernetes.go

Line 42 in 609817b

GetStateMetricsFamilies(prometheus p.Prometheus) ([]*p.MetricFamily, error)

The watches need to be set in a module level. We need to take into account elastic-agent where each metricset starts as a new kubernetes module. So when we need to share stuff between metricsets of kubernetes module, we need to define a global cache on Module's level shared across all Module's instances.
See @ChrsMark implementation in #25640 (comment)

[constanca-m]

We will need to decide what a configuration like eg add_resource_metadata.namespace.enabled: false will do in our case

I think maybe we should move those new decisions to a new PR @gizas

[constanca-m]

Without having checked the code line by line, I believe that this approach is not aligned with kubeStateMetricsCache and kubeletStatsCache approach where we try to solve a similar issue.

I think this approach still works this way and does basically the same thing.

It is harder to use util/kubernetes with the kubernetes in the parent folder, because we would have the import cycle error in go at all times. The only workaround i could find for that, would be to pass the funcitons as parameters, but it is very hard to read the code that way.

I added unit tests for every function, and they work just fine. @MichaelKatsoulis

Edit: It is the same approach we are already using for state_metricset shared map.

[MichaelKatsoulis]

It is harder to use util/kubernetes with the kubernetes in the parent folder, because we would have the import cycle error in go at all times

You need to define a watchersCache in utils like we do with MetricsRepo

beats/metricbeat/module/kubernetes/kubernetes.go

Line 88 in a8d1567

metricsRepo *util.MetricsRepo

and

beats/metricbeat/module/kubernetes/util/metrics_repo.go

Line 71 in a8d1567

type MetricsRepo struct {

Then its pointer can be passed to NewResourceMetadataEnricher like metricsRepo.

Did you test how many watchers are created under the hood with e2e tests? Did you test elastic-agent and metricbeat?

[constanca-m]

Did you test how many watchers are created under the hood with e2e tests? Did you test elastic-agent and metricbeat?

Yes, the number of watchers are correct. I posted the results in Results in the description from running metricbeat. I also added the unit tests. EA was also tested, results are now in the description. @MichaelKatsoulis

Any test in specific I should do? Any specific situation?

[elasticmachine]

💔 Build Failed

• Buildkite Build
• Commit: a7f709a

Failed CI Steps

• :windows:-family/core-windows-2016 Unit Tests
• :windows:-family/core-windows-2022 Unit Tests

History

• 💚 Build #3203 succeeded 8fcdfc2
• 💔 Build #3105 failed 64ac188
• 💔 Build #3104 failed b51c873
• 💔 Build #3103 failed 74d51e2

cc @constanca-m

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: a7f709a

History

• 💚 Build #1567 succeeded 8fcdfc2
• 💚 Build #1460 succeeded 64ac188
• 💔 Build #1457 failed a888b88
• 💔 Build #1456 failed 519e3fb
• 💚 Build #1389 succeeded 57135c9
• 💚 Build #1359 succeeded 15b673a

cc @constanca-m

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: a7f709a

History

• 💚 Build #1575 succeeded 8fcdfc2
• 💔 Build #1457 failed 64ac188
• 💔 Build #1454 failed a888b88
• 💔 Build #1453 failed 519e3fb
• 💚 Build #1382 succeeded 57135c9
• 💚 Build #1352 succeeded 15b673a

cc @constanca-m

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: a7f709a

History

• 💚 Build #731 succeeded 8fcdfc2
• 💔 Build #624 failed 64ac188
• 💔 Build #621 failed a888b88
• 💔 Build #620 failed 519e3fb
• 💚 Build #553 succeeded 57135c9
• 💚 Build #523 succeeded 15b673a

cc @constanca-m

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: a7f709a

History

• 💚 Build #2782 succeeded 8fcdfc2
• 💔 Build #2674 failed 64ac188
• 💔 Build #2671 failed a888b88
• 💔 Build #2670 failed 519e3fb
• 💔 Build #2602 failed 57135c9
• 💚 Build #2572 succeeded 15b673a

cc @constanca-m

[tetianakravchenko]

regarding the documentation:

• I think it makes more sense to keep structs and function parameters details as comments in the code, otherwise there is high chances this doc will be not kept in sync with code changes. I believe it would be helpful to keep implementation details in the code, and enrichers.md for more high level overview
• could the documentation be restructured to have sub-topics? it is hard to read 100+ lines of text as one piece
• could maybe be added a high level architecture diagram? It would simplify the documentation a lot

[tetianakravchenko]

why not only?

Suggested change

	-The Kubernetes autodiscover provider enriches with metadata mainly when it comes to log collection when it is configured. It is by default in the `container_logs` integration in the Elastic Agent.
	-The Kubernetes autodiscover provider enriches with metadata only when it comes to log collection when it is configured. It is by default in the `container_logs` integration in the Elastic Agent.

maybe instead of referring to different project (Elastic Agent) - add link to the filebeat https://github.com/elastic/beats/blob/main/deploy/kubernetes/filebeat-kubernetes.yaml#L133-L150?

[MichaelKatsoulis]

The kubernetes provider is also used with templates to start a metricbeat module (https://www.elastic.co/guide/en/beats/metricbeat/current/configuration-autodiscover.html#configuration-autodiscover) and enrich the metrics with kubernetes metadata

[tetianakravchenko]

I would move such detailed explanation on the metaWatcher struct to the code, otherwise there is high chances this doc will be not kept in sync with code changes

[constanca-m]

These details of the variables are already in the code like this:

	watcher kubernetes.Watcher
	started bool // true if watcher has started, false otherwise

	metricsetsUsing []string // list of metricsets using this watcher

	enrichers       map[string]*enricher // map of enrichers using this watcher. The key is the metricset name
	metadataObjects map[string]bool      // map of ids of each object received by the handler functions

	nodeScope      bool               // whether this watcher is only for current node
	restartWatcher kubernetes.Watcher // whether this watcher needs a restart

[tetianakravchenko]

can maybe this list be structured somehow with sub-titles? for example metadata generation can be split to another list?

[tetianakravchenko]

can we add comments regarding this struct and the struct fields here, instead of in documentation - to keep the implementation details in code and the high level overview in the https://github.com/elastic/beats/blob/a7f709a153a66bdb5369128342bd21c36da6439e/metricbeat/module/kubernetes/util/enrichers.md instead ?

[tetianakravchenko]

In that case, we need to check if we are trying to create a new resourceMetaWatcher that will track the resources of multiple nodes.

it is not the multiple nodes, but rather the whole cluster/clusterScope, right?
Such behavior is expected only on the leader node, right?
what will happen in case of the re-election, is it needed in that case to switch from clusterScope to nodeScope (now this condition would only work for the nodeScope: true)?

[MichaelKatsoulis]

it is not the multiple nodes, but rather the whole cluster/clusterScope, right?

You are right

what will happen in case of the re-election, is it needed in that case to switch from clusterScope to nodeScope

Under what conditions the re-election happens ? I think it only happens if an agent is down/restarted, so in that case the whole metricsets initialisation will take place again and the ones with clusterscope will not start at this node.

[tetianakravchenko]

almost all the links are from the private repo - https://github.com/constanca-m/beats, that can be outdated quite soon or removed when the feature branch will be merged in

[tetianakravchenko]

First pod metricsets starts and then state_pod
does it always happen like this? or it is an example here?

[MichaelKatsoulis]

It always happens. The reason is that for the state_* metricsets to start , the leader election needs to take place first which takes sometime.

[tetianakravchenko]

In that case the pod enricher's updateFunc

this sentence is not finished, no?

[tetianakravchenko]

is it an explanation of the restartWatcher ? this seems to be not finished

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-multiple-watchers upstream/fix-multiple-watchers
git merge upstream/main
git push upstream fix-multiple-watchers

[constanca-m]

I think all points up until this one are not necessary in this file, and they might add confusion. We just need to say the enrichers are created in the metricset struct. We talk a lot about the kubernetes.Init, and I think that code might change in the future, and then this file won't be updated. Can we remove all these points and just add a sentence explaining where we start enrichers? @MichaelKatsoulis

[blakerouse]

The inverse of this function addToMetricsetsUsing grabs the lock and this function doesn't. That inconsistency is not clear. I would prefer to see that consistent between the two functions.

[constanca-m]

At the time we call this function, we are already holding the lock. That is why it is not consistent. I will add a comment to the addToMetricsetsUsing mentioning the lock

[blakerouse]

Thanks for the additional comment and cleanup of the go.mod file.