podgc metrics should count all pod deletion behaviors #118480
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
sig/apps
carlory
changed the title
|
/cc @xing-yang |
| if err != nil { | ||
| t.Errorf("Error getting actualDeletingPodsTotal") | ||
| } | ||
| if actualDeletingPodsTotal != float64(inputDeletingPodsTotal) { | ||
| t.Errorf("Expected desiredDeletingPodsTotal to be %d, got %v", inputDeletingPodsTotal, actualDeletingPodsTotal) | ||
| } | ||
|
|
||
| actualDeletingPodsErrorTotal, err := metricstestutil.GetCounterMetricValue(deletingPodsErrorTotal.WithLabelValues()) | ||
| actualDeletingPodsErrorTotal, err := metricstestutil.GetCounterMetricValue(deletingPodsErrorTotal.WithLabelValues("out-of-service")) | ||
| if err != nil { |
There was a problem hiding this comment.
Can you add unit tests for all the other labels?
k8s-ci-robot
added
size/M
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
/check-cla |
|
/easycla |
|
/assign @gnufied |
| @@ -213,9 +213,11 @@ func (gcc *PodGCController) gcTerminated(ctx context.Context, pods []*v1.Pod) { | |||
| wait.Add(1) | |||
| go func(pod *v1.Pod) { | |||
| defer wait.Done() | |||
| deletingPodsTotal.WithLabelValues("terminated").Inc() | |||
There was a problem hiding this comment.
I am thinking, if we should emit this metric before or after the completion of delete operation. As it stands, we emit when we start deleting.
Also, I think we should add namespace to these metrics, so as if we force deleted pods from a particular namespace, a k8s admin can investigate why that happened. @msau42 what do you think?
There was a problem hiding this comment.
Waiting for a feedback from @msau42
There was a problem hiding this comment.
namespace would be good to add. I have a slight preference to add the metric after completion. Do we care about deletion errors?
There was a problem hiding this comment.
@xing-yang what do you think？
carlory
force-pushed
the
gc_metrics
branch
3 times, most recently
from
b09a043 to
04ac55e
Compare
|
@xing-yang @msau42 @gnufied the namespace label is added. |
|
/test pull-kubernetes-node-e2e-containerd |
| t.Helper() | ||
|
|
||
| actualDeletingPodsTotal, err := metricstestutil.GetCounterMetricValue(deletingPodsTotal.WithLabelValues()) | ||
| actualDeletingPodsTotal, err := metricstestutil.GetCounterMetricValue(metrics.DeletingPodsTotal.WithLabelValues("", reason)) |
There was a problem hiding this comment.
What is the namespace for these pods?
There was a problem hiding this comment.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: b87e82ce5aab1c88b98e1351fc76a6dd201c19ed
|
|
/assign @janetkuo |
|
/lgtm |
|
/assign @andrewsykim |
|
/assign @soltysh |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carlory, soltysh 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/priority important-longterm |
k8s-ci-robot
added
priority/important-longterm
What type of PR is this?
/kind feature
What this PR does / why we need it:
Literally,
force_delete_pods_totalandforce_delete_pod_errors_totalmetrics should count all the behaviors of the gc controller that forcefully delete pods.The
reasonlabel indicates the reason why the pod was forcibly deleted by GCSpecial notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: