kubelet: CAdvisor sometimes injects old Pod metrics from an old namespaces

[rphillips]

What type of PR is this?
/kind bug

What this PR does / why we need it:
The CAdvisor stats backend has a bug where it does not reflect pods terminating in an initial namespace and then being created in a new namespace. This is due to cinfoID.podRef.UID being used within the map key {PodName, PodNamespace, PodUID}. This tuple for the map key will erroneously inject a newly created pod in a different runtime namespace (UID) and the terminated pod in the initial namespace into the list causing duplicate metrics to be returned.

Which issue(s) this PR fixes:
500 errors in /metrics endpoint
https://bugzilla.redhat.com/show_bug.cgi?id=1748073

Special notes for your reviewer:
/cc @kubernetes/sig-node-pr-reviews
/cc @Random-Liu @derekwaynecarr @sjenning

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

[derekwaynecarr]

so buildPodRef will have set the podRef.UID , we then strip it, wont every result in infos in ListPodCPUAndMemoryStats have an empty UID?

[derekwaynecarr]

which if i look at this code, makes me think the pod ref in pod stats will always have an empty uid.

[rphillips]

this is ok since removeTerminatedContainerInfo only sets up the slice to iterate around. This line iterates around the active containers, and initializes podToStats which contains the correct UID to the container namespace.

kubernetes/pkg/kubelet/stats/cadvisor_stats_provider.go

Line 93 in c677061

for key, cinfo := range infos {

[derekwaynecarr]

ah ok, i missed that.

[derekwaynecarr]

ah missed that! thanks for clarifiying.

[sjenning]

/lgtm

[derekwaynecarr]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, rphillips

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/kubelet/stats/OWNERS [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sjenning]

fyi @dashpole

[dashpole]

Why does creating a container in a new runtime namespace change the pod UID?

[rphillips]

@dashpole that isn't the pod uid... it is the container namespace UID.

[rphillips]

Same logic that is in the cri stats provider:

kubernetes/pkg/kubelet/stats/cri_stats_provider.go

Line 712 in c677061

// UID is intentionally left empty.

[dashpole]

@rphillips I can't seem to figure out why that would be the case... I sshed into a node in a running cluster, and verified with docker inspect that the label io.kubernetes.pod.uid contains the pod UID. When we create container labels in kuberuntime, we use pod.UID for that label, and we are retrieving labels using GetPodUID, which inspects that same label. In what case would it contain the namespace UID?

/hold
Bear with me while we figure this out.

[rphillips]

No problem. Looking into this.

note: we are running crio.