feat: implement status reporting on ExporterSet resources#904
Conversation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controller/internal/exporterset/reconciler.go (1)
475-481: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the deprecated requeue path
Result.Requeueis deprecated in controller-runtime v0.21. Return the conflict error here instead so the standard rate-limited retry path applies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/exporterset/reconciler.go` around lines 475 - 481, Update requeueConflict to return the original conflict error instead of ctrl.Result{Requeue: true}, while preserving the conflict log message and standard result shape so controller-runtime’s rate-limited retry path handles it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/internal/exporterset/reconciler.go`:
- Around line 153-158: Update the status selector assignment in the reconciler
after metav1.LabelSelectorAsSelector succeeds: use the parsed selector’s
String() value for es.Status.Selector instead of
metav1.FormatLabelSelector(&es.Spec.Selector), preserving the existing
validation and error handling.
---
Nitpick comments:
In `@controller/internal/exporterset/reconciler.go`:
- Around line 475-481: Update requeueConflict to return the original conflict
error instead of ctrl.Result{Requeue: true}, while preserving the conflict log
message and standard result shape so controller-runtime’s rate-limited retry
path handles it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af5dc1f8-1586-490b-a0d2-1856ac710cc2
📒 Files selected for processing (4)
controller/api/virtualtarget/v1alpha1/exporterset_types.gocontroller/deploy/operator/config/crd/bases/virtualtarget.jumpstarter.dev_exportersets.yamlcontroller/internal/exporterset/reconciler.gocontroller/internal/exporterset/reconciler_test.go
aeee053 to
0f2ae54
Compare
Add deployment-style status conditions and replica counters to ExporterSet CRs, providing visibility into pod health, exporter availability, and scaling state on every reconcile loop. Status fields: - Replica counters: total, ready, available, unavailable, leased - Pod phase aggregation: pending, running, failed, unknown - Exporter state breakdown: active, idle, disabled - Selector field for HPA/KEDA scale subresource compatibility Conditions (Available, Progressing, Degraded, ScalingLimited): - Available: true when readyReplicas >= minReplicas - Progressing: true when pods pending or replicas not ready - Degraded: true when pods failed or exporters offline with no pending pods - ScalingLimited: true when at maxReplicas and need more available Events emitted on condition transitions (e.g. ExporterSetAvailable, ExporterSetDegraded, ScalingLimited). Ref: JEP-0014 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
0f2ae54 to
65de675
Compare
kirkbrauer
left a comment
There was a problem hiding this comment.
I think this looks good, WDYT @mangelajo ?
| var replicas, ready, available, leased int32 | ||
| var active, idle, disabled, offline int32 | ||
|
|
||
| for i := range ownedExporters { | ||
| exp := &ownedExporters[i] | ||
| replicas++ | ||
|
|
||
| isOnline := meta.IsStatusConditionTrue( | ||
| exp.Status.Conditions, | ||
| string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), | ||
| ) | ||
| isEnabled := exp.IsEnabled() | ||
| isLeased := exp.Status.LeaseRef != nil | ||
|
|
||
| if isOnline { | ||
| ready++ | ||
| } | ||
| if isLeased { | ||
| leased++ | ||
| } | ||
| if isOnline && isEnabled && !isLeased { | ||
| available++ | ||
| } | ||
|
|
||
| if !isEnabled { | ||
| disabled++ | ||
| } else if isLeased && isOnline { | ||
| active++ | ||
| } else if isOnline { | ||
| idle++ | ||
| } else { | ||
| offline++ |
There was a problem hiding this comment.
looking at the way we calculate the different fields, it looks like available and idle will always hold the same value that represents exporters that are online, enabled, and not leased. given that, do we actually need both fields in the status? AvailableReplicas and ExportersIdle.
There was a problem hiding this comment.
I have thought about it and ultimately decided to keep it to allow repurposing, but I don't mind either way
…starter-dev#908) This PR extends the Jumpstarter operator to deploy and manage **ExporterSet provisioner controllers** as Kubernetes Deployments, as defined in [JEP-0014](https://jumpstarter.dev/main/contributing/jeps/JEP-0014-virtual-scalable-exporters.html). It builds on the ExporterSet controller bootstrap in jumpstarter-dev#864. When `spec.exporterSets.provisioners` is configured, the operator creates one Deployment per enabled provisioner, running the shared `jumpstarter-exporterset-controller` image with `--provisioner=<name>`. Each provisioner gets its own ServiceAccount, Role, and RoleBinding. ### API changes Adds `spec.exporterSets` to the `Jumpstarter` CR: - **Global settings**: `image`, `imagePullPolicy`, `resources` - **Per-provisioner config** (`provisioners[]`): `name`, `enabled`, optional `image` override, `replicas`, optional `resources` override - **`ProvisionerConfig.name` validation**: DNS-subdomain-safe pattern (`^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$`); names that collide after sanitization are rejected at reconcile time Adds status condition `ExporterSetControllersReady`, included in overall `Ready` only when at least one provisioner is enabled. ### Operator behavior - Reconciles Deployment + RBAC (SA, Role, RoleBinding) for each enabled provisioner - Cleans up Deployments and owned Role/RoleBinding when a provisioner is disabled or removed (ServiceAccounts are preserved, matching existing operator SA patterns) - Reports `ExporterSetControllersReady` based on each enabled provisioner Deployment's `Available` condition - Extends operator ClusterRole with `roles` delete/patch, `pods` get/list/watch, and `virtualtarget.jumpstarter.dev` read/status permissions (required to create provisioner Roles with those rules) - Default provisioner controller resources when unset: 100m CPU / 256Mi memory requests, 500m CPU / 512Mi memory limits ### Exporter-set-controller runtime fix The provisioner controller binary is updated to work with namespaced Roles: - Requires `NAMESPACE` env var (set via downward API in the Deployment) - Restricts the controller-runtime cache to that namespace - Sets `LeaderElectionNamespace` to the pod namespace Without this, informers issue cluster-scoped List/Watch calls that are rejected by the API server. ### Dev / deploy tooling - `make deploy` and `make deploy-operator` now also build `jumpstarter-exporterset-controller` - `hack/deploy_with_operator.sh` deploys `qemu.jumpstarter.dev` as a default enabled provisioner in the generated Jumpstarter CR ### Example CR snippet ```yaml spec: exporterSets: image: quay.io/jumpstarter-dev/jumpstarter-exporterset-controller:latest imagePullPolicy: IfNotPresent resources: requests: cpu: 100m memory: 256Mi provisioners: - name: qemu.jumpstarter.dev enabled: true replicas: 1 # optional per-provisioner overrides: # image: ... # resources: ... ``` ## Test plan - [x] Unit tests for resource constructors, RBAC rules, resource defaults, and provisioner name sanitization (`exporterset_test.go`) - [x] Controller integration tests for full lifecycle: create, per-provisioner image/resources override, disable/cleanup, multiple provisioners, mixed enabled/disabled, spec removal, name collision rejection, and `ExporterSetControllersReady` status (`jumpstarter_controller_test.go`) - [x] `make -C controller/deploy/operator test` - [x] Manual: `make deploy` and verify `qemu.jumpstarter.dev` provisioner Deployment, SA, and Role come up in `jumpstarter-lab` ## Related - [JEP-0014: Virtual Scalable Exporters](https://jumpstarter.dev/main/contributing/jeps/JEP-0014-virtual-scalable-exporters.html) - jumpstarter-dev#864 — ExporterSet controller bootstrap (Phase 2) - jumpstarter-dev#904 — ExporterSet status reporting --------- Co-authored-by: Miguel Angel Ajo Pelayo <miguelangel@ajo.es>
Add deployment-style status conditions and replica counters to ExporterSet CRs, providing visibility into pod health, exporter availability, and scaling state on every reconcile loop.
depends on #864
tested with: