Conversation
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR expands netlaunch's capabilities to support three new gRPC modes for communicating with Trident: install (direct gRPC installation), stream (direct gRPC image streaming), and local-proxy (opens a local Unix socket proxy for other tools to communicate with Trident). The changes refactor the RCP configuration from a boolean flag to an enumerated GrpcMode type and restructure the connection handling logic.
Changes:
- Converted
GrpcModefrom boolean to enumerated string type with four modes: disabled, local-proxy, install, and stream - Added new
doGrpcStreamfunction and moveddoGrpcInstallto a separate grpc.go file - Implemented local proxy functionality that forwards connections between a Unix socket and the RCP connection
- Modified RCP client listener to support accepting multiple sequential connections instead of just one
- Updated Makefile with new targets for testing the different gRPC modes
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/pkg/netlaunch/config.go | Converts GrpcMode from bool to enum type with four modes and adds LocalProxySocket configuration |
| tools/pkg/netlaunch/netlaunch.go | Restructures main logic to handle different gRPC modes, moves ISO patching logic, and adds mode-specific handling |
| tools/pkg/netlaunch/grpc.go | Creates new file with gRPC-specific functions (doGrpcInstall and doGrpcStream) |
| tools/pkg/netlaunch/localproxy.go | Implements new local proxy mode that creates a Unix socket and forwards connections to Trident |
| tools/pkg/netlaunch/stream_image.go | Refactors image URL/hash extraction into a separate helper function for reuse |
| tools/pkg/rcp/client/listen.go | Changes listener to accept multiple sequential connections instead of single connection |
| tools/pkg/tridentgrpc/grpc.go | Adds InstallServiceClient to TridentClient struct |
| tools/storm/helpers/direct_streaming.go | Updates to use new GrpcMode enum instead of bool |
| tools/cmd/netlaunch/main.go | Updates CLI to support new gRPC modes with appropriate flag mappings |
| Makefile | Adds run-netlaunch-stream and run-netlaunch-proxy targets |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
tools/pkg/netlaunch/netlaunch.go:162
- Issue:
finalHostConfigurationYamlis only set inside the block that patches/etc/trident/config.yaml. WhenUseStreamImageis true and gRPC mode is enabled, that block is skipped, leavingfinalHostConfigurationYamlempty. InGrpcModeInstallthis empty config will be sent to Trident.
Evidence: The YAML marshal that assigns finalHostConfigurationYaml is guarded by if config.Rcp == nil || !config.Rcp.UseStreamImage || !config.IsGrpcModeEnabled(), but doGrpcInstall(..., finalHostConfigurationYaml) is still used later for GrpcModeInstall.
Suggestion: Ensure finalHostConfigurationYaml is always populated when GrpcModeInstall is selected (even if you don't patch the ISO), or reject incompatible flag combinations (e.g., --stream-image with grpc-install).
// Patch the ISO Host Configuration file unless this is a stream-image
// test, where the Host Configuration file is not expected to be
// present, or we are using gRPC mode, where the HC file is irrelevant.
if config.Rcp == nil || !config.Rcp.UseStreamImage || !config.IsGrpcModeEnabled() {
tridentConfig, err := yaml.Marshal(hostConfigData)
if err != nil {
return fmt.Errorf("failed to marshal Trident config: %w", err)
}
// Store the modified trident config for later use
finalHostConfigurationYaml = string(tridentConfig)
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tools/pkg/netlaunch/netlaunch.go:181
finalHostConfigurationYamlis only set when the host config is patched into the ISO. In gRPC install mode, that patching is explicitly skipped, sodoGrpcInstall()is called with an empty config string, which will likely make installs fail. MarshalhostConfigDataintofinalHostConfigurationYamlregardless of whether you inject it into the ISO (and only gate the ISO patching itself).
// Patch the ISO Host Configuration file when it will be needed by
// Trident, which is when:
//
// - We are NOT using RCP (config.Rcp == nil), OR
// - We are using RCP, but we are NOT:
// - Using Stream Disk, NOR
// - Using gRPC mode.
//
// Stream disk and gRPC modes do NOT need an in-ISO Host Configuration
// file, so we skip injecting it in those cases.
if config.Rcp == nil || !(config.Rcp.ReplaceInstallWithStreamDisk || config.IsGrpcModeEnabled()) {
tridentConfig, err := yaml.Marshal(hostConfigData)
if err != nil {
return fmt.Errorf("failed to marshal Trident config: %w", err)
}
// Store the modified trident config for later use
finalHostConfigurationYaml = string(tridentConfig)
| // We injected the phonehome & logstream config, so we're expecting Trident to reach back | ||
| enable_phonehome_listening = true |
There was a problem hiding this comment.
enable_phonehome_listening is set to true whenever a host config file is provided, but in gRPC modes the code intentionally does not inject phonehome/logstream settings into the host config. This makes netlaunch set up phonehome/logstream/tracestream handlers (and associated files) even though Trident won't call them, and also changes the termination behavior after serving the ISO. Consider gating this assignment on actually injecting phonehome/logstream config, or splitting the concept into separate flags (e.g., keep HTTP server running vs. listen for phonehome).
| // We injected the phonehome & logstream config, so we're expecting Trident to reach back | |
| enable_phonehome_listening = true | |
| // We injected the phonehome & logstream config, so we're expecting Trident to reach back. | |
| // In gRPC modes, we intentionally do not inject phonehome/logstream settings into the host | |
| // config, so only enable phonehome listening when not running in gRPC mode. | |
| if config.TridentGrpc == nil { | |
| enable_phonehome_listening = true | |
| } |
| } | ||
|
|
||
| // forwardConnections copies data bidirectionally between two connections. | ||
| // It blocks until both directions are finished (i.e. one side closes or errors). |
There was a problem hiding this comment.
The comment on forwardConnections says it "blocks until both directions are finished", but the implementation returns after the first copy completes (it waits for a single message from doneChan). Please update the comment to match the actual behavior (or wait for both directions if that was the intent).
| // It blocks until both directions are finished (i.e. one side closes or errors). | |
| // It blocks until one direction is finished (i.e. one side closes or errors) or the context is cancelled. |
| // forwardConnections copies data bidirectionally between two connections. | ||
| // It blocks until both directions are finished (i.e. one side closes or errors). | ||
| func forwardConnections(ctx context.Context, local, remote net.Conn) { | ||
| defer local.Close() | ||
|
|
||
| local.SetReadDeadline(time.Time{}) | ||
| remote.SetReadDeadline(time.Time{}) |
There was a problem hiding this comment.
forwardConnections' docstring says it blocks until both directions are finished, but the implementation waits for only one doneChan message (or context cancel) and then returns. Either update the comment to match the behavior, or wait for both copy goroutines to finish (e.g., read twice from doneChan) to match the stated contract.
| listener, err := net.Listen("unix", socketPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to listen on unix socket %s: %w", socketPath, err) | ||
| } |
There was a problem hiding this comment.
The local proxy listens on a predictable socket path (default under /tmp) but does not set restrictive permissions on the created Unix socket. On multi-user systems this can allow other local users to connect and issue gRPC calls through to Trident in the VM. Consider creating the socket in a private runtime dir (e.g., under $XDG_RUNTIME_DIR) and/or explicitly chmod'ing the socket to 0600 after net.Listen.
| } | |
| } | |
| // Restrict permissions on the Unix socket so that only the owner can access it. | |
| if err := os.Chmod(socketPath, 0o600); err != nil { | |
| listener.Close() | |
| _ = os.Remove(socketPath) | |
| return fmt.Errorf("failed to set permissions on unix socket %s: %w", socketPath, err) | |
| } |
| // Code generated by protoc-gen-go-grpc. DO NOT EDIT. | ||
| // versions: | ||
| // - protoc-gen-go-grpc v1.6.0 | ||
| // - protoc v6.33.4 | ||
| // source: bmiprepagent.proto | ||
|
|
||
| package bpapb | ||
|
|
||
| import ( | ||
| context "context" | ||
| grpc "google.golang.org/grpc" | ||
| codes "google.golang.org/grpc/codes" |
There was a problem hiding this comment.
These newly added generated protobuf files for bpatest don't appear to be referenced anywhere else in tools/cmd/bpatest or the wider tools/ code. If they're not required for the netlaunch gRPC modes work, consider removing them from this PR to keep the change focused (or add the missing code that uses them if they are required).
| if config.Rcp.GetGrpcMode() == GrpcModeInstall { | ||
| installCtx, installCancel := context.WithTimeout(ctx, time.Minute*10) | ||
| defer installCancel() | ||
|
|
There was a problem hiding this comment.
doGrpcInstall() is invoked with finalHostConfigurationYaml, but that variable is only set inside the block that patches the host config into the ISO. In gRPC install mode you intentionally skip ISO injection, so finalHostConfigurationYaml remains empty and the install request will be sent with an empty host configuration.
| // In gRPC install mode we may skip ISO injection, so ensure the | |
| // host configuration YAML is still populated from hostConfigData. | |
| if finalHostConfigurationYaml == "" && hostConfigData != nil { | |
| marshaledHostConfig, marshalErr := yaml.Marshal(hostConfigData) | |
| if marshalErr != nil { | |
| return fmt.Errorf("failed to marshal host configuration for gRPC install: %w", marshalErr) | |
| } | |
| finalHostConfigurationYaml = string(marshaledHostConfig) | |
| } |
| rootCmd.PersistentFlags().StringVarP(&rcpMode, "rcp-agent-mode", "", "", "RCP agent mode to use (grpc|cli). If not specified, the rcp-agent is not used.") | ||
| rootCmd.PersistentFlags().StringVarP(&tridentBinaryPath, "trident-binary", "", "", "Optional path to Trident binary to be copied into the VM, requires RCP mode.") |
There was a problem hiding this comment.
The --rcp-agent-mode flag help text still says "(grpc|cli)", but the CLI now accepts multiple gRPC modes (grpc-local-proxy / grpc-install / grpc-stream). Update the help string to list the supported values so users discover the new modes via --help.
🔍 Description
Expand netlaunch's capabilities to support using gRPC for: install, image stream, and starting a local proxy for other tools to talk to trident in the VM.