Conversation
2038d0d to
e6cada9
Compare
|
@krishicks @drew this is a PR for introducing an OTEL relay to the supervisor, forwarding OTLP traces to a collector via the gateway. This is the hardened OTEL-relay spike that I did for creating this report in #2641 (comment) It would be great if you could put your review agent army on this PR (I did some self-review, but we all know the more reviews the merrier :) See also https://github.com/rhuss/OpenShell/blob/48e9ece979889aa49b1df146c3d0d08e7118496a/architecture/sandbox.md#telemetry-relay for a recap of the overall architecture. |
| // OTEL relay: bind OTLP receiver for all Linux topologies. | ||
| // All current topologies keep the process supervisor co-located with | ||
| // the agent, so 127.0.0.1 is reachable from agent processes. Future | ||
| // topologies that move the supervisor out of the workload pod would | ||
| // need to derive the bind address from the topology (e.g., pod IP | ||
| // via downward API) and update OTEL_EXPORTER_OTLP_ENDPOINT to match. | ||
| let otel_rx = { | ||
| #[cfg(target_os = "linux")] | ||
| { | ||
| let otlp_addr = openshell_core::sandbox_env::OTLP_RECEIVER_ADDR; | ||
|
|
||
| let (otel_session_tx, otel_session_rx) = | ||
| tokio::sync::mpsc::channel::<openshell_core::proto::SupervisorMessage>(64); | ||
|
|
||
| let relay_config = openshell_supervisor_network::otlp::RelayConfig::default(); | ||
| let metadata = openshell_supervisor_network::otlp::SandboxMetadata { | ||
| sandbox_id: sandbox_id.clone().unwrap_or_default(), | ||
| workspace_id: workspace_rx.borrow().clone(), | ||
| policy: sandbox_name_for_agg.clone().unwrap_or_default(), | ||
| user: resolved_process_identity | ||
| .uid() | ||
| .map_or_else(String::new, |uid| uid.to_string()), | ||
| image: std::env::var("OPENSHELL_CONTAINER_IMAGE").unwrap_or_default(), | ||
| driver: std::env::var(openshell_core::sandbox_env::SUPERVISOR_TOPOLOGY) | ||
| .unwrap_or_else(|_| "container".to_string()), | ||
| }; | ||
| let relay = openshell_supervisor_network::otlp::OtelRelay::new( | ||
| relay_config, | ||
| metadata, | ||
| otel_session_tx, | ||
| ); | ||
|
|
||
| let bind_addr: std::net::SocketAddr = otlp_addr.parse().unwrap(); | ||
|
|
||
| if let Some(ns) = netns.as_ref() { | ||
| match ns.bind_tcp_in_netns(otlp_addr).await { | ||
| Ok(listener) => { | ||
| let handle = relay.start_with_listener(listener); | ||
| tracing::info!(bind = %bind_addr, "OTEL relay started (netns)"); | ||
| otel_relay_handle = Some(handle); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(error = %e, "OTEL relay failed to bind in netns; continuing without relay"); | ||
| } | ||
| } | ||
| } else { | ||
| match relay.start(bind_addr).await { | ||
| Ok(handle) => { | ||
| tracing::info!(bind = %bind_addr, "OTEL relay started"); | ||
| otel_relay_handle = Some(handle); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(error = %e, "OTEL relay failed to start; continuing without relay"); | ||
| } | ||
| } | ||
| } | ||
| Some(otel_session_rx) |
There was a problem hiding this comment.
The OTLP listener appears to bind before the gateway confirms the otel_export capability, and it remains active when the gateway declines it. This reserves the standard 127.0.0.1:4318 port even when OTLP export is not configured. The receiver can also return 200 OK for telemetry that will not be forwarded because the session does not drain otel_rx unless the capability is confirmed. Could listener startup be gated on the negotiated capability, or could the listener be shut down when the capability is declined?
There was a problem hiding this comment.
Confirmed. The bind in crates/openshell-sandbox/src/lib.rs:962-1011 is unconditional and happens before run_process opens the supervisor session, while the drain arm in crates/openshell-supervisor-process/src/supervisor_session.rs:461 is gated on otel_confirmed. So a declined capability leaves 4318 reserved and the receiver answering 200 for spans that then die on try_send into the 64-slot channel. architecture/sandbox.md already describes bind-on-confirm as the intended behavior, so this is the code catching up with the docs.
Planned route: bind only on confirmation, no open-then-close. The relay moves into the supervisor session task (the otlp module relocates to openshell-supervisor-process, which is its only consumer). The session binds the listener on the first SessionAccepted that confirms otel_export, keeps it across reconnects, and drains the telemetry buffer directly into the outbound stream. That removes the forwarder task and the intermediate otel_session_tx channel, so the pipeline becomes receiver -> buffer -> session -> gateway with one bounded buffer instead of two, and the silent drop point you found goes away with it.
Two things worth calling out. The agent starts before the handshake (run.rs:247 vs :374), so spans exported in that window get a refused connection instead of a 200; OTel exporters retry with backoff, so in practice they land once the port is up. And the existing handle.shutdown().await in lib.rs:1211 runs after run.rs:490 has already aborted the session, so it drains into a channel nobody reads. With the session owning the relay, run.rs asks it to stop the receiver and flush the buffer after the entrypoint exits and before report_main_process_exit, with a bounded wait, so final spans actually reach the gateway.
There was a problem hiding this comment.
Applied in 0890eee. The receiver is now bound by the session on the first confirming SessionAccepted (RelayLifecycle::ensure_started in crates/openshell-supervisor-process/src/otlp/mod.rs), the forwarder task and intermediate channel are gone, and run_process flushes buffered telemetry before reporting the exit.
| tokio::spawn(async move { | ||
| let svc = service_fn(move |req| { | ||
| let buf_tx = buf_tx.clone(); | ||
| let metadata = metadata.clone(); | ||
| async move { | ||
| handle_request(req, &buf_tx, &metadata, enrichment_enabled).await | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think an accepted keep-alive connection can block shutdown. Each detached serve_connection task retains a TelemetrySender, but receiver_handle tracks only the accept loop. RelayHandle::shutdown() drops only its own sender and then waits for the forwarder. The forwarder cannot observe channel closure until every connection task exits, so this wait is unbounded. Should connection tasks receive shutdown cancellation or be tracked and explicitly drained or aborted?
P.S sorry for the two reviews I selected the wrong lines earlier.
There was a problem hiding this comment.
Confirmed. Each connection task in receiver.rs:85-100 holds a cloned TelemetrySender, receiver_handle only covers the accept loop, and spawn_forwarder exits only when the last sender drops. With HTTP/1.1 keep-alive and no idle timeout on http1::Builder, RelayHandle::shutdown() can wait forever, and it sits on the sandbox teardown path.
Planned fix: connections are tracked in a JoinSet and watched by hyper_util::server::graceful::GracefulShutdown, so shutdown disables keep-alive on every open connection (idle ones close immediately), waits a bounded 2s for in-flight requests, and aborts stragglers. http1::Builder also gets a header read timeout. The final drain no longer waits for senders to drop at all; it uses the buffer's non-blocking drain(), so a straggler cannot stall it. Regression test: keep-alive connection held open, shutdown must complete within the deadline.
No need to apologise for the two reviews, both were worth filing.
There was a problem hiding this comment.
Applied in 0890eee. Connections are tracked in a JoinSet under GracefulShutdown with a header read timeout; ReceiverHandle::shutdown is bounded at 2s plus abort. Regression test: shutdown_completes_with_idle_keepalive_connection in otlp/receiver.rs.
|
@2000krysztof thanks for the review, that was very helpful! I've did some reordering and it should be ensured now that the port is not opened if otel is not enabled. |
There was a problem hiding this comment.
All my concerns were addressed LGMT, nice work @rhuss
aff4ff9 to
0d94083
Compare
|
Rebuilt on top of #2942 (RFC 0012 split boundary); the previously approved commits no longer applied. Same gateway contract as reviewed. What changed: the receiver no longer binds |
0d94083 to
ec9e7fb
Compare
ec9e7fb to
fbf3f69
Compare
|
Hey Roland, would you be open to us helping harden #3196 and improve NeMo Relay compatibility? We found a few issues around buffering, message limits, collector reconnects, and OTLP configuration. We can send small PRs against your branch if helpful. |
|
@afourniernv I think we do want to make sure NeMo Relay compatibility is improved, either in this PR or in a follow-up. I also think we should use a separate lane specifically for agent traces rather than re-use the one for infrastructure-related traces. The end-user can choose to use the same collector endpoint for both but I think making it so you can send infra spans to one and agent spans to another would be a better experience. It could scale independently, be a small collector deployment near the workload, and not require distinguishing spans within the collector via labeling or other mechanisms before export to the appropriate backends. |
Add the gateway half of the supervisor OTLP telemetry relay, rebuilt on top of the RFC 0012 split-boundary architecture (NVIDIA#2942). Protocol: `SupervisorHello.capabilities` and `SessionAccepted.capabilities` negotiate optional supervisor features; `OtelExportData` is a new `SupervisorMessage` payload carrying OTLP trace bytes and OCSF events. Both repeated fields default to empty and the new oneof case is ignored by older peers, so the change is wire-compatible in both directions. Gateway: `confirm_capabilities` grants `otel_export` only when `[openshell.gateway.otlp]` produced a relay exporter. `handle_otel_export` forwards trace data through a dedicated `OtelRelayExporter` that bypasses the gateway's own tracer provider so supervisor-enriched resource attributes survive, and re-emits bounded, well-formed OCSF events on the `ocsf_relay` target. The capability selection and OCSF acceptance checks are pure functions with unit tests; the exporter has decode and URI tests that need no collector. The public RPC schema inventory grows by one message; the fingerprint and architecture/gateway.md are updated accordingly. The supervisor sends an empty capability list until its relay lands in a later commit. Refs NVIDIA#2641 Signed-off-by: Roland Huß <rhuss@redhat.com>
Relocate the supervisor-side OTLP relay onto the RFC 0012 split boundary. The supervisor no longer shares a network namespace with the agent, so the receiver cannot bind a loopback port the agent can reach. Instead the agent exports to a reserved, unroutable destination (192.0.0.8:4318, the RFC 7600 dummy address). The sandbox seccomp broker stages every non-loopback connect for the supervisor, whose proxy will hand the staged stream to the relay rather than dialing upstream. No boundary-protocol or isolation-contract change is required. `openshell-supervisor-process::otlp` keeps the PR's bounded buffer, span enrichment, and OTLP HTTP parsing unchanged. The accept loop becomes `OtlpConnectionServer::serve`, which runs one hyper connection on a stream the caller supplies; `try_reserve` lets the proxy refuse an open before the sandbox commits the workload socket. Shutdown keeps the previous semantics (refuse new streams, disable keep-alive, bounded grace, cut stragglers) with hyper-util's graceful watcher plus an abort signal instead of a task set. `RelayLifecycle` drops the lazy `Pending` state: the relay starts before networking, so there is no window where the agent sees a refused connection. Receiver tests run over `tokio::io::duplex`, standing in for the boundary stream. A constant test pins the reserved address as non-loopback and outside the policy DNS synthetic pool. The proxy hook and session wiring follow in separate commits. Refs NVIDIA#2641 Signed-off-by: Roland Huß <rhuss@redhat.com>
Add a `ReservedStreamHandler` hook to the transparent-open path so a staged workload connection to a reserved, unroutable address is served by the supervisor itself instead of being policy-evaluated and dialed upstream. The branch sits at the top of `preauthorize_transparent_open`, ahead of host mapping, OPA, SSRF checks, and destination validation, because the address is a label the supervisor switches on rather than a place anything could reach. The handler is consulted before `RelayReady` is sent, so an exhausted handler refuses the open with `ResourceExhausted` and the sandbox never commits the workload socket. A connect to the OTLP relay address with no handler installed is denied outright rather than falling through. `ReservedDestination` is threaded through `ProxyHandle::start_with_bind_addr` and `run_networking`; both supervisor call sites pass `None` until the relay is wired in the next commit. No hyper or OTLP dependency enters this crate. Unit tests cover the four cases: served without policy (bytes from the workload side reach the handler), refused at capacity, denied without a handler, and other destinations unaffected. Refs NVIDIA#2641 Signed-off-by: Roland Huß <rhuss@redhat.com>
Start the relay before networking and register its connection server as the proxy's reserved destination, so the first staged workload stream to the relay address is served immediately. The relay lifecycle rides into the gateway session through `SessionRuntimeContext`: the session advertises `otel_export` only when a relay is running, gates forwarding on the gateway's confirmation, and forwards buffered items with a non-blocking send so telemetry can never stall control traffic. Items received while no session has confirmed accumulate in the bounded buffer. `BoundaryAccess::drain_telemetry` sends a one-shot drain request to the session, which stops the receiver and flushes the buffer onto its stream before acking. The supervisor runs it as a completion phase ahead of the main-process exit report, bounded at three seconds so an unreachable gateway cannot delay the report. Span enrichment takes the driver name from the runtime descriptor's fence evidence and the uid from its resolved workload identity, captured before the identity moves into the sandbox context. The image attribute still reads `OPENSHELL_CONTAINER_IMAGE`, which only the Podman driver sets; the OCSF context shares that gap and it is left for a follow-up. Tests cover the drain handshake (no relay, session gone, ack, deadline), capability advertisement and gating, and an end-to-end HTTP round trip through the reserved-destination adapter over an in-memory boundary stream. Refs NVIDIA#2641 Signed-off-by: Roland Huß <rhuss@redhat.com>
When the gateway has a relay exporter, `CreateSandbox` sets `OTEL_EXPORTER_OTLP_ENDPOINT` to the reserved relay address and `OTEL_EXPORTER_OTLP_PROTOCOL` to `http/protobuf` in the sandbox spec's environment. A caller-supplied endpoint is left untouched together with its protocol, so an agent pointed at its own collector through policy-approved egress keeps working. The injection runs after the template merge and before the final size recheck, so it is covered by the existing environment cap and reaches every driver through the declared environment path. The values persist in the stored spec: restarts launch from the stored record and inherit them, and `sandbox get` shows where an agent's traces go. This replaces the supervisor-side injection the relay used when it shared the agent's process tree. Tests cover the injection rule and both create outcomes with and without an exporter on the gateway. Refs NVIDIA#2641 Signed-off-by: Roland Huß <rhuss@redhat.com>
Document the relay as it works on the RFC 0012 architecture: the reserved unroutable destination, the seccomp-staged path into the supervisor, the gateway-side environment injection, capability gating, buffering, drain bounds, and the enriched resource attributes. The Kubernetes runtime page notes that agent telemetry crosses the Pod boundary over the existing channel with no NetworkPolicy change, and the gateway config reference explains what `[openshell.gateway.otlp]` now enables for sandboxes. The AGENTS.md row for openshell-supervisor-process reflects what the crate owns after the isolation split. Refs NVIDIA#2641 Signed-off-by: Roland Huß <rhuss@redhat.com>
`mise run go:proto:gen` after adding `OtelExportData` and the capability fields on `SupervisorHello` and `SessionAccepted`. Generated output only. Refs NVIDIA#2641 Signed-off-by: Roland Huß <rhuss@redhat.com>
…field) - Update public RPC schema fingerprint for the upstream+OtelExportData closure - Add capabilities field to provider_readiness_tests SupervisorHello literal - Remove stale conflict marker in supervisor lib.rs module list Signed-off-by: Roland Huß <rhuss@redhat.com>
fbf3f69 to
f69cc29
Compare
…traces Agent traces relayed from supervisors can now target their own OTLP collector via agent_endpoint in [openshell.gateway.otlp], so operators can scale or place agent-trace collection independently of infrastructure tracing. When the field is absent the agent lane uses the shared endpoint, preserving current behavior. Helm renders the field from server.otlp.agentEndpoint. Signed-off-by: Roland Huß <rhuss@redhat.com>
|
The branch is rebased onto current main (8719fc9) and ready for another look. The gateway contract and the relay design are unchanged from the state @2000krysztof approved. @krishicks the separate agent-trace lane is in: @afourniernv yes, gladly. Send the hardening PRs against The two rebases had to adapt to these main changes:
One gap to flag for review: the backend-neutral isolation refactor (#3366) removed Verification: |
|
@rhuss expect some PRs in the next day or two! Thx for being open to colab. |
This branch has not been deployed
4 participants
Summary
Relay OpenTelemetry traces from agent processes to the gateway's OTLP collector through the supervisor, rebuilt on the RFC 0012 split boundary (#2942). The agent exports to a reserved, unroutable address; the sandbox seccomp broker stages that connection for the supervisor, which serves the OTLP receiver on the staged stream, enriches spans with sandbox attributes, and forwards them over the existing session protocol. OTel-instrumented agents get traces out of a network-isolated sandbox with no egress policy entry and no change to the isolation contract or boundary protocol.
Related Issue
Closes #2641
Changes
Gateway (
crates/openshell-server,proto/openshell.proto)OtelExportDatapayload onSupervisorMessage;capabilitiesonSupervisorHelloandSessionAccepted. New repeated fields and a new oneof case, so older peers on either side keep working.confirm_capabilitiesgrantsotel_exportonly when[openshell.gateway.otlp]produced a relay exporter.handle_otel_exportforwards trace bytes through a dedicatedOtelRelayExporterso supervisor-set resource attributes survive, and re-emits bounded, valid OCSF events on theocsf_relaytarget.CreateSandboxsetsOTEL_EXPORTER_OTLP_ENDPOINT=http://192.0.0.8:4318andOTEL_EXPORTER_OTLP_PROTOCOL=http/protobufin the sandbox environment when the relay exporter exists and the caller did not set an endpoint. The values persist in the stored spec, so restarts inherit them andsandbox getshows them.architecture/gateway.mdupdated.Supervisor relay (
crates/openshell-supervisor-process/src/otlp)OtlpConnectionServer::serve, which runs one hyper connection on a stream the caller supplies;try_reservelets the proxy refuse an open before the sandbox commits the socket.Proxy hook (
crates/openshell-supervisor-network)ReservedStreamHandlerandReservedDestination. A staged open to the reserved address is served inside the supervisor, decided ahead of host mapping, OPA, SSRF checks, and destination validation. An exhausted handler answersResourceExhaustedbeforeRelayReady. A connect to the relay address with no handler installed is denied rather than falling through.Supervisor wiring (
crates/openshell-supervisor)run_networking, its server registered as the reserved destination, its lifecycle handed to the session throughSessionRuntimeContext.otel_exportonly with a running relay, gates forwarding on gateway confirmation, and forwards withtry_sendso telemetry never stalls control traffic.BoundaryAccess::drain_telemetryruns as a completion phase before the main-process exit report, bounded at 3 s.DriverFenceEvidence::driver_name(), uid from the resolved workload identity.Reserved address
192.0.0.8:4318(RFC 7600 dummy address). Non-loopback (loopback connects complete locally without mediation), outside198.18.0.0/15(policy DNS synthetic pool), never routed. A unit test pins these properties.Docs
architecture/sandbox.md(Telemetry Relay section),docs/kubernetes/sandbox-runtime.mdx,docs/reference/gateway-config.mdx,AGENTS.mdcrate row.Known gaps, deliberately left for follow-ups
OcsfRelayLayer/RateLimitedOcsfSinkare defined but not installed in the supervisor's tracing subscriber. The gateway side accepts OCSF events; no supervisor sends them yet.openshell.sandbox.imageis empty on Kubernetes, Docker, and VM because only the Podman driver setsOPENSHELL_CONTAINER_IMAGE. Pre-existing; the OCSF event context shares it.Testing
mise run pre-commitpassesmise run cion macOS: all tasks green except fiveopenshell-driver-vmrootfs identity tests that requiredebugfs(crate untouched here, tool absent locally).mise run go:ciafter regenerating bindings.mise run e2e:kubernetes:isolationandmise run e2e:dockerstill to run; the development machine has no k3d and no Docker daemon.Contract note:
backend_conformance.rsand the boundary protocol are untouched; the relay rides the existing TCP mediation path.Checklist