-
Notifications
You must be signed in to change notification settings - Fork 33
feat: controller lease acquisition metrics (JEP-0013 Phase 2) #932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /* | ||
| Copyright 2026. The Jumpstarter Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package jumpstarter | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| operatorv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/deploy/operator/api/v1alpha1" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| // Stdlib unit test (no envtest): asserts Controller Deployment metrics bind for JEP-0013 Phase 2. | ||
| func TestControllerDeploymentMetricsBind(t *testing.T) { | ||
| r := &JumpstarterReconciler{} | ||
| js := &operatorv1alpha1.Jumpstarter{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "jumpstarter", | ||
| Namespace: "jumpstarter-lab", | ||
| }, | ||
| Spec: operatorv1alpha1.JumpstarterSpec{ | ||
| Controller: operatorv1alpha1.ControllerConfig{ | ||
| Image: "example.com/controller:test", | ||
| ImagePullPolicy: corev1.PullIfNotPresent, | ||
| Replicas: 1, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| dep := r.createControllerDeployment(js, "testhash") | ||
| if dep == nil { | ||
| t.Fatal("expected non-nil deployment") | ||
| } | ||
| if len(dep.Spec.Template.Spec.Containers) == 0 { | ||
| t.Fatal("expected at least one container") | ||
| } | ||
|
|
||
| c := dep.Spec.Template.Spec.Containers[0] | ||
| foundArg := false | ||
| for _, arg := range c.Args { | ||
| if arg == "-metrics-bind-address=:8080" { | ||
| foundArg = true | ||
| break | ||
| } | ||
| } | ||
| if !foundArg { | ||
| t.Fatalf("expected -metrics-bind-address=:8080 in args, got %#v", c.Args) | ||
| } | ||
|
|
||
| foundPort := false | ||
| for _, p := range c.Ports { | ||
| if p.Name == "metrics" { | ||
| foundPort = true | ||
| if p.ContainerPort != 8080 { | ||
| t.Fatalf("metrics port = %d, want 8080", p.ContainerPort) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| if !foundPort { | ||
| t.Fatal("expected container port named metrics") | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,7 @@ import ( | |||||||||||||||||||
| "time" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1" | ||||||||||||||||||||
| jmpmetrics "github.com/jumpstarter-dev/jumpstarter/controller/internal/metrics" | ||||||||||||||||||||
| corev1 "k8s.io/api/core/v1" | ||||||||||||||||||||
| k8serrors "k8s.io/apimachinery/pkg/api/errors" | ||||||||||||||||||||
| "k8s.io/apimachinery/pkg/api/meta" | ||||||||||||||||||||
|
|
@@ -242,7 +243,7 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( | |||||||||||||||||||
| Name: lease.Spec.ExporterRef.Name, | ||||||||||||||||||||
| }, &exporter); err != nil { | ||||||||||||||||||||
| if k8serrors.IsNotFound(err) { | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable( | ||||||||||||||||||||
| setUnsatisfiableAndRecord(lease, | ||||||||||||||||||||
| "ExporterNotFound", | ||||||||||||||||||||
| "Requested exporter %s was not found", | ||||||||||||||||||||
| lease.Spec.ExporterRef.Name, | ||||||||||||||||||||
|
|
@@ -252,7 +253,7 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( | |||||||||||||||||||
| return fmt.Errorf("reconcileStatusExporterRef: failed to get requested exporter: %w", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| if !selector.Empty() && !selector.Matches(labels.Set(exporter.Labels)) { | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable( | ||||||||||||||||||||
| setUnsatisfiableAndRecord(lease, | ||||||||||||||||||||
| "SelectorMismatch", | ||||||||||||||||||||
| "Requested exporter %s does not match selector %s", | ||||||||||||||||||||
| exporter.Name, | ||||||||||||||||||||
|
|
@@ -262,7 +263,7 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( | |||||||||||||||||||
| } | ||||||||||||||||||||
| // Check if the explicitly requested exporter is disabled | ||||||||||||||||||||
| if !exporter.IsEnabled() && !lease.Spec.AllowDisabled { | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable( | ||||||||||||||||||||
| setUnsatisfiableAndRecord(lease, | ||||||||||||||||||||
| "ExporterDisabled", | ||||||||||||||||||||
| "Requested exporter %s is disabled. "+ | ||||||||||||||||||||
| "To lease a disabled exporter, set spec.allowDisabled: true on the Lease, "+ | ||||||||||||||||||||
|
|
@@ -281,7 +282,7 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( | |||||||||||||||||||
| // Filter out disabled exporters from selector-based listing | ||||||||||||||||||||
| matchingExporters = filterOutDisabledExporters(listed.Items) | ||||||||||||||||||||
| if len(matchingExporters) == 0 && len(listed.Items) > 0 { | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable( | ||||||||||||||||||||
| setUnsatisfiableAndRecord(lease, | ||||||||||||||||||||
| "AllDisabled", | ||||||||||||||||||||
| "All %d exporters matching the selector are disabled", | ||||||||||||||||||||
| len(listed.Items), | ||||||||||||||||||||
|
|
@@ -301,12 +302,12 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( | |||||||||||||||||||
| if len(desc) > 4096 { | ||||||||||||||||||||
| desc = desc[:4096] + "..." | ||||||||||||||||||||
| } | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable("NoAccess", | ||||||||||||||||||||
| setUnsatisfiableAndRecord(lease, "NoAccess", | ||||||||||||||||||||
| "While there are %d exporters matching the selector, none of them are approved by any policy for your client. Matching policies: %s", | ||||||||||||||||||||
| len(matchingExporters), desc, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable("NoAccess", | ||||||||||||||||||||
| setUnsatisfiableAndRecord(lease, "NoAccess", | ||||||||||||||||||||
| "While there are %d exporters matching the selector, none of them are approved by any policy for your client", | ||||||||||||||||||||
| len(matchingExporters), | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
@@ -336,7 +337,7 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( | |||||||||||||||||||
| orderedExporters := orderApprovedExporters(onlineApprovedExporters) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if len(orderedExporters) > 0 && orderedExporters[0].Policy.SpotAccess { | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable("SpotAccess", | ||||||||||||||||||||
| setUnsatisfiableAndRecord(lease, "SpotAccess", | ||||||||||||||||||||
| "The only possible exporters are under spot access (i.e. %s), but spot access is still not implemented", | ||||||||||||||||||||
| orderedExporters[0].Exporter.Name) | ||||||||||||||||||||
| return nil | ||||||||||||||||||||
|
|
@@ -389,12 +390,35 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( | |||||||||||||||||||
| lease.Status.ExporterRef = &corev1.LocalObjectReference{ | ||||||||||||||||||||
| Name: selected.Exporter.Name, | ||||||||||||||||||||
| } | ||||||||||||||||||||
| recordLeaseAcquisition(lease, jmpmetrics.ResultSuccess) | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The success metric is recorded here before status is persisted to the API server. |
||||||||||||||||||||
| return nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func recordLeaseAcquisition(lease *jumpstarterdevv1alpha1.Lease, result string) { | ||||||||||||||||||||
| exemplars := map[string]string{} | ||||||||||||||||||||
| if lease != nil { | ||||||||||||||||||||
| exemplars["lease_id"] = lease.Name | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from the pov of exemplars and useful data, each lease_id is different, so it would not provide much statistical value. What do you think about recording the assigned exporter name instead?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having only really run this in development in my own homelab I requested an evaluation of the implications of having lease_id both with and without and additional exporter as the initial take was 'The JEP has a specific design and the lease_id should not be removed'. The results sound reasonable, but as one of the authors of the JEP-0013 you might have a better idea than me if it simply "sounds reasonable" or is actually reasonable:
Verdict for #932Keep JEP defaults: Optional later (or a short PR reply to mangelajo): on success only, also attach That matches the JEP table:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kirkbrauer @raballew As others who reviewed the original JEP-0013 PR #631 will likely be afk for a few days at least maybe you have an educated opinion to chime in with? |
||||||||||||||||||||
| if lease.Spec.ClientRef.Name != "" { | ||||||||||||||||||||
| exemplars["client"] = lease.Spec.ClientRef.Name | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| jmpmetrics.Default.RecordAcquisition(result, exemplars) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func setUnsatisfiableAndRecord(lease *jumpstarterdevv1alpha1.Lease, reason, messageFormat string, a ...any) { | ||||||||||||||||||||
| already := meta.IsStatusConditionTrue( | ||||||||||||||||||||
| lease.Status.Conditions, | ||||||||||||||||||||
| string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| lease.SetStatusUnsatisfiable(reason, messageFormat, a...) | ||||||||||||||||||||
| if !already { | ||||||||||||||||||||
| recordLeaseAcquisition(lease, jmpmetrics.ResultFailure) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some test for the check that prevents double-counting on re-reconciliation would be nice. |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // attachMatchingPolicies attaches the matching policies to the list of online exporters | ||||||||||||||||||||
| // if the exporter matches the policy and the client matches the policy's client selector | ||||||||||||||||||||
| // the exporter is approved for leasing | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| /* | ||
| Copyright 2026. The Jumpstarter Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| // Package metrics provides Jumpstarter controller Prometheus metrics (JEP-0013 Phase 2). | ||
| package metrics | ||
|
|
||
| import ( | ||
| "github.com/prometheus/client_golang/prometheus" | ||
| ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" | ||
| ) | ||
|
|
||
| const ( | ||
| // LeaseAcquisitionsTotal is the JEP-0013 counter for lease acquire attempts. | ||
| LeaseAcquisitionsTotal = "jumpstarter_lease_acquisitions_total" | ||
|
|
||
| ResultSuccess = "success" | ||
| ResultFailure = "failure" | ||
| ) | ||
|
|
||
| // DefaultExemplarKeys are the JEP-0013 default exemplar allowlist keys. | ||
| var DefaultExemplarKeys = []string{"client", "lease_id"} | ||
|
|
||
| // LeaseMetrics holds lease-related Prometheus collectors. | ||
| type LeaseMetrics struct { | ||
| acquisitions *prometheus.CounterVec | ||
| } | ||
|
|
||
| // NewLeaseMetrics constructs lease metrics. Call Register before RecordAcquisition. | ||
| func NewLeaseMetrics() *LeaseMetrics { | ||
| return &LeaseMetrics{ | ||
| acquisitions: prometheus.NewCounterVec( | ||
| prometheus.CounterOpts{ | ||
| Name: LeaseAcquisitionsTotal, | ||
| Help: "Lease acquire attempts on the Jumpstarter controller.", | ||
| }, | ||
| []string{"result"}, | ||
| ), | ||
| } | ||
| } | ||
|
Comment on lines
+51
to
+61
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Counter time series not pre-initialized with zero values. So, before any lease acquisition, scraping
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need to pre-initialize to 0 ? I assumed that was the default in go.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Zero values in go - " Variables declared without an explicit initial value are given their zero value. "
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics - its lazily initialized hence if you do do not prepopulate no graph will show. |
||
|
|
||
| // Register registers collectors with the given registerer. | ||
| func (m *LeaseMetrics) Register(r prometheus.Registerer) error { | ||
| return r.Register(m.acquisitions) | ||
| } | ||
|
|
||
| // MustRegister registers collectors and panics on error. | ||
| func (m *LeaseMetrics) MustRegister(r prometheus.Registerer) { | ||
| r.MustRegister(m.acquisitions) | ||
| } | ||
|
|
||
| // MustRegisterWithControllerRuntime registers on the controller-runtime metrics registry. | ||
| func (m *LeaseMetrics) MustRegisterWithControllerRuntime() { | ||
| m.MustRegister(ctrlmetrics.Registry) | ||
| } | ||
|
|
||
| // RecordAcquisition increments jumpstarter_lease_acquisitions_total for result | ||
| // and attaches exemplar labels from the allowlist (client, lease_id when present). | ||
| func (m *LeaseMetrics) RecordAcquisition(result string, exemplars map[string]string) { | ||
| if m == nil || m.acquisitions == nil { | ||
| return | ||
| } | ||
| metric, err := m.acquisitions.GetMetricWithLabelValues(result) | ||
| if err != nil { | ||
| return | ||
| } | ||
| labels := filterExemplarLabels(exemplars) | ||
| if len(labels) > 0 { | ||
| if adder, ok := metric.(prometheus.ExemplarAdder); ok { | ||
| adder.AddWithExemplar(1, labels) | ||
| return | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| } | ||
| metric.Inc() | ||
| } | ||
|
|
||
| func filterExemplarLabels(in map[string]string) prometheus.Labels { | ||
| if len(in) == 0 { | ||
| return nil | ||
| } | ||
| out := prometheus.Labels{} | ||
| for _, key := range DefaultExemplarKeys { | ||
| if v, ok := in[key]; ok && v != "" { | ||
| out[key] = v | ||
| } | ||
| } | ||
| if len(out) == 0 { | ||
| return nil | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| // Default is the process-wide lease metrics instance used by the controller. | ||
| var Default = NewLeaseMetrics() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All controller tests share the global counter, breaking test isolation.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't those tests run in serial?, is it worth complicating the code in favor of testability? could be, just sharing the question here, I am not familiar with the prometheus metrics framework to be able to figure out if it'd get too complex, or if it really won't.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Evaluated and deferred as not required right now, although may be in the future.
|
||
|
|
||
| // RegisterDefaults registers Default with the controller-runtime registry. | ||
| func RegisterDefaults() { | ||
| Default.MustRegisterWithControllerRuntime() | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.