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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Feb 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
696d5c6
Fix container enricher with correct container ids for pods with multi…
MichaelKatsoulis Feb 8, 2023
8562596
Merge branch 'main' into fix-container-id-in-pod-with-multiple-contai…
MichaelKatsoulis Feb 8, 2023
cdc87ae
Align kubernetes.container.id with ecs container.id
MichaelKatsoulis Feb 8, 2023
600fa30
Update tests
MichaelKatsoulis Feb 8, 2023
bf865e7
Update changelog to reflect the bugfix
MichaelKatsoulis Feb 9, 2023
e916112
Update CHANGELOG.next.asciidoc
MichaelKatsoulis Feb 9, 2023
2018930
Merge branch 'main' into fix-container-id-in-pod-with-multiple-contai…
MichaelKatsoulis Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
250 changes: 125 additions & 125 deletions metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.4.2.expected
View file
Open in desktop

Large diffs are not rendered by default.

288 changes: 144 additions & 144 deletions metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.5.0.expected
View file
Open in desktop

Large diffs are not rendered by default.

286 changes: 143 additions & 143 deletions metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.6.0.expected
View file
Open in desktop

Large diffs are not rendered by default.

270 changes: 135 additions & 135 deletions metricbeat/module/kubernetes/state_container/_meta/test/ksm.v2.7.0.expected
View file
Open in desktop

Large diffs are not rendered by default.

230 changes: 115 additions & 115 deletions metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.4.2.plain-expected.json
View file
Open in desktop

Large diffs are not rendered by default.

328 changes: 164 additions & 164 deletions metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.5.0.plain-expected.json
View file
Open in desktop

Large diffs are not rendered by default.

318 changes: 159 additions & 159 deletions metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.6.0.plain-expected.json
View file
Open in desktop

Large diffs are not rendered by default.

254 changes: 127 additions & 127 deletions metricbeat/module/kubernetes/state_container/_meta/testdata/ksm.v2.7.0.plain-expected.json
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions metricbeat/module/kubernetes/state_container/state_container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,10 @@ func (m *MetricSet) Fetch(reporter mb.ReporterV2) error {
if split != -1 {
kubernetes.ShouldPut(containerFields, "runtime", cID[:split], m.Logger())

// Add splitted container.id ECS field and update kubernetes.container.id with splitted value
kubernetes.ShouldPut(containerFields, "id", cID[split+3:], m.Logger())
kubernetes.ShouldPut(event, "id", cID[split+3:], m.Logger())

}
}
if containerImage, ok := event["image"]; ok {
Expand Down
10 changes: 6 additions & 4 deletions metricbeat/module/kubernetes/util/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ func NewContainerMetadataEnricher(
if !ok {
base.Logger().Debugf("Error while casting event: %s", ok)
}
meta := metaGen.Generate(pod)
pmeta := metaGen.Generate(pod)

statuses := make(map[string]*kubernetes.PodContainerStatus)
mapStatuses := func(s []kubernetes.PodContainerStatus) {
Expand All @@ -293,6 +293,7 @@ func NewContainerMetadataEnricher(
podStore, _ := nodeStore.AddPodStore(podId)

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

@gizas gizas Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can reassure you it works because I tested it!

cmeta := mapstr.M{}
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

metrics := NewContainerMetrics()

if cpu, ok := container.Resources.Limits["cpu"]; ok {
Expand All @@ -314,14 +315,15 @@ func NewContainerMetadataEnricher(
// which is in the form of <container.runtime>://<container.id>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@gizas gizas Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Screenshot 2023-02-08 at 2 45 38 PM

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

Copy link
Contributor Author

@MichaelKatsoulis MichaelKatsoulis Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@gizas @ChrsMark fixed in cdc87ae

split := strings.Index(s.ContainerID, "://")
if split != -1 {
kubernetes2.ShouldPut(meta, "container.id", s.ContainerID[split+3:], base.Logger())
kubernetes2.ShouldPut(cmeta, "container.id", s.ContainerID[split+3:], base.Logger())

kubernetes2.ShouldPut(meta, "container.runtime", s.ContainerID[:split], base.Logger())
kubernetes2.ShouldPut(cmeta, "container.runtime", s.ContainerID[:split], base.Logger())
}
}

id := join(pod.GetObjectMeta().GetNamespace(), pod.GetObjectMeta().GetName(), container.Name)
m[id] = meta
cmeta.DeepUpdate(pmeta)
m[id] = cmeta
}
},
// delete
Expand Down