Add restart and container state metrics to kubelet

[schleyfox]

What does this PR do?

This PR adds some metrics that would normally come from kube-state-metrics to the kubelet check. We (airbnb) were having problems getting kube-state-metrics integration to work on our very large clusters. The specific metrics that we want are available in the kubelet podlist and are similar to other metrics that are already being collected here.

• Reports kubernetes.containers.restarts from the restartCount of the container status
• Reports kubernetes.containers.{state,last_state}.{running,waiting,terminated} with reasons from the container status fields of the same name. The running metric is pretty redundant and included only for completeness.

Motivation

We were running into issues similar to #1346 (comment) . The response size of the metrics endpoint on kube-state-metrics was too large to submit through DataDog. Sharding by namespace was not possible for our use case because we use a large number of dynamic and variably sized namespaces. We are most interested in being able to detect containers in erroneous or crashloop states.

Review checklist

• PR has a meaningful title or PR has the no-changelog label attached
• Feature or bugfix has tests
• Git history is clean
• If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

Anything else we should know when reviewing?

[xvello]

Thanks for your contribution, I'll give it a round of testing tomorrow.
For your information, we're freezing 6.8.0 on Thursday 29th, if that works for you, I'd be happy to work on getting this merged before.

Other than my suggestion to re-scope the restarts metric, you'll need to add metadata for these new gauges. Here is the documentation for it.

[xvello]

I think the .containers.restarts gauge has legit use cases on a per-pod basis, instead of aggregated per deployment/image. What do you think about moving it to _report_container_state_metrics?

I would tag the metric by high-cardinality pod tags (calling tags_for_pod(pod_id, True)) + the kube_container_name tag (and instance tags of course)

[schleyfox]

That was definitely my goal (per pod/container high cardinality), but I don't really understand how to figure out which tags are available in which contexts from get_tags. I was basing this on observing that the container spec metrics which get tags via

integrations-core/kubelet/datadog_checks/kubelet/kubelet.py

Line 331 in 407294e

tags = get_tags('%s' % cid, True) + instance_tags

seem to have tags for kube_pod_name, kube_container_name, image_id, etc.

Separate from whether the tags are correct, moving this to _report_container_state_metrics could be better from a code organization standpoint.

[xvello]

In this loop, container tags are retrieved in low cardinality (tags = get_tags(container_id, False)). Low cardinality excludes tags like container_name, container_id, pod_name that churns when a pod would be re-created with the same spec. You would basically get the same tags as in AutoDiscovery (kube_deployment, kube_container_name, kube_namespace, docker_image...).

What we want here is to be able to identify what exact pod might be restarting, instead of just the deployment, this is why I suggested using high cardinality for these. In high cardinality, there is no need to go through a counter, as we will have one timeserie per container, the logic would be closer to what we have in _report_container_spec_metrics (iterate over pods, then containers, directly submit gauges for each)

If you want to dig into what tag is what, you can have a look at our tag extractors:

• kubelet: https://github.com/DataDog/datadog-agent/blob/master/pkg/tagger/collectors/kubelet_extract.go
• docker: https://github.com/DataDog/datadog-agent/blob/master/pkg/tagger/collectors/docker_extract.go

You'll see tags added with either AddLow or AddHigh.

[schleyfox]

Thank you for the detailed explanation, I completely missed the high/low distinction. I've moved the restarts metrics to the state metrics.

[schleyfox]

Thanks for the review! Getting this in before the code freeze would be fantastic.

I've updated the metadata and can make any other changes that are needed to get this ready.

[codecov-io]

Codecov Report

Merging #2605 into master will increase coverage by 5.21%.
The diff coverage is 80%.

@@            Coverage Diff            @@
##           master   #2605      +/-   ##
=========================================
+ Coverage   83.89%   89.1%   +5.21%     
=========================================
  Files         638      10     -628     
  Lines       36323    1138   -35185     
  Branches     4425     152    -4273     
=========================================
- Hits        30472    1014   -29458     
+ Misses       4545      84    -4461     
+ Partials     1306      40    -1266

[xvello]

Thanks for your contribution 👍

[xvello]

Sorry for the back-pedaling, I think there's an issue with the whitelist filtering: if the reason is not in the whitelist, the gauge will still be emitted. Could you please have a look at it?

[xvello]

Thanks for the quick reply, tests look great!

Indeed, container_ids on the running gauge are probably not very useful. We do have known issue that k.pods.running and k.containers.running lists all pods, not just the running ones. DataDog/datadog-agent#2701 should address that though.

[schleyfox]

Excellent! Thanks for all of your help on this and sorry for all of the back and forth.

Do I need to take any further action?

[xvello]

@schleyfox Code is merged and will be build into 6.8.0-rc.1 Monday. Thanks 👍

Our current release target for 6.8.0 is the week of Dec. 10th, in the meantime, the RCs will be available on the Docker Hub if you want to give it a last round of testing.

[xvello]

Hi @schleyfox

Feedback from other users is leading us to investigate the following changes:

• make the metric a monotonic_count instead of a gauge: the agent will compute the diff directly and expose it as a counter instead of a gauge
• remove the container_id and container_name tags from the metric, to avoid churning the timeseries at every restart (see image below). The metric would still be tagged with pod_name and kube_container_name for uniqueness.

This example graph would now translate to one single time-series for this pod, showing 1 restart/minute during this 6 minute period, then back to zero after things are settled.

Would that make sense for your use case of this metric?

[schleyfox]

Yeah, that strikes me as very reasonable.

[xvello]

Thanks @schleyfox for your answer.

• 6.11 will include Do not tag container restarts/state metrics by container_id anymore #3424, that removes the container-level tags from the metric. This metric will now have one timeseries per pod*kube_container_name, which will make diff-ing it easier. The issue is that the diff goes negative when pods churn.
• The metric type change to counter is in Change .container.restarts to be actionnable in a monitor #3417 but is not yet merged. We are investigating current uses of the metric by our other users to decide whether that might break use cases.