kube-state-metrics icon indicating copy to clipboard operation
kube-state-metrics copied to clipboard

fix: `.` is removed in nodeName

Open CatherineF-dev opened this issue 1 year ago • 10 comments

What this PR does / why we need it:

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality) No

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 # an issue raised in slack chat We see the fieldSelector get created with the node-name … but it’s missing the .

CatherineF-dev avatar Apr 17 '24 21:04 CatherineF-dev

This should fix https://github.com/kubernetes/kube-state-metrics/issues/2374. Thank you, I hope this can get merged quickly!

diranged avatar Apr 17 '24 21:04 diranged

/assign @dgrisonnet /triage accepted

logicalhan avatar Apr 18 '24 16:04 logicalhan

There was a conversation here: https://github.com/kubernetes/kube-state-metrics/pull/2217/files#r1356982673 https://github.com/kubernetes/kube-state-metrics/pull/2217/files#r1357024577

Do we still need this to differentiate between "" and nil?

mrueg avatar Apr 18 '24 17:04 mrueg

Good point. Updated to only add \\.

Tested: https://go.dev/play/p/K5GtruNWjO0

CatherineF-dev avatar Apr 18 '24 17:04 CatherineF-dev

https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names

We can reuse https://github.com/kubernetes/apimachinery/blob/0ee3e6150890f56b226a3fe5a95ba33b1b2bf7c7/pkg/util/validation/validation.go#L177 probably.

To be exact: IsDNS1123Label https://github.com/kubernetes/apimachinery/blob/0ee3e6150890f56b226a3fe5a95ba33b1b2bf7c7/pkg/util/validation/validation.go#L187 function?

mrueg avatar Apr 18 '24 17:04 mrueg

There was a conversation here: https://github.com/kubernetes/kube-state-metrics/pull/2217/files#r1356982673 https://github.com/kubernetes/kube-state-metrics/pull/2217/files#r1357024577

Do we still need this to differentiate between "" and nil?

I read https://github.com/kubernetes/kube-state-metrics/pull/2217/files#r1356982673 https://github.com/kubernetes/kube-state-metrics/pull/2217/files#r1357024577 again and find it is not related to differentiate between "" and nil.

CatherineF-dev avatar Apr 19 '24 13:04 CatherineF-dev

Updated. Need review again.

CatherineF-dev avatar Apr 19 '24 13:04 CatherineF-dev

I don't get why we have this NodeType structure from https://github.com/kubernetes/kube-state-metrics/pull/2217/. Pods in scheduling state could be handled separately, even maybe with a dedicated CLI option. I think that the code could be simplified.

Also one thing I am curious about is what would happen once the pod is scheduled on the node. Do we continue to produce a stale metrics with the state of the pod when it wasn't schedule? Or do we have a mechanism to check that this pod shouldn't be watched by this shard anymore?

dgrisonnet avatar Apr 19 '24 16:04 dgrisonnet

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: CatherineF-dev, diranged, LaikaN57 Once this PR has been reviewed and has the lgtm label, please ask for approval from dgrisonnet. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

k8s-ci-robot avatar Apr 23 '24 02:04 k8s-ci-robot

Do we continue to produce a stale metrics with the state of the pod when it wasn't schedule?

No. I tested for both daemonset (watch scheduled pods) and deployment (watch not scheduled pods).

# Test for the deployment

# 1. deploy a pod
kubectl apply -f https://raw.githubusercontent.com/CatherineF-dev/k8s-hello-world/main/unschedule_pod.yaml

kubectl get pods --field-selector spec.nodeName= -A
NAMESPACE   NAME                          READY   STATUS    RESTARTS   AGE
default     hello-node-7d78fd84d9-rqbjh   0/1     Pending   0          38s

# 2. can see metrics when this pod is not scheduled

curl localhost:8080/metrics | grep hello-node | grep schedule

kube_pod_status_scheduled{namespace="default",pod="hello-node-7d78fd84d9-7grpc",uid="1c9982aa-e99b-46a0-97c0-b03bf1b94b47",condition="true"} 0
kube_pod_status_scheduled{namespace="default",pod="hello-node-7d78fd84d9-7grpc",uid="1c9982aa-e99b-46a0-97c0-b03bf1b94b47",condition="false"} 1
kube_pod_status_scheduled{namespace="default",pod="hello-node-7d78fd84d9-7grpc",uid="1c9982aa-e99b-46a0-97c0-b03bf1b94b47",condition="unknown"} 0
kube_pod_scheduler{namespace="default",pod="hello-node-7d78fd84d9-7grpc",uid="1c9982aa-e99b-46a0-97c0-b03bf1b94b47",name="default-scheduler"} 1

# 3. can't see metrics when this pod is scheduled 
curl localhost:8080/metrics | grep hello-node | grep schedule

CatherineF-dev avatar May 09 '24 11:05 CatherineF-dev

@dgrisonnet need review again.

CatherineF-dev avatar May 14 '24 00:05 CatherineF-dev

Closing this in favor of https://github.com/kubernetes/kube-state-metrics/pull/2388

Feel free to reopoen if anything changes.

mrueg avatar May 16 '24 17:05 mrueg