Conversation
f12d10d to
9ad65e1
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The accepted issue makes this work project-valid, but the initial review found two blocking compatibility problems: deterministic credential-driver incompatibility is retried until the startup deadline, and the published migration order has no interoperable old/new pairing.
Action required: fix the two findings below, then push a new head for a focused follow-up review.
Blocking findings:
GATOR-9ad65e15-01: Fail immediately when a launched credential driver proves protocol incompatibility.GATOR-9ad65e15-02: Define and implement a coherent mixed-version rollout contract, or document the required coordinated outage.
Carried findings:
- None
Gator metadata
- Validation: Project-valid through accepted issue #3057, whose negotiated-extension scope matches this PR.
- Docs: Updated, but the migration sequence is materially inconsistent with mandatory missing-metadata rejection.
- Checks: Current-head Branch Checks and Helm Lint are pending; the other reported gates are green.
- E2E:
test:e2eis required for gateway/supervisor and credential-flow changes after review blockers are resolved; dispatch is not started yet. - Head SHA:
9ad65e15efa283e00069209ff7bd43ed8de575e9 - Base SHA:
c502be9fd73c41bab25f0a88587b7a3d90c96b55 - Merge base SHA:
c502be9fd73c41bab25f0a88587b7a3d90c96b55 - Patch ID:
afcb6592f103bb232869cad6bb63c5d4019f40cd - Gator payload:
9 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
the
gator:in-review
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
Requesting changes for the extension-side handshake enforcement gap. The Go constant re-export is non-blocking, but worth fixing in this PR for a complete curated API.
| type ExtensionInfo = types.ExtensionInfo | ||
|
|
||
| // ExtensionKind identifies one supported extension family. | ||
| type ExtensionKind = types.ExtensionKind |
There was a problem hiding this comment.
The curated v1 package aliases ExtensionKind but does not re-export ExtensionKindComputeDriver, ExtensionKindCredentialDriver, ExtensionKindGatewayInterceptor, ExtensionKindSupervisorMiddleware, or ExtensionKindUnknown. Every comparable enum alias in this package re-exports its constants; without them, normal consumers must import the nested types package or compare raw strings. This is non-blocking, but please add the constants here.
johntmyers
added
gator:blocked
9ac6e0e to
7b0384c
Compare
BlockedGator is blocked because this branch conflicts with the current Next action: @sjenning, please merge or rebase the current Gator metadata
|
Closes NVIDIA#3057 Introduce a shared extension handshake, enforce protocol and capability compatibility across extension families, and expose immutable negotiated snapshots through gateway info and the Go SDK. Signed-off-by: Seth Jennings <sjenning@redhat.com>
Signed-off-by: Seth Jennings <sjenning@redhat.com>
Signed-off-by: Seth Jennings <sjenning@redhat.com>
Signed-off-by: Seth Jennings <sjenning@redhat.com>
7b0384c to
874a1c8
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @sjenning. I checked the three focused remediation commits after the rebase. The merge conflict is resolved, the migration guide now clearly requires a coordinated gateway-and-extension outage, and the Go extension-kind constants are re-exported. I also checked @drew's extension-side handshake concern: the first-party handlers and examples now validate gateway metadata before accepting the peer. One carried readiness-path blocker remains.
Action required: classify a credential driver's deterministic handshake rejection, including FAILED_PRECONDITION, as terminal and cover the real RPC-status path with the immediate-failure regression test.
Blocking findings:
- None newly introduced.
Carried findings:
GATOR-9ad65e15-01: A launched credential driver that rejects incompatible gateway metadata throughFAILED_PRECONDITIONis still treated as retryable, so startup waits forstartup_timeout_secsinstead of failing immediately. Preserve the tonic status through readiness classification, treat deterministic handshake statuses as terminal, and retain retries for connection or genuinely transient readiness failures.
Resolved findings:
GATOR-9ad65e15-02: The documentation now states that legacy-to-current migration requires coordinated gateway-and-extension downtime.
Gator metadata
- Validation: Project-valid through accepted issue #3057, whose extension-negotiation scope matches this PR.
- Docs: Updated with a coherent coordinated-outage migration contract.
- Checks: Required test dispatch is deferred until the carried review blocker is resolved.
- E2E:
test:e2eis required for gateway/supervisor and credential-flow changes; it has not been dispatched yet. - Head SHA:
874a1c835f2057c40ab353f6cad9feea3cfbd780 - Base SHA:
9708ba999985ca18ffec5a3310f11e4b04b615f6 - Merge base SHA:
9708ba999985ca18ffec5a3310f11e4b04b615f6 - Patch ID:
13ce8739b93c73dac362f6c8d52aa6bd435211cd - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
9ad65e15efa283e00069209ff7bd43ed8de575e9 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
gator:in-review
Signed-off-by: Seth Jennings <sjenning@redhat.com>
|
Label |
|
/ok to test bc2c686 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @sjenning. I reviewed the focused credential-readiness update: deterministic RPC rejections, including FAILED_PRECONDITION, now fail immediately while connection and transient readiness failures remain retryable. The regression coverage exercises the real RPC-status path, so the last carried finding is resolved and no blocking findings remain.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None.
Resolved findings:
GATOR-9ad65e15-01: The readiness loop now preserves and classifies tonic statuses, and the 30-second-timeout regression verifies an immediate failure for an incompatible driver.
Gator metadata
- Validation: Project-valid through accepted issue #3057, whose extension-negotiation scope matches this PR.
- Docs: Updated with the coordinated-outage migration contract; no additional docs are needed for this focused readiness fix.
- Checks: Current-head Branch Checks and Helm Lint are queued or running.
- E2E:
test:e2eis applied; the current-head mirror exists and Branch E2E Checks is running. - Head SHA:
bc2c68643082283e6a47d153b62fdf994c31e029 - Base SHA:
9708ba999985ca18ffec5a3310f11e4b04b615f6 - Merge base SHA:
9708ba999985ca18ffec5a3310f11e4b04b615f6 - Patch ID:
4ca6982861e8dd9690753ade6cd6c082b52ad591 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
874a1c835f2057c40ab353f6cad9feea3cfbd780 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
johntmyers
added
gator:in-review
3 participants
Summary
Normalize extension startup around a shared version and capability handshake. The gateway now enforces protocol compatibility across compute drivers, credential drivers, gateway interceptors, and supervisor middleware, and exposes immutable negotiated extension snapshots through gateway info and the Go SDK.
This is a breaking wire-contract change for external extensions: peers must return protocol
1.0metadata and the family base capability, and supervisor middlewareDescribenow acceptsMiddlewareDescribeRequest.Related Issue
Closes #3057
Changes
openshell.extension.v1.PeerMetadataand a shared validator supporting same-major minor skew, mutual capability requirements, and actionable rejection errors.GetGatewayInfo,openshell gateway info, and the curated Go SDK.Deviations from Plan
The legacy version fields are documented as deprecated migration fields but do not use protobuf's
[deprecated = true]option. Generated Rust accessors for deprecated fields trigger warnings under the workspace's-D warningspolicy while the gateway intentionally continues populating them during migration.Testing
mise run pre-commitOPENSHELL_NO_BROWSER=0 mise run testOPENSHELL_NO_BROWSER=0 mise run cimise run go:cimise run sdk:ts:ci(109 tests)e2e/changedThe
OPENSHELL_NO_BROWSER=0override neutralizes a shell-exported local setting that otherwise intentionally changes two unrelated OIDC rollback tests.Tests added or updated:
mise run testandmise run ci, including 1,551 servertest-supporttests.e2e/.Checklist
Documentation updated:
docs/extensibility/extension-negotiation.mdx: protocol envelope, skew policy, capabilities, migration, and inspection.architecture/gateway.mdandarchitecture/compute-runtimes.md: negotiated extension boundaries and snapshots.skills/debug-openshell-cluster/SKILL.mdandskills/openshell-cli/SKILL.md: operator diagnostics and gateway-info output.