Add "Terminating" status in kube_pod_status_phase metrics #1013

clamoriniere · 2019-12-24T16:42:47Z

What this PR does:

Aim of this PullRequest is to add several new pod metrics:

kube_pod_deleted: Unix deletion timestamp
kube_pod_status_reason: The pod status reasons (NodeLost, Evicted)
These new metrics can be used to determine the pod.status.phase
displayed by kubectl: Running, Terminated, Unknown…
It will allow removing the kubectl display logic added previously
to compute the “Unknown” phase.

kube-state-metrics/internal/store/pod.go

Line 288 in d35e7ba

    
           {phase == v1.PodUnknown || (p.DeletionTimestamp != nil && p.Status.Reason == nodeUnreachablePodReason), string(v1.PodUnknown)},

why we need it:

In some cases, a Pod can be stuck in the "Terminating" phase due to a "Kubelet" issue; for example: the Kubelet is not able to communicate with the container runtime, or the container runtime is not able to delete the associated container.

And so, it can be interesting to have a way to create an alert on this kind of "bad" Pod state, which is currently not possible to do since the pod is flag by kube_pod_status_phase metrics as "Running".

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

jinnovation · 2020-01-10T18:12:05Z

This PR seems like it addresses #348 as well, which was closed w/o resolution.

clamoriniere · 2020-01-10T21:04:05Z

This PR seems like it addresses #348 as well, which was closed w/o resolution.

@jinnovation it is exactly the use case that we want to monitor with this fix: Pod blocked in Terminated state that require specific operation on the node.

brancz · 2020-01-13T08:48:20Z

internal/store/pod.go

@@ -274,6 +278,9 @@ var (
 					}
 				}

+				// compute the "Terminating" phase in order to reflect what a user can see with kubectl: https://github.com/kubernetes/kubernetes/blob/6a4216ba59ce1d09c8ac1c6229649bb71b7f6c85/pkg/printers/internalversion/printers.go#L737-L741
+				isPodTerminating := p.DeletionTimestamp != nil && p.Status.Reason != nodeUnreachablePodReason


This is not really how kube state metrics is implemented otherwise. We usually just expose the raw data and leave computations like this to query time.

I completely agree, but some logic was already added to compute the Unknow status.
https://github.com/kubernetes/kube-state-metrics/pull/1013/files#diff-791ee1d484ea70de1ecd1d8875f8f5c0L286

Here we want to reflect what is displayed by "kubectl" to avoid confusion, and avoid having a Pod in a Running state when it is not.

I think that was a mistake and since we're in the process of cleaning up kube-state-metrics and releasing preparing a breaking release, the time is right to change this. @lilic @tariq1890 what do you think? We should definitely make sure that for these cases we have the required raw data available so we can compute these things at runtime, but we should strive to remove any of these types of things from kube-state-metrics, just like we state in our docs :) .

Yes agreed, we should expose just raw metrics, users can compute whatever they want afterwards, and/or create recording rules for this.

Is there a way with the current information to know how many pods have the deletionTimestamp set?
With this info, we could indeed easily compute the "Terminating" status

@brancz and @lilic
I can rework this PR to add missing metrics of "raw" information to allow computing the Terminating and Unknown pod state from metrics.
Does that sound good?

Yes that sounds perfect! :)

PR updated with 2 new metrics:

kube_pod_deleted

kube_pod_status_reason

I can create another PR to remove the current logic for the Unknown pod phase.

lilic · 2020-01-16T12:22:53Z

Tests are failing as you need to update the docs for the metrics.

clamoriniere · 2020-01-16T17:15:23Z

Hi @lilic
sorry about the failing tests.

I updated the PR with proper documentation and also flagged new metrics as "EXPERIMENTAL" in the documentation.

docs/pod-metrics.md

internal/store/pod_test.go

clamoriniere · 2020-01-24T20:15:48Z

Hi @tariq1890
I'm wondering if you may have some time to review my last code update?
thanks

tariq1890 · 2020-01-24T20:42:42Z

@clamoriniere Sorry for the delay and Thank you so much for your patience with this PR! It's greatly appreciated :).

The code looks good, my last request is for you to provide a few examples (doesn;'t have to be complete) of using these raw basic metrics together for asserting on particular pod states (such as Terminating) in the README. This will be of great help to people who are transitioning over to the next version.

clamoriniere · 2020-01-24T21:18:38Z

Ok no problem.

tariq1890 · 2020-02-04T14:43:28Z

@clamoriniere Just following up on this :)

clamoriniere · 2020-02-05T10:42:12Z

@clamoriniere Just following up on this :)

Hi @tariq1890,
Sorry I was in vacation last week.

I added the promQL queries in the docs/pod-metrics.md file. I thought it makes more sens in this file than in the README.md.
I also needed to update a bit the make doccheck to support extra documentation in the docs/pod-metrics.md file.

Let me know if you prefer that I move the doc in the README.md

Aim of this PullRequest is to add several new pod metrics: - kube_pod_deleted: Unix deletion timestamp - kube_pod_status_reason: The pod status reasons (NodeLost, Evicted) These new metrics can be used to determine the `pod.status.phase` displayed by `kubectl`: Running, Terminated, Unknown… It will allow removing the `kubectl` display logic added previously to compute the “Unknown” phase. Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

tariq1890

@clamoriniere Thank you! Last set of review comments and this should be good to go :)

docs/pod-metrics.md

clamoriniere · 2020-02-05T20:25:28Z

@tariq1890
thanks for the review and suggestions. PR updated

let me know if you want me to do another PR that removes the current logic to generate the kube_pod_status_phase{phase="Unknown"} metric. (link:

kube-state-metrics/internal/store/pod.go

Line 285 in aa8a0af

    
           // This logic is directly copied from: https://github.com/kubernetes/kubernetes/blob/d39bfa0d138368bbe72b0eaf434501dcb4ec9908/pkg/printers/internalversion/printers.go#L597-L601

)

tariq1890 · 2020-02-05T20:31:59Z

Sure! That sounds good to me :)

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

tariq1890

/lgtm
/approve

k8s-ci-robot · 2020-02-05T22:05:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clamoriniere, tariq1890

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:

~~OWNERS~~ [tariq1890]

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

As explained in the doc: "kube-state-metrics exposes raw data unmodified from the Kubernetes API". The goal of this change was to remove the kubctl logic introduced to get the `Unknown` phase in `kube_pod_status_phase` metric. With the introduction of `kube_pod_deleted` metric, it is now possible to get the same result with a promQL query. (more info in this kubernetes#1013). Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

buttonbuilder · 2020-03-02T18:57:24Z

Which release will this be included in ?

clamoriniere · 2020-03-02T19:59:10Z

Which release will this be included in ?

I think in the next major release: v2.0.0

vc43 · 2023-01-13T13:05:52Z

I have a pod in status Terminating but whith kube-state-metrics:v2.7.0 can not see kube_pod_status_phase{phase="Terminating"}

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 24, 2019

k8s-ci-robot requested review from lilic and zouyee December 24, 2019 16:43

brancz reviewed Jan 13, 2020

View reviewed changes

clamoriniere force-pushed the feature/addTerminatingPhaseInPod_Status_Phase_metric branch from 5fd6a40 to e69b6fc Compare January 15, 2020 17:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2020

tariq1890 reviewed Jan 16, 2020

View reviewed changes

docs/pod-metrics.md Show resolved Hide resolved

clamoriniere force-pushed the feature/addTerminatingPhaseInPod_Status_Phase_metric branch from fd9409a to 680456e Compare January 16, 2020 17:40

tariq1890 reviewed Jan 16, 2020

View reviewed changes

internal/store/pod_test.go Outdated Show resolved Hide resolved

clamoriniere force-pushed the feature/addTerminatingPhaseInPod_Status_Phase_metric branch from 680456e to 2aff65e Compare January 16, 2020 18:22

clamoriniere force-pushed the feature/addTerminatingPhaseInPod_Status_Phase_metric branch from 54e369a to f0f27ae Compare February 5, 2020 12:04

clamoriniere added 4 commits February 5, 2020 18:55

update doc with new pod's metrics and fix benchmark

0650fd1

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

[doc] add useful pod.state metrics query

aa320cf

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

remove deprecated metrics after rebase

02844c3

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

clamoriniere force-pushed the feature/addTerminatingPhaseInPod_Status_Phase_metric branch from f0f27ae to 02844c3 Compare February 5, 2020 18:11

tariq1890 reviewed Feb 5, 2020

View reviewed changes

docs/pod-metrics.md Outdated Show resolved Hide resolved

docs/pod-metrics.md Outdated Show resolved Hide resolved

docs/pod-metrics.md Outdated Show resolved Hide resolved

docs/pod-metrics.md Outdated Show resolved Hide resolved

update pod metrics docs after review

311c682

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

clamoriniere force-pushed the feature/addTerminatingPhaseInPod_Status_Phase_metric branch from 564ed16 to 311c682 Compare February 5, 2020 20:40

tariq1890 approved these changes Feb 5, 2020

View reviewed changes

k8s-ci-robot assigned tariq1890 Feb 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2020

k8s-ci-robot merged commit 2148cb9 into kubernetes:master Feb 5, 2020

clamoriniere deleted the feature/addTerminatingPhaseInPod_Status_Phase_metric branch February 6, 2020 08:19

clamoriniere mentioned this pull request Feb 6, 2020

[Pod] remove kubctl logic in kube_pod_status_phase #1050

Merged

olivierlemasle mentioned this pull request Feb 17, 2020

Documentation on pod status queries is incorrect #1063

Closed

lilic mentioned this pull request Jul 13, 2020

kube_pod_status_reason metric can't come on Prometheus #1158

Closed

roystchiang mentioned this pull request Mar 30, 2021

Add "Terminating" status in kube_persistentvolumeclaim_status_phase metrics #1437

Closed

rgaiacs mentioned this pull request Aug 2, 2024

Break down Pending vs Terminating status jupyterhub/mybinder.org-deploy#3048

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Terminating" status in kube_pod_status_phase metrics #1013

Add "Terminating" status in kube_pod_status_phase metrics #1013

clamoriniere commented Dec 24, 2019 •

edited

Loading

jinnovation commented Jan 10, 2020

clamoriniere commented Jan 10, 2020

brancz Jan 13, 2020

clamoriniere Jan 13, 2020

brancz Jan 13, 2020

lilic Jan 13, 2020

lbernail Jan 13, 2020

clamoriniere Jan 13, 2020 •

edited

Loading

brancz Jan 14, 2020

clamoriniere Jan 16, 2020

lilic commented Jan 16, 2020

clamoriniere commented Jan 16, 2020

clamoriniere commented Jan 24, 2020

tariq1890 commented Jan 24, 2020

clamoriniere commented Jan 24, 2020

tariq1890 commented Feb 4, 2020

clamoriniere commented Feb 5, 2020

tariq1890 left a comment

clamoriniere commented Feb 5, 2020 •

edited

Loading

tariq1890 commented Feb 5, 2020

tariq1890 left a comment

k8s-ci-robot commented Feb 5, 2020

buttonbuilder commented Mar 2, 2020

clamoriniere commented Mar 2, 2020

vc43 commented Jan 13, 2023

Add "Terminating" status in kube_pod_status_phase metrics #1013

Add "Terminating" status in kube_pod_status_phase metrics #1013

Conversation

clamoriniere commented Dec 24, 2019 • edited Loading

jinnovation commented Jan 10, 2020

clamoriniere commented Jan 10, 2020

brancz Jan 13, 2020

Choose a reason for hiding this comment

clamoriniere Jan 13, 2020

Choose a reason for hiding this comment

brancz Jan 13, 2020

Choose a reason for hiding this comment

lilic Jan 13, 2020

Choose a reason for hiding this comment

lbernail Jan 13, 2020

Choose a reason for hiding this comment

clamoriniere Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

brancz Jan 14, 2020

Choose a reason for hiding this comment

clamoriniere Jan 16, 2020

Choose a reason for hiding this comment

lilic commented Jan 16, 2020

clamoriniere commented Jan 16, 2020

clamoriniere commented Jan 24, 2020

tariq1890 commented Jan 24, 2020

clamoriniere commented Jan 24, 2020

tariq1890 commented Feb 4, 2020

clamoriniere commented Feb 5, 2020

tariq1890 left a comment

Choose a reason for hiding this comment

clamoriniere commented Feb 5, 2020 • edited Loading

tariq1890 commented Feb 5, 2020

tariq1890 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 5, 2020

buttonbuilder commented Mar 2, 2020

clamoriniere commented Mar 2, 2020

vc43 commented Jan 13, 2023

clamoriniere commented Dec 24, 2019 •

edited

Loading

clamoriniere Jan 13, 2020 •

edited

Loading

clamoriniere commented Feb 5, 2020 •

edited

Loading