Add full support for ExporterStatus field and state transitions #195
Conversation
WalkthroughAdds new exporter status fields (ExporterStatusValue, StatusMessage), updates protobuf conversion and controller/service status handling (including a ReportStatus gRPC), introduces CRD manifests for Client, Lease, ExporterAccessPolicy, and Exporter (with printer columns), and adjusts Helm/Makefile steps to copy CRDs into chart templates. Changes
Sequence DiagramsequenceDiagram
participant Exporter as Exporter (gRPC client)
participant Service as ControllerService (gRPC handler)
participant API as Kubernetes API (Exporter CRD)
Exporter->>Service: ReportStatus(pb.ExporterStatus, message)
Service->>Service: Authenticate exporter identity
Service->>Service: protoStatusToString(pb.Status) => CRD string
Service->>API: Patch Exporter.status {exporterStatus, statusMessage, lastSeen}
Service->>Service: syncOnlineConditionWithStatus(crdStatus)
Service-->>Exporter: Ack / status updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/controller/lease_controller_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yaml (1)
1-69: Consider adding printer columns for better kubectl experience.The Client CRD is well-structured, but adding printer columns (e.g., for username and endpoint) would improve the kubectl get output experience.
Example printer columns to consider:
- additionalPrinterColumns: - jsonPath: .spec.username name: Username type: string - jsonPath: .status.endpoint name: Endpoint type: string name: v1alpha1deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml (1)
1-166: Consider adding printer columns for better kubectl experience.The ExporterAccessPolicy CRD is well-structured with proper schema validation. Adding printer columns would enhance the user experience when viewing these resources with kubectl.
Example printer columns to consider:
- additionalPrinterColumns: - jsonPath: .spec.policies[0].priority name: Priority type: integer - jsonPath: .spec.policies[0].spotAccess name: Spot Access type: boolean name: v1alpha1internal/service/controller_service.go (1)
278-303: Consider whether empty string should be treated as online when explicitly set.The
isOnlinelogic treats empty string ("") as offline. This is reasonable for uninitialized state, but if an exporter explicitly reports an empty status, the behavior might be unexpected.Also, consider extracting the online status values into a helper or constant set for maintainability if more statuses are added later.
func syncOnlineConditionWithStatus(exporter *jumpstarterdevv1alpha1.Exporter) { - isOnline := exporter.Status.ExporterStatusValue != jumpstarterdevv1alpha1.ExporterStatusOffline && - exporter.Status.ExporterStatusValue != jumpstarterdevv1alpha1.ExporterStatusUnspecified && - exporter.Status.ExporterStatusValue != "" + status := exporter.Status.ExporterStatusValue + isOnline := status != "" && + status != jumpstarterdevv1alpha1.ExporterStatusOffline && + status != jumpstarterdevv1alpha1.ExporterStatusUnspecified
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
Makefile(1 hunks)api/v1alpha1/exporter_helpers.go(2 hunks)api/v1alpha1/exporter_types.go(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_leases.yaml(1 hunks)deploy/helm/jumpstarter/crds/jumpstarter.dev_exporters.yaml(3 hunks)deploy/operator/config/crd/bases/jumpstarter.dev_exporters.yaml(3 hunks)hack/deploy_with_helm.sh(1 hunks)internal/controller/exporter_controller.go(2 hunks)internal/service/controller_service.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-07T09:38:12.913Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter-controller PR: 155
File: internal/service/controller_service.go:125-125
Timestamp: 2025-08-07T09:38:12.913Z
Learning: In internal/service/controller_service.go, authentication failures in the Register() method should be logged at Info level because Register() is the initial connection point where misconfigurations on the exporter side are expected and not considered errors of the controller or client API. Other methods like Unregister(), Status(), and Dial() assume previously authenticated connections, so authentication failures there are more concerning and warrant Error level logging.
Applied to files:
internal/service/controller_service.go
📚 Learning: 2025-11-14T15:47:36.325Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 190
File: api/v1alpha1/exporter_helpers.go:16-24
Timestamp: 2025-11-14T15:47:36.325Z
Learning: In the jumpstarter-controller project, migration annotations (jumpstarter.dev/migrated-namespace and jumpstarter.dev/migrated-uid) that override namespace and UID values in authentication tokens are acceptable without additional validation webhooks because the security model assumes only administrators have write access to Exporter and Client resources via K8s RBAC.
Applied to files:
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yamldeploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml
🧬 Code graph analysis (1)
internal/controller/exporter_controller.go (1)
api/v1alpha1/exporter_types.go (2)
ExporterStatusOffline(58-58)ExporterConditionTypeOnline(52-52)
🪛 GitHub Actions: Unit/Functional tests
Makefile
[error] 1-1: Test suite failed: 53 Passed, 3 Failed, 0 Pending, 0 Skipped (make test returned exit code 2).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-test-operator
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-release-0-7
🔇 Additional comments (20)
hack/deploy_with_helm.sh (1)
50-53: LGTM! CRD management approach is well-documented.The addition of
--skip-crdswith the explanatory comment correctly reflects the shift to managing CRDs via templates/crds/ rather than the special crds/ directory. This aligns with the Makefile changes that copy CRDs into the templates directory.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_leases.yaml (1)
1-235: LGTM! Well-structured CRD definition.The Lease CRD follows Kubernetes conventions with proper schema validation, status subresource, and useful printer columns. The required fields and standard condition structure are correctly defined.
Makefile (2)
67-67: LGTM! CRD copy step properly integrates with Helm template workflow.The additional copy step ensures CRDs are available in the Helm chart templates directory, supporting the templated CRD management approach introduced in this PR.
1-292: Based on my thorough verification of the codebase, I can now provide the rewritten review comment.
The review comment references test failures without providing evidence or actual CI logs.
The claim of "3 failed tests out of 56 total" cannot be verified in the codebase. The actual test structure contains:
- 9 non-e2e test files (15 standard Go tests + 78 Ginkgo test cases)
- 2 e2e test files (run separately)
The provided shell script searches for patterns (
exporterStatus,statusMessage) that do not exist in any test files, suggesting the investigation was speculative rather than based on observed failures.Regarding the Makefile change itself: The modification is valid and safe. It adds a single copy command to the
manifeststarget:cp deploy/helm/jumpstarter/crds/* deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/All source and destination directories exist and are properly formed. The change does not introduce issues.
To properly investigate the claimed test failures, actual CI workflow run logs showing:
- Which specific tests failed (test names and packages)
- Error messages or stack traces
- Confirmation that failures are reproducible
would be required. Without this evidence, the review comment cannot be validated.
api/v1alpha1/exporter_types.go (3)
41-45: LGTM! Status fields properly defined with validation.The new ExporterStatusValue and StatusMessage fields are correctly added with proper enum validation and documentation. The enum covers all expected states including failure scenarios.
56-65: LGTM! Status constants follow Kubernetes conventions.The status constants use PascalCase (Kubernetes style) and align perfectly with the enum validation. The constants cover the complete state machine including offline, available, hook states, and failure states.
69-70: LGTM! Printer columns enhance kubectl output.The printer columns for Status and Message provide useful visibility in kubectl get output. The Message column with priority=1 ensures it's only shown with verbose output.
deploy/helm/jumpstarter/crds/jumpstarter.dev_exporters.yaml (3)
17-24: LGTM! Printer columns properly configured.The Status and Message printer columns match the kubebuilder markers in the Go code and will provide useful visibility in kubectl output.
144-156: LGTM! ExporterStatus field correctly defined with enum validation.The exporterStatus field has proper enum validation with all 8 status values matching the constants defined in api/v1alpha1/exporter_types.go. The description clearly explains its purpose.
176-179: LGTM! StatusMessage field properly defined.The statusMessage field is correctly added as an optional field with a clear description of its purpose for human-readable state information.
deploy/operator/config/crd/bases/jumpstarter.dev_exporters.yaml (1)
17-24: LGTM! CRD manifest consistent with Helm version.The operator CRD manifest correctly includes the same printer columns and status fields (exporterStatus and statusMessage) as the Helm CRD version. The Makefile ensures consistency by copying CRDs to both locations.
Also applies to: 144-156, 176-179
api/v1alpha1/exporter_helpers.go (2)
28-39: LGTM!The
ToProtobufmethod correctly populates the newStatusandStatusMessagefields. The backward compatibility comment is helpful for understanding the deprecatedOnlinefield derivation.
41-61: LGTM!The
stringToProtoStatushelper correctly maps all CRD status string constants to their corresponding protobuf enum values. The default case returningEXPORTER_STATUS_UNSPECIFIEDis appropriate for unknown/empty values.internal/service/controller_service.go (2)
215-254: LGTM!The
ReportStatushandler correctly follows the established patterns:
- Authentication failure logged at Info level (consistent with learnings from
Register)- Updates both
ExporterStatusValueandStatusMessagefrom the request- Updates
LastSeento maintain online tracking- Syncs the deprecated Online condition for backward compatibility
256-276: LGTM!The
protoStatusToStringmapping is the exact inverse ofstringToProtoStatusinexporter_helpers.go, ensuring consistent bidirectional conversion between proto enum and CRD string values.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml (2)
17-24: LGTM!The printer columns are well-configured. The
Statuscolumn will appear by default, whileMessagehaspriority: 1to keep the default output clean while still accessible via-o wide.
144-156: LGTM!The
exporterStatusenum values correctly match the Go constants defined inexporter_types.go. The enum validation will help catch invalid status values at the API server level.internal/controller/exporter_controller.go (3)
170-172: LGTM!Correctly initializes the exporter to Offline status with an appropriate message when it has never been seen.
182-186: Good defensive check for status preservation.The conditional
if exporter.Status.ExporterStatusValue != ExporterStatusOfflineprevents overwriting an existing Offline status (and its message) that may have been set by the exporter itself during graceful shutdown.
189-210: LGTM!The logic correctly handles the case where an exporter gracefully reports Offline status even when
LastSeenis recent. This allows exporters to signal intentional shutdown without waiting for the 1-minute timeout, improving user experience and status accuracy.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
look at the check-bundle CI too, you need to run the make command specified in there to update the bundle.
I am suspecting that I can drop all that bundle from being included in the code, and generate in runtime when submitting to other repos, or creating the bundle container. Sorry about that ;)
| @@ -0,0 +1,69 @@ | |||
| --- | |||
There was a problem hiding this comment.
Not sure why they got added here, now they must be on the crd directory only
| scope: Namespaced | ||
| versions: | ||
| - name: v1alpha1 | ||
| - additionalPrinterColumns: |
| echo -e "${GREEN}Performing helm ${METHOD} ...${NC}" | ||
|
|
||
| # install/update with helm | ||
| # --skip-crds: CRDs are managed via templates/crds/ instead of the special crds/ directory |
There was a problem hiding this comment.
The problem is that they are replicated now in two places deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds and deploy/helm/jumpstarter/crds
Should only be on the last one.
This PR supports jumpstarter-dev/jumpstarter#606 by providing a controller-side ExporterStatus implementation and proper online status handling when the exporter marks itself as OFFLINE.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.