Fix container enricher with correct container ids for pods with multiple containers #34516

MichaelKatsoulis · 2023-02-08T09:34:31Z

What does this PR do?

This PR fixes a bug in NewContainerMetadataEnricher used by Kubernetes module's container metricset.
In case a pod consists of more than one container, the ecs field container.id was the same for all containers.
This was happening because the meta struct used for both containers was the same and the container.id value was getting overwritten.

Also this PR aligns kubernetes.container.id and container.id of state_container metricset.
So far kubernetes.container.id was not splitted and contained both the runtime and the id (containerd://99781780c8d2eaa7a0333e158c49969b20cdf067f7b00d8b93022b508ee09724)
On the other hand container.id was splitted to 99781780c8d2eaa7a0333e158c49969b20cdf067f7b00d8b93022b508ee09724 leading to confusion.
Now on it will be splitted in all cases.

Why is it important?

This was leading to errors and containers not found.
Issue https://github.com/elastic/observability-dev/issues/4012

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

To test this locally just create an elastic-stack and deploy default kubernetes metricbeat.
Deploy a pod in the k8s cluster with two containers and then in Kibana for that specific pod.name and container metricset, observe the container.ids of the different container names of that pod.

Issues

Closes Metricbeat on Kubernetes stores incorrect container.id for pods with multiple containers #34533

Screenshots

Before the fix:

After the fix:

Logs

…ple containers

mergify · 2023-02-08T09:35:25Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @MichaelKatsoulis? 🙏.
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

gizas · 2023-02-08T09:44:57Z

I think we also need a test for that, because we missed this case.
So a test with 2 containers and reporting names

…ners

gizas · 2023-02-08T09:48:12Z

metricbeat/module/kubernetes/util/kubernetes.go

@@ -314,14 +315,15 @@ func NewContainerMetadataEnricher(
 					// which is in the form of <container.runtime>://<container.id>


Do you think is time to allign here also with kube_state container.id where container.runtime is not splitted?

Hmm, that is another issue right? Good if we can fix them both.

That would be a breaking change. For state_container metricset as far as I see, the container.id is the same form.

Indeed. So it is time maybe to test here both container.ids and align them.

What does fixing mean? I prefer the splitted container.id

Check below the kubernetes.container.id (vs container.id)

I also prefer the splitted, so to remove it from kubernetes.container.id

@gizas @ChrsMark fixed in cdc87ae

ChrsMark

Good catch! Make sure you add the proper backports for it. Being a bugfix we need to backport it to the versions that the bug exists.

ChrsMark · 2023-02-08T09:47:43Z

metricbeat/module/kubernetes/util/kubernetes.go

@@ -293,6 +293,7 @@ func NewContainerMetadataEnricher(
 			podStore, _ := nodeStore.AddPodStore(podId)

 			for _, container := range append(pod.Spec.Containers, pod.Spec.InitContainers...) {
+				cmeta := mapstr.M{}


MichaelKatsoulis · 2023-02-08T09:51:13Z

Good catch! Make sure you add the proper backports for it. Being a bugfix we need to backport it to the versions that the bug exists.

The addition of container.id ecs fields was introduced in 8.1. So We need to backport till that version

gizas · 2023-02-08T09:56:19Z

metricbeat/module/kubernetes/util/kubernetes.go

@@ -293,6 +293,7 @@ func NewContainerMetadataEnricher(
 			podStore, _ := nodeStore.AddPodStore(podId)

 			for _, container := range append(pod.Spec.Containers, pod.Spec.InitContainers...) {


I know we have a for here, but just doublehcecking if we can make a test wit 3 containers. In case we miss sth and compare with kube_state.container dataset

I can reassure you it works because I tested it!

elasticmachine · 2023-02-08T10:01:04Z

💚 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

Start Time: 2023-02-09T08:47:53.907+0000
Duration: 52 min 24 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4279
Skipped	876
Total	5155

💚 Flaky test report

Tests succeeded.

🤖 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!)

MichaelKatsoulis · 2023-02-08T12:11:35Z

I think we also need a test for that, because we missed this case.
So a test with 2 containers and reporting names

We don't have metricset tests currently that test the enrichers. All the tests we have in the container metricset just test the eventMapping method.
There is one test of the buildMetadataEnricher but even there we pass mock update function and not the one used by NewContainerMetadataEnricher

gizas · 2023-02-08T12:49:51Z

We don't have metricset tests currently that test the enrichers. All the tests we have in the container metricset just test the eventMapping method. There is one test of the buildMetadataEnricher but even there we pass mock update function and not the one used by NewContainerMetadataEnricher

In order not to delay fix, please merge this. But make sure you add a new story for this in backlog

ChrsMark · 2023-02-09T00:34:34Z

I think we also need a test for that, because we missed this case.
So a test with 2 containers and reporting names

We don't have metricset tests currently that test the enrichers. All the tests we have in the container metricset just test the eventMapping method. There is one test of the buildMetadataEnricher but even there we pass mock update function and not the one used by NewContainerMetadataEnricher

Maybe we need to isolate the enricher's functionalities in smaller functions and that would make it easier for writing unit tests for them.

MichaelKatsoulis · 2023-02-09T07:45:17Z

Maybe we need to isolate the enricher's functionalities in smaller functions and that would make it easier for writing unit tests for them.

Yes we need to rethink the code to make it testable. I will open an issue for adding tests.

ChrsMark

I think we need a changelog here since it is a bugfix. Make sure you include both issues as you nicely do in the PR's description.

…ners

…ple containers (#34516) * Fix container enricher with correct container ids for pods with multiple containers * Align kubernetes.container.id with ecs container.id (cherry picked from commit 4ac4973) # Conflicts: # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.3.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.8.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.0.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.7.0.expected # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.4.2.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.5.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.6.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.7.0.plain-expected.json # metricbeat/module/kubernetes/state_container/state_container.go # metricbeat/module/kubernetes/util/kubernetes.go

…ple containers (#34516) * Fix container enricher with correct container ids for pods with multiple containers * Align kubernetes.container.id with ecs container.id (cherry picked from commit 4ac4973) # Conflicts: # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.3.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.8.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.0.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.7.0.expected # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.4.2.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.5.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.6.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.7.0.plain-expected.json

…ple containers (#34516) * Fix container enricher with correct container ids for pods with multiple containers * Align kubernetes.container.id with ecs container.id (cherry picked from commit 4ac4973)

…ple containers (#34516) (#34538) * Fix container enricher with correct container ids for pods with multiple containers * Align kubernetes.container.id with ecs container.id (cherry picked from commit 4ac4973) Co-authored-by: Michael Katsoulis <[email protected]>

…ids for pods with multiple containers (#34537) * Fix container enricher with correct container ids for pods with multiple containers (#34516) --------- Co-authored-by: Michael Katsoulis <[email protected]>

MichaelKatsoulis · 2023-02-10T12:34:36Z

@Mergifyio backport 8.2

mergify · 2023-02-10T12:35:18Z

backport 8.2

✅ Backports have been created

#34552 Fix container enricher with correct container ids for pods with multiple containers (backport #34516) has been created for branch 8.2

…ple containers (#34516) * Fix container enricher with correct container ids for pods with multiple containers * Align kubernetes.container.id with ecs container.id (cherry picked from commit 4ac4973) # Conflicts: # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.3.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.8.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.0.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.7.0.expected # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.4.2.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.5.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.6.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.7.0.plain-expected.json # metricbeat/module/kubernetes/state_container/state_container.go # metricbeat/module/kubernetes/util/kubernetes.go

MichaelKatsoulis · 2023-02-10T12:40:25Z

@Mergifyio backport 8.1

…ple containers (#34516) * Fix container enricher with correct container ids for pods with multiple containers * Align kubernetes.container.id with ecs container.id (cherry picked from commit 4ac4973) # Conflicts: # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.3.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v1.8.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.0.0.expected # metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.7.0.expected # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.4.2.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.5.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.6.0.plain-expected.json # metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.7.0.plain-expected.json # metricbeat/module/kubernetes/state_container/state_container.go # metricbeat/module/kubernetes/util/kubernetes.go

mergify · 2023-02-10T12:40:45Z

backport 8.1

✅ Backports have been created

#34553 Fix container enricher with correct container ids for pods with multiple containers (backport #34516) has been created for branch 8.1

…ids for pods with multiple containers (#34536) * Fix container enricher with correct container ids for pods with multiple containers (#34516) --------- Co-authored-by: Michael Katsoulis <[email protected]>

…ids for pods with multiple containers (#34535) * Fix container enricher with correct container ids for pods with multiple containers (#34516) --------- Co-authored-by: Michael Katsoulis <[email protected]>

…ids for pods with multiple containers (#34534) * Fix container enricher with correct container ids for pods with multiple containers (#34516) --------- Co-authored-by: Michael Katsoulis <[email protected]>

…ple containers (#34516) * Fix container enricher with correct container ids for pods with multiple containers * Align kubernetes.container.id with ecs container.id

Fix container enricher with correct container ids for pods with multi…

696d5c6

…ple containers

MichaelKatsoulis added the bug label Feb 8, 2023

MichaelKatsoulis requested a review from a team as a code owner February 8, 2023 09:34

MichaelKatsoulis requested review from gsantoro and constanca-m and removed request for a team February 8, 2023 09:34

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 8, 2023

MichaelKatsoulis requested a review from ChrsMark February 8, 2023 09:34

MichaelKatsoulis added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Feb 8, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 8, 2023

mergify bot assigned MichaelKatsoulis Feb 8, 2023

Merge branch 'main' into fix-container-id-in-pod-with-multiple-contai…

8562596

…ners

gizas reviewed Feb 8, 2023

View reviewed changes

ChrsMark approved these changes Feb 8, 2023

View reviewed changes

gizas reviewed Feb 8, 2023

View reviewed changes

MichaelKatsoulis added 2 commits February 8, 2023 15:49

Align kubernetes.container.id with ecs container.id

cdc87ae

Update tests

600fa30

ChrsMark approved these changes Feb 9, 2023

View reviewed changes

Update changelog to reflect the bugfix

bf865e7

gizas approved these changes Feb 9, 2023

View reviewed changes

MichaelKatsoulis added backport-v8.3.0 Automated backport with mergify backport-v8.1.0 Automated backport with mergify labels Feb 9, 2023

MichaelKatsoulis added backport-v8.6.0 Automated backport with mergify backport-v8.7.0 Automated backport with mergify labels Feb 9, 2023

MichaelKatsoulis added 2 commits February 9, 2023 10:43

Update CHANGELOG.next.asciidoc

e916112

Merge branch 'main' into fix-container-id-in-pod-with-multiple-contai…

2018930

…ners

MichaelKatsoulis merged commit 4ac4973 into elastic:main Feb 9, 2023

mergify bot mentioned this pull request Feb 9, 2023

[8.3](backport #34516) Fix container enricher with correct container ids for pods with multiple containers #34534

Merged

mergify bot mentioned this pull request Feb 9, 2023

[8.4](backport #34516) Fix container enricher with correct container ids for pods with multiple containers #34535

Merged

mergify bot mentioned this pull request Feb 9, 2023

[8.5](backport #34516) Fix container enricher with correct container ids for pods with multiple containers #34536

Merged

mergify bot mentioned this pull request Feb 9, 2023

[8.6](backport #34516) Fix container enricher with correct container ids for pods with multiple containers #34537

Merged

mergify bot mentioned this pull request Feb 9, 2023

[8.7](backport #34516) Fix container enricher with correct container ids for pods with multiple containers #34538

Merged

mergify bot mentioned this pull request Feb 10, 2023

[8.2](backport #34516) Fix container enricher with correct container ids for pods with multiple containers #34552

Closed

mergify bot mentioned this pull request Feb 10, 2023

[8.1](backport #34516) Fix container enricher with correct container ids for pods with multiple containers #34553

Closed

gizas mentioned this pull request Aug 27, 2024

[Kubernetes]Overview dashboard - total cpu and memory usage are incorrect elastic/integrations#10887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix container enricher with correct container ids for pods with multiple containers #34516

Fix container enricher with correct container ids for pods with multiple containers #34516

MichaelKatsoulis commented Feb 8, 2023 •

edited

Loading

mergify bot commented Feb 8, 2023

gizas commented Feb 8, 2023

gizas Feb 8, 2023

ChrsMark Feb 8, 2023

MichaelKatsoulis Feb 8, 2023

gizas Feb 8, 2023 •

edited

Loading

MichaelKatsoulis Feb 8, 2023

gizas Feb 8, 2023

MichaelKatsoulis Feb 8, 2023 •

edited

Loading

ChrsMark left a comment

ChrsMark Feb 8, 2023

MichaelKatsoulis commented Feb 8, 2023

gizas Feb 8, 2023 •

edited

Loading

MichaelKatsoulis Feb 8, 2023

elasticmachine commented Feb 8, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

MichaelKatsoulis commented Feb 8, 2023

gizas commented Feb 8, 2023

ChrsMark commented Feb 9, 2023

MichaelKatsoulis commented Feb 9, 2023

ChrsMark left a comment

MichaelKatsoulis commented Feb 10, 2023

mergify bot commented Feb 10, 2023

MichaelKatsoulis commented Feb 10, 2023

mergify bot commented Feb 10, 2023

		@@ -314,14 +315,15 @@ func NewContainerMetadataEnricher(
		// which is in the form of <container.runtime>://<container.id>

		@@ -293,6 +293,7 @@ func NewContainerMetadataEnricher(
		podStore, _ := nodeStore.AddPodStore(podId)

		for _, container := range append(pod.Spec.Containers, pod.Spec.InitContainers...) {

Fix container enricher with correct container ids for pods with multiple containers #34516

Fix container enricher with correct container ids for pods with multiple containers #34516

Conversation

MichaelKatsoulis commented Feb 8, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Issues

Screenshots

Logs

mergify bot commented Feb 8, 2023

gizas commented Feb 8, 2023

gizas Feb 8, 2023

Choose a reason for hiding this comment

ChrsMark Feb 8, 2023

Choose a reason for hiding this comment

MichaelKatsoulis Feb 8, 2023

Choose a reason for hiding this comment

gizas Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

MichaelKatsoulis Feb 8, 2023

Choose a reason for hiding this comment

gizas Feb 8, 2023

Choose a reason for hiding this comment

MichaelKatsoulis Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

ChrsMark left a comment

Choose a reason for hiding this comment

ChrsMark Feb 8, 2023

Choose a reason for hiding this comment

MichaelKatsoulis commented Feb 8, 2023

gizas Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

MichaelKatsoulis Feb 8, 2023

Choose a reason for hiding this comment

elasticmachine commented Feb 8, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

MichaelKatsoulis commented Feb 8, 2023

gizas commented Feb 8, 2023

ChrsMark commented Feb 9, 2023

MichaelKatsoulis commented Feb 9, 2023

ChrsMark left a comment

Choose a reason for hiding this comment

MichaelKatsoulis commented Feb 10, 2023

mergify bot commented Feb 10, 2023

✅ Backports have been created

MichaelKatsoulis commented Feb 10, 2023

mergify bot commented Feb 10, 2023

✅ Backports have been created

MichaelKatsoulis commented Feb 8, 2023 •

edited

Loading

gizas Feb 8, 2023 •

edited

Loading

MichaelKatsoulis Feb 8, 2023 •

edited

Loading

gizas Feb 8, 2023 •

edited

Loading

elasticmachine commented Feb 8, 2023 •

edited by jenkins-beats-ci bot

Loading