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 typing stubs; controller registration, lease handling, and status reporting; before/after-lease HookExecutor; per-logger LogSource logging; client-side GetStatus preflight checks; many generated .pyi stubs and accompanying tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Exporter
participant HookExec as HookExecutor
participant LeaseCtx as LeaseContext
Note over Exporter,Controller: Exporter startup & registration
Exporter->>Controller: Register(session metadata)
Controller-->>Exporter: Ack / UUID
Note over Controller,Exporter: Lease allocation via listen stream
Client->>Controller: RequestLease
Controller->>Exporter: Listen stream -> lease_name
Exporter->>LeaseCtx: create LeaseContext(lease_name)
rect rgb(235,245,255)
Note left of HookExec: Before-lease phase (may block or fail)
Exporter->>HookExec: run_before_lease_hook(lease_ctx)
HookExec->>Exporter: report_status(BEFORE_LEASE_HOOK)
HookExec-->>Exporter: hook complete or HookExecutionError
end
Exporter->>Exporter: update session status -> LEASE_READY
Client->>Exporter: DriverCall
Exporter->>Exporter: check_exporter_status (GetStatus)
alt status == LEASE_READY
Exporter-->>Client: forward DriverCall to driver, return response
else not ready
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 and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
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
| 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.
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
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.