From 169c3a46de43a61c04e866d29ad4b384b876e060 Mon Sep 17 00:00:00 2001 From: Richard Liaw Date: Mon, 7 Sep 2020 09:56:41 -0700 Subject: [PATCH] [k8s] Broken Command Interactivity (#10297) Co-authored-by: Alex Wu --- python/ray/autoscaler/command_runner.py | 18 +++++------ python/ray/tests/test_command_runner.py | 43 +++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/python/ray/autoscaler/command_runner.py b/python/ray/autoscaler/command_runner.py index 6adf114c9..3df2ae4fb 100644 --- a/python/ray/autoscaler/command_runner.py +++ b/python/ray/autoscaler/command_runner.py @@ -93,9 +93,10 @@ def _with_environment_variables(cmd: str, def _with_interactive(cmd): - force_interactive = ("true && source ~/.bashrc && " - "export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && ") - return ["bash", "--login", "-c", "-i", quote(force_interactive + cmd)] + force_interactive = ( + f"true && source ~/.bashrc && " + f"export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && ({cmd})") + return ["bash", "--login", "-c", "-i", quote(force_interactive)] class CommandRunnerInterface: @@ -170,7 +171,7 @@ class KubernetesCommandRunner(CommandRunnerInterface): self.log_prefix = log_prefix self.process_runner = process_runner - self.node_id = node_id + self.node_id = str(node_id) self.namespace = namespace self.kubectl = ["kubectl", "-n", self.namespace] @@ -215,18 +216,17 @@ class KubernetesCommandRunner(CommandRunnerInterface): self.node_id, "--", ] - cmd = _with_interactive(cmd) if environment_variables: cmd = _with_environment_variables(cmd, environment_variables) - final_cmd += _with_interactive(cmd) + cmd = _with_interactive(cmd) + final_cmd += cmd logger.info(self.log_prefix + "Running {}".format(final_cmd)) try: if with_output: return self.process_runner.check_output( - " ".join(final_cmd), shell=True) + final_cmd, shell=True) else: - self.process_runner.check_call( - " ".join(final_cmd), shell=True) + self.process_runner.check_call(final_cmd, shell=True) except subprocess.CalledProcessError: if exit_on_fail: quoted_cmd = " ".join(final_cmd[:-1] + diff --git a/python/ray/tests/test_command_runner.py b/python/ray/tests/test_command_runner.py index 12f34eb7b..fd7b32741 100644 --- a/python/ray/tests/test_command_runner.py +++ b/python/ray/tests/test_command_runner.py @@ -1,7 +1,7 @@ import pytest from ray.tests.test_autoscaler import MockProvider, MockProcessRunner from ray.autoscaler.command_runner import SSHCommandRunner, \ - _with_environment_variables, DockerCommandRunner + _with_environment_variables, DockerCommandRunner, KubernetesCommandRunner from ray.autoscaler.docker import DOCKER_MOUNT_PREFIX from getpass import getuser import hashlib @@ -85,7 +85,44 @@ def test_ssh_command_runner(): "--login", "-c", "-i", - """'true && source ~/.bashrc && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && export var1='"'"'"quote between this \\" and this"'"'"';export var2='"'"'"123"'"'"';echo helloo'""" # noqa: E501 + """'true && source ~/.bashrc && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && (export var1='"'"'"quote between this \\" and this"'"'"';export var2='"'"'"123"'"'"';echo helloo)'""" # noqa: E501 + ] + + # Much easier to debug this loop than the function call. + for x, y in zip(process_runner.calls[0], expected): + assert x == y + process_runner.assert_has_call("1.2.3.4", exact=expected) + + +def test_kubernetes_command_runner(): + process_runner = MockProcessRunner() + provider = MockProvider() + provider.create_node({}, {}, 1) + args = { + "log_prefix": "prefix", + "namespace": "namespace", + "node_id": 0, + "auth_config": auth_config, + "process_runner": process_runner, + } + cmd_runner = KubernetesCommandRunner(**args) + + env_vars = {"var1": "quote between this \" and this", "var2": "123"} + cmd_runner.run("echo helloo", environment_variables=env_vars) + + expected = [ + "kubectl", + "-n", + "namespace", + "exec", + "-it", + "0", + "--", + "bash", + "--login", + "-c", + "-i", + """\'true && source ~/.bashrc && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && (export var1=\'"\'"\'"quote between this \\" and this"\'"\'"\';export var2=\'"\'"\'"123"\'"\'"\';echo helloo)\'""" # noqa: E501 ] # Much easier to debug this loop than the function call. @@ -123,7 +160,7 @@ def test_docker_command_runner(): # This string is insane because there are an absurd number of embedded # quotes. While this is a ridiculous string, the escape behavior is # important and somewhat difficult to get right for environment variables. - cmd = """'true && source ~/.bashrc && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && docker exec -it container /bin/bash -c '"'"'bash --login -c -i '"'"'"'"'"'"'"'"'true && source ~/.bashrc && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && export var1='"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"quote between this \\" and this"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"';export var2='"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"123"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"';echo hello'"'"'"'"'"'"'"'"''"'"' '""" # noqa: E501 + cmd = """'true && source ~/.bashrc && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && (docker exec -it container /bin/bash -c '"'"'bash --login -c -i '"'"'"'"'"'"'"'"'true && source ~/.bashrc && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore && (export var1='"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"quote between this \\" and this"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"';export var2='"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"123"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"';echo hello)'"'"'"'"'"'"'"'"''"'"' )'""" # noqa: E501 expected = [ "ssh", "-tt", "-i", "8265.pem", "-o", "StrictHostKeyChecking=no", "-o",