Add pre/post lease hooks #606
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds exporter lifecycle and readiness: new ExporterStatus/LogSource enums and protobuf fields/stubs; controller registration, listen/status streams, and EndSession RPC; per-lease LeaseContext and session orchestration; HookExecutor for before/after-lease hooks; logging source mapping; client preflight GetStatus gating; tests and CLI/status display updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Exporter
participant Controller
participant HookExec as HookExecutor
participant LeaseCtx as LeaseContext
Note over Exporter,Controller: Exporter registers and streams with Controller
Exporter->>Controller: Register(metadata)
Controller-->>Exporter: Ack (uuid)
Controller->>Exporter: Listen stream -> lease assignment
Exporter->>LeaseCtx: create LeaseContext(lease_name)
Exporter->>HookExec: run_before_lease_hook(lease_ctx)
HookExec->>Controller: report_status(BEFORE_LEASE_HOOK)
HookExec-->>Exporter: success / HookExecutionError
Exporter->>Exporter: update session -> LEASE_READY
Client->>Exporter: DriverCall (preflight)
Exporter->>Controller: GetStatus (if needed)
alt status in [LEASE_READY, BEFORE_LEASE_HOOK, AFTER_LEASE_HOOK]
Exporter-->>Client: forward DriverCall -> driver response
else
Exporter-->>Client: raise ExporterNotReady
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Nice!! this will be really really useful |
|
This would be a hook point when the connection is ready and you can start talking to the exporter. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (1)
1-20: Generated gRPC typing stub looks correctThe new
_MaybeAsyncIterator,_ServicerContext, and GRPC version symbols follow the same pattern as existing*_pb2_grpc.pyifiles and are clearly marked as generated; no manual changes needed here.
🧹 Nitpick comments (17)
packages/jumpstarter/jumpstarter/exporter/logging_protocol.py (1)
1-22: LoggerRegistration protocol is clear and minimalThe
LoggerRegistrationProtocol cleanly captures the one needed capability (add_logger_source) and avoids additional coupling; good fit for typing and circular‑dependency avoidance. If you ever needisinstancechecks on this, consider adding@runtime_checkable, but it’s not required today.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py (1)
303-309: Exporter status test expectations are updated consistentlyThe fixtures now populate
exporter_status="Available"and the JSON/YAML expectations includeexporterStatusplusstatusMessage: null, with corresponding assertions that"Available"appears in human‑readable output. This keeps the admin CLI tests in sync with the extended exporter status surface across single exporters, device‑rich exporters, and list views.It may be worth adding a future test case where
statusMessageis non‑null to exercise that branch of the serialization as well, but the current coverage is fine for this change.Also applies to: 324-327, 331-345, 358-359, 400-408, 437-440, 445-463, 476-477, 523-540, 560-563, 588-603, 614-616, 636-637, 677-684, 691-697, 725-727, 750-753, 760-777, 791-793, 812-813
packages/jumpstarter/jumpstarter/config/client.py (1)
123-133:channeltyping +include_statuswiring look good, but double‑check status propagation with leases
- The explicit
grpc.aio.Channelreturn type onchannel()and the newinclude_statusflag onlist_exporters()are both clean and consistent with the CLI changes.- Mirroring the existing
include_onlinebehavior by taggingexporters_response.include_statusis reasonable for display logic.One thing to verify: when
include_leasesisTrue, you rebuildexporters_response.exportersusingExporter(namespace=..., name=..., labels=..., online=exporter.online, lease=lease). IfExporterhas newly added status fields (e.g., exporter status / message), they are not currently forwarded into this wrapper, which could drop status information precisely in the--with leases,statuscase.It’s worth confirming the
Exportertype and, if it now carries status, passing those fields through when constructingExporterso status remains visible when leases are included.Also applies to: 156-171, 184-197
packages/jumpstarter/jumpstarter/config/exporter_test.py (1)
140-149: Consider adding timeout assertions for completeness.The test verifies script content and round-trip behavior but doesn't assert the timeout values. Since the YAML specifies
timeout: 600for both hooks, consider adding assertions to verify timeout parsing:assert config.hooks.before_lease.script == 'echo "Pre-lease hook for $LEASE_NAME"\nj power on\n' assert config.hooks.after_lease.script == 'echo "Post-lease hook for $LEASE_NAME"\nj power off\n' + assert config.hooks.before_lease.timeout == 600 + assert config.hooks.after_lease.timeout == 600 # Test that it round-trips correctly path.unlink() ExporterConfigV1Alpha1.save(config) reloaded_config = ExporterConfigV1Alpha1.load("test-hooks") assert reloaded_config.hooks.before_lease.script == config.hooks.before_lease.script assert reloaded_config.hooks.after_lease.script == config.hooks.after_lease.script + assert reloaded_config.hooks.before_lease.timeout == config.hooks.before_lease.timeout + assert reloaded_config.hooks.after_lease.timeout == config.hooks.after_lease.timeoutpackages/jumpstarter/jumpstarter/client/grpc.py (1)
96-99: Consider simplifying the status parsing logic.The
hasattrcheck is likely unnecessary since protobuf message fields are always defined (they default to their zero value). You could simplify to just check ifdata.statusis truthy:- status = None - if hasattr(data, "status") and data.status: - status = ExporterStatus.from_proto(data.status) - return cls(namespace=namespace, name=name, labels=data.labels, online=data.online, status=status) + status = ExporterStatus.from_proto(data.status) if data.status else None + return cls(namespace=namespace, name=name, labels=data.labels, online=data.online, status=status)However, if this is defensive against older controller versions that may not send the field, the current approach is fine.
packages/jumpstarter/jumpstarter/common/utils.py (1)
22-27: Consider moving the import to module level.The
ExporterStatusimport insideserve_asyncworks but is unconventional. SinceExporterStatusis a simple enum (not causing circular imports), consider importing it at the module level for consistency:+from jumpstarter.common import ExporterStatus from jumpstarter.client import client_from_path from jumpstarter.config.env import JMP_DRIVERS_ALLOW, JUMPSTARTER_HOST from jumpstarter.exporter import Session from jumpstarter.utils.env import env if TYPE_CHECKING: from jumpstarter.driver import Driver ... @asynccontextmanager async def serve_async(root_device: "Driver", portal: BlockingPortal, stack: ExitStack): - from jumpstarter.common import ExporterStatus - with Session(root_device=root_device) as session:Otherwise, the status update logic and comment explaining the local testing context are clear.
packages/jumpstarter/jumpstarter/common/enums.py (1)
8-76: Tidy up docstring typo; consider unknown-status fallback (optional)The enum wiring against
common_pb2looks good. Two small nits:
- There’s a typo in the
AFTER_LEASE_HOOKdocstring:"Lease was releaseed"→"Lease was released".- Optional:
ExporterStatus.from_protowill raise if a future proto introduces a new status value; if you expect newer servers/clients to mix, you might want to catchValueErrorand map unknowns toUNSPECIFIED.A minimal docstring fix could look like:
- AFTER_LEASE_HOOK = common_pb2.EXPORTER_STATUS_AFTER_LEASE_HOOK - """Lease was releaseed, but exporter is executing after lease hook""" + AFTER_LEASE_HOOK = common_pb2.EXPORTER_STATUS_AFTER_LEASE_HOOK + """Lease was released, but exporter is executing after lease hook"""packages/jumpstarter/jumpstarter/client/core.py (1)
53-57: Exporter readiness gating looks correct; watch per-call GetStatus overheadThe new
ExporterNotReadypluscheck_exporter_status()flow is a good fit for guarding driver calls onLEASE_READYand handling older exporters by treatingUNIMPLEMENTEDas “ready”. The integration into bothcall_asyncandstreamingcall_asyncis clean.One thing to keep an eye on: this adds an extra unary
GetStatusRPC before every driver RPC. If you ever have very chatty drivers (tight loops of small calls), you may want to cache a “known-good”LEASE_READYstate and invalidate it on failures or via a status stream, rather than checking on every single call. That’s a future optimization, not a blocker.Also applies to: 87-102, 103-137
packages/jumpstarter/jumpstarter/exporter/lease_context.py (1)
1-5: Align naming and docs for LeaseContext (vs LeaseScope) and socket_path semanticsFunctionally this looks good, but there are a couple of inconsistencies worth tightening up:
- The module/class docs and
__post_init__assertion messages still refer toLeaseScopewhile the class isLeaseContext.- The attribute docs say
socket_path: str | None, but the field is a plainstrthat uses""as “unset”.A minimal cleanup:
-"""LeaseScope: Context manager for lease-related resources. +"""LeaseContext: Context manager for lease-related resources. @@ - socket_path: Unix socket path where the session is serving (set in handle_lease) + socket_path: Unix socket path where the session is serving (set in handle_lease) @@ - assert self.before_lease_hook is not None, "LeaseScope requires a before_lease_hook event" - assert self.lease_name, "LeaseScope requires a non-empty lease_name" + assert self.before_lease_hook is not None, "LeaseContext requires a before_lease_hook event" + assert self.lease_name, "LeaseContext requires a non-empty lease_name"And either adjust the docstring to say
socket_path: strwith""meaning “not set”, or change the field type/default tostr | None = Noneso the docs and type align.Also applies to: 26-34, 44-48
packages/jumpstarter/jumpstarter/config/exporter.py (1)
28-28: Consider adding script validation.The
scriptfield has no default value, which is fine since it's required, but there's no validation for minimum content. An empty string would pass model validation but would be silently skipped at runtime (line 90-92 in hooks.py). Consider adding amin_length=1validator for early feedback.- script: str = Field(alias="script", description="The j script to execute for this hook") + script: str = Field(alias="script", min_length=1, description="The j script to execute for this hook")packages/jumpstarter/jumpstarter/exporter/hooks.py (3)
49-54:device_factoryfield appears unused.The
HookExecutorclass declares adevice_factoryfield but it's never referenced within the class methods. The device is accessed vialease_scope.sessioninstead. If this is dead code, consider removing it; if it's intended for future use, document the intent.#!/bin/bash # Verify device_factory is not used anywhere in the HookExecutor rg -n 'device_factory' packages/jumpstarter/jumpstarter/exporter/hooks.py
279-281: Using assert for runtime validation may be stripped in optimized mode.Python's
-Oflag removes assert statements. If this validation is critical for safety, consider using an explicitif not ... raisepattern instead.- assert lease_scope.is_ready(), "LeaseScope must be fully initialized before running before-lease hooks" + if not lease_scope.is_ready(): + raise RuntimeError("LeaseScope must be fully initialized before running before-lease hooks")Also applies to: 353-355
319-319: Acknowledged: TODO for controller-side lease termination.The TODO notes that a controller-side mechanism is needed to actually end the lease when
on_failure='endLease'. Currently, the status is reported but the lease isn't forcibly terminated.Would you like me to open an issue to track implementing the controller-side lease termination mechanism?
packages/jumpstarter/jumpstarter/exporter/logging.py (2)
31-39: Potential performance concern with prefix matching.The
get_source_for_recordmethod iterates through all child handlers for every log record. If many loggers are registered, this could become a bottleneck in high-frequency logging scenarios.For now this is likely fine given the expected number of hooks, but consider using a more efficient lookup structure (like a trie or sorted list with binary search) if performance becomes an issue.
36-38: Prefix matching order is non-deterministic.Dictionary iteration order is insertion order in Python 3.7+, but if multiple prefixes could match the same logger name (e.g.,
hookandhook.lease1), the first match wins. This behavior may be surprising. Consider documenting this or using longest-prefix matching.packages/jumpstarter/jumpstarter/exporter/exporter.py (2)
178-215: Retry logic is resilient but consider narrowing exception scope.The
_retry_streammethod provides good retry semantics with backoff. The retry counter reset on success (line 215) ensures sustained connectivity issues are handled well.The broad
Exceptioncatch at line 201 is pragmatic for gRPC streaming resilience, but could mask non-retriable programming errors. Consider catchinggrpc.RpcErrorandOSErrorspecifically to allow genuine bugs to surface during development.- except Exception as e: + except (grpc.RpcError, OSError, ConnectionError) as e:
268-287: Consider lazy string formatting for logging.Lines 285 and 287 use f-strings for logging, which evaluates the string regardless of whether the log level is enabled. While not critical for this infrequent code path, using
%formatting is the idiomatic pattern.- logger.info(f"Updated status to {status}: {message}") + logger.info("Updated status to %s: %s", status, message) except Exception as e: - logger.error(f"Failed to update status: {e}") + logger.error("Failed to update status: %s", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
buf.gen.yaml(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py(2 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(22 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py(4 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyi(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi(1 hunks)packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py(2 hunks)packages/jumpstarter/jumpstarter/client/core.py(5 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(9 hunks)packages/jumpstarter/jumpstarter/common/__init__.py(1 hunks)packages/jumpstarter/jumpstarter/common/enums.py(1 hunks)packages/jumpstarter/jumpstarter/common/types.py(1 hunks)packages/jumpstarter/jumpstarter/common/utils.py(2 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(12 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(5 hunks)packages/jumpstarter/jumpstarter/exporter/hooks.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/hooks_test.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/lease_context.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/logging.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/logging_protocol.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
📚 Learning: 2025-09-02T07:32:04.548Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py:0-0
Timestamp: 2025-09-02T07:32:04.548Z
Learning: *_pb2.py files in the jumpstarter codebase are auto-generated from gRPC/protobuf definitions and should not be manually modified. Issues with these files should be addressed at the protobuf generation tooling level or runtime environment setup.
Applied to files:
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyipackages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi
📚 Learning: 2025-10-02T15:29:09.070Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 681
File: packages/jumpstarter/jumpstarter/client/lease.py:198-220
Timestamp: 2025-10-02T15:29:09.070Z
Learning: In the jumpstarter codebase, `client_from_path` and `client_from_channel` functions in `packages/jumpstarter/jumpstarter/client/client.py` automatically call `GetReport(empty_pb2.Empty())` during client initialization. This means that successfully entering the client context manager guarantees the exporter is responsive and has answered the GetReport RPC.
Applied to files:
packages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi
📚 Learning: 2025-10-06T15:44:51.111Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 690
File: packages/jumpstarter/jumpstarter/config/exporter.py:51-56
Timestamp: 2025-10-06T15:44:51.111Z
Learning: In jumpstarter config instantiation (packages/jumpstarter/jumpstarter/config/exporter.py), passing **self.root.config to driver_class constructors is intentional validation behavior: since Driver classes are dataclasses, the __init__ will reject any config keys that don't match declared fields with a TypeError, ensuring only declared/supported fields can be used in config YAML files. This is the desired behavior for config safety.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter/jumpstarter/exporter/session.pypackages/jumpstarter/jumpstarter/config/exporter.py
📚 Learning: 2025-08-14T13:11:35.034Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
Applied to files:
packages/jumpstarter/jumpstarter/exporter/hooks.pypackages/jumpstarter/jumpstarter/exporter/exporter.py
🧬 Code graph analysis (15)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (4)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (2)
_MaybeAsyncIterator(28-28)_ServicerContext(30-31)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (2)
_MaybeAsyncIterator(22-22)_ServicerContext(24-25)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (2)
_MaybeAsyncIterator(21-21)_ServicerContext(23-24)
packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py (1)
packages/jumpstarter/jumpstarter/exporter/session.py (1)
update_status(152-158)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (4)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (2)
_MaybeAsyncIterator(28-28)_ServicerContext(30-31)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (2)
_MaybeAsyncIterator(22-22)_ServicerContext(24-25)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (2)
_MaybeAsyncIterator(21-21)_ServicerContext(23-24)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (3)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (2)
_MaybeAsyncIterator(22-22)_ServicerContext(24-25)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)
packages/jumpstarter/jumpstarter/client/core.py (4)
packages/jumpstarter/jumpstarter/common/enums.py (3)
ExporterStatus(8-45)from_proto(39-41)from_proto(70-72)packages/jumpstarter/jumpstarter/exporter/session.py (1)
GetStatus(172-178)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (2)
GetStatus(613-617)GetStatus(801-825)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi (1)
GetStatusRequest(690-695)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (1)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (1)
_MaybeAsyncIterator(14-14)
packages/jumpstarter/jumpstarter/common/enums.py (1)
packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)
packages/jumpstarter/jumpstarter/exporter/hooks.py (3)
packages/jumpstarter/jumpstarter/config/exporter.py (2)
HookConfigV1Alpha1(44-50)HookInstanceConfigV1Alpha1(23-41)packages/jumpstarter/jumpstarter/exporter/session.py (1)
Session(33-178)packages/jumpstarter/jumpstarter/exporter/lease_context.py (2)
LeaseContext(19-84)is_ready(49-55)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
packages/jumpstarter/jumpstarter/common/enums.py (3)
ExporterStatus(8-45)from_proto(39-41)from_proto(70-72)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(164-182)packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
Exporter(39-541)
packages/jumpstarter-cli/jumpstarter_cli/get.py (1)
packages/jumpstarter/jumpstarter/config/client.py (1)
list_exporters(156-197)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (4)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (2)
_MaybeAsyncIterator(28-28)_ServicerContext(30-31)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (2)
_MaybeAsyncIterator(21-21)_ServicerContext(23-24)
packages/jumpstarter/jumpstarter/exporter/logging.py (4)
packages/jumpstarter/jumpstarter/exporter/logging_protocol.py (2)
LoggerRegistration(8-22)add_logger_source(15-22)packages/jumpstarter/jumpstarter/common/enums.py (1)
LogSource(48-76)packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)packages/jumpstarter/jumpstarter/exporter/session.py (2)
context_log_source(168-170)add_logger_source(160-162)
packages/jumpstarter/jumpstarter/config/exporter.py (4)
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
session(353-373)packages/jumpstarter/jumpstarter/exporter/lease_context.py (1)
update_status(73-84)packages/jumpstarter/jumpstarter/exporter/session.py (1)
update_status(152-158)packages/jumpstarter/jumpstarter/exporter/hooks.py (1)
HookExecutor(50-402)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyi (1)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi (4)
ClearField(44-44)ClearField(67-67)ClearField(89-89)ClearField(105-105)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (4)
rich_add_columns(81-91)rich_add_columns(139-140)rich_add_rows(110-125)rich_add_rows(142-144)
⏰ 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). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (39)
buf.gen.yaml (1)
9-12: Mypy plugin additions look consistentThe extra mypy / mypy‑grpc plugins are configured consistently with the existing buf plugins (same
out, pinned versions) and should integrate cleanly into codegen.packages/jumpstarter-cli/jumpstarter_cli/get.py (1)
24-38: Status option wiring forget exporterslooks correctIncluding
"status"in thewithoptions, derivinginclude_status, and passing it through toconfig.list_exportersaligns with the updated client config API and keeps the help text accurate.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py (1)
224-258: Exporter JSON/YAML expectations updated consistentlyThe additional
exporterStatusandstatusMessage: nullfields in the exporter status JSON/YAML fixtures align with the extended status model and mirror the formats used elsewhere in the test suite.packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py (1)
4-5: Explicitly setting LEASE_READY in tests is a sensible shortcutImporting
ExporterStatusand callingsession.update_status(ExporterStatus.LEASE_READY)inside theserve_unix()context is a straightforward way to emulate a ready exporter in local pytest runs where there’s no real lease/hook flow. This keeps the test focused on the client behavior rather than full lifecycle orchestration.Also applies to: 20-27
packages/jumpstarter/jumpstarter/driver/base.py (1)
30-31: LGTM! Logging refactored to use centralized exporter logging infrastructure.The driver now uses the centralized
get_loggerwithLogSource.DRIVERfor consistent log source tagging across the exporter. The logger name patterndriver.{ClassName}provides good granularity for filtering.Also applies to: 38-39, 89-90
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (1)
1-20: Auto-generated protobuf stub file.This file is auto-generated by
mypy-protobufas indicated by the header comment. The structure matches other generated*_pb2_grpc.pyistubs in the codebase.packages/jumpstarter/jumpstarter/client/grpc.py (2)
15-16: LGTM! Status display capability properly integrated.The exporter status feature is consistently integrated across:
WithOptionswith the newshow_statusflag- Column/row rendering functions
Exportermodel with the newstatusfieldExporterListwith conditional status inclusion in serializationThe rendering gracefully handles
Nonestatus by displaying "UNKNOWN".Also applies to: 23-24, 32-34, 48-51, 90-91, 257-258, 267-275, 292-307
489-489: Good type refinement.Changing
dict[Channel, Any]todict[Channel, SimpleNamespace]provides better type safety for the internal stubs dictionary.packages/jumpstarter/jumpstarter/common/__init__.py (1)
1-22: LGTM! Clean public API expansion.The
__all__list is properly updated and alphabetically sorted, making the newExporterStatus,LogSource, and type aliases (AsyncChannel,ControllerStub,ExporterStub,RouterStub) available as part of the public API.packages/jumpstarter/jumpstarter/exporter/hooks_test.py (2)
61-117: LGTM! Comprehensive test coverage for hook execution scenarios.The test suite covers key scenarios:
- Basic executor creation and empty config handling
- Successful execution with environment variable verification
- Failed execution with
on_failure="endLease"raisingHookExecutionError- Timeout handling with process termination
- Environment variable sanitization (verifying removed fields)
- Post-lease hook execution
- Timeout with
on_failure="warn"not raisingGood use of
AsyncMockandpatchfor subprocess simulation.Also applies to: 119-172, 174-205, 246-303
43-58: I'll help you verify the review comment about thesocket_pathusage in the fixture. Let me search for howHookExecutoraccesses the socket path from the lease context.
<function_calls>
#!/bin/bashVerify how HookExecutor accesses socket_path from lease context
rg -n "socket_path" packages/jumpstarter/jumpstarter/exporter/hooks.py -B2 -A2
</function_calls>
Let me also check the HookExecutor class definition and its usage of lease_scope:
<function_calls>
#!/bin/bashSearch for HookExecutor class and its initialization/usage of lease_scope
rg -n "HookExecutor" packages/jumpstarter/jumpstarter/exporter/hooks.py -B1 -A5
</function_calls>
Now let me check the LeaseContext class to understand its structure:
<function_calls>
#!/bin/bashFind LeaseContext definition to understand its structure
rg -n "class LeaseContext" packages/jumpstarter/jumpstarter/exporter/lease_context.py -A10
</function_calls>
Let me also verify if there are any other tests using the lease_scope fixture to understand the pattern:
<function_calls>
#!/bin/bashCheck how the lease_scope fixture is used in tests
rg -n "lease_scope" packages/jumpstarter/jumpstarter/exporter/hooks_test.py -B1 -A3
</function_calls>
packages/jumpstarter/jumpstarter/common/types.py (1)
8-25: Centralized gRPC type aliases look consistentThe stub and channel aliases are straightforward and keep imports tidy; exporting them via
__all__makes the public surface clear. No issues from my side here.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (1)
25-31: Exporter STATUS fields and table rendering look coherentMapping
exporterStatus/statusMessageintoV1Alpha1ExporterStatusand surfacing aSTATUScolumn (with"Unknown"fallback) fits well with the existing list output and avoids new failure modes. The devices vs non-devices views both use the same status value, which keeps the UX consistent.Also applies to: 60-62, 65-106
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyi (1)
1-96: Generated protobuf enum stubs look fineThis file is clearly marked as mypy-protobuf generated and matches the ExporterStatus/LogSource usage elsewhere. Treating it as generated artifact and not hand-editing it is the right approach.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (1)
1-96: Generated RouterService gRPC typing surface is consistentThe RouterService stub/servicer typings follow the same pattern as the other v1 *_pb2_grpc.pyi files and are correctly marked as generated. Looks good to keep checked in as-is.
packages/jumpstarter/jumpstarter/config/exporter.py (5)
23-41: Well-structured hook configuration model.The
HookInstanceConfigV1Alpha1model has sensible defaults: 120s timeout and "warn" on failure which provides safe defaults for users. The use of aliases (onFailure) ensures YAML compatibility with the example config in the PR description.
44-50: LGTM!The
HookConfigV1Alpha1model cleanly separates before and after lease hooks with proper optional typing and aliases.
163-163: Good fix for YAML serialization consistency.Adding
by_alias=Trueensures that the exported YAML uses the camelCase field names (e.g.,beforeLease,onFailure) that match the documented configuration format.Also applies to: 174-174
186-198: Appropriate status initialization for local serving.Setting
LEASE_READYstatus directly for local/Unix socket usage makes sense since there's no controller or lease lifecycle involved.
224-246: I'll verify the device_factory usage in HookExecutor to determine if the difference in configuration parameters is intentional.
<function_calls>
#!/bin/bashCheck HookExecutor implementation and how device_factory is used
rg -n -A 20 'class HookExecutor' packages/jumpstarter/jumpstarter/exporter/hooks.py
</function_calls>
<function_calls>
#!/bin/bashCheck how device_factory is used within HookExecutor
rg -n -B 5 -A 15 'device_factory' packages/jumpstarter/jumpstarter/exporter/hooks.py
</function_calls>packages/jumpstarter/jumpstarter/exporter/hooks.py (6)
23-47: Well-designed error type with clear semantics.The
HookExecutionErrordataclass provides clear predicates (should_shutdown_exporter,should_end_lease) that make the control flow in callers much more readable.
56-74: Security: Hooks run with elevated driver permissions.Setting
JMP_DRIVERS_ALLOW: "UNSAFE"grants full driver access to hook scripts. This is documented in the PR objectives, but ensure operators understand this grants significant access to the system. Consider adding a security note in the docstring.
160-167: Shell injection risk is acceptable here by design.Using
create_subprocess_shellwith user-provided commands is intentional per the PR design (hook scripts are meant to run arbitrary commands). Ensure documentation clarifies that hook scripts should only be configured by trusted administrators.
176-184: Potential issue: stdout.readline() may not be available if process exits.If the process exits or crashes before output reading completes,
process.stdoutcould beNone. However, sincestdout=asyncio.subprocess.PIPEis explicitly set on line 165, this should always be available. The implementation looks correct.
197-208: Good defensive timeout handling on process termination.The two-phase termination (SIGTERM with 5s grace period, then SIGKILL) is a robust pattern for cleaning up hung processes.
329-331: Critical: Event is always set, ensuring connections aren't blocked.The
finallyblock ensuresbefore_lease_hook.set()is called regardless of success or failure, preventing deadlocks where clients wait indefinitely for the hook to complete.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyi (1)
1-73: Auto-generated protobuf stub file.This file is generated by mypy-protobuf and follows the standard patterns observed in other protocol stub files. No manual changes should be made here.
Based on learnings, issues with these files should be addressed at the protobuf generation tooling level.
packages/jumpstarter/jumpstarter/exporter/logging.py (3)
13-19: Good thread-safety initialization.Using
RLockis appropriate here since it allows the same thread to acquire the lock multiple times (reentrant), which could happen if logging triggers nested logging calls.
59-66: Context manager ensures cleanup on exception.The
context_log_sourceproperly uses try/finally to remove the handler mapping even if an exception occurs, preventing resource leaks.
69-89: Clean helper for logger creation with source registration.The
get_loggerfunction provides a clean API for obtaining a logger with automatic source mapping when a session is provided. This simplifies hook logging setup.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (1)
1-307: Auto-generated gRPC stub file.This file is generated by mypy-protobuf for the client service and follows the standard patterns for typed gRPC stubs. The structure is consistent with other
*_pb2_grpc.pyifiles in the codebase.Based on learnings, issues with these files should be addressed at the protobuf generation tooling level.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.pyi (1)
1-148: Auto-generated protobuf stub looks correct.This file is generated by
mypy-protobufand follows the standard patterns for Kubernetes v1 message type stubs. The type definitions forLabelSelectorRequirement,LabelSelector,Time, andConditionare consistent with other generated stubs in the codebase. Based on learnings, any issues should be addressed at the protobuf generation tooling level.packages/jumpstarter/jumpstarter/exporter/exporter.py (4)
1-46: Clean import organization and enhanced documentation.The new imports for
Event,HookExecutor,LeaseContext, andExporterStatusare well-organized. UsingTYPE_CHECKINGfor theDriverimport is the correct pattern to avoid circular imports while maintaining type hints.
81-151: Well-documented lifecycle state fields.The new internal state fields (
_registered,_exporter_status,_lease_context, etc.) are properly declared withinit=Falseand appropriate defaults. The comprehensive docstrings clearly explain each field's purpose and lifecycle.
375-437: Clean lease handling implementation.The
handle_leasemethod properly coordinates the session lifecycle with hook execution. The pattern of waiting for thebefore_lease_hookevent before accepting connections ensures clients don't connect during setup. The status synchronization after hook completion (line 424) correctly propagates the final status to the session.
502-531: Robust hook execution with proper shielding.The before-lease hook is correctly spawned asynchronously (line 504-509), allowing the lease handling to continue preparing while the hook runs. The after-lease hook uses
CancelScope(shield=True)to ensure cleanup runs even during task cancellation - this is essential for reliable teardown.The
has_client()check (lines 523, 466) properly gates after-lease hooks to only run when a client actually connected, avoiding unnecessary cleanup for unused leases.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (1)
1-752: Auto-generated gRPC stub follows established patterns.This file is generated by
mypy-protobufand provides type-safe stubs forControllerServiceandExporterService. The_MaybeAsyncIteratorand_ServicerContextscaffolding matches the patterns in other v1 gRPC stubs (as seen in the relevant code snippets). Based on learnings, any issues should be addressed at the protobuf generation tooling level.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi (1)
1-318: Auto-generated client protobuf stub is well-structured.This file is generated by
mypy-protobuffor the v1 client API. The message definitions (Exporter,Lease, request/response types) properly reference the common protobuf types likeExporterStatusandkubernetes_pb2.Condition. The structure aligns with protobuf 6.30.1 conventions. Based on learnings, any issues should be addressed at the protobuf generation tooling level.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi (1)
1-717: Comprehensive auto-generated protocol message stubs.This file is generated by
mypy-protobufand provides type stubs for all core Jumpstarter v1 protocol messages. The definitions properly integrate with related types (ExporterStatus,LogSource,LabelSelector,Condition) and follow the established patterns for nested message types and map fields. Based on learnings, any issues should be addressed at the protobuf generation tooling level.
| _current_status: ExporterStatus = field(init=False, default=ExporterStatus.AVAILABLE) | ||
| _status_message: str = field(init=False, default="") | ||
| _status_update_event: Event = field(init=False) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Check for uses of _status_update_event across the repo
rg -n "_status_update_event" -C2Repository: jumpstarter-dev/jumpstarter
<shell_result>
Repository: jumpstarter-dev/jumpstarter
Exit code: 0
stdout:
packages/jumpstarter/jumpstarter/exporter/session.py-44- _current_status: ExporterStatus = field(init=False, default=ExporterStatus.AVAILABLE)
packages/jumpstarter/jumpstarter/exporter/session.py-45- _status_message: str = field(init=False, default="")
packages/jumpstarter/jumpstarter/exporter/session.py:46: _status_update_event: Event = field(init=False)
packages/jumpstarter/jumpstarter/exporter/session.py-47-
packages/jumpstarter/jumpstarter/exporter/session.py-48- @contextmanager
--
packages/jumpstarter/jumpstarter/exporter/session.py-74- self._logging_queue = deque(maxlen=32)
packages/jumpstarter/jumpstarter/exporter/session.py-75- self._logging_handler = LogHandler(self._logging_queue, LogSource.SYSTEM)
packages/jumpstarter/jumpstarter/exporter/session.py:76: self._status_update_event = Event()
packages/jumpstarter/jumpstarter/exporter/session.py-77-
packages/jumpstarter/jumpstarter/exporter/session.py-78- # Map all driver logs to DRIVER source
</shell_result>
Script executed:
#!/bin/bash
# Read the relevant sections of session.py
sed -n '1,100p' packages/jumpstarter/jumpstarter/exporter/session.pyRepository: jumpstarter-dev/jumpstarter
<shell_result>
Repository: jumpstarter-dev/jumpstarter
Exit code: 0
stdout:
import logging
from collections import deque
from collections.abc import Generator
from contextlib import asynccontextmanager, contextmanager, suppress
from dataclasses import dataclass, field
from logging.handlers import QueueHandler
from typing import TYPE_CHECKING, Self
from uuid import UUID
import grpc
from anyio import ContextManagerMixin, Event, TypedAttributeLookupError, sleep
from anyio.from_thread import start_blocking_portal
from jumpstarter_protocol import (
jumpstarter_pb2,
jumpstarter_pb2_grpc,
router_pb2_grpc,
)
from .logging import LogHandler
from jumpstarter.common import ExporterStatus, LogSource, Metadata, TemporarySocket
from jumpstarter.common.streams import StreamRequestMetadata
from jumpstarter.streams.common import forward_stream
from jumpstarter.streams.metadata import MetadataStreamAttributes
from jumpstarter.streams.router import RouterStream
if TYPE_CHECKING:
from jumpstarter.driver import Driver
logger = logging.getLogger(__name__)
@dataclass(kw_only=True)
class Session(
jumpstarter_pb2_grpc.ExporterServiceServicer,
router_pb2_grpc.RouterServiceServicer,
Metadata,
ContextManagerMixin,
):
root_device: "Driver"
mapping: dict[UUID, "Driver"]
_logging_queue: deque = field(init=False)
_logging_handler: QueueHandler = field(init=False)
_current_status: ExporterStatus = field(init=False, default=ExporterStatus.AVAILABLE)
_status_message: str = field(init=False, default="")
_status_update_event: Event = field(init=False)
@contextmanager
def __contextmanager__(self) -> Generator[Self]:
logging.getLogger().addHandler(self._logging_handler)
self.root_device.reset()
try:
yield self
finally:
try:
self.root_device.close()
except Exception as e:
# Get driver name from report for more descriptive logging
try:
report = self.root_device.report()
driver_name = report.labels.get("jumpstarter.dev/name", self.root_device.__class__.__name__)
except Exception:
driver_name = self.root_device.__class__.__name__
logger.error("Error closing driver %s: %s", driver_name, e, exc_info=True)
finally:
logging.getLogger().removeHandler(self._logging_handler)
def __init__(self, *args, root_device, **kwargs):
super().__init__(*args, **kwargs)
self.root_device = root_device
self.mapping = {u: i for (u, _, _, i) in self.root_device.enumerate()}
self._logging_queue = deque(maxlen=32)
self._logging_handler = LogHandler(self._logging_queue, LogSource.SYSTEM)
self._status_update_event = Event()
# Map all driver logs to DRIVER source
self._logging_handler.add_child_handler("driver.", LogSource.DRIVER)
@asynccontextmanager
async def serve_port_async(self, port):
server = grpc.aio.server()
server.add_insecure_port(port)
jumpstarter_pb2_grpc.add_ExporterServiceServicer_to_server(self, server)
router_pb2_grpc.add_RouterServiceServicer_to_server(self, server)
await server.start()
try:
yield
finally:
await server.stop(grace=None)
@asynccontextmanager
async def serve_unix_async(self):
with TemporarySocket() as path:
async with self.serve_port_async(f"unix://{path}"):
yield path
</shell_result>
Script executed:
#!/bin/bash
# Read the method implementations around lines 152-178
sed -n '150,200p' packages/jumpstarter/jumpstarter/exporter/session.pyRepository: jumpstarter-dev/jumpstarter
<shell_result>
Repository: jumpstarter-dev/jumpstarter
Exit code: 0
stdout:
await sleep(0.5)
def update_status(self, status: int | ExporterStatus, message: str = ""):
"""Update the current exporter status for the session."""
if isinstance(status, int):
self._current_status = ExporterStatus.from_proto(status)
else:
self._current_status = status
self._status_message = message
def add_logger_source(self, logger_name: str, source: LogSource):
"""Add a log source mapping for a specific logger."""
self._logging_handler.add_child_handler(logger_name, source)
def remove_logger_source(self, logger_name: str):
"""Remove a log source mapping for a specific logger."""
self._logging_handler.remove_child_handler(logger_name)
def context_log_source(self, logger_name: str, source: LogSource):
"""Context manager to temporarily set a log source for a specific logger."""
return self._logging_handler.context_log_source(logger_name, source)
async def GetStatus(self, request, context):
"""Get the current exporter status."""
logger.debug("GetStatus() -> %s", self._current_status)
return jumpstarter_pb2.GetStatusResponse(
status=self._current_status.to_proto(),
message=self._status_message,
)
</shell_result>
Script executed:
#!/bin/bash
# Search for all references to _status_update_event in the entire repo to confirm it's never used
rg -n "_status_update_event" --type pyRepository: jumpstarter-dev/jumpstarter
<shell_result>
Repository: jumpstarter-dev/jumpstarter
Exit code: 0
stdout:
packages/jumpstarter/jumpstarter/exporter/session.py:46: _status_update_event: Event = field(init=False)
packages/jumpstarter/jumpstarter/exporter/session.py:76: self._status_update_event = Event()
</shell_result>
Remove unused _status_update_event field or wire it into update_status() to signal changes
The verification confirms the original concern: _status_update_event is initialized in __init__ (line 76) but never referenced anywhere in the codebase. It is not called in update_status(), not awaited in GetStatus(), and not used by any other module.
Either remove the field entirely if it's no longer needed, or if it's intended to signal status changes to waiting consumers, wire it into update_status() like:
def update_status(self, status: int | ExporterStatus, message: str = ""):
"""Update the current exporter status for the session."""
if isinstance(status, int):
self._current_status = ExporterStatus.from_proto(status)
else:
self._current_status = status
self._status_message = message
self._status_update_event.set()(Note: This assumes an async consumer pattern; adjust as needed for your use case.)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py (2)
215-224: Consider asserting exporter list column names, not just count (no devices)Right now this test only checks
add_columnis called 5 times. To catch regressions where the STATUS column is dropped or reordered, you could also assert the actual column labels, mirroring the single-exporter test:mock_table = MagicMock() V1Alpha1ExporterList.rich_add_columns(mock_table, devices=False) - assert mock_table.add_column.call_count == 5 + assert mock_table.add_column.call_count == 5 + mock_table.add_column.assert_any_call("NAME", no_wrap=True) + mock_table.add_column.assert_any_call("STATUS") + mock_table.add_column.assert_any_call("ENDPOINT") + mock_table.add_column.assert_any_call("DEVICES") + mock_table.add_column.assert_any_call("AGE")
226-235: Similarly strengthen exporter list column assertions for devices=TrueFor the
devices=Truevariant you only assert 6 columns. Mirroring the exporter-level test would better lock in the STATUS column and device columns:mock_table = MagicMock() V1Alpha1ExporterList.rich_add_columns(mock_table, devices=True) - assert mock_table.add_column.call_count == 6 + assert mock_table.add_column.call_count == 6 + mock_table.add_column.assert_any_call("NAME", no_wrap=True) + mock_table.add_column.assert_any_call("STATUS") + mock_table.add_column.assert_any_call("ENDPOINT") + mock_table.add_column.assert_any_call("AGE") + mock_table.add_column.assert_any_call("LABELS") + mock_table.add_column.assert_any_call("UUID")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
⏰ 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). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (6)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py (6)
24-55: JSON dump expectations updated for new status fields look correctThe JSON expectation now asserts
exporterStatusandstatusMessageasnullwhen unset, which cleanly fixes the schema for clients expecting those keys to always be present.
58-81: YAML serialization correctly mirrors exporterStatus/statusMessage shapeIncluding
exporterStatus: nullandstatusMessage: nullin the YAML snapshot keeps JSON/YAML aligned and makes the presence of the new fields explicit even when unset.
112-125: STATUS column added for exporter tables without devices is well coveredThe test now enforces 5 columns and explicitly checks for a
STATUScolumn alongsideNAME,ENDPOINT,DEVICES, andAGE, which matches the expected table layout.
128-142: Exporter columns with devices correctly include STATUS and device metadataAsserting 6 columns and the presence of
STATUSplus the devices-related columns (LABELS,UUID) provides good coverage of the richer table layout with devices.
145-159: Row layout without devices validates STATUS fallback semanticsThe row assertions (
NAME,"Unknown"status whenexporter_statusisNone, endpoint, device count, age) accurately exercise the new STATUS column and its defaulting behavior.
161-176: Row layout with devices exercises STATUS plus device fieldsChecking the row as
(name, "Unknown", endpoint, age, labels, uuid)confirms both the STATUS default and the correct positioning of devices-related fields in the table.
|
This is green!, nice! :D |
| exporters = config.list_exporters(filter=selector, include_leases=include_leases, include_online=include_online) | ||
| include_status = "status" in with_options | ||
| exporters = config.list_exporters( | ||
| filter=selector, include_leases=include_leases, include_online=include_online, include_status=include_status |
There was a problem hiding this comment.
It is not your patches fault, but I hate this API, we need to improve it hehehe :D
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
looks great in general amazing work
| """Make DriverCall by method name and arguments""" | ||
|
|
||
| # Check exporter status before making the call | ||
| await self.check_exporter_status() |
There was a problem hiding this comment.
Should we cache this and assume it will stay ready once it declares itself at ready?
This would duplicate the grpc calls for any call impacting performance.
There was a problem hiding this comment.
also delay, if the controller is 200ms Away, you would need 300ms at least to get it working, in contrast to 100ms before
There was a problem hiding this comment.
I am wondering if we want to do this exporter side instead (the exporter knows and will prevent the call if not possible)
I am ok to just open an Issue, and tackle this later, but document it because we will forget of this otherwise :D
| super().__post_init__() | ||
|
|
||
| self.logger = logging.getLogger(self.__class__.__name__) | ||
| self.logger = get_logger(f"driver.{self.__class__.__name__}", LogSource.DRIVER) |
| channel_factory: Callable[[], Awaitable[grpc.aio.Channel]] | ||
| device_factory: Callable[[], Driver] | ||
| lease_name: str = field(init=False, default="") | ||
| """Factory function for creating gRPC channels to communicate with the controller. |
| # Create a logger with automatic source registration | ||
| hook_logger = get_logger(f"hook.{lease_scope.lease_name}", log_source, logging_session) | ||
|
|
||
| # Stream output line-by-line for real-time logging |
There was a problem hiding this comment.
wow! nice! I was checking if you had time to implement this :)
| out: ./packages/jumpstarter-protocol/jumpstarter_protocol | ||
| - remote: buf.build/grpc/python | ||
| out: ./packages/jumpstarter-protocol/jumpstarter_protocol | ||
| - remote: buf.build/community/nipunn1313-mypy:v3.7.0 |
There was a problem hiding this comment.
ahh that's where the improved stubs came from, nice!
|
For a follow up no need to address anything of that in this PR, I was trying with jumpstarter git:(add-hooks) ✗ uv run jmp shell --exporter-config test-hooks.yaml would it make sense to execute the hooks if they exist in a local shell too? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/jumpstarter/jumpstarter/exporter/exporter.py (5)
77-83: Aligngrpc_optionstyping with actual usage and gRPC expectations
grpc_optionsis declared asdict[str, str], but:
_handle_client_conn()acceptsgrpc_options: dict[str, Any] | None.- gRPC channel options can legitimately be non-string (e.g. ints, booleans).
To keep type-checkers happy and match the call site, it’s better to type this as
dict[str, Any]on the dataclass as well.- grpc_options: dict[str, str] = field(default_factory=dict) + grpc_options: dict[str, Any] = field(default_factory=dict)Also applies to: 347-360
139-145: Fix stale doc references (_update_status,LeaseScopevsLeaseContext)There are a couple of small doc inconsistencies:
_exporter_statusdoc mentions_update_status(), but status is now maintained via_report_status()._lease_context’s doc andhandle_lease()’s arg doc still refer to “LeaseScope” even though the type and field areLeaseContext.These don’t affect behavior but can confuse future readers.
You could tighten the docs like:
- """Current status of the exporter. - - Updated via _update_status() and reported to controller and session. + """Current status of the exporter. + + Updated via _report_status() and reported to controller and session. @@ - The session and socket are managed by the context manager in handle_lease(), - ensuring proper cleanup when the lease ends. The LeaseScope itself is just + The session and socket are managed by the context manager in handle_lease(), + ensuring proper cleanup when the lease ends. The LeaseContext itself is just @@ - lease_scope: LeaseScope with before_lease_hook event (session/socket set here) + lease_scope: LeaseContext with before_lease_hook event (session/socket set here)Also applies to: 156-172, 397-415
290-310: Avoid creating two controller channels during unregistrationIn
_unregister_with_controller()you:
- Open a channel via
channel = await self.channel_factory()and use it forcontroller.Unregister(...).- Also call
_report_status(...), which internally calls_get_controller_stub(), which in turn callschannel_factory()again.If
channel_factory()creates new channels, this path will allocate two channels for a single shutdown, and only the one bound tochannelis explicitly closed. It’s not a correctness bug per se (the process is exiting), but it’s wasteful and makes resource ownership harder to reason about.One way to tidy this up is to let
_report_status()optionally reuse the controller stub/channel:- async def _report_status(self, status: ExporterStatus, message: str = ""): + async def _report_status( + self, + status: ExporterStatus, + message: str = "", + controller: jumpstarter_pb2_grpc.ControllerServiceStub | None = None, + ): @@ - try: - controller = await self._get_controller_stub() + try: + if controller is None: + controller = await self._get_controller_stub() await controller.ReportStatus( @@ async def _unregister_with_controller(self): @@ - with move_on_after(10): # 10 second timeout - channel = await self.channel_factory() - try: - controller = jumpstarter_pb2_grpc.ControllerServiceStub(channel) - await self._report_status(ExporterStatus.OFFLINE, "Exporter shutting down") - await controller.Unregister( + with move_on_after(10): # 10 second timeout + channel = await self.channel_factory() + try: + controller = jumpstarter_pb2_grpc.ControllerServiceStub(channel) + await self._report_status( + ExporterStatus.OFFLINE, + "Exporter shutting down", + controller=controller, + ) + await controller.Unregister(This keeps a single controller channel in play during shutdown and makes ownership explicit.
Also applies to: 311-333
480-499: Ensure before/after hooks can’t overlap on lease-name changesThe lease-change branch:
lease_changed = ( self._lease_context and self._lease_context.is_active() and self._lease_context.lease_name != status.lease_name ) if lease_changed: if self.hook_executor and self._lease_context.has_client(): with CancelScope(shield=True): await self.hook_executor.run_after_lease_hook(...)runs
after_leasesolely based onlease_nameandhas_client(), independent of whether thebefore_leasehook has completed (its event may not yet be set). In a scenario where the controller changeslease_namewhile abefore_leasehook is still running, this could cause before/after hooks to run concurrently on the same exporter (e.g. “power on” and “power off” scripts racing).I don’t know whether your controller ever sends such a sequence, but the code path allows it.
Consider gating the
after_leasehook on the before-hook having finished, or otherwise ensuring serialization, for example:- if lease_changed: - # After-lease hook for the previous lease (lease name changed) - if self.hook_executor and self._lease_context.has_client(): + if lease_changed: + # After-lease hook for the previous lease (lease name changed) + if ( + self.hook_executor + and self._lease_context.has_client() + # Optional: only run after-hook once before-lease hook has finished + and self._lease_context.before_lease_hook.is_set() + ):or by letting
HookExecutorhandle cancellation/ordering explicitly. It’d be good to add a test for “lease_name changes while before-hook is in progress” to confirm intended behavior.Also applies to: 503-538
423-431: Optionally close memory object streams explicitlyBoth in
handle_lease()andserve():listen_tx, listen_rx = create_memory_object_stream[ListenResponse]() status_tx, status_rx = create_memory_object_stream[StatusResponse]()the send/receive streams are used without
async with/aclose(). Because the enclosingTaskGroupcancellation will eventually tear everything down and the process is short-lived (per retrieved learnings), this isn’t a correctness issue, but explicitly closing the streams is a bit cleaner and makes lifetime more obvious.For example:
- listen_tx, listen_rx = create_memory_object_stream[jumpstarter_pb2.ListenResponse]() + async with create_memory_object_stream[jumpstarter_pb2.ListenResponse]() as (listen_tx, listen_rx): @@ - status_tx, status_rx = create_memory_object_stream[jumpstarter_pb2.StatusResponse]() + async with create_memory_object_stream[jumpstarter_pb2.StatusResponse]() as (status_tx, status_rx):and keep the rest of the logic the same. This makes ownership and shutdown behavior explicit and plays nicer with linters.
Also applies to: 468-478
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter/jumpstarter/exporter/exporter.py
🧠 Learnings (1)
📚 Learning: 2025-08-14T13:11:35.034Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
Applied to files:
packages/jumpstarter/jumpstarter/exporter/exporter.py
🧬 Code graph analysis (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py (5)
packages/jumpstarter/jumpstarter/common/enums.py (3)
ExporterStatus(8-45)to_proto(43-45)to_proto(74-76)packages/jumpstarter/jumpstarter/common/streams.py (1)
connect_router_stream(37-48)packages/jumpstarter/jumpstarter/exporter/hooks.py (3)
HookExecutor(50-402)run_after_lease_hook(333-402)run_before_lease_hook(259-331)packages/jumpstarter/jumpstarter/exporter/lease_context.py (5)
LeaseContext(19-84)update_status(73-84)is_active(57-59)has_client(61-63)update_client(65-67)packages/jumpstarter/jumpstarter/exporter/session.py (3)
Session(33-178)GetReport(110-119)update_status(152-158)
⏰ 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). (9)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/exporter/hooks.py (2)
296-307: Consider using an event-based wait instead of polling.The busy-wait loop with
anyio.sleep(0.1)works but is less efficient than an event-based approach. IfLeaseContextcould expose ananyio.Eventthat fires when ready, you could usewith anyio.fail_after(30): await lease_scope.ready_event.wait()for cleaner synchronization.
51-57: Remove or document the unuseddevice_factoryfield.The
device_factoryfield is defined but never referenced within this class. Consider removing it if no longer needed, or add a comment explaining its intended purpose if it's reserved for future use.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/exporter/hooks.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter/jumpstarter/exporter/hooks.py
🧠 Learnings (1)
📚 Learning: 2025-08-14T13:11:35.034Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
Applied to files:
packages/jumpstarter/jumpstarter/exporter/hooks.py
🪛 GitHub Actions: Lint
packages/jumpstarter/jumpstarter/exporter/hooks.py
[error] 144-144: Ruff check failed: C901 _execute_hook_process is too complex (12 > 10).
🪛 GitHub Check: ruff
packages/jumpstarter/jumpstarter/exporter/hooks.py
[failure] 144-144: Ruff (C901)
packages/jumpstarter/jumpstarter/exporter/hooks.py:144:15: C901 _execute_hook_process is too complex (12 > 10)
⏰ 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). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter/jumpstarter/exporter/hooks.py (5)
1-49: LGTM!The imports are well-organized and the
HookExecutionErrordataclass provides a clean interface with helpful predicates for determining failure behavior. The exception chaining support via thecauseparameter in_handle_hook_failureis a good practice.
78-142: LGTM!The
_execute_hookmethod properly validates empty commands and the_handle_hook_failuremethod correctly implements the three failure modes with appropriate exception chaining.
233-271: LGTM!Clean wrapper methods that properly check for configured hooks and delegate to
_execute_hookwith the appropriateLogSource.
359-432: LGTM!The
run_after_lease_hookmethod correctly handles the post-lease semantics whereon_failure='endLease'just reports the failure (since the lease has already ended) while the exporter remains available. The error handling is comprehensive and consistent withrun_before_lease_hook.
144-231: This review comment contains incorrect technical claims and should be reconsidered.The primary issue claimed—that
anyio.open_processwith a string command usesshlex.split()on POSIX and won't handle shell features—is factually incorrect. According to anyio's official documentation, a string command is passed directly to the system shell (equivalent tosubprocesswithshell=True), not split viashlex. The current code comment stating "Pass the command as a string to use shell mode" is accurate, and the implementation correctly executes shell commands as intended.The proposed fix to use
["sh", "-c", command]would also work but is redundant given the current approach.The cyclomatic complexity claim (C901) could not be verified due to tooling timeout, so that aspect remains unconfirmed.
If refactoring this method is still desired for code clarity and maintainability (extracting helper methods for output reading and process termination), that should be justified on code quality grounds alone, not on correcting a non-existent shell execution bug.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @packages/jumpstarter/jumpstarter/exporter/hooks.py:
- Around line 51-56: HookExecutor defines an unused field device_factory; remove
the device_factory attribute from the dataclass declaration and any
constructor/initializer parameters and tests that reference it (leave
HookExecutor, config: HookConfigV1Alpha1 intact), and update any instantiations
of HookExecutor so they no longer pass a device_factory argument; ensure imports
or type annotations referencing "Driver" are cleaned up if no longer needed.
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/exporter/exporter.py (2)
206-243: Consider excludingCancelledErrorfrom retry logic.The
except Exceptionblock catches all exceptions includingCancelledError. This could prevent clean task cancellation during shutdown.🔎 Proposed fix
try: controller = await self._get_controller_stub() async for item in stream_factory(controller): await send_tx.send(item) + except anyio.get_cancelled_exc_class(): + raise # Don't retry on cancellation except Exception as e: if retries_left > 0:
313-315: Consider using lazy % formatting for logger calls.Using f-strings in logger calls evaluates the string even when the log level is disabled.
🔎 Proposed fix
- logger.info(f"Updated status to {status}: {message}") + logger.info("Updated status to %s: %s", status, message) except Exception as e: - logger.error(f"Failed to update status: {e}") + logger.error("Failed to update status: %s", e)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-cli/jumpstarter_cli/run.pypackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/exporter/hooks.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-cli/jumpstarter_cli/run.pypackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/exporter/hooks.py
🧠 Learnings (3)
📚 Learning: 2025-08-14T13:11:35.034Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
Applied to files:
packages/jumpstarter-cli/jumpstarter_cli/run.pypackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/exporter/hooks.py
📚 Learning: 2025-10-06T15:44:51.111Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 690
File: packages/jumpstarter/jumpstarter/config/exporter.py:51-56
Timestamp: 2025-10-06T15:44:51.111Z
Learning: In jumpstarter config instantiation (packages/jumpstarter/jumpstarter/config/exporter.py), passing **self.root.config to driver_class constructors is intentional validation behavior: since Driver classes are dataclasses, the __init__ will reject any config keys that don't match declared fields with a TypeError, ensuring only declared/supported fields can be used in config YAML files. This is the desired behavior for config safety.
Applied to files:
packages/jumpstarter/jumpstarter/exporter/exporter.py
📚 Learning: 2025-10-02T15:29:09.070Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 681
File: packages/jumpstarter/jumpstarter/client/lease.py:198-220
Timestamp: 2025-10-02T15:29:09.070Z
Learning: In the jumpstarter codebase, `client_from_path` and `client_from_channel` functions in `packages/jumpstarter/jumpstarter/client/client.py` automatically call `GetReport(empty_pb2.Empty())` during client initialization. This means that successfully entering the client context manager guarantees the exporter is responsive and has answered the GetReport RPC.
Applied to files:
packages/jumpstarter/jumpstarter/exporter/exporter.py
🧬 Code graph analysis (2)
packages/jumpstarter/jumpstarter/exporter/exporter.py (4)
packages/jumpstarter/jumpstarter/common/enums.py (3)
ExporterStatus(8-45)to_proto(43-45)to_proto(74-76)packages/jumpstarter/jumpstarter/exporter/hooks.py (2)
HookExecutor(52-434)run_before_lease_hook(273-358)packages/jumpstarter/jumpstarter/exporter/lease_context.py (5)
LeaseContext(19-84)update_status(73-84)is_active(57-59)has_client(61-63)update_client(65-67)packages/jumpstarter/jumpstarter/exporter/session.py (3)
Session(33-178)GetReport(110-119)update_status(152-158)
packages/jumpstarter/jumpstarter/exporter/hooks.py (2)
packages/jumpstarter/jumpstarter/config/exporter.py (2)
HookConfigV1Alpha1(44-50)HookInstanceConfigV1Alpha1(23-41)packages/jumpstarter/jumpstarter/exporter/lease_context.py (2)
LeaseContext(19-84)is_ready(49-55)
🪛 GitHub Actions: Lint
packages/jumpstarter/jumpstarter/exporter/hooks.py
[error] 144-144: Ruff check failed: C901 _execute_hook_process is too complex (12 > 10).
🪛 GitHub Check: ruff
packages/jumpstarter/jumpstarter/exporter/hooks.py
[failure] 144-144: Ruff (C901)
packages/jumpstarter/jumpstarter/exporter/hooks.py:144:15: C901 _execute_hook_process is too complex (12 > 10)
⏰ 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). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (9)
packages/jumpstarter/jumpstarter/exporter/exporter.py (5)
1-34: LGTM! Clean imports and type organization.Good use of
TYPE_CHECKINGto avoid circular imports forDriver. The imports are well-organized and the forward reference pattern is correctly applied.
38-166: Excellent documentation and field organization.The dataclass fields are well-documented with clear lifecycle explanations. The consolidation of per-lease state into
LeaseContextimproves maintainability. The_previous_leasedfield is now properly declared as a dataclass field (addressing the previous review feedback).
168-200: LGTM! Stop method correctly handles exit code propagation.The
stop()method now properly accepts and stores an exit code, enabling hooks withon_failure='exit'to signal permanent termination. The property accessor is clean.
403-465: LGTM! Clean lease handling with proper hook synchronization.The
handle_leasemethod correctly:
- Populates the
LeaseContextwith session and socket path- Waits for before-lease hook completion before accepting connections
- Syncs status to session after hook completes
The coordination between concurrent tasks (handle_lease and run_before_lease_hook) via the
before_lease_hookevent is well-designed.
467-569: LGTM! Comprehensive lease lifecycle orchestration.The
serve()method correctly handles:
- Lease state transitions with proper hook invocation
- Before-lease hooks spawned as concurrent tasks when transitioning to leased state
- After-lease hooks shielded from cancellation with
CancelScope(shield=True)- Lease context cleanup after hook completion
The distinction between lease name changes (line 487-506) and lease state transitions (line 508-568) is well-handled.
packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
79-96: LGTM! Correct exit code priority handling.The exit code logic correctly prioritizes:
- Signal number (for signal-based termination)
- Exporter's exit code (for hook failures with
on_failure='exit')- Default 0 (triggers restart in fork architecture)
This integrates well with the fork-based restart architecture where exit code 0 triggers a restart and non-zero prevents restart.
packages/jumpstarter/jumpstarter/exporter/hooks.py (3)
25-48: LGTM! Well-designed exception class.The
HookExecutionErrordataclass provides clear semantics for hook failures with:
- Meaningful helper methods (
should_shutdown_exporter,should_end_lease)- Proper
__str__implementation- Clear distinction between failure modes
273-358: LGTM! Robust before-lease hook orchestration.The method handles all edge cases well:
- Timeout waiting for lease scope readiness
- Proper status reporting through the lifecycle
- Correct error handling for different
on_failuremodes- Critical: The
finallyblock ensuresbefore_lease_hook.set()is always called, preventing deadlocksThe polling approach for
is_ready()is acceptable given the short expected wait time.
360-433: LGTM! Correct after-lease hook semantics.The after-lease hook correctly:
- Validates lease scope is ready defensively
- Reports
AFTER_LEASE_HOOK_FAILEDon failure but returns toAVAILABLEstatus (since lease is already ended)- Only triggers shutdown for
on_failure='exit', notendLease(which is a no-op for after-lease since the lease already ended)Good distinction between before-lease and after-lease failure semantics.
| @dataclass(kw_only=True) | ||
| class HookExecutor: | ||
| """Executes lifecycle hooks with access to the j CLI.""" | ||
|
|
||
| config: HookConfigV1Alpha1 | ||
| device_factory: Callable[[], "Driver"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the HookExecutor class structure
cd packages/jumpstarter && cat -n jumpstarter/exporter/hooks.py | head -100Repository: jumpstarter-dev/jumpstarter
Length of output: 4070
🏁 Script executed:
# Search for device_factory references in the hooks.py file
rg -n "device_factory" jumpstarter/exporter/hooks.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 140
🏁 Script executed:
# Search for HookExecutor instantiation across the codebase
rg -n "HookExecutor\(" --type=py -B2 -A5Repository: jumpstarter-dev/jumpstarter
Length of output: 7953
🏁 Script executed:
# Search for all device_factory references in the entire jumpstarter package
rg -n "device_factory" --type=py packages/jumpstarter/Repository: jumpstarter-dev/jumpstarter
Length of output: 3297
🏁 Script executed:
# Check the full implementation of HookExecutor methods to see if device_factory is used
rg -n "device_factory" packages/jumpstarter/jumpstarter/exporter/hooks.py -B2 -A10Repository: jumpstarter-dev/jumpstarter
Length of output: 513
🏁 Script executed:
# Get the complete hooks.py file to see all methods
wc -l packages/jumpstarter/jumpstarter/exporter/hooks.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 125
🏁 Script executed:
# Read the full HookExecutor class implementation
head -200 packages/jumpstarter/jumpstarter/exporter/hooks.py | tail -150Repository: jumpstarter-dev/jumpstarter
Length of output: 5508
🏁 Script executed:
# Search the entire hooks.py file for any method that might use device_factory
rg -n "self\.device_factory" packages/jumpstarter/jumpstarter/exporter/hooks.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Also check if there are any other methods in the file that might reference device_factory
grep -n "def " packages/jumpstarter/jumpstarter/exporter/hooks.py | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 695
🏁 Script executed:
# Check the public execute_before_lease_hook and execute_after_lease_hook methods
sed -n '233,272p' packages/jumpstarter/jumpstarter/exporter/hooks.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 1439
🏁 Script executed:
# Check if there are any other uses of device_factory in the entire file
grep -n "device_factory" packages/jumpstarter/jumpstarter/exporter/hooks.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 116
Remove unused device_factory field.
The device_factory field is declared but never used within HookExecutor. All hook execution uses lease_scope.session instead. This field can be safely removed unless there are plans for future use.
🤖 Prompt for AI Agents
In @packages/jumpstarter/jumpstarter/exporter/hooks.py around lines 51-56,
HookExecutor defines an unused field device_factory; remove the device_factory
attribute from the dataclass declaration and any constructor/initializer
parameters and tests that reference it (leave HookExecutor, config:
HookConfigV1Alpha1 intact), and update any instantiations of HookExecutor so
they no longer pass a device_factory argument; ensure imports or type
annotations referencing "Driver" are cleaned up if no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter/jumpstarter/client/core.py (1)
171-202: Add status checks tostream_asyncandresource_asyncmethods.The streaming methods at lines 171 and 182 should include
await self.check_exporter_status()calls before establishing connections, matching the pattern used incall_async(line 119) andstreamingcall_async(line 148). Both streaming methods interact withself.stub.Stream(), which requires the exporter to be ready—the same precondition enforced by the status checks in the RPC call methods.
🤖 Fix all issues with AI Agents
In @packages/jumpstarter/jumpstarter/client/core.py:
- Line 87: The async method check_exporter_status is missing a return type
annotation; update its signature (async def check_exporter_status(self) ->
<Type>: ) to include the correct return type—use -> bool if it returns a success
flag, -> None if it never returns a value, or the specific type it yields—so add
the appropriate annotation on the check_exporter_status definition to match its
implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/client/core.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter/jumpstarter/client/core.py
🧠 Learnings (4)
📚 Learning: 2025-10-02T15:29:09.070Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 681
File: packages/jumpstarter/jumpstarter/client/lease.py:198-220
Timestamp: 2025-10-02T15:29:09.070Z
Learning: In the jumpstarter codebase, `client_from_path` and `client_from_channel` functions in `packages/jumpstarter/jumpstarter/client/client.py` automatically call `GetReport(empty_pb2.Empty())` during client initialization. This means that successfully entering the client context manager guarantees the exporter is responsive and has answered the GetReport RPC.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter/jumpstarter/client/core.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.py
📚 Learning: 2025-09-02T07:32:04.548Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py:0-0
Timestamp: 2025-09-02T07:32:04.548Z
Learning: *_pb2.py files in the jumpstarter codebase are auto-generated from gRPC/protobuf definitions and should not be manually modified. Issues with these files should be addressed at the protobuf generation tooling level or runtime environment setup.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.py
🧬 Code graph analysis (1)
packages/jumpstarter/jumpstarter/client/core.py (5)
packages/jumpstarter/jumpstarter/common/enums.py (3)
ExporterStatus(8-45)from_proto(39-41)from_proto(70-72)packages/jumpstarter/jumpstarter/common/metadata.py (1)
Metadata(8-14)packages/jumpstarter/jumpstarter/exporter/session.py (1)
GetStatus(172-178)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (2)
GetStatus(613-617)GetStatus(801-825)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi (1)
GetStatusRequest(690-695)
⏰ 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)
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (6)
packages/jumpstarter/jumpstarter/client/core.py (6)
5-6: LGTM!Standard import for postponed evaluation of type annotations, enabling cleaner forward references.
19-19: LGTM!The
ExporterStatusimport is necessary for the new readiness check functionality.
53-57: LGTM!The
ExporterNotReadyexception follows the existing exception hierarchy and provides a clear signal for readiness issues.
101-113: Excellent backward compatibility handling.The graceful fallback when
GetStatusis unimplemented ensures compatibility with older exporters while enabling the new hook functionality for updated exporters.
118-119: Status check properly integrated.The readiness check is correctly placed before the driver call, ensuring operations are only attempted when the exporter is in an appropriate state.
147-148: Status check properly integrated.The readiness check is correctly placed before the streaming driver call, consistent with the
call_asyncimplementation.
|
Nice, first I tested it with an older version of the server: it complains for the missing "ReportStatus", but it all works Sometimes I see the post hook hanging a bit before it gets to connect/work, i.e.: We can will have to figure this out, but in follow up/separate PR/issue. |
| # Statuses that allow driver commands | ||
| ALLOWED_STATUSES = { | ||
| ExporterStatus.LEASE_READY, | ||
| ExporterStatus.BEFORE_LEASE_HOOK, |
There was a problem hiding this comment.
we can check that later, bud do we want to let commands during before/after when a call is remote?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/jumpstarter/jumpstarter/exporter/hooks_test.py`:
- Around line 159-162: The test currently patches asyncio.wait_for but the
implementation uses anyio.move_on_after, so update the test to either (A) mock
anyio.move_on_after instead of asyncio.wait_for — patch the symbol
anyio.move_on_after and have the mock act as an async context manager that
triggers cancellation (e.g., yields a context with .cancelled True / raises
CancellationError inside) while keeping the patch of
asyncio.create_subprocess_shell returning mock_process, or (B) convert the test
to an integration-style run using a real short timeout and a subprocess that
sleeps briefly to trigger the timeout; locate the test that patches
asyncio.create_subprocess_shell and replace the wait_for patch with a
mock/behavior for anyio.move_on_after (or switch to a real short-timeout run) so
the timeout mock actually affects the code path.
♻️ Duplicate comments (5)
packages/jumpstarter/jumpstarter/client/core.py (2)
87-113: Missing return type annotation.The method should include a return type annotation for completeness:
- async def check_exporter_status(self): + async def check_exporter_status(self) -> None:
118-119: Status check integration is correct, but note prior performance discussion.The status checks are correctly placed before driver calls. However, as noted in past review comments, calling
check_exporter_status()before every driver call introduces additional gRPC round-trips that may impact latency, especially with remote controllers. The suggestion to cache the status or move this check exporter-side was raised previously.Also applies to: 147-148
packages/jumpstarter/jumpstarter/exporter/session.py (1)
44-46:_status_update_eventremains unused.As noted in the previous review (and acknowledged by the maintainer),
_status_update_eventis initialized but never used. Since the maintainer confirmed this is intentional dead code, consider removing it to avoid confusion, or add a TODO comment if it's planned for future use.packages/jumpstarter/jumpstarter/exporter/hooks.py (2)
51-56: Remove unuseddevice_factoryfield.The
device_factoryfield is declared but never referenced withinHookExecutor. All hook execution useslease_scope.sessioninstead. This should be removed to avoid confusion.Suggested fix
`@dataclass`(kw_only=True) class HookExecutor: """Executes lifecycle hooks with access to the j CLI.""" config: HookConfigV1Alpha1 - device_factory: Callable[[], "Driver"]
144-231: Reduce complexity to fix Ruff C901 violation (pipeline blocking).The
_execute_hook_processmethod has cyclomatic complexity of 12, exceeding the limit of 10. This is causing the pipeline to fail. Extract the output reading logic into a separate method.Suggested refactor
+ async def _read_process_output(self, process) -> list[str]: + """Read stdout line by line and log each line.""" + output_lines: list[str] = [] + assert process.stdout is not None + buffer = b"" + async for chunk in process.stdout: + buffer += chunk + while b"\n" in buffer: + line, buffer = buffer.split(b"\n", 1) + line_decoded = line.decode().rstrip() + output_lines.append(line_decoded) + logger.info("[hook output] %s", line_decoded) + # Handle any remaining data without newline + if buffer: + line_decoded = buffer.decode().rstrip() + if line_decoded: + output_lines.append(line_decoded) + logger.info("[hook output] %s", line_decoded) + return output_lines + async def _execute_hook_process( self, hook_config: HookInstanceConfigV1Alpha1, lease_scope: "LeaseContext", log_source: LogSource, hook_env: dict[str, str], logging_session: Session, hook_type: Literal["before_lease", "after_lease"], ) -> None: - """Execute the hook process with the given environment and logging session. - - Uses anyio for subprocess execution to be compatible with the anyio-based exporter. - """ - + """Execute the hook process with the given environment and logging session.""" command = hook_config.script timeout = hook_config.timeout on_failure = hook_config.on_failure @@ ... @@ async with await open_process( command, env=hook_env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) as process: - output_lines: list[str] = [] - - async def read_output() -> None: - """Read stdout line by line.""" - assert process.stdout is not None - buffer = b"" - async for chunk in process.stdout: - buffer += chunk - while b"\n" in buffer: - line, buffer = buffer.split(b"\n", 1) - line_decoded = line.decode().rstrip() - output_lines.append(line_decoded) - logger.info("[hook output] %s", line_decoded) - # Handle any remaining data without newline - if buffer: - line_decoded = buffer.decode().rstrip() - if line_decoded: - output_lines.append(line_decoded) - logger.info("[hook output] %s", line_decoded) - # Use move_on_after for timeout with anyio.move_on_after(timeout) as cancel_scope: - await read_output() + await self._read_process_output(process) await process.wait()
🧹 Nitpick comments (5)
packages/jumpstarter/jumpstarter/common/types.py (1)
16-18: Remove emptyTYPE_CHECKINGblock or add planned content.The
TYPE_CHECKINGguard with onlypassinside serves no current purpose. Consider removing it or adding the async stub type aliases mentioned in the comment.♻️ Proposed fix
-# Async stub type aliases are only available for type checking (defined in .pyi files) -if TYPE_CHECKING: - passpackages/jumpstarter/jumpstarter/config/exporter_test.py (1)
106-156: LGTM! Comprehensive hook configuration test.The test effectively validates:
- Parsing of
beforeLeaseandafterLeasehooks from YAML- Multiline script content preservation
- Round-trip save/load functionality
- Correct use of camelCase aliases in YAML output
Consider adding assertions for
timeoutandonFailurefields to ensure all hook configuration properties are tested:💡 Optional: Add timeout/onFailure assertions
assert config.hooks.before_lease.script == 'echo "Pre-lease hook for $LEASE_NAME"\nj power on\n' + assert config.hooks.before_lease.timeout == 600 + assert config.hooks.before_lease.on_failure == "warn" # default value assert config.hooks.after_lease.script == 'echo "Post-lease hook for $LEASE_NAME"\nj power off\n' + assert config.hooks.after_lease.timeout == 600 + assert config.hooks.after_lease.on_failure == "warn" # default valuepackages/jumpstarter/jumpstarter/exporter/hooks_test.py (1)
207-244: Test name doesn't match what's being verified.The test is named
test_real_time_output_loggingbut only verifies wrapper messages ("Executing before-lease hook...","Hook executed successfully"), not the actual output lines being logged in real-time.Looking at
hooks.py, output lines are logged vialogger.info("[hook output] %s", line_decoded). If the intent is to verify real-time output logging, the expected calls should include:call("[hook output] %s", "Line 1"), call("[hook output] %s", "Line 2"), call("[hook output] %s", "Line 3"),However, since the mocks target the wrong subprocess API (as noted above), fixing the mock infrastructure should be prioritized first.
packages/jumpstarter/jumpstarter/exporter/logging.py (1)
31-39: Prefix matching uses first-match semantics — consider if this is intentional.The loop returns on the first prefix match. If multiple prefixes could match (e.g.,
"driver."and"driver.power."), the result depends on insertion order rather than specificity.Current usage appears safe since mappings don't overlap (
"driver."vs"hook.{lease_name}"), but if future use cases require hierarchical overrides, consider sorting by prefix length (longest first) or documenting the first-match behavior.packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
296-315: Consider using % formatting for logger calls.The f-strings in logger calls (lines 313, 315) eagerly evaluate even when the log level is disabled. Using
%formatting allows lazy evaluation.Suggested fix
- logger.info(f"Updated status to {status}: {message}") + logger.info("Updated status to %s: %s", status, message) except Exception as e: - logger.error(f"Failed to update status: {e}") + logger.error("Failed to update status: %s", e)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
buf.gen.yamlpackages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.pypackages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.pypackages/jumpstarter-cli/jumpstarter_cli/get.pypackages/jumpstarter-cli/jumpstarter_cli/run.pypackages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.pypackages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyipackages/jumpstarter-testing/jumpstarter_testing/pytest_test.pypackages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter/jumpstarter/common/__init__.pypackages/jumpstarter/jumpstarter/common/enums.pypackages/jumpstarter/jumpstarter/common/types.pypackages/jumpstarter/jumpstarter/common/utils.pypackages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/config/exporter.pypackages/jumpstarter/jumpstarter/config/exporter_test.pypackages/jumpstarter/jumpstarter/driver/base.pypackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/exporter/hooks.pypackages/jumpstarter/jumpstarter/exporter/hooks_test.pypackages/jumpstarter/jumpstarter/exporter/lease_context.pypackages/jumpstarter/jumpstarter/exporter/logging.pypackages/jumpstarter/jumpstarter/exporter/logging_protocol.pypackages/jumpstarter/jumpstarter/exporter/session.py
🚧 Files skipped from review as they are similar to previous changes (11)
- buf.gen.yaml
- packages/jumpstarter/jumpstarter/driver/base.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py
- packages/jumpstarter/jumpstarter/exporter/lease_context.py
- packages/jumpstarter/jumpstarter/common/init.py
- packages/jumpstarter-cli/jumpstarter_cli/get.py
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/common/enums.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py
- packages/jumpstarter/jumpstarter/common/utils.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.pyi
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-cli/jumpstarter_cli/run.pypackages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.pypackages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.pypackages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter/jumpstarter/exporter/session.pypackages/jumpstarter-testing/jumpstarter_testing/pytest_test.pypackages/jumpstarter/jumpstarter/config/exporter_test.pypackages/jumpstarter/jumpstarter/exporter/logging_protocol.pypackages/jumpstarter/jumpstarter/exporter/hooks_test.pypackages/jumpstarter/jumpstarter/exporter/logging.pypackages/jumpstarter/jumpstarter/common/types.pypackages/jumpstarter/jumpstarter/exporter/hooks.pypackages/jumpstarter/jumpstarter/config/exporter.pypackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/client/grpc.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
📚 Learning: 2025-08-14T13:11:35.034Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
Applied to files:
packages/jumpstarter-cli/jumpstarter_cli/run.pypackages/jumpstarter/jumpstarter/exporter/hooks.pypackages/jumpstarter/jumpstarter/exporter/exporter.py
📚 Learning: 2025-09-15T08:18:48.571Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Applied to files:
packages/jumpstarter-cli/jumpstarter_cli/run.py
📚 Learning: 2025-10-02T15:29:09.070Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 681
File: packages/jumpstarter/jumpstarter/client/lease.py:198-220
Timestamp: 2025-10-02T15:29:09.070Z
Learning: In the jumpstarter codebase, `client_from_path` and `client_from_channel` functions in `packages/jumpstarter/jumpstarter/client/client.py` automatically call `GetReport(empty_pb2.Empty())` during client initialization. This means that successfully entering the client context manager guarantees the exporter is responsive and has answered the GetReport RPC.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.py
📚 Learning: 2025-09-02T07:32:04.548Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py:0-0
Timestamp: 2025-09-02T07:32:04.548Z
Learning: *_pb2.py files in the jumpstarter codebase are auto-generated from gRPC/protobuf definitions and should not be manually modified. Issues with these files should be addressed at the protobuf generation tooling level or runtime environment setup.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi
📚 Learning: 2025-10-06T15:44:51.111Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 690
File: packages/jumpstarter/jumpstarter/config/exporter.py:51-56
Timestamp: 2025-10-06T15:44:51.111Z
Learning: In jumpstarter config instantiation (packages/jumpstarter/jumpstarter/config/exporter.py), passing **self.root.config to driver_class constructors is intentional validation behavior: since Driver classes are dataclasses, the __init__ will reject any config keys that don't match declared fields with a TypeError, ensuring only declared/supported fields can be used in config YAML files. This is the desired behavior for config safety.
Applied to files:
packages/jumpstarter/jumpstarter/config/exporter.py
🧬 Code graph analysis (12)
packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
exit_code(194-200)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (4)
rich_add_columns(81-91)rich_add_columns(139-140)rich_add_rows(110-125)rich_add_rows(142-144)
packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py (3)
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
session(381-401)packages/jumpstarter/jumpstarter/exporter/session.py (1)
update_status(152-158)packages/jumpstarter/jumpstarter/client/lease.py (1)
update_status(376-399)
packages/jumpstarter/jumpstarter/config/exporter_test.py (1)
packages/jumpstarter/jumpstarter/config/exporter.py (4)
ExporterConfigV1Alpha1(112-260)load(148-151)save(166-175)dump_yaml(162-163)
packages/jumpstarter/jumpstarter/exporter/hooks_test.py (2)
packages/jumpstarter/jumpstarter/exporter/hooks.py (4)
HookExecutionError(26-48)HookExecutor(52-434)execute_before_lease_hook(233-251)execute_after_lease_hook(253-271)packages/jumpstarter/jumpstarter/exporter/lease_context.py (1)
LeaseContext(19-84)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (4)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (2)
_MaybeAsyncIterator(28-28)_ServicerContext(30-31)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (2)
_MaybeAsyncIterator(22-22)_ServicerContext(24-25)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (2)
_MaybeAsyncIterator(21-21)_ServicerContext(23-24)
packages/jumpstarter/jumpstarter/exporter/logging.py (4)
packages/jumpstarter/jumpstarter/exporter/logging_protocol.py (2)
LoggerRegistration(8-22)add_logger_source(15-22)packages/jumpstarter/jumpstarter/common/enums.py (1)
LogSource(48-76)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi (1)
LogStreamResponse(499-521)packages/jumpstarter/jumpstarter/exporter/session.py (2)
context_log_source(168-170)add_logger_source(160-162)
packages/jumpstarter/jumpstarter/config/exporter.py (4)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyi (1)
ExporterStatus(42-46)packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
session(381-401)packages/jumpstarter/jumpstarter/exporter/lease_context.py (1)
update_status(73-84)packages/jumpstarter/jumpstarter/exporter/hooks.py (1)
HookExecutor(52-434)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (4)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (2)
_MaybeAsyncIterator(28-28)_ServicerContext(30-31)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (2)
_MaybeAsyncIterator(22-22)_ServicerContext(24-25)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (2)
_MaybeAsyncIterator(21-21)_ServicerContext(23-24)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (3)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (2)
_MaybeAsyncIterator(22-22)_ServicerContext(24-25)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (2)
_MaybeAsyncIterator(21-21)_ServicerContext(23-24)
packages/jumpstarter/jumpstarter/exporter/exporter.py (5)
packages/jumpstarter/jumpstarter/common/enums.py (3)
ExporterStatus(8-45)to_proto(43-45)to_proto(74-76)packages/jumpstarter/jumpstarter/common/streams.py (1)
connect_router_stream(37-48)packages/jumpstarter/jumpstarter/exporter/hooks.py (3)
HookExecutor(52-434)run_after_lease_hook(360-434)run_before_lease_hook(273-358)packages/jumpstarter/jumpstarter/exporter/lease_context.py (5)
LeaseContext(19-84)update_status(73-84)is_active(57-59)has_client(61-63)update_client(65-67)packages/jumpstarter/jumpstarter/exporter/session.py (4)
Session(33-178)GetReport(110-119)update_status(152-158)serve_unix_async(96-99)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi (2)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyi (1)
ExporterStatus(42-46)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.pyi (9)
Condition(107-146)HasField(97-97)HasField(133-133)WhichOneof(100-100)WhichOneof(102-102)WhichOneof(136-136)WhichOneof(138-138)WhichOneof(140-140)WhichOneof(142-142)
🪛 GitHub Actions: Lint
packages/jumpstarter/jumpstarter/exporter/hooks.py
[error] 144-144: C901 _execute_hook_process is too complex (12 > 10)
🪛 GitHub Check: ruff
packages/jumpstarter/jumpstarter/exporter/hooks.py
[failure] 144-144: Ruff (C901)
packages/jumpstarter/jumpstarter/exporter/hooks.py:144:15: C901 _execute_hook_process is too complex (12 > 10)
⏰ 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). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (54)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (3)
29-30: LGTM!The new optional fields are properly defined with Pydantic aliases to match the API's camelCase convention (
exporterStatus,statusMessage) while using snake_case internally.
60-61: LGTM!Safe extraction of optional fields using
.get()is consistent with the existing pattern for other optional fields likecredentialanddevices.
81-106: LGTM!The status rendering logic correctly handles both cases where
self.statusmight be missing and whereexporter_statusis None, defaulting to "Unknown" in either case. The STATUS column placement after NAME is consistent with the pattern inleases.py.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py (5)
299-309: LGTM!Test fixture correctly updated to include the new
exporter_statusfield with the expected value "Available".
324-326: LGTM!Expected JSON and YAML outputs correctly include both
exporterStatusandstatusMessagefields with proper casing to match the Pydantic field aliases.Also applies to: 342-343
354-360: LGTM!Test assertion correctly verifies that "Available" status appears in the CLI table output.
517-542: LGTM!Exporter list fixtures consistently include
exporter_status="Available"for all items, maintaining test consistency.
671-700: LGTM!Device list fixtures updated consistently with the status field.
packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
79-94: LGTM! Exit code priority logic is clear and correct.The implementation properly handles the three exit scenarios in priority order: signal-based termination, hook failure exit codes, and normal restart. The code is well-documented with comments explaining the priority order.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (1)
1-20: Auto-generated stub file is consistent with existing patterns.This file follows the same structure as other
*_pb2_grpc.pyifiles in the codebase (e.g.,common_pb2_grpc.pyi,router_pb2_grpc.pyi). Based on learnings, these files are auto-generated from gRPC/protobuf definitions and should not be manually modified.packages/jumpstarter/jumpstarter/client/core.py (1)
53-57: Clean exception class definition.The
ExporterNotReadyexception is well-placed in the exception hierarchy, inheriting fromDriverErrorfor consistent error handling.packages/jumpstarter/jumpstarter/common/types.py (1)
8-14: LGTM! Useful centralization of gRPC type aliases.The type aliases provide a clean abstraction for gRPC stubs and channels, making imports more consistent across the codebase.
Also applies to: 20-25
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (1)
1-20: Auto-generated stub file follows established patterns.This file is generated by mypy-protobuf and matches the structure of other
*_pb2_grpc.pyifiles in the protocol package. Based on learnings, these files should not be manually modified.packages/jumpstarter/jumpstarter/client/grpc.py (7)
15-23: LGTM! Status display capability added correctly.The import of
ExporterStatusand the newshow_statusflag inWithOptionsalign with the broader status integration across the PR.
32-33: LGTM!The STATUS column is conditionally added based on the
show_statusoption, consistent with the existing pattern forshow_onlineandshow_leases.
48-50: LGTM!The status string rendering handles the
Nonecase gracefully by defaulting to"UNKNOWN".
90-99: LGTM! Backward-compatible status field handling.The
from_protobufmethod defensively checks forstatusfield presence usinghasattrand truthiness before converting, ensuring backward compatibility with older protobuf messages that may lack the status field.
257-275: LGTM!The
include_statusfield and its propagation throughrich_add_columnsandrich_add_rowsfollows the established pattern forinclude_onlineandinclude_leases.
292-305: LGTM!The serialization logic correctly excludes the
statusfield wheninclude_statusisFalse, maintaining consistent behavior with other conditional fields.
491-491: LGTM!The type annotation change from untyped to
dict[Channel, SimpleNamespace]improves type safety for the internal stubs dictionary.packages/jumpstarter/jumpstarter/config/exporter.py (7)
19-20: LGTM!The
TYPE_CHECKINGguard for theDriverimport avoids circular import issues at runtime while preserving type annotation support.
23-51: LGTM! Well-structured hook configuration models.The
HookInstanceConfigV1Alpha1andHookConfigV1Alpha1models are cleanly defined with:
- Proper field aliases (
onFailure,beforeLease,afterLease) for YAML/JSON serialization- Sensible defaults (120s timeout,
warnon failure)- Clear descriptions for each field
The
on_failureliteral types (warn,endLease,exit) align with the handling inHookExecutor._handle_hook_failurefrom the relevant code snippets.
76-88: LGTM!The return type annotation
-> "Driver"provides better type information for callers ofinstantiate().
128-128: LGTM!The
hooksfield withField(default_factory=HookConfigV1Alpha1)ensures a default empty hooks configuration, maintaining backward compatibility with existing config files that don't specify hooks.
163-174: LGTM! Consistent serialization with aliases.Adding
by_alias=Trueto bothdump_yamlandsaveensures YAML output uses camelCase keys (e.g.,beforeLease,afterLease,onFailure) matching the expected config file format.
184-199: LGTM!Setting
ExporterStatus.LEASE_READYfor local usage viaserve_unix_asyncis appropriate since there's no lease/hook lifecycle flow in local mode—the exporter is immediately ready for commands.
224-246: LGTM! Conditional HookExecutor creation.The
HookExecutoris only instantiated when at least one hook (before_leaseorafter_lease) is configured, avoiding unnecessary overhead when hooks aren't used. The executor is correctly passed to theExporterconstructor.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (1)
1-96: Auto-generated file - no manual modifications needed.This file is generated by
mypy-protobufas indicated in the header. Per project learnings,*_pb2.pyand related stub files are auto-generated from gRPC/protobuf definitions and should not be manually modified. Any issues should be addressed at the protobuf generation tooling level.The generated types provide proper sync/async gRPC stub support for
RouterService, which aligns with the broader typed protocol surface introduced in this PR.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyi (1)
1-307: Auto-generated file - no manual modifications needed.This file is generated by
mypy-protobufas indicated in the header. Per project learnings,*_pb2.pyand related stub files are auto-generated from gRPC/protobuf definitions and should not be manually modified. Any issues should be addressed at the protobuf generation tooling level.The generated types provide a complete typed surface for
ClientServicewith proper sync/async support, consistent with other v1 protocol stubs in this PR.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.pyi (1)
1-96: LGTM - Well-formed auto-generated protobuf stubs.The
ExporterStatusandLogSourceenum stubs are correctly generated with appropriate values supporting the new hook lifecycle states (BEFORE_LEASE_HOOK, AFTER_LEASE_HOOK, and their failed variants). The type aliases and docstrings are properly defined. Based on learnings, these files should not be manually modified.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi (1)
1-318: LGTM - Well-formed auto-generated client protobuf stubs.The client message stubs are correctly generated with proper type references to
common_pb2.ExporterStatusandkubernetes_pb2.Condition. The Lease and Exporter message structures support the new status reporting capabilities introduced by this PR. Based on learnings, issues with these files should be addressed at the protobuf generation tooling level.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi (1)
1-717: LGTM - Well-formed auto-generated jumpstarter protobuf stubs.The generated stubs correctly define:
ReportStatusRequestwithExporterStatus.ValueTypefor hook status reportingLogStreamResponsewith optionalsourcefield usingLogSource.ValueTypeto distinguish log originsGetStatusRequest/GetStatusResponsefor client status preflight checksThese align with the PR's hook execution and status propagation features. Based on learnings, these files should not be manually modified.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.pyi (1)
1-73: LGTM - Well-formed auto-generated router protobuf stubs.The router streaming message stubs are correctly generated with
FrameTypeenum values following HTTP/2 conventions and symmetricStreamRequest/StreamResponsestructures. Based on learnings, these files should not be manually modified.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (1)
1-752: LGTM - Well-formed auto-generated gRPC service stubs.The gRPC service stubs are correctly generated with:
ControllerService.ReportStatusRPC for exporters to report hook execution statusExporterService.GetStatusRPC for client preflight status checks- Proper generic typing supporting both sync (
grpc.Channel) and async (grpc.aio.Channel) channels- Abstract servicer classes with correct
Union[T, Awaitable[T]]return types for sync/async compatibilityBased on learnings, these files should not be manually modified.
packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py (1)
4-4: LGTM!The import and status update are correctly integrated. Setting
ExporterStatus.LEASE_READYfor local testing appropriately simulates the post-hook state that would normally be set after the before-lease hook completes in production.Also applies to: 22-23
packages/jumpstarter/jumpstarter/exporter/logging_protocol.py (1)
1-22: LGTM!Clean protocol definition that properly decouples the logger registration interface from concrete implementations. This enables
get_logger()inlogging.pyto accept any object implementingadd_logger_source()without creating circular imports.packages/jumpstarter/jumpstarter/exporter/session.py (1)
152-178: LGTM!The new status and logging methods are well-implemented:
update_status()correctly handles both proto int values andExporterStatusenum- Logger source management methods properly delegate to the underlying
LogHandlerGetStatus()gRPC handler returns the appropriate proto responseThe API aligns with the
LoggerRegistrationprotocol defined inlogging_protocol.py.packages/jumpstarter/jumpstarter/exporter/logging.py (2)
69-89: LGTM!The
get_loggerfunction provides a clean API for obtaining loggers with automatic source registration. The optionalsessionparameter allows flexible usage — callers can get a plain logger or register it with a session in one call.
12-19: Thread-safe initialization looks good.Using
RLockfor_child_handlersaccess is appropriate since the same thread might callget_source_for_recordwhile holding the lock fromadd_child_handler(though current code paths don't do this).packages/jumpstarter/jumpstarter/exporter/hooks.py (6)
25-49: LGTM!The
HookExecutionErrordataclass properly encapsulates hook failure context with appropriate helper methods for determining shutdown behavior based on theon_failuresetting.
58-76: LGTM!The environment setup correctly passes the Unix socket path, driver permissions, and lease metadata to hook scripts.
78-106: LGTM!The hook execution entry point properly validates the command and delegates to the process execution method.
108-142: LGTM!The failure handling logic properly differentiates between warning-only and fatal failures with correct exception chaining.
273-358: LGTM!The before-lease hook orchestration is well-structured with proper timeout handling, status reporting, and the critical
finallyblock that always signals the event to prevent deadlocks.
360-433: LGTM!The after-lease hook orchestration correctly handles the cleanup phase with appropriate error handling and status reporting.
packages/jumpstarter/jumpstarter/exporter/exporter.py (9)
1-34: LGTM!The imports are well-organized with proper use of
TYPE_CHECKINGfor theDriverforward reference to avoid circular imports.
38-166: Excellent documentation on fields.The comprehensive docstrings for each field significantly improve code maintainability and developer onboarding. The
_previous_leasedfield is now properly declared as a dataclass field (addressing the earlier review comment).
168-191: LGTM!The
stop()method properly integrates the newexit_codeparameter with clear logging about unregistration behavior.
206-243: LGTM!The generic retry wrapper correctly handles stream reconnection with backoff. The retry counter reset after successful iteration ensures long-running streams maintain resilience.
269-294: LGTM!The registration logic correctly handles the conditional status reporting to avoid race conditions with hook execution.
317-339: LGTM!The unregistration logic is properly defensive with timeout handling and shielded cleanup.
403-465: LGTM!The lease handling correctly orchestrates session creation, hook synchronization, and connection processing. The status sync after hook completion ensures the session receives the correct
LEASE_READYstatus.
467-568: LGTM!The
serve()method correctly orchestrates the status stream handling, lease transitions, and hook execution. The shieldedCancelScopeblocks ensure after-lease hooks complete even during cancellation. Based on learnings, the fork-based architecture restarts the exporter process on lease changes, which explains thenot self._startedcheck for handle_lease.
380-401: LGTM!The
session()context manager cleanly encapsulates session creation, Unix socket serving, and controller registration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| with ( | ||
| patch("asyncio.create_subprocess_shell", return_value=mock_process), | ||
| patch("asyncio.wait_for", side_effect=asyncio.TimeoutError()), | ||
| ): |
There was a problem hiding this comment.
Timeout mock also targets wrong API.
The implementation uses anyio.move_on_after for timeout handling, not asyncio.wait_for. This mock won't affect the actual timeout behavior.
🔧 Suggested approach
For anyio timeout testing, consider either:
- Using a real short timeout with a command that actually sleeps (integration test approach)
- Mocking
anyio.move_on_afterto simulate cancellation, though this requires careful handling of the cancel scope pattern
🤖 Prompt for AI Agents
In `@packages/jumpstarter/jumpstarter/exporter/hooks_test.py` around lines 159 -
162, The test currently patches asyncio.wait_for but the implementation uses
anyio.move_on_after, so update the test to either (A) mock anyio.move_on_after
instead of asyncio.wait_for — patch the symbol anyio.move_on_after and have the
mock act as an async context manager that triggers cancellation (e.g., yields a
context with .cancelled True / raises CancellationError inside) while keeping
the patch of asyncio.create_subprocess_shell returning mock_process, or (B)
convert the test to an integration-style run using a real short timeout and a
subprocess that sleeps briefly to trigger the timeout; locate the test that
patches asyncio.create_subprocess_shell and replace the wait_for patch with a
mock/behavior for anyio.move_on_after (or switch to a real short-timeout run) so
the timeout mock actually affects the code path.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
144-200: Address linter complexity warning (C901).The pipeline reports
_shell_with_signal_handlinghas cyclomatic complexity 12 > 10. Consider extracting helpers to reduce nesting:
- Extract token validation (lines 153-162) into a helper like
_validate_token_not_expired(token)- Extract the token expiry warning logic (lines 186-194) into
_check_token_expiry_on_cancel(token, lease_used, selector)♻️ Suggested refactor to reduce complexity
def _validate_token_not_expired(token: str | None) -> None: """Raise ConnectionError if token is expired.""" if not token: return try: from jumpstarter_cli_common.oidc import get_token_remaining_seconds remaining = get_token_remaining_seconds(token) if remaining is not None and remaining <= 0: from jumpstarter.common.exceptions import ConnectionError raise ConnectionError("token is expired") except ImportError: pass def _handle_cancel_token_expiry(token: str | None, lease_used, selector: str) -> None: """Warn about token expiry if cancellation was due to expired token.""" if not (lease_used and token): return try: from jumpstarter_cli_common.oidc import get_token_remaining_seconds remaining = get_token_remaining_seconds(token) if remaining is not None and remaining <= 0: _warn_about_expired_token(lease_used.name, selector) except ImportError: passThen in
_shell_with_signal_handling:# Check token before starting token = getattr(config, "token", None) - if token: - try: - from jumpstarter_cli_common.oidc import get_token_remaining_seconds - remaining = get_token_remaining_seconds(token) - if remaining is not None and remaining <= 0: - from jumpstarter.common.exceptions import ConnectionError - raise ConnectionError("token is expired") - except ImportError: - pass + _validate_token_not_expired(token)except cancelled_exc_class: # Check if cancellation was due to token expiry token = getattr(config, "token", None) - if lease_used and token: - try: - from jumpstarter_cli_common.oidc import get_token_remaining_seconds - remaining = get_token_remaining_seconds(token) - if remaining is not None and remaining <= 0: - _warn_about_expired_token(lease_used.name, selector) - except ImportError: - pass + _handle_cancel_token_expiry(token, lease_used, selector) exit_code = 2
♻️ Duplicate comments (4)
packages/jumpstarter/jumpstarter/client/core.py (2)
103-103: Add return type annotation.The method is missing the
-> Nonereturn type annotation for completeness.Proposed fix
- async def check_exporter_status(self): + async def check_exporter_status(self) -> None:
148-149: Status check adds latency per call.Each
call_asyncnow incurs an additionalGetStatusRPC roundtrip. As noted in prior discussions, consider caching the status or moving this check server-side to avoid the overhead, particularly when the controller has significant network latency.packages/jumpstarter/jumpstarter/exporter/hooks.py (2)
51-56: Remove unuseddevice_factoryfield.The
device_factoryfield is declared but never used withinHookExecutor. All hook execution useslease_scope.sessioninstead. This field should be removed to avoid confusion.♻️ Proposed fix
`@dataclass`(kw_only=True) class HookExecutor: """Executes lifecycle hooks with access to the j CLI.""" config: HookConfigV1Alpha1 - device_factory: Callable[[], "Driver"]Also update any instantiation sites to remove the
device_factoryargument.
144-235: Reduce complexity to fix Ruff C901 violation and address test failure.Two issues need attention:
Complexity (C901): This method has cyclomatic complexity of 12, exceeding the limit of 10. Extract the
read_outputnested function to a separate method.Test failure: The pipeline shows
TypeError: 'Mock' object does not support the context manager protocolat line 169. Tests mockinglogging_sessionneed to provide a proper context manager forcontext_log_source.♻️ Suggested refactor for complexity
+ async def _read_process_output(self, process, hook_label: str) -> list[str]: + """Read stdout line by line and log each line.""" + output_lines: list[str] = [] + assert process.stdout is not None + buffer = b"" + async for chunk in process.stdout: + buffer += chunk + while b"\n" in buffer: + line, buffer = buffer.split(b"\n", 1) + line_decoded = line.decode().rstrip() + output_lines.append(line_decoded) + logger.info("[%s hook] %s", hook_label, line_decoded) + if buffer: + line_decoded = buffer.decode().rstrip() + if line_decoded: + output_lines.append(line_decoded) + logger.info("[%s hook] %s", hook_label, line_decoded) + return output_lines + async def _execute_hook_process( ... ) -> None: ... - async def read_output() -> None: - """Read stdout line by line.""" - ... - with anyio.move_on_after(timeout) as cancel_scope: - await read_output() + hook_label = "beforeLease" if hook_type == "before_lease" else "afterLease" + await self._read_process_output(process, hook_label) await process.wait()🧪 Test fix for context manager mock
In test files, update the mock for
logging_session.context_log_source:from contextlib import nullcontext from unittest.mock import MagicMock # When creating the mock session: mock_session = MagicMock() mock_session.context_log_source.return_value = nullcontext()
🧹 Nitpick comments (4)
packages/jumpstarter/jumpstarter/client/core.py (1)
110-115: Consider promotingALLOWED_STATUSESto a class-level constant.The set is recreated on every call. Since this method is invoked before each driver call, moving the constant to the class level avoids repeated allocations.
Proposed refactor
Add at the class level (e.g., after line 74):
_ALLOWED_STATUSES = frozenset({ ExporterStatus.LEASE_READY, ExporterStatus.BEFORE_LEASE_HOOK, ExporterStatus.AFTER_LEASE_HOOK, })Then in the method:
- # Statuses that allow driver commands - ALLOWED_STATUSES = { - ExporterStatus.LEASE_READY, - ExporterStatus.BEFORE_LEASE_HOOK, - ExporterStatus.AFTER_LEASE_HOOK, - } - status = await self.get_status_async() if status is None: # GetStatus not implemented, assume ready for backward compatibility return - if status not in ALLOWED_STATUSES: + if status not in self._ALLOWED_STATUSES: raise ExporterNotReady(f"Exporter status is {status}")packages/jumpstarter/jumpstarter/exporter/lease_context.py (1)
48-51: Consider using explicit exceptions instead of assertions for validation.Assertions can be disabled when Python runs with optimization flags (
-O), which could allow invalidLeaseContextinstances to be created in production. Since this validates constructor arguments, explicit exceptions would be more robust.♻️ Suggested alternative
def __post_init__(self): """Validate that required resources are present.""" - assert self.before_lease_hook is not None, "LeaseScope requires a before_lease_hook event" - assert self.lease_name, "LeaseScope requires a non-empty lease_name" + if self.before_lease_hook is None: + raise ValueError("LeaseContext requires a before_lease_hook event") + if not self.lease_name: + raise ValueError("LeaseContext requires a non-empty lease_name")packages/jumpstarter/jumpstarter/exporter/hooks.py (1)
277-362: LGTM with optional improvement suggestion.The orchestration logic is well-structured with proper error handling and the
finallyblock correctly ensures the event is always set. The TODO at line 350 is noted for follow-up.💡 Optional: Simplify polling with anyio primitives
The manual polling loop (lines 300-311) could be simplified using
anyio.fail_afteror a condition variable pattern, but the current implementation is readable and functional.# Alternative approach using fail_after: with anyio.fail_after(30): while not lease_scope.is_ready(): await anyio.sleep(0.1)packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
296-315: Consider using%formatting for logger calls.Lines 313 and 315 use f-strings in
logger.infoandlogger.error. While functionally correct,%-style formatting allows lazy evaluation (string only constructed if log level is enabled).♻️ Suggested change
- logger.info(f"Updated status to {status}: {message}") + logger.info("Updated status to %s: %s", status, message) except Exception as e: - logger.error(f"Failed to update status: {e}") + logger.error("Failed to update status: %s", e)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/jumpstarter-cli/jumpstarter_cli/shell.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/exporter/hooks.pypackages/jumpstarter/jumpstarter/exporter/lease_context.pypackages/jumpstarter/jumpstarter/exporter/session.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter/jumpstarter/exporter/session.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter/jumpstarter/exporter/lease_context.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pypackages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pypackages/jumpstarter-cli/jumpstarter_cli/shell.pypackages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/exporter/hooks.py
🧠 Learnings (5)
📚 Learning: 2025-10-02T15:29:09.070Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 681
File: packages/jumpstarter/jumpstarter/client/lease.py:198-220
Timestamp: 2025-10-02T15:29:09.070Z
Learning: In the jumpstarter codebase, `client_from_path` and `client_from_channel` functions in `packages/jumpstarter/jumpstarter/client/client.py` automatically call `GetReport(empty_pb2.Empty())` during client initialization. This means that successfully entering the client context manager guarantees the exporter is responsive and has answered the GetReport RPC.
Applied to files:
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pypackages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.py
📚 Learning: 2025-09-02T07:32:04.548Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py:0-0
Timestamp: 2025-09-02T07:32:04.548Z
Learning: *_pb2.py files in the jumpstarter codebase are auto-generated from gRPC/protobuf definitions and should not be manually modified. Issues with these files should be addressed at the protobuf generation tooling level or runtime environment setup.
Applied to files:
packages/jumpstarter/jumpstarter/client/core.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pypackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi
📚 Learning: 2025-08-14T13:11:35.034Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
Applied to files:
packages/jumpstarter/jumpstarter/exporter/exporter.pypackages/jumpstarter/jumpstarter/exporter/hooks.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyipackages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi
🧬 Code graph analysis (3)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
packages/jumpstarter/jumpstarter/common/utils.py (1)
launch_shell(87-163)packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py (3)
format_duration(196-214)get_token_remaining_seconds(161-172)client(55-56)packages/jumpstarter/jumpstarter/client/core.py (2)
log_stream(238-240)end_session_async(125-143)packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (3)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2_grpc.pyi (2)
_MaybeAsyncIterator(14-14)_ServicerContext(16-17)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2_grpc.pyi (2)
_MaybeAsyncIterator(21-21)_ServicerContext(23-24)
packages/jumpstarter/jumpstarter/exporter/hooks.py (3)
packages/jumpstarter/jumpstarter/config/exporter.py (2)
HookConfigV1Alpha1(44-50)HookInstanceConfigV1Alpha1(23-41)packages/jumpstarter/jumpstarter/exporter/session.py (2)
Session(34-214)context_log_source(170-172)packages/jumpstarter/jumpstarter/exporter/lease_context.py (2)
LeaseContext(19-88)is_ready(53-59)
🪛 GitHub Actions: Lint
packages/jumpstarter-cli/jumpstarter_cli/shell.py
[error] 144-144: C901 _shell_with_signal_handling is too complex (12 > 10).
🪛 GitHub Actions: Run Tests
packages/jumpstarter/jumpstarter/exporter/hooks.py
[error] 169-169: TypeError: 'Mock' object does not support the context manager protocol when using 'logging_session.context_log_source'. The tests mock logging_session but do not provide a compatible context manager, causing failures in real-time hook logging.
🪛 GitHub Check: ruff
packages/jumpstarter/jumpstarter/exporter/hooks.py
[failure] 144-144: Ruff (C901)
packages/jumpstarter/jumpstarter/exporter/hooks.py:144:15: C901 _execute_hook_process is too complex (12 > 10)
⏰ 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). (5)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (40)
packages/jumpstarter/jumpstarter/client/core.py (5)
5-6: LGTM!The
__future__.annotationsimport enables PEP 563 deferred evaluation, andExporterStatusis correctly imported to support the new status-checking functionality.Also applies to: 19-19
53-57: LGTM!The
ExporterNotReadyexception appropriately inherits fromDriverError, ensuring it integrates with existing error-handling patterns.
87-101: LGTM!The
get_status_asyncmethod has proper backward compatibility handling for older exporters that don't implementGetStatus, and consistent error wrapping.
125-143: LGTM!The
end_session_asyncmethod has proper backward compatibility handling and consistent error wrapping, matching the pattern established byget_status_async.
177-178: Consistent withcall_async.The same status check is applied before streaming calls. The performance considerations noted for
call_asyncapply here as well.packages/jumpstarter-cli/jumpstarter_cli/shell.py (5)
1-1: LGTM!Standard module-level logger setup using
__name__is correct.Also applies to: 18-19
21-26: LGTM!Good extraction of shell launch logic into a reusable helper. Parameter mapping to
launch_shellis correct.
38-46: LGTM!Defensive ImportError handling makes OIDC monitoring optional, improving resilience in environments without OIDC support.
80-89: LGTM!Clean refactor using
_run_shell_onlyhelper. The docstring clarification about no afterLease hook waiting is helpful.
92-141: Well-structured async lease workflow.The implementation correctly orchestrates the shell execution with afterLease hook handling. The docstring clearly documents the behavior including Ctrl+C handling.
One observation: the
except Exceptionat line 133 is broad but acceptable here since EndSession errors shouldn't prevent shell exit. Thelogger.warningprovides visibility into failures.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py (1)
1-131: Auto-generated protobuf code looks correct.This file is auto-generated from protobuf definitions. The changes correctly add the
EndSessionRequestandEndSessionResponsemessages and update the serialized descriptor offsets for the newEndSessionRPC inExporterService. Based on learnings, any issues with*_pb2.pyfiles should be addressed at the protobuf generation tooling level.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
53-54: Auto-generated protobuf code correctly addsstatus_messagefield.The new
status_messagefield is properly wired into theExportermessage with OUTPUT_ONLY field behavior options. This aligns with the PR's goal of exposing exporter status information. Based on learnings, this file is auto-generated and should not be manually modified.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (1)
575-579: Auto-generated gRPC code correctly addsEndSessionRPC.The
EndSessionRPC is properly wired across all layers:
- Stub (
ExporterServiceStub.EndSession)- Servicer (
ExporterServiceServicer.EndSession) with appropriate docstring explaining the afterLease hook trigger- Server registration (
add_ExporterServiceServicer_to_server)- Experimental API (
ExporterService.EndSession)The
unary_unarypattern is correct for this RPC design where the client waits for the hook to complete.Also applies to: 624-631, 666-670, 845-871
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi (1)
1-321: Auto-generated type stubs look correct.This new file provides comprehensive type stubs for the client protobuf messages. The
Exporterclass correctly includes the newstatus_messagefield (lines 56, 60, 70), and all message types follow the standard mypy-protobuf generation patterns with properDESCRIPTOR, field constants, typed__init__, and accessor methods.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.pyi (3)
603-705: Auto-generatedEndSessiontype definitions look correct.The
EndSessionRPC is properly typed across:
- Type variable
_ExporterServiceEndSessionType(lines 603-617)ExporterServiceStubgeneric parameter and overloads (lines 619-686)- Stub attribute with docstring (lines 701-705)
The
UnaryUnaryMultiCallabletyping is appropriate for this synchronous RPC pattern.
786-795:ExporterServiceServicer.EndSessionabstract method correctly defined.The servicer abstract method properly types the request/response and preserves the docstring explaining the afterLease hook behavior. This matches the implementation in
jumpstarter_pb2_grpc.py.
15-18: > Likely an incorrect or invalid review comment.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.pyi (1)
1-747: Auto-generated protobuf stub file looks correct.This file is auto-generated by mypy-protobuf (as noted in line 2) and should not be manually modified. The generated stubs correctly expose the new protocol messages supporting the hooks feature:
ReportStatusRequest/ReportStatusResponsefor exporter status reportingGetStatusRequest/GetStatusResponsefor client preflight status checksEndSessionRequest/EndSessionResponsefor session terminationLogStreamResponsewith new optionalsourcefield for log source identificationThe structure follows standard mypy-protobuf output patterns with proper type annotations, field number constants, and accessor methods. Based on learnings, any issues with these files should be addressed at the protobuf generation tooling level.
packages/jumpstarter/jumpstarter/exporter/lease_context.py (2)
1-16: LGTM!Clean module setup with appropriate imports. The
TYPE_CHECKINGguard for theSessionimport correctly avoids circular dependencies.
53-88: LGTM!The predicate methods (
is_ready,is_active,has_client) and mutators (update_client,clear_client,update_status) are well-designed. Theupdate_statusmethod correctly handles the race condition where status needs to be available before the session is created.packages/jumpstarter/jumpstarter/exporter/hooks.py (6)
25-49: LGTM!Well-designed exception class that cleanly encapsulates hook failure semantics. The
should_shutdown_exporterandshould_end_leasemethods provide clear intent for error handling logic.
58-76: LGTM!The environment setup correctly provides the hook script with necessary context (socket path, lease metadata) and intentionally allows unsafe drivers for local CLI access.
78-106: LGTM!Clean implementation with proper validation of empty commands and clear delegation to
_execute_hook_process.
108-142: LGTM!Proper failure handling with appropriate logging levels and correct exception chaining using
raise error from cause.
237-275: LGTM!Clean wrapper methods with proper configuration checks and delegation. The early returns when no hook is configured avoid unnecessary work.
364-437: LGTM!Well-structured after-lease hook orchestration with proper error handling. The comment at line 430 correctly notes that
endLeasefailures don't trigger shutdown since the lease has already ended.packages/jumpstarter/jumpstarter/exporter/exporter.py (14)
1-46: LGTM!Clean imports and well-documented class definition. The
TYPE_CHECKINGguard correctly avoids circular imports for theDrivertype.
47-88: LGTM!Excellent documentation for all public configuration fields. The
hook_executorfield is appropriately optional with clear explanation of when it's created.
90-166: LGTM!Internal state fields are well-documented. The
_previous_leasedfield is now properly declared as a dataclass field (addressing past review feedback), and_lease_contextcleanly consolidates lease-related state.
168-200: LGTM!The
stop()method extension withexit_codeparameter cleanly supports the hook failure shutdown mechanism, and the property provides appropriate read access.
202-243: LGTM!Clean abstraction for gRPC stream retry logic. The retry counter correctly resets after successful iteration, allowing streams to recover from transient failures during long-running operation.
245-267: LGTM!Clean factory pattern that enables the generic
_retry_streamto work with different gRPC stream types.
269-294: LGTM!The conditional status reporting at lines 291-294 correctly avoids overwriting
LEASE_READYwithAVAILABLEduring per-lease registration, as clearly documented in the comment.
317-339: LGTM!Robust unregistration with proper timeout handling, shielded channel cleanup, and defensive error logging.
341-350: LGTM!Proper async context manager implementation with cleanup that doesn't mask original exceptions.
352-378: LGTM!Correct connection proxying implementation. Minor note: line 378 uses
.format()style while other logging uses%formatting, but this is a minor inconsistency.
380-409: LGTM!Well-implemented
EndSessionhandler. TheCancelScope(shield=True)correctly protects the after-lease hook from cancellation, and thefinallyblock ensures the done event is always signaled.
411-432: LGTM!Clean session context manager that properly creates the device session, serves on Unix socket, and registers with the controller before yielding the session and path for
LeaseContextpopulation.
434-501: LGTM!Well-orchestrated lease handling that:
- Populates
lease_scopewith session and socket path- Waits for the before-lease hook before accepting connections
- Syncs status after hook completion to fix the race condition
- Spawns the
EndSessionhandler for after-lease hooks
503-612: Well-structured serve loop with hook orchestration.The implementation correctly handles:
- Lease change detection (lines 523-527) with after-lease hook execution
- Before-lease hook on
unleased → leasedtransition (lines 571-583)- After-lease hook on
leased → unleasedtransition (lines 589-602) with duplicate prevention viaafter_lease_hook_done.is_set()checkThe complexity is inherent to the state machine, and the checks at lines 531-535 and 589-594 correctly prevent duplicate after-lease hook execution when triggered via both
EndSessionand status change.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Ok, this is close, but we need passing linter and unit tests :D |
Implement new hook scripts that can be configured in the exporter config to run
jcommands within a temporary lease environment.The following environment variables are exposed to hook scripts:
JUMPSTARTER_HOST- Path to the Unix socket for j CLI accessJMP_DRIVERS_ALLOW=UNSAFE- Allows all drivers for local accessLEASE_NAME- The name of the current leaseCLIENT_NAME- Name of the client that acquired the leaseExample YAML config:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.