feat: reconcile ExporterSet provisioner controllers in operator#908
Conversation
📝 WalkthroughWalkthroughAdds optional exporter-set provisioner configuration to the Jumpstarter API and CRD, reconciles per-provisioner Deployments and RBAC resources, cleans up disabled controllers, and includes exporter-set availability in Jumpstarter readiness status. ChangesExporterSet controller support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JumpstarterCR
participant JumpstarterReconciler
participant KubernetesAPI
participant ExporterSetController
JumpstarterCR->>JumpstarterReconciler: Provide exporterSets configuration
JumpstarterReconciler->>KubernetesAPI: Reconcile RBAC and provisioner Deployments
KubernetesAPI->>ExporterSetController: Start configured controller
ExporterSetController->>KubernetesAPI: Report Deployment availability
JumpstarterReconciler->>JumpstarterCR: Update readiness conditions
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controller/deploy/operator/internal/controller/jumpstarter/exporterset.go (1)
542-546: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden provisioner-name sanitization (and validate
Name).
sanitizeProvisionerNameonly lowercases and replaces.; any other character invalid in a DNS-1123 resource name (e.g._,/, spaces) passes through into%s-exporterset-%snames and label values, causingCreateOrUpdateto be rejected by the API server and the whole reconcile to fail.ProvisionerConfig.Namehas onlyMinLength=1(noPattern), so nothing upstream prevents such input. Additionally, distinct names can collapse to the same sanitized suffix (e.g.qemu.devandqemu-dev), colliding on Deployment/Role names and inenabledProvisioners(last-write-wins).Consider adding a
+kubebuilder:validation:PatternonProvisionerConfig.Name(injumpstarter_types.go) to constrain input to a DNS-subdomain-like form, and/or reject/normalize remaining invalid characters here.🤖 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/deploy/operator/internal/controller/jumpstarter/exporterset.go` around lines 542 - 546, Harden sanitizeProvisionerName and ProvisionerConfig.Name validation so provisioner names produce valid, collision-resistant DNS-1123 resource and label suffixes. Add the appropriate kubebuilder Pattern to ProvisionerConfig.Name in jumpstarter_types.go, and update sanitizeProvisionerName to validate or reject characters and transformations that could create invalid or colliding names; ensure invalid input cannot reach the %s-exporterset-%s resources or enabledProvisioners.
🤖 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.
Nitpick comments:
In `@controller/deploy/operator/internal/controller/jumpstarter/exporterset.go`:
- Around line 542-546: Harden sanitizeProvisionerName and ProvisionerConfig.Name
validation so provisioner names produce valid, collision-resistant DNS-1123
resource and label suffixes. Add the appropriate kubebuilder Pattern to
ProvisionerConfig.Name in jumpstarter_types.go, and update
sanitizeProvisionerName to validate or reject characters and transformations
that could create invalid or colliding names; ensure invalid input cannot reach
the %s-exporterset-%s resources or enabledProvisioners.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6bdaccc-67e6-4acf-8265-82e2bd593128
📒 Files selected for processing (9)
controller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/config/rbac/role.yamlcontroller/deploy/operator/internal/controller/jumpstarter/exporterset.gocontroller/deploy/operator/internal/controller/jumpstarter/exporterset_test.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.gocontroller/deploy/operator/internal/controller/jumpstarter/status.go
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
I didn't really read everything, but some overall things I saw.
You may want to extend this to deploy the qemu provisioner even if currently does nothing: https://github.com/jumpstarter-dev/jumpstarter/blob/main/controller/hack/deploy_with_operator.sh#L174
| { | ||
| APIGroups: []string{"virtualtarget.jumpstarter.dev"}, | ||
| Resources: []string{"exportersets"}, | ||
| Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete"}, |
There was a problem hiding this comment.
can we double check if we need update/patch/delete here and why? we may need to update the status, which is a different policy rule, but the object itself?
There was a problem hiding this comment.
I've checked this. for the resource exportersets the only read verbs get list watch are sufficient for controller.
addressed in bc26068
… to its namespace
maboras-rh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hack/deploy_with_operator.sh`:
- Line 35: Make exporter image loading in deploy_with_operator.sh conditional on
DEPLOY_JUMPSTARTER=true, so operator-only deployment does not require it. In
controller/Makefile lines 198-199, remove docker-build-exporter-set-controller
from the operator-only target.
🪄 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 Plus
Run ID: 13010112-d351-4a8f-9978-1b7972a4b506
📒 Files selected for processing (12)
controller/Makefilecontroller/cmd/exporter-set-controller/main.gocontroller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/config/rbac/role.yamlcontroller/deploy/operator/internal/controller/jumpstarter/exporterset.gocontroller/deploy/operator/internal/controller/jumpstarter/exporterset_test.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.gocontroller/hack/deploy_varscontroller/hack/deploy_with_operator.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
- controller/deploy/operator/api/v1alpha1/jumpstarter_types.go
- controller/deploy/operator/internal/controller/jumpstarter/exporterset_test.go
- controller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.go
- controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go
- controller/deploy/operator/internal/controller/jumpstarter/exporterset.go
| # load the container images into the cluster | ||
| load_image "${IMG}" | ||
| load_image "${OPERATOR_IMG}" | ||
| load_image "${EXPORTER_SET_CONTROLLER_IMG}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not require the exporter image for operator-only deployment.
deploy-operator sets DEPLOY_JUMPSTARTER=false, but deploy_with_operator.sh loads the exporter image before checking that flag. This makes the operator-only target unnecessarily build and require an exporter image, and can fail even though no Jumpstarter CR is deployed.
controller/hack/deploy_with_operator.sh#L35-L35: load the exporter image only whenDEPLOY_JUMPSTARTER=true.controller/Makefile#L198-L199: removedocker-build-exporter-set-controllerfrom the operator-only target.
📍 Affects 2 files
controller/hack/deploy_with_operator.sh#L35-L35(this comment)controller/Makefile#L198-L199
🤖 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/hack/deploy_with_operator.sh` at line 35, Make exporter image
loading in deploy_with_operator.sh conditional on DEPLOY_JUMPSTARTER=true, so
operator-only deployment does not require it. In controller/Makefile lines
198-199, remove docker-build-exporter-set-controller from the operator-only
target.
There was a problem hiding this comment.
the operator will deploy jumpstarter and the exporterset controller as well.. (see lines 207-212). so we will need this image.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- deploy-operator target context ---'
rg -n -A12 -B4 '^[[:space:]]*deploy-operator[[:space:]]*:' controller/Makefile
printf '%s\n' '--- DEPLOY_JUMPSTARTER assignments and calls ---'
rg -n -A3 -B3 '\bDEPLOY_JUMPSTARTER\b|deploy_with_operator\.sh' controllerLength of output: 2960
@mangelajo, you’re right for the normal deploy flow: Lines 207-212 configure the Jumpstarter CR, which causes the operator to create the exporter-set controller Deployment, so that image must be built and loaded there.
However, the finding is specifically about deploy-operator. That target invokes the script with DEPLOY_JUMPSTARTER=false (Makefile Line 200), and the script exits at Lines 58-60 before generating or applying the CR. Thus, in this path, no exporter-set controller is deployed and the image build/load is unnecessary.
The scope should remain narrow: conditionally build/load EXPORTER_SET_CONTROLLER_IMG only for the full Jumpstarter deployment path.
|
PTAL - @mangelajo @bennyz @bkhizgiy @evakhoni |
The deploy_with_operator.sh script loads the exporterset-controller image into the kind cluster, but the E2E workflow never built it. With SKIP_BUILD=true in CI, kind_load_image fails because the image does not exist locally. Add a build-exporterset-controller-image job (parallel with the existing controller/operator builds) that builds, caches, and uploads the image as an artifact. Update load-e2e-artifacts to download and docker-load it alongside the other images.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/e2e.yaml:
- Around line 145-148: Update the checkout steps in the build-controller-image,
build-operator-image, and build-exporterset-controller-image jobs to set
persist-credentials to false alongside fetch-depth, preventing checkout
credentials from entering build contexts or uploaded image artifacts.
🪄 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 Plus
Run ID: b9a37bdb-9bf2-4005-9e63-ea30abb68bb7
📒 Files selected for processing (2)
.github/actions/load-e2e-artifacts/action.yaml.github/workflows/e2e.yaml
|
Added a commit to handle the new image E2E setup and caching |
The operator now deploys the exporter-set-controller (#908), so remove the standalone YAML and Makefile targets. Keep sample CR manifests for local kind testing after operator install. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
This PR extends the Jumpstarter operator to deploy and manage ExporterSet provisioner controllers as Kubernetes Deployments, as defined in JEP-0014. It builds on the ExporterSet controller bootstrap in #864.
When
spec.exporterSets.provisionersis configured, the operator creates one Deployment per enabled provisioner, running the sharedjumpstarter-exporterset-controllerimage with--provisioner=<name>. Each provisioner gets its own ServiceAccount, Role, and RoleBinding.API changes
Adds
spec.exporterSetsto theJumpstarterCR:image,imagePullPolicy,resourcesprovisioners[]):name,enabled, optionalimageoverride,replicas, optionalresourcesoverrideProvisionerConfig.namevalidation: DNS-subdomain-safe pattern (^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$); names that collide after sanitization are rejected at reconcile timeAdds status condition
ExporterSetControllersReady, included in overallReadyonly when at least one provisioner is enabled.Operator behavior
ExporterSetControllersReadybased on each enabled provisioner Deployment'sAvailableconditionrolesdelete/patch,podsget/list/watch, andvirtualtarget.jumpstarter.devread/status permissions (required to create provisioner Roles with those rules)Exporter-set-controller runtime fix
The provisioner controller binary is updated to work with namespaced Roles:
NAMESPACEenv var (set via downward API in the Deployment)LeaderElectionNamespaceto the pod namespaceWithout this, informers issue cluster-scoped List/Watch calls that are rejected by the API server.
Dev / deploy tooling
make deployandmake deploy-operatornow also buildjumpstarter-exporterset-controllerhack/deploy_with_operator.shdeploysqemu.jumpstarter.devas a default enabled provisioner in the generated Jumpstarter CRExample CR snippet
Test plan
exporterset_test.go)ExporterSetControllersReadystatus (jumpstarter_controller_test.go)make -C controller/deploy/operator testmake deployand verifyqemu.jumpstarter.devprovisioner Deployment, SA, and Role come up injumpstarter-labRelated