feat: add pod resource requests and count metrics #2735
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DerekFrank The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| if errors.IsNotFound(err) { | ||
| // notify cluster state of the node deletion | ||
| c.cluster.DeletePod(req.NamespacedName) | ||
| c.podResources.DeletePod(pod) |
There was a problem hiding this comment.
Moving @ellistarn's comment over:
Would this be better encapsulated by cluster state?
We could write this into cluster.go, the reason it was made a separate object was for mutex contention purposes. We've been trying to separate cluster state out into different data stores where it make sense. I think this is one of those cases
There was a problem hiding this comment.
Cool. FWIW I really meant "state" controllers in general. I see them as a bunch of efficient indexes, and strongly agree with factoring them into multiple modules.
There was a problem hiding this comment.
But, we could optionally do all of that delegation in the cluster object -- might be able to help us coordinate mutexes. Informers are also a reasonable place to do this. I don't have a mental model of this entire state gomod right now, but I suspect we may need to refactor at some point.
Pull Request Test Coverage Report for Build 20438554973Details
💛 - Coveralls |
| []string{resourceType}, | ||
| ) | ||
| // Stage: alpha | ||
| PodCount = opmetrics.NewPrometheusGauge( |
There was a problem hiding this comment.
I think we do want to be daemonset aware. WDYT about a label that includes the first ownerreference gvk?
ellistarn
left a comment
ellistarn
left a comment
There was a problem hiding this comment.
I could have sworn we already had this. Why not use https://github.com/ellistarn/karpenter/blob/72387e317dd1868733631e34f7919611f08ce77f/pkg/controllers/metrics/node/controller.go#L74 ?
|
Fair point, I must have missed that when I was looking for this metric. That metric does only include bound pods, but I think that is fine for now. For #2626, I can surface the information through cluster.go to the decision tracker, but the node controller will need to still recompute the information to get it to have the correct labels. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Fixes #N/A
Description
Separating this out from #2626 for ease of review.
How was this change tested?
make presubmit
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.