From 0158f0ef8069d1742ee976b50cc4e68056c7585b Mon Sep 17 00:00:00 2001 From: lonexreb Date: Sun, 25 Oct 2026 01:22:27 -0700 Subject: [PATCH 1/2] fix(operator): preserve argv in multinode vLLM and SGLang shell wrappers The Ray leader wrapper in injectRayDistributedLaunchFlags and the shell wrapped Python worker path in injectFlagsIntoContainerCommand quoted tokens with shellQuoteForBashC. Its '"'"' escaping is built for the nested bash -c command the TensorRT-LLM mpirun launcher assembles. Through a single sh -c layer it leaves an unterminated string when a value holds a single quote, and empty values vanish. Switch those call sites to shellQuotePOSIX, which renders one argv token that survives sh -c unchanged and is already used by the elastic-EP Ray launch. The TensorRT-LLM path keeps shellQuoteForBashC; its doc comment now says what it is for. TestMultinodeShellWrappersPreserveArgv runs each rendered wrapper through /bin/sh with a stub python3 and checks the engine sees the original argv. All three subtests fail before this change and pass after. Picks up the fix from #15167, which was closed before review. Closes #16973 Signed-off-by: lonexreb --- .../operator/internal/dynamo/backend_vllm.go | 5 +- .../internal/dynamo/backend_vllm_test.go | 2 +- .../dynamo/multinode_shell_argv_test.go | 97 +++++++++++++++++++ deploy/operator/internal/dynamo/utils.go | 17 ++-- 4 files changed, 111 insertions(+), 10 deletions(-) create mode 100644 deploy/operator/internal/dynamo/multinode_shell_argv_test.go diff --git a/deploy/operator/internal/dynamo/backend_vllm.go b/deploy/operator/internal/dynamo/backend_vllm.go index f2f29e5b5441..9b026c7663d7 100644 --- a/deploy/operator/internal/dynamo/backend_vllm.go +++ b/deploy/operator/internal/dynamo/backend_vllm.go @@ -476,14 +476,15 @@ func injectMpDistributedLaunchFlags( func injectRayDistributedLaunchFlags(container *corev1.Container, role Role, serviceName string, multinodeDeployer MultinodeDeployer) { switch role { case RoleLeader: + // The leader runs through one sh -c layer, so quote for exactly that. quotedCmd := make([]string, len(container.Command)) for i, tok := range container.Command { - quotedCmd[i] = shellQuoteForBashC(tok) + quotedCmd[i] = shellQuotePOSIX(tok) } fullCommand := strings.Join(quotedCmd, " ") quotedArgs := make([]string, len(container.Args)) for i, arg := range container.Args { - quotedArgs[i] = shellQuoteForBashC(arg) + quotedArgs[i] = shellQuotePOSIX(arg) } originalArgs := strings.Join(quotedArgs, " ") vllmMultinodeFlags := fmt.Sprintf("%s ray", distributedExecutorFlag) diff --git a/deploy/operator/internal/dynamo/backend_vllm_test.go b/deploy/operator/internal/dynamo/backend_vllm_test.go index e74342f89e66..c4da51c6b57d 100644 --- a/deploy/operator/internal/dynamo/backend_vllm_test.go +++ b/deploy/operator/internal/dynamo/backend_vllm_test.go @@ -165,7 +165,7 @@ func TestVLLMBackend_UpdateContainer(t *testing.T) { }, containerGPUs: 4, expectedArgs: []string{fmt.Sprintf( - `ray start --head --port=%s && python3 -m dynamo.vllm --model test %s 8 --kv-transfer-config "{\"kv_connector\": \"NixlConnector\", \"kv_role\": \"kv_producer\"}" --distributed-executor-backend ray`, + `ray start --head --port=%s && python3 -m dynamo.vllm --model test %s 8 --kv-transfer-config '{"kv_connector": "NixlConnector", "kv_role": "kv_producer"}' --distributed-executor-backend ray`, VLLMPort, tensorParallelSizeFlag, )}, expectProbesRemoved: true, diff --git a/deploy/operator/internal/dynamo/multinode_shell_argv_test.go b/deploy/operator/internal/dynamo/multinode_shell_argv_test.go new file mode 100644 index 000000000000..7b0ca543bac4 --- /dev/null +++ b/deploy/operator/internal/dynamo/multinode_shell_argv_test.go @@ -0,0 +1,97 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package dynamo + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/ai-dynamo/dynamo/deploy/operator/api/v1alpha1" + commonconsts "github.com/ai-dynamo/dynamo/deploy/operator/internal/consts" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +// TestMultinodeShellWrappersPreserveArgv runs each single-layer `sh -c` wrapper +// the operator renders for multinode vLLM and SGLang through a real shell and +// checks that the user's argv reaches the engine unchanged. +func TestMultinodeShellWrappersPreserveArgv(t *testing.T) { + t.Log("Stub python3 to print its argv NUL-delimited and ray to succeed, so the rendered wrapper can run locally") + binDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(binDir, "python3"), []byte("#!/bin/sh\nprintf '%s\\000' \"$@\"\n"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(binDir, "ray"), []byte("#!/bin/sh\nexit 0\n"), 0o755)) + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GROVE_PCLQ_POD_INDEX", "0") + + // Every value here is legal in a container args list and must survive the + // shell verbatim: a single quote, an empty string, whitespace, JSON, and + // characters the shell would otherwise expand. + userArgs := []string{ + "--model", "models/it's a model", tensorParallelSizeFlag, "8", + "--served-model-name", "", + "--chat-template", `{{ 'hello' }}`, + "--kv-transfer-config", `{"kv_connector": "NixlConnector", "kv_role": "$ROLE"}`, + "--literal", `glob*?[x]`, + "--whitespace", "tab\tand\nnewline", + } + + tests := []struct { + name string + backend Backend + module string + role Role + executor string + }{ + {name: "vllm ray leader", backend: &VLLMBackend{}, module: "dynamo.vllm", role: RoleLeader, executor: "ray"}, + {name: "vllm mp grove worker", backend: &VLLMBackend{}, module: "dynamo.vllm", role: RoleWorker, executor: "mp"}, + {name: "sglang grove worker", backend: &SGLangBackend{}, module: "dynamo.sglang", role: RoleWorker}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Log("Render the multinode wrapper for an argv-form python command") + container := &corev1.Container{ + Command: []string{"python3", "-m", tt.module}, + Args: append([]string(nil), userArgs...), + } + spec := &v1alpha1.DynamoComponentDeploymentSharedSpec{} + if tt.executor != "" { + spec.Annotations = map[string]string{commonconsts.KubeAnnotationVLLMDistributedExecutorBackend: tt.executor} + } + require.NoError(t, tt.backend.UpdateContainer(container, 2, tt.role, betaComponent(t, spec), "worker", &GroveMultinodeDeployer{}, staticContainerGPUCount(4))) + require.Equal(t, "-c", container.Command[len(container.Command)-1], "expected a sh -c wrapper, got %v", container.Command) + + t.Log("Stand in for kubelet $(VAR) expansion, then execute the wrapper through the shell it targets") + kubelet := strings.NewReplacer("$(GROVE_PCSG_NAME)", "pcs", "$(GROVE_PCSG_INDEX)", "0", "$(GROVE_HEADLESS_SERVICE)", "svc") + script := kubelet.Replace(container.Args[0]) + out, err := exec.Command(container.Command[0], append(container.Command[1:], script)...).CombinedOutput() + require.NoError(t, err, "shell wrapper failed:\n%s\n%s", script, out) + got := strings.Split(strings.TrimSuffix(string(out), "\x00"), "\x00") + + t.Log("The user's argv must lead the engine command unchanged, followed by the injected multinode flags") + want := append([]string{"-m", tt.module}, userArgs...) + require.GreaterOrEqual(t, len(got), len(want), "engine argv: %q", got) + require.Equal(t, want, got[:len(want)]) + if tt.role == RoleWorker { + require.Contains(t, got[len(want):], "1", "injected node rank should evaluate to 1, got %q", got[len(want):]) + } + }) + } +} diff --git a/deploy/operator/internal/dynamo/utils.go b/deploy/operator/internal/dynamo/utils.go index 0bf6d594218f..e60a5b8c0343 100644 --- a/deploy/operator/internal/dynamo/utils.go +++ b/deploy/operator/internal/dynamo/utils.go @@ -38,9 +38,12 @@ import ( * - Insert flags after the Python command but before any shell operators (|, &, ;) */ -// shellQuoteForBashC quotes a string so it survives shell interpretation inside sh -c. -// Simple args (flags, paths) pass through unchanged; args containing special characters -// (JSON, env vars, spaces, quotes) are wrapped in double quotes with inner escaping. +// shellQuoteForBashC quotes a string for the nested `bash -c '...'` command the +// TensorRT-LLM mpirun launcher builds. Simple args (flags, paths) pass through +// unchanged; args containing special characters (JSON, env vars, spaces, quotes) +// are wrapped in double quotes with inner escaping, and single quotes are closed +// and reopened so they survive the outer single-quoted wrapper. It is not +// argv-preserving through a single sh -c layer: use shellQuotePOSIX there. func shellQuoteForBashC(s string) string { if strings.ContainsAny(s, " \t\n'\"\\{}[]$`!") { escaped := s @@ -123,16 +126,16 @@ func injectFlagsIntoContainerCommand(container *corev1.Container, flags string, // Direct python command case if needsShell { // Transform to shell wrapper for env var interpretation. - // Quote each token individually so paths with spaces or special - // characters survive shell interpretation. + // Quote each token for a single sh -c layer so every argv value, + // including empty strings and embedded quotes, reaches the engine intact. quotedCmd := make([]string, len(container.Command)) for i, tok := range container.Command { - quotedCmd[i] = shellQuoteForBashC(tok) + quotedCmd[i] = shellQuotePOSIX(tok) } fullCommand := strings.Join(quotedCmd, " ") quotedArgs := make([]string, len(container.Args)) for i, arg := range container.Args { - quotedArgs[i] = shellQuoteForBashC(arg) + quotedArgs[i] = shellQuotePOSIX(arg) } originalArgs := strings.Join(quotedArgs, " ") var shellCommand string From e3a70656c2499166fb73e7e0bd7a18dbd86b05de Mon Sep 17 00:00:00 2001 From: lonexreb Date: Wed, 28 Oct 2026 01:01:40 -0700 Subject: [PATCH 2/2] test(operator): cover kubelet $(VAR) args in the multinode argv test The old shellQuoteForBashC escaped $ to \$ inside double quotes. Kubelet still expands the $(VAR) reference, so the shell received a stray backslash in front of the value. Add a $(MODEL_CACHE) argument to the table so the test pins the corrected behavior, and drop a doc comment that only restated the test name. Refs #16973 Signed-off-by: lonexreb --- .../dynamo/multinode_shell_argv_test.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/deploy/operator/internal/dynamo/multinode_shell_argv_test.go b/deploy/operator/internal/dynamo/multinode_shell_argv_test.go index 7b0ca543bac4..825a6685e901 100644 --- a/deploy/operator/internal/dynamo/multinode_shell_argv_test.go +++ b/deploy/operator/internal/dynamo/multinode_shell_argv_test.go @@ -30,9 +30,6 @@ import ( corev1 "k8s.io/api/core/v1" ) -// TestMultinodeShellWrappersPreserveArgv runs each single-layer `sh -c` wrapper -// the operator renders for multinode vLLM and SGLang through a real shell and -// checks that the user's argv reaches the engine unchanged. func TestMultinodeShellWrappersPreserveArgv(t *testing.T) { t.Log("Stub python3 to print its argv NUL-delimited and ray to succeed, so the rendered wrapper can run locally") binDir := t.TempDir() @@ -42,8 +39,8 @@ func TestMultinodeShellWrappersPreserveArgv(t *testing.T) { t.Setenv("GROVE_PCLQ_POD_INDEX", "0") // Every value here is legal in a container args list and must survive the - // shell verbatim: a single quote, an empty string, whitespace, JSON, and - // characters the shell would otherwise expand. + // shell verbatim: a single quote, an empty string, whitespace, JSON, + // characters the shell would otherwise expand, and a kubelet $(VAR) reference. userArgs := []string{ "--model", "models/it's a model", tensorParallelSizeFlag, "8", "--served-model-name", "", @@ -51,7 +48,9 @@ func TestMultinodeShellWrappersPreserveArgv(t *testing.T) { "--kv-transfer-config", `{"kv_connector": "NixlConnector", "kv_role": "$ROLE"}`, "--literal", `glob*?[x]`, "--whitespace", "tab\tand\nnewline", + "--download-dir", "$(MODEL_CACHE)", } + kubelet := strings.NewReplacer("$(MODEL_CACHE)", "/cache/models", "$(GROVE_PCSG_NAME)", "pcs", "$(GROVE_PCSG_INDEX)", "0", "$(GROVE_HEADLESS_SERVICE)", "svc") tests := []struct { name string @@ -79,14 +78,16 @@ func TestMultinodeShellWrappersPreserveArgv(t *testing.T) { require.Equal(t, "-c", container.Command[len(container.Command)-1], "expected a sh -c wrapper, got %v", container.Command) t.Log("Stand in for kubelet $(VAR) expansion, then execute the wrapper through the shell it targets") - kubelet := strings.NewReplacer("$(GROVE_PCSG_NAME)", "pcs", "$(GROVE_PCSG_INDEX)", "0", "$(GROVE_HEADLESS_SERVICE)", "svc") script := kubelet.Replace(container.Args[0]) out, err := exec.Command(container.Command[0], append(container.Command[1:], script)...).CombinedOutput() require.NoError(t, err, "shell wrapper failed:\n%s\n%s", script, out) got := strings.Split(strings.TrimSuffix(string(out), "\x00"), "\x00") - t.Log("The user's argv must lead the engine command unchanged, followed by the injected multinode flags") - want := append([]string{"-m", tt.module}, userArgs...) + t.Log("The user's argv, after kubelet expansion, must lead the engine command unchanged, followed by the injected multinode flags") + want := []string{"-m", tt.module} + for _, arg := range userArgs { + want = append(want, kubelet.Replace(arg)) + } require.GreaterOrEqual(t, len(got), len(want), "engine argv: %q", got) require.Equal(t, want, got[:len(want)]) if tt.role == RoleWorker {