add pod metrics controller #744
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: a41d432 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/618ad414296e4900076343e7 |
| metricLabelProvisioner: provisioner, | ||
| metricLabelZone: zone, | ||
| } | ||
| errors = append(errors, publishCount(runningPodCountByProvisionerZone, metricLabels, countByZone[zone])) |
There was a problem hiding this comment.
check out multierr.Append instead of managing the slice of errors yourself.
There was a problem hiding this comment.
In the test cluster len(errors) > 1500. Thoughts on pre-allocation vs repeated multierr.Append()?
There was a problem hiding this comment.
Hopefully they wouldn't all error. The typical pattern I've seen used across the k8s space (and in this project) is:
if err := publicCount(...); err != nil {
errs = multierr.Append(errs, err)
}
| ) | ||
|
|
||
| var ( | ||
| knownValuesForNodeLabels = v1alpha4.WellKnownLabels |
There was a problem hiding this comment.
Why alias this? Isn't it cleaner to just use it as is?
There was a problem hiding this comment.
To improve clarity and avoid package name "noise" -- in this case it doesn't provide helpful context.
zoneValues := knownValuesForNodeLabels[nodeLabelZone]
// vs
zoneValues := v1alpha4.WellKnownLabels[nodeLabelZone]There was a problem hiding this comment.
Perhaps its my own familiarity with the k8s ecosystem that causes v1alpha4.WellKnownLabels to read very clearly to me. I'd lean the latter, since it avoids introducing another concept to be aware of, but I'll leave it to you.
| } | ||
|
|
||
| podList := v1.PodList{} | ||
| withNodeName := client.MatchingFields{"spec.nodeName": node.Name} |
There was a problem hiding this comment.
any reason to not inline this?
| return controllerruntime. | ||
| NewControllerManagedBy(m). | ||
| Named(controllerName). | ||
| For(&v1alpha4.Provisioner{}, builder.WithPredicates( |
There was a problem hiding this comment.
I'd drop all of these predicates and just use the default. I don't see the harm in running this on an update.
| } | ||
|
|
||
| // 2. Update pod counts associated with the provisioner. | ||
| podsByZone, err := c.podsByZone(ctx, &provisioner) |
There was a problem hiding this comment.
Curious what the design options are here:
- pods w/ zone data
- pods w/o zone data
- other instance attributes (arch/os/etc)
I wonder if we should follow https://en.wikipedia.org/wiki/KISS_principle and just include phase.
| return reconcile.Result{Requeue: true}, err | ||
| } | ||
|
|
||
| // The provisioner has been deleted. Reset all the associated counts to zero. |
There was a problem hiding this comment.
Isn't this the same issue we ran into w/ the previous PR? Thoughts on just letting the data expire rather than trying to zero it out?
There was a problem hiding this comment.
I'd feel better zeroing out here. Letting the data expire seems pretty inaccurate. Is there a default expire duration for the metric data or is it configurable?
But it's fairly easy to zero it out on this clear signal that the provisioner has been deleted, might as well, right?
There was a problem hiding this comment.
If we did want some sort of deletion assurance, we could use a finalizer. Maybe overkill though.
pkg/controllers/metrics/common.go
Outdated
|
|
||
| func publishCount(gaugeVec *prometheus.GaugeVec, labels prometheus.Labels, count int) error { | ||
| gauge, err := gaugeVec.GetMetricWith(labels) | ||
| if err == nil { |
There was a problem hiding this comment.
Generally in go, the happy-path is left indented. So the err would be returned in a branch statement rather than at the end of the func. I'm guessing you did this to save the extra return nil but I think it's better to stick with the convention here.
There was a problem hiding this comment.
This is known as the "short circuitting pattern"
if statement: exit
if statement: exit
success
pkg/controllers/metrics/constants.go
Outdated
| requeueInterval = 10 * time.Second | ||
|
|
||
| metricNamespace = metrics.KarpenterNamespace | ||
| metricSubsystemCapacity = "capacity" |
There was a problem hiding this comment.
I'd call this variable nodeSubsystem to avoid stuttering both the package name. Also, is it cleaner to write this as capacity -> node
There was a problem hiding this comment.
Also, is it cleaner to write this as capacity -> node
Please clarify. Do you mean you want nodeSubsystem = "capacity -> node"? If so, 1) that string is not a valid subsystem name for Prometheus 2) the name "capacity" was suggested in a previous PR here
There was a problem hiding this comment.
Sorry for the thrash. It occurred to me that node might be a more accurate name, but it sounds like prometheus has this name reserved?
pkg/controllers/metrics/constants.go
Outdated
| metricLabelInstanceType = "instancetype" | ||
| metricLabelOS = "os" | ||
| metricLabelPhase = "phase" | ||
| metricLabelProvisioner = metrics.ProvisionerLabel |
There was a problem hiding this comment.
similarly why do we need to alias this?
There was a problem hiding this comment.
The name "label" is a bit overloaded in this package: there are labels on Nodes and labels on metrics. The aliases are to provide clarity to which object each label name applies.
pkg/controllers/metrics/constants.go
Outdated
| metricLabelProvisioner = metrics.ProvisionerLabel | ||
| metricLabelZone = "zone" | ||
|
|
||
| nodeLabelArch = v1.LabelArchStable |
There was a problem hiding this comment.
See response to previous comment.
| } | ||
|
|
||
| func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { | ||
| loggerName := fmt.Sprintf("%s.provisioner/%s", strings.ToLower(controllerName), req.Name) |
There was a problem hiding this comment.
consider inlining to avoid creation of an extra variable/concept/line.
pkg/controllers/metrics/pods.go
Outdated
| zoneValues := knownValuesForNodeLabels[nodeLabelZone] | ||
| countByZone := make(map[string]int, len(zoneValues)) | ||
|
|
||
| for zone, pods := range podsByZone { |
There was a problem hiding this comment.
Reposting this comment since it got hidden in the refactor:
Curious what the design options are here:
pods w/ zone data
pods w/o zone data
other instance attributes (arch/os/etc)
I wonder if we should follow https://en.wikipedia.org/wiki/KISS_principle and just include phase.
There was a problem hiding this comment.
The divisions were made to minimize metric cardinality.
cjerad
force-pushed
the
metrics-pod-controller
branch
from
0dfed97 to
9d34d21
Compare
|
|
||
| var nodeLabelProvisioner = v1alpha5.ProvisionerNameLabelKey | ||
|
|
||
| func publishCount(gaugeVec *prometheus.GaugeVec, labels prometheus.Labels, count int) error { |
There was a problem hiding this comment.
optional: Given that the labels are deterministic, we could use https://github.com/prometheus/client_golang/blob/v1.11.0/prometheus/counter.go#L256
gaugeVec.With(labels).Set(float64(count))
which would collapse this helper.
1. Issue, if available:
#612 "Emit Metrics for Karpenter"
This PR does not fully resolve the issue. More changes will be needed.
2. Description of changes:
Add a controller that updates pod count metrics across multiple dimensions, e.g. provisioner, phase, and zone.
3. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.