refactor(compute): register compiled drivers - #2786
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-2786.docs.buildwithfern.com/openshell |
0074c73 to
86c96e0
Compare
86c96e0 to
75af6cc
Compare
|
Label |
0862fc2 to
5e3695f
Compare
elezar
left a comment
There was a problem hiding this comment.
I think this needs to be split before it can be reviewed confidently. The PR currently combines several independently meaningful changes and then repairs their intermediate states in later commits, which makes it difficult to tell whether each boundary is complete and preserves behavior.
Could we also take #2786 out of the active landing stack for now? #2743 and #2744 are independently useful and should be able to progress without waiting for this restructuring. I do not mean that they should land unchanged:
- #2743 still needs its requested lifecycle corrections: bounded shutdown concurrency, acquiring the lifecycle gate and re-reading current intent in both sweeps, and paging through all persisted sandboxes. The
e2e_stop_gatewayprocess-wait fix from this PR also belongs naturally with #2743 because it compensates for that PR's graceful-shutdown stop sweep. - #2744 should then be rebased on the revised #2743. It can provide the external-driver lifecycle-ownership capability that resolves #2743's built-in-only policy, while also incorporating the pending naming and endpoint-override documentation clarifications as appropriate.
Once those PRs have been updated and merged, #2786 can be rebuilt as a smaller stack on top of the resulting base.
Could we restructure it into a stack along these lines?
- Introduce the compiled-driver registry as a behavior-preserving PREfactor.
- Add the standalone first-party driver executables and their external-driver E2E paths. Please fold the VM default-image fix, Kubernetes supervisor-image reuse, post-renderer-based Kubernetes isolation, and scoped external-Podman job into this change so it is complete when introduced.
- Move first-party composition and VM ownership into
openshell-gatewayand makeopenshell-serverbackend-independent. The backend-neutral server comments and the bundled-Z3/CMake corrections should land with this move. - Update packaging and build references for the new gateway crate.
- Finish the compiled-driver boundary simplification as a focused cleanup.
The gateway shutdown wait in e2e_stop_gateway appears independent of this work and tied to the lifecycle sweep introduced by #2743, so I suggest moving that to #2743 or a small follow-up based on #2744 rather than including it in this stack.
I reconstructed this sequence locally on the current #2744 head. Each proposed boundary compiles, the registry-focused test passes, and the final tree matches this PR, so the split appears mechanically feasible.
8d5c344 to
64c20da
Compare
|
Restructured this work in response to the review. The overall stack is now:
I consolidated the proposed composition, packaging, and final boundary-cleanup steps into #2823 because the packaging references are direct consumers of the crate move and the cleanup removes the transient server-owned boundary. Keeping them together leaves one complete architectural outcome without an intermediate packaging-only PR. All three rewritten PRs are drafts. |
64c20da to
0fe5a3a
Compare
|
/ok to test |
0fe5a3a to
a71ecf1
Compare
a71ecf1 to
2c41046
Compare
1 similar comment
2c41046 to
25f9a63
Compare
25f9a63 to
ceaa9cf
Compare
|
/ok to test |
ceaa9cf to
25f9a63
Compare
25f9a63 to
ceaa9cf
Compare
ceaa9cf to
25f9a63
Compare
|
/ok-to-test 25f9a63 |
|
@drew A quick summary of the review changes I folded into the single commit in this PR and restored at 25f9a63:
This aligns the registry behavior with the extensibility concerns discussed in #2163. The current PR head is 25f9a63. |
25f9a63 to
25920c4
Compare
elezar
left a comment
There was a problem hiding this comment.
Reviewed and addressed issues as called out in the comment.
LGTM, but feel free to get a second opinion.
|
/ok-to-test 25920c4 |
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
25920c4 to
0e2ac3d
Compare
Summary
Introduce an explicit compiled compute-driver registry, modeled after SQLx driver installation, as a behavior-preserving pre-factor. The gateway still ships the same first-party drivers at this boundary; selection and auto-detection now consume generic registrations instead of a closed driver-name switch.
Related Issue
Part of #2417.
Stacked on #2744. Followed by #2822.
Changes
ComputeDriverRegistry,ComputeDriverRegistration, andComputeDriverFactoryThis PR intentionally does not add standalone driver binaries or move first-party composition out of
openshell-server; those changes are isolated in the following PRs.Testing
mise run pre-commitcargo test -p openshell-server registry_detection_ --libChecklist