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..825a6685e901 --- /dev/null +++ b/deploy/operator/internal/dynamo/multinode_shell_argv_test.go @@ -0,0 +1,98 @@ +/* + * 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" +) + +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, + // characters the shell would otherwise expand, and a kubelet $(VAR) reference. + 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", + "--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 + 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") + 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, 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 { + 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