feat: add enabled field to Exporter CRD and allowDisabled to Lease (JEP-0014 Phase 1)#863
Merged
Merged
Conversation
…EP-0014 Phase 1) Add spec.enabled boolean field to the Exporter CRD (default: true). When set to false, the jumpstarter-controller will not assign new leases to the exporter. This is useful for: - Temporarily taking a physical exporter offline for maintenance - Graceful scale-down of virtual exporter pools (JEP-0014) - Investigating broken exporters without interference Add spec.allowDisabled boolean field to the Lease CRD. When set to true alongside exporterRef, allows leasing a disabled exporter for investigation purposes. If a user targets a disabled exporter without allowDisabled, the lease is marked Unsatisfiable with a helpful error message explaining how to use --allow-disabled. Changes: - Exporter CRD: add spec.enabled field with kubebuilder default=true - Exporter CRD: add Enabled printer column for kubectl get exporters - Lease CRD: add spec.allowDisabled field - Controller: add filterOutDisabledExporters() for selector-based leases - Controller: check enabled status for ExporterRef-based leases - Proto: add enabled field to Exporter message (field 6) - Proto: add allow_disabled field to Lease message (field 14) - CLI: add --allow-disabled flag to jmp create lease - CLI: show ENABLED column in jmp get exporters output - Tests: unit tests for filterOutDisabledExporters - Tests: integration tests for disabled exporter scenarios Implements: JEP-0014 Phase 1 Ref: #41
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds exporter enabled-state and lease allow-disabled fields across the Go API, CRDs, controller logic, protobuf contracts, and Python client/CLI/config code. Also switches protobuf generation to local proto sources and regenerates protocol stubs and docs. ChangesExporter Enabled / Lease Allow-Disabled Feature
Protobuf Toolchain Migration and Generated Stub Docstring Refresh
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ClientConfig
participant ClientService
participant LeaseReconciler
CLI->>ClientConfig: create_lease(allow_disabled)
ClientConfig->>ClientService: CreateLease(allow_disabled)
ClientService->>LeaseReconciler: Lease.allow_disabled=true
LeaseReconciler->>LeaseReconciler: check exporter IsEnabled()
alt Exporter disabled and not allowed
LeaseReconciler-->>ClientService: Unsatisfiable(ExporterDisabled)
else Exporter enabled or allowed
LeaseReconciler-->>ClientService: lease assignment proceeds
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
mangelajo
commented
Jul 7, 2026
Per review feedback, disabled exporters are now hidden from jmp get exporters output by default. Use --allow-disabled to include them (which also adds an ENABLED column to distinguish them). - ExporterList filters out disabled exporters unless include_disabled=True - jmp get exporters: add --allow-disabled flag - ENABLED column only shown when --allow-disabled is used - WithOptions: add show_disabled field - Tests: revert unconditional ENABLED column, add TestWithDisabledOption
Update buf.gen.yaml in both controller/ and python/ to use the local protocol/proto directory instead of the external jumpstarter-protocol git repo. This makes the monorepo the single source of truth for proto definitions and removes the need to sync changes to an external repo before regenerating stubs. The Makefile protobuf-gen targets now mount the monorepo root into the container so the ../protocol/proto relative path resolves correctly.
Add missing allow_disabled argument to test calls that invoke get_exporters and CreateLease after the new parameter was added.
Add tests for _visible_exporters(), rich_add_rows, rich_add_names, model_dump_json, and model_dump with disabled exporter filtering to meet the 80% diff coverage requirement.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/grpc.py (1)
341-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared
exclude_fieldslogic.
model_dump_jsonandmodel_dumpduplicate the sameexclude_fieldsconstruction and_visible_exporters()iteration. Extracting a helper would reduce maintenance burden when new conditional-exclusion fields are added.♻️ Proposed helper extraction
+def _build_exporters_data(self): + """Build the exporters list with conditional field exclusion.""" + exclude_fields = set() + if not self.include_leases: + exclude_fields.add("lease") + if not self.include_online: + exclude_fields.add("online") + if not self.include_status: + exclude_fields.add("status") + if not self.include_disabled: + exclude_fields.add("enabled") + return { + "exporters": [ + exporter.model_dump(mode="json", exclude=exclude_fields) + for exporter in self._visible_exporters() + ] + } + def model_dump_json(self, **kwargs): json_kwargs = {k: v for k, v in kwargs.items() if k in {"indent", "separators", "sort_keys", "ensure_ascii"}} - - # Determine which fields to exclude - exclude_fields = set() - if not self.include_leases: - exclude_fields.add("lease") - if not self.include_online: - exclude_fields.add("online") - if not self.include_status: - exclude_fields.add("status") - if not self.include_disabled: - exclude_fields.add("enabled") - - data = { - "exporters": [ - exporter.model_dump(mode="json", exclude=exclude_fields) - for exporter in self._visible_exporters() - ] - } + data = self._build_exporters_data() return json.dumps(data, **json_kwargs) def model_dump(self, **kwargs): - exclude_fields = set() - if not self.include_leases: - exclude_fields.add("lease") - if not self.include_online: - exclude_fields.add("online") - if not self.include_status: - exclude_fields.add("status") - if not self.include_disabled: - exclude_fields.add("enabled") - - return { - "exporters": [ - exporter.model_dump(mode="json", exclude=exclude_fields) - for exporter in self._visible_exporters() - ] - } + return self._build_exporters_data()🤖 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 `@python/packages/jumpstarter/jumpstarter/client/grpc.py` around lines 341 - 379, Extract the duplicated exporter serialization logic in grpc.py by introducing a shared helper around model_dump_json and model_dump, reusing the same exclude_fields construction and _visible_exporters() iteration in both methods. Keep the helper centered on the unique symbols model_dump_json, model_dump, and _visible_exporters so any future include_* field changes only need to be updated in one place.
🤖 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/controller/lease_controller_test.go`:
- Around line 818-834: The “should be unsatisfiable when all matching exporters
are disabled” test in lease_controller_test.go only checks that
Status.ExporterRef is nil, which can also match a Pending result. Update this
test to assert the lease status condition is explicitly Unsatisfiable, using the
same condition helper/pattern already used in the other lease reconciliation
tests (for example, the assertions around reconcileLease and getLease elsewhere
in this file), so the test verifies both no ExporterRef and the correct terminal
condition.
In `@controller/internal/controller/lease_controller.go`:
- Around line 263-270: The selector-based path in reconcileStatusExporterRef is
conflating “no exporters matched” with “all matching exporters were disabled”
after filterOutDisabledExporters. Add an explicit check right after filtering
list results from ListMatchingExporters so that when listed.Items is non-empty
but matchingExporters becomes empty, you return a disabled-exporter-specific
status/reason instead of falling through to the NoAccess branch. Keep the
existing no-match and policy-no-access handling separate by using the
matchingExporters, listed.Items, and reconcileStatusExporterRef branches to
distinguish the cases.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py`:
- Around line 341-379: Extract the duplicated exporter serialization logic in
grpc.py by introducing a shared helper around model_dump_json and model_dump,
reusing the same exclude_fields construction and _visible_exporters() iteration
in both methods. Keep the helper centered on the unique symbols model_dump_json,
model_dump, and _visible_exporters so any future include_* field changes only
need to be updated in one place.
🪄 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: 1c80cb44-48e3-4a34-98ed-c3e495add5cc
⛔ Files ignored due to path filters (8)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/client/v1/client_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/common.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/jumpstarter.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/jumpstarter_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/kubernetes.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/router.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/router_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (35)
controller/Makefilecontroller/api/v1alpha1/exporter_helpers.gocontroller/api/v1alpha1/exporter_types.gocontroller/api/v1alpha1/lease_helpers.gocontroller/api/v1alpha1/lease_types.gocontroller/api/v1alpha1/zz_generated.deepcopy.gocontroller/buf.gen.yamlcontroller/deploy/operator/config/crd/bases/jumpstarter.dev_exporteraccesspolicies.yamlcontroller/deploy/operator/config/crd/bases/jumpstarter.dev_exporters.yamlcontroller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlcontroller/internal/controller/lease_controller.gocontroller/internal/controller/lease_controller_test.goprotocol/proto/jumpstarter/client/v1/client.protopython/Makefilepython/buf.gen.yamlpython/packages/jumpstarter-cli/jumpstarter_cli/create.pypython/packages/jumpstarter-cli/jumpstarter_cli/create_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/get.pypython/packages/jumpstarter-cli/jumpstarter_cli/get_test.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyipython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/grpc_test.pypython/packages/jumpstarter/jumpstarter/config/client.pypython/packages/jumpstarter/jumpstarter/config/client_config_test.py
bennyz
reviewed
Jul 8, 2026
- proto: change Exporter.enabled to optional bool for backward compat When an old server omits the field, clients now correctly treat exporters as enabled (default true) instead of disabled (proto3 bool default false). Matches existing pattern: optional bool only_active, optional bool release_lease. - controller: add explicit AllDisabled status when all selector-matching exporters are disabled, instead of falling through to the misleading NoAccess/0-exporters message. - test: add Unsatisfiable condition assertion to the all-disabled test case, matching the pattern used in other lease reconciliation tests.
bennyz
approved these changes
Jul 8, 2026
mangelajo
commented
Jul 8, 2026
| string status_message = 5 [(google.api.field_behavior) = OUTPUT_ONLY]; | ||
| // Whether the exporter is enabled for lease assignment. | ||
| // When false, the controller will not assign new leases to this exporter. | ||
| bool enabled = 6 [(google.api.field_behavior) = OUTPUT_ONLY]; |
Member
Author
There was a problem hiding this comment.
Claude AI analysis
@bennyz is right — this should be optional bool for backward compatibility.
In proto3, a non-optional bool has an implicit default of false with no presence tracking. When an old server (that doesn't know about the enabled field) sends an Exporter message to a new client, the field will be absent on the wire and the client deserializes it as false — making every exporter from an old server appear disabled.
The generated Go getter confirms this (client.pb.go):
func (x *Exporter) GetEnabled() bool {
if x != nil {
return x.Enabled
}
return false // wrong default for compat!
}With optional bool enabled = 6, proto3 generates presence-tracking, so client code can distinguish "server didn't send this field" (old server -> treat as enabled) from "server explicitly sent false" (disabled).
The project already follows this pattern:
optional bool only_active = 5inListLeasesRequest(this file, line 207)optional bool release_lease = 3injumpstarter.proto- The K8s CRD side already uses
*boolwith nil-means-true inexporter_helpers.go:15-16
allow_disabled (field 14) is fine as plain bool — its default of false is the correct backward-compatible behavior.
Fix: change this line to:
optional bool enabled = 6 [(google.api.field_behavior) = OUTPUT_ONLY];Then update Go serialization in exporter_helpers.go:44 to use a pointer:
Enabled: proto.Bool(e.IsEnabled()),And the Python client (grpc.py:112) to handle the optional:
enabled=data.enabled if data.HasField("enabled") else True,
kirkbrauer
approved these changes
Jul 8, 2026
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.9
git worktree add -d .worktree/backport-863-to-release-0.9 origin/release-0.9
cd .worktree/backport-863-to-release-0.9
git switch --create backport-863-to-release-0.9
git cherry-pick -x d46e91a777ecbb988ef8e5284487e280b1574c10 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Phase 1 of JEP-0014: Virtual Scalable Exporters — the
enabledfield on the Exporter CRD.This is the foundational building block for graceful scale-down of virtual exporter pools and is immediately useful for physical exporter maintenance workflows.
What Changed
Exporter CRD:
spec.enabledfieldtrue) that controls whether the controller will assign new leases to this exporterfalse, the exporter is excluded from selector-based lease matching and explicitly rejected forexporterRef-based leasesenabledfield behave asenabled: trueEnabledprinter column visible inkubectl get exportersLease CRD:
spec.allowDisabledfieldexporterRefallowDisabled, the lease gets anUnsatisfiablecondition with a helpful error message:Controller Logic
filterOutDisabledExporters(): filters disabled exporters from selector-based lease matching (before policy checks)ExporterRefpath: checks if the targeted exporter is disabled, returnsUnsatisfiable("ExporterDisabled")unlessallowDisabledis setProtocol (gRPC)
Exportermessage: addedenabledfield (field 6,OUTPUT_ONLY)Leasemessage: addedallow_disabledfield (field 14,IMMUTABLE)ToProtobuf()methods updated to serialize both fieldsbuf.gen.yamlupdated to use localprotocol/protoas source of truth (replaces external git repo reference)CLI
jmp create lease --allow-disabled: new flag to permit leasing disabled exportersjmp get exporters: disabled exporters are hidden by default; use--allow-disabledto show them (adds anENABLEDcolumn)Use Cases
Lab maintenance
Graceful scale-down (JEP-0014)
Tests
Go (controller)
filterOutDisabledExporterswith nil/true/falseEnabledvalues, slice immutabilityExporterRef→ExporterDisabledreason with helpful messageExporterRef+AllowDisabled: true→ lease succeedsPython (CLI)
create_test.pyto passallow_disabledparametergrpc_test.pydisplay tests — ENABLED column only shown withshow_disabledTestWithDisabledOptiontests for conditional ENABLED columnBackward Compatibility
spec.enableddefaults totrue— existing Exporters behave exactly as beforespec.allowDisableddefaults tofalse— existing Leases behave exactly as beforeReferences