Skip to content

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.

Sign up for GitHub

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

Open
RoddieKieley wants to merge 3 commits into
base: main
Choose a base branch
from
+707 −0
Open

feat: controller lease acquisition metrics (JEP-0013 Phase 2) #932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d15976d
feat: controller lease acquisition metrics (JEP-0013 Phase 2)
RoddieKieley Jul 29, 2026
2fac66d
fix: evaluate and address coderabbitai feedback.
RoddieKieley Aug 6, 2026
b1b44f2
fix: evaluate and address human feedback.
RoddieKieley Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions controller/cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import (
"github.com/jumpstarter-dev/jumpstarter/controller/internal/authorization"
"github.com/jumpstarter-dev/jumpstarter/controller/internal/config"
"github.com/jumpstarter-dev/jumpstarter/controller/internal/controller"
jmpmetrics "github.com/jumpstarter-dev/jumpstarter/controller/internal/metrics"
"github.com/jumpstarter-dev/jumpstarter/controller/internal/oidc"
"github.com/jumpstarter-dev/jumpstarter/controller/internal/service"
"github.com/jumpstarter-dev/jumpstarter/controller/internal/service/login"
Expand Down Expand Up @@ -201,6 +202,8 @@ func main() {
os.Exit(1)
}

jmpmetrics.RegisterDefaults()

oidcCert, err := service.NewSelfSignedCertificate("jumpstarter oidc", []string{"localhost"}, []net.IP{})
if err != nil {
setupLog.Error(err, "unable to generate certificate for internal oidc provider")
Expand Down
66 changes: 66 additions & 0 deletions controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
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 (
operatorv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/deploy/operator/api/v1alpha1"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

var _ = Describe("createControllerDeployment metrics bind", func() {
var r *JumpstarterReconciler
var js *operatorv1alpha1.Jumpstarter

BeforeEach(func() {
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,
},
},
}
})

It("exposes -metrics-bind-address=:8080 and metrics port 8080", func() {
dep := r.createControllerDeployment(js, "testhash")
Expect(dep).NotTo(BeNil())
Expect(dep.Spec.Template.Spec.Containers).NotTo(BeEmpty())

c := dep.Spec.Template.Spec.Containers[0]
Expect(c.Args).To(ContainElement("-metrics-bind-address=:8080"))

var metricsPort *corev1.ContainerPort
for i := range c.Ports {
if c.Ports[i].Name == "metrics" {
metricsPort = &c.Ports[i]
break
}
}
Expect(metricsPort).NotTo(BeNil(), "expected container port named metrics")
Expect(metricsPort.ContainerPort).To(Equal(int32(8080)))
})
})
75 changes: 75 additions & 0 deletions controller/internal/controller/lease_acquisition_metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
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 controller

import (
"testing"

jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1"
jmpmetrics "github.com/jumpstarter-dev/jumpstarter/controller/internal/metrics"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestLeaseAcquisitionTransitionResult_SuccessOnce(t *testing.T) {
lease := &jumpstarterdevv1alpha1.Lease{
ObjectMeta: metav1.ObjectMeta{Name: "lease-1"},
Status: jumpstarterdevv1alpha1.LeaseStatus{
ExporterRef: &corev1.LocalObjectReference{Name: "exporter-a"},
},
}

result, ok := leaseAcquisitionTransitionResult(nil, false, lease)
if !ok || result != jmpmetrics.ResultSuccess {
t.Fatalf("first transition: got (%q, %v), want (%q, true)", result, ok, jmpmetrics.ResultSuccess)
}

// Reconcile retry after status persisted: priorExporterRef is set.
result, ok = leaseAcquisitionTransitionResult(lease.Status.ExporterRef, false, lease)
if ok {
t.Fatalf("second observation after persist must not record, got %q", result)
}
}

func TestLeaseAcquisitionTransitionResult_FailureOnce(t *testing.T) {
lease := &jumpstarterdevv1alpha1.Lease{
ObjectMeta: metav1.ObjectMeta{Name: "lease-2"},
}
lease.SetStatusUnsatisfiable("NoAccess", "no exporters approved")

result, ok := leaseAcquisitionTransitionResult(nil, false, lease)
if !ok || result != jmpmetrics.ResultFailure {
t.Fatalf("first unsatisfiable transition: got (%q, %v), want (%q, true)", result, ok, jmpmetrics.ResultFailure)
}

// Reconcile retry after Unsatisfiable was persisted.
result, ok = leaseAcquisitionTransitionResult(nil, true, lease)
if ok {
t.Fatalf("second unsatisfiable observation after persist must not record, got %q", result)
}
}

func TestLeaseAcquisitionTransitionResult_NoTransition(t *testing.T) {
lease := &jumpstarterdevv1alpha1.Lease{
ObjectMeta: metav1.ObjectMeta{Name: "lease-3"},
}

result, ok := leaseAcquisitionTransitionResult(nil, false, lease)
if ok {
t.Fatalf("pending lease with no exporter/unsatisfiable must not record, got %q", result)
}
}
60 changes: 60 additions & 0 deletions controller/internal/controller/lease_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -93,6 +94,12 @@ func (r *LeaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
ctx = ctrl.LoggerInto(ctx, logger)

var result ctrl.Result
priorExporterRef := lease.Status.ExporterRef
priorUnsatisfiable := meta.IsStatusConditionTrue(
lease.Status.Conditions,
string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable),
)

if err := r.reconcileStatusExporterRef(ctx, &result, &lease); err != nil {
return result, err
}
Expand All @@ -109,6 +116,10 @@ func (r *LeaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
return RequeueConflict(logger, result, err)
}

// Record acquisition only after status is persisted so a failed Update
// cannot double-count on requeue (success or failure).
recordLeaseAcquisitionTransition(ctx, &lease, priorExporterRef, priorUnsatisfiable)

if lease.Labels == nil {
lease.Labels = make(map[string]string)
}
Expand Down Expand Up @@ -395,6 +406,55 @@ func (r *LeaseReconciler) reconcileStatusExporterRef(
return nil
}

// leaseAcquisitionTransitionResult returns the metric result for a persisted
// status transition, or ("", false) when no acquisition should be recorded.
// Recording is based on the pre-reconcile persisted snapshot so requeues after
// a successful Status().Update do not double-count.
func leaseAcquisitionTransitionResult(
priorExporterRef *corev1.LocalObjectReference,
priorUnsatisfiable bool,
lease *jumpstarterdevv1alpha1.Lease,
) (string, bool) {
if lease == nil {
return "", false
}
if priorExporterRef == nil && lease.Status.ExporterRef != nil {
return jmpmetrics.ResultSuccess, true
}
nowUnsatisfiable := meta.IsStatusConditionTrue(
lease.Status.Conditions,
string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable),
)
if !priorUnsatisfiable && nowUnsatisfiable {
return jmpmetrics.ResultFailure, true
}
return "", false
}

func recordLeaseAcquisitionTransition(
ctx context.Context,
lease *jumpstarterdevv1alpha1.Lease,
priorExporterRef *corev1.LocalObjectReference,
priorUnsatisfiable bool,
) {
result, ok := leaseAcquisitionTransitionResult(priorExporterRef, priorUnsatisfiable, lease)
if !ok {
return
}
recordLeaseAcquisition(ctx, lease, result)
}

func recordLeaseAcquisition(ctx context.Context, lease *jumpstarterdevv1alpha1.Lease, result string) {
exemplars := map[string]string{}
if lease != nil {
exemplars["lease_id"] = lease.Name

@mangelajo mangelajo Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

@RoddieKieley RoddieKieley Aug 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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:

H5 in detail: lease_id vs exporter name on controller exemplars

mangelajo’s point is right for aggregation: each lease_id is unique, so it does not help “which hardware fails often?” style rates. Exemplars are not for that. JEP-0013 puts unbounded IDs in exemplars for drill-down, and keeps bounded identity like exporter on Prom/Loki labels where series exist for that purpose.

This PR’s series is only:

jumpstarter_lease_acquisitions_total{result="success|failure"}

with default exemplars client + lease_id. There is no exporter label on this counter.

1. With only client + lease_id (today / JEP defaults)

Operator flow when a dashboard shows a spike in failures (or a success blip):

  1. Grafana shows an exemplar dot on jumpstarter_lease_acquisitions_total{result="failure"} (last-seen exemplar per series).
  2. Click → see e.g. {client="ci-bot", lease_id="lease-abc"}.
  3. Use lease_id as the join key into Loki, e.g.
    {component="controller"} | json | lease_id="lease-abc"
    (and the same filter on exporter/cli streams once Phase 1/3 correlation fields are on the lines).
  4. From those log lines you recover the rest: assigned exporter (if any), selector, unsatisfiable reason, spec.context, etc.

So without an exporter exemplar key, lease_id is the primary bridge from “this metric sample” → “the one lease’s logs / CR”. client answers “which client,” not “which DUT.”

Limits of this path:

  • You need a working Loki (or API) lookup; the exemplar panel alone does not name the exporter.
  • Failure acquires often have no exporter assigned (ExporterNotFound, NoAccess, …), so an exporter key would be empty anyway.
  • Last-exemplar-wins: on a busy result="failure" series you only see the most recent lease, not a histogram of exporters (that still comes from logs or from exporter-labeled metrics elsewhere).

2. If exporter is also on the exemplar (in addition to lease_id)

On success (and only when Status.ExporterRef is set), the exemplar might look like:

{client="ci-bot", lease_id="lease-abc", exporter="sidekick-t450s"}

Operator flow:

  1. Same spike / click on the exemplar.
  2. Immediately see which exporter was assigned — useful for “was this the flaky board?” without opening Loki first.
  3. Still use lease_id for precise correlation: one exporter has many leases over time; lease_id picks the exact run and joins to that lease’s log cluster / spec.context.
  4. Optionally pivot: Loki by exporter="sidekick-t450s" for hardware trends, or by lease_id for one CI run.

What this does not replace:

  • Rates per exporter still belong on metrics that already have an exporter label (e.g. planned jumpstarter_operations_total{exporter=...}), not on unique exemplar values.
  • Replacing lease_id with exporter alone would weaken the join: many samples share one exporter name; you lose one-click identity of the lease that caused this observation.

Budget note: OpenMetrics caps exemplars at 128 runes. Adding exporter plus key name costs ~15–40+ characters and can force truncation of lease_id/client (you already log when that happens).

Side-by-side

Goal lease_id only lease_id + exporter
Join metric sample → one lease’s logs Direct Direct (same)
See DUT name in the exemplar popup Indirect (via Loki/API) Direct on success
Failure with no assignment Fine (lease_id still set) exporter omitted/empty
“Which exporter fails often?” Wrong tool (use labeled series / LogQL) Still wrong tool for rates; only a hint on last sample
Cardinality of the Prom series Unchanged Unchanged

Verdict for #932

Keep JEP defaults: client + lease_id. Do not replace lease_id with exporter.

Optional later (or a short PR reply to mangelajo): on success only, also attach exporter when Status.ExporterRef is set, still under the allowlist / 128-rune budget — additive drill-down, not a substitute for lease_id.

That matches the JEP table: lease_id is exemplar-for-drill-down; exporter is primarily a bounded Prom/Loki label on exporter-scoped metrics, not the lease-acquire join key.

@RoddieKieley RoddieKieley Aug 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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(ctx, result, exemplars)
}

// 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
Expand Down
Loading