search

google / cadvisor

Analyzes resource usage and performance characteristics of running containers.
Other
17.06k stars 2.32k forks source link

Why don't we use container ID directly #3280

Open tomatopunk opened 1 year ago

tomatopunk commented 1 year ago

Hi folks:

Although we can already obtain the container id by using the full container name, why isn't it exposed?

https://github.com/google/cadvisor/blob/3888dda254a8fa12f1c96be24ffdc33288ea9071/container/docker/factory.go#L173-L181

Why don't we use container ID directly, and instead use the container name as the label ID value in DefaultContainerLabels?

https://github.com/google/cadvisor/blob/d91f2e682a1dba71a5f917131d23da3ef37c2a62/metrics/prometheus.go#L1769-L1784

The problem with this is that we cannot map back to Kubernetes information using the container ID

SergeyKanzhelev commented 1 year ago

is it referring to https://github.com/google/cadvisor/pull/2931? When we use cadvisor from k8s we in fact fas this problem, I sent a PR documenting it couple days back https://github.com/kubernetes/website/pull/40192. I think some users depend on the current default labels format.

I understand you can build a version of cadisor that changes it: https://github.com/google/cadvisor/pull/1429/

tomatopunk commented 1 year ago

is it referring to #2931? When we use cadvisor from k8s we in fact fas this problem, I sent a PR documenting it couple days back kubernetes/website#40192. I think some users depend on the current default labels format.

I understand you can build a version of cadisor that changes it: #1429

Actually, what I don't understand is why we need to rewrite the container name as id when outputting it.

In the container handler, we have already obtained the correct ID through regular expressions without exposing it.

https://github.com/google/cadvisor/blob/3888dda254a8fa12f1c96be24ffdc33288ea9071/container/docker/factory.go https://github.com/google/cadvisor/blob/3888dda254a8fa12f1c96be24ffdc33288ea9071/container/containerd/factory.go

The reason why the container ID is very important is that when we call the Kubernetes API to obtain the container status of a pod, the "terminated" state only exposes the container ID.

https://github.com/kubernetes/api/blob/e3f1032953d3776ef5da09db363f00babf681791/core/v1/types.go#L2625

Regarding issue #1429, I believe that it cannot expose the real container ID. Here, it can only expose the configured label, and "container.id" is not a label. It should be at the same level as "container.name".

https://github.com/google/cadvisor/blob/d91f2e682a1dba71a5f917131d23da3ef37c2a62/metrics/prometheus.go#L1788-L1812

If possible, I can create a pull request to expose the container_id in BaseContainerLabels.