From 4ddb22b4fa481c931ec21678eeadc07fabde92c4 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Tue, 6 Oct 2026 12:47:11 +0200 Subject: [PATCH] Pass stress options through to the tool "osism openstack stress" re-declared every option of openstack-simple-stress with argparse and built the tool's command line itself. The two drifted apart: --profile, --burnin, --burnin-duration and --no-network were never available through osism, and newer options such as --yes would not have been either. It also always passed every option, including --interval 10. kolla's haproxy closes idle keep-alive connections after 10 s, and a 10 s poll reuses pooled connections right at that moment: measured against nova on an OSISM 10.2.0 cluster, 3-5 of 40 requests failed with RemoteDisconnected at 9.990-9.998 s idle, none elsewhere. The tool's default is now 2 s, but the wrapper would keep forcing 10. The wrapper now only handles --cloud (credential setup from secrets.yml or secure.yml, as before) and passes every other argument to the tool unchanged, so the tool's own defaults and options apply. Exit codes are passed through as before. Options that the osism CLI consumes itself (--debug, -h/--help, -v, -q, --version, --log-file) must follow a "--", e.g. "osism openstack stress -- --help" shows the tool's options. --cloud after "--" is rejected: the tool would run against a cloud whose credentials were never set up. Verified in the osismclient container (Python 3.13) of an OSISM 10.2.0 test cluster: "-- --help" shows the tool's help, "-- --cloud admin" exits 2, and a run through the wrapper succeeds. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Roger Luethi --- osism/commands/stress.py | 223 +++++------------------------ tests/unit/commands/test_stress.py | 121 ++++++++-------- 2 files changed, 94 insertions(+), 250 deletions(-) diff --git a/osism/commands/stress.py b/osism/commands/stress.py index 372d5e151..7370b012a 100644 --- a/osism/commands/stress.py +++ b/osism/commands/stress.py @@ -1,167 +1,49 @@ # SPDX-License-Identifier: Apache-2.0 +import argparse import subprocess from cliff.command import Command from loguru import logger +STRESS_TOOL = "/openstack-simple-stress/openstack_simple_stress/main.py" -class OpenStackStress(Command): - """Run OpenStack stress testing tool""" - def get_parser(self, prog_name): - parser = super(OpenStackStress, self).get_parser(prog_name) +class _PassThroughParser(argparse.ArgumentParser): + """Keep every argument this parser does not know for the stress tool.""" - # Boolean flags - parser.add_argument( - "--no-cleanup", - action="store_true", - help="Do not clean up resources after test", - ) - parser.add_argument( - "--debug", - action="store_true", - help="Enable debug mode", - ) - parser.add_argument( - "--no-delete", - action="store_true", - help="Do not delete resources", - ) - parser.add_argument( - "--no-volume", - action="store_true", - help="Do not create volumes", - ) - parser.add_argument( - "--no-boot-volume", - action="store_true", - help="Do not use boot volumes", - ) - parser.add_argument( - "--no-wait", - action="store_true", - help="Do not wait for resources", - ) - parser.add_argument( - "--clean", - action="store_true", - help="Clean up leftover resources matching the prefix", - ) + def parse_args(self, args=None, namespace=None): # type: ignore[override] + parsed, extra = self.parse_known_args(args, namespace) + if "--" in extra: + extra.remove("--") + # Credentials are set up for parsed.cloud; a second --cloud would make + # the tool run against a cloud whose credentials were never prepared. + if any(a == "--cloud" or a.startswith("--cloud=") for a in extra): + self.error("--cloud must come before '--'") + parsed.tool_args = extra + return parsed - # Integer parameters with defaults - parser.add_argument( - "--interval", - type=int, - default=10, - help="Interval in seconds (default: %(default)s)", - ) - parser.add_argument( - "--number", - type=int, - default=1, - help="Number of instances (default: %(default)s)", - ) - parser.add_argument( - "--parallel", - type=int, - default=1, - help="Parallel operations (default: %(default)s)", - ) - parser.add_argument( - "--timeout", - type=int, - default=600, - help="Timeout in seconds (default: %(default)s)", - ) - parser.add_argument( - "--volume-number", - type=int, - default=1, - help="Number of volumes per instance (default: %(default)s)", - ) - parser.add_argument( - "--volume-size", - type=int, - default=1, - help="Volume size in GB (default: %(default)s)", - ) - parser.add_argument( - "--boot-volume-size", - type=int, - default=20, - help="Boot volume size in GB (default: %(default)s)", - ) - # String parameters with defaults +class OpenStackStress(Command): + """Run the OpenStack stress testing tool (openstack-simple-stress). + + All options except --cloud are passed to the tool unchanged. Options that + the osism CLI itself uses (--debug, -h/--help, -v, -q, --version, + --log-file) must follow a "--": e.g. "osism openstack stress -- --help" + shows the tool's own options. + """ + + def get_parser(self, prog_name): + parser = _PassThroughParser( + prog=prog_name, + description=self.get_description(), + add_help=False, + ) parser.add_argument( "--cloud", - type=str, default="simple-stress", help="Cloud name in clouds.yaml (default: %(default)s)", ) - parser.add_argument( - "--flavor", - type=str, - default="SCS-1V-2", - help="Flavor name (default: %(default)s)", - ) - parser.add_argument( - "--image", - type=str, - default="Ubuntu 24.04", - help="Image name (default: %(default)s)", - ) - parser.add_argument( - "--subnet-cidr", - type=str, - default="10.100.0.0/16", - help="Subnet CIDR (default: %(default)s)", - ) - parser.add_argument( - "--prefix", - type=str, - default="simple-stress", - help="Resource name prefix (default: %(default)s)", - ) - parser.add_argument( - "--compute-zone", - type=str, - default="nova", - help="Compute availability zone (default: %(default)s)", - ) - parser.add_argument( - "--storage-zone", - type=str, - default="nova", - help="Storage availability zone (default: %(default)s)", - ) - parser.add_argument( - "--affinity", - type=str, - default="soft-anti-affinity", - choices=[ - "soft-affinity", - "soft-anti-affinity", - "affinity", - "anti-affinity", - ], - help="Server group policy (default: %(default)s)", - ) - parser.add_argument( - "--volume-type", - type=str, - default="__DEFAULT__", - help="Volume type (default: %(default)s)", - ) - parser.add_argument( - "--mode", - type=str, - default="rolling", - choices=["rolling", "block"], - help="Execution mode (default: %(default)s)", - ) - return parser def take_action(self, parsed_args): @@ -180,49 +62,14 @@ def take_action(self, parsed_args): ) return 1 - # Build the command command = [ "python3", - "/openstack-simple-stress/openstack_simple_stress/main.py", + STRESS_TOOL, + "--cloud", + parsed_args.cloud, + *parsed_args.tool_args, ] - # Add boolean flags - if parsed_args.no_cleanup: - command.append("--no-cleanup") - if parsed_args.debug: - command.append("--debug") - if parsed_args.no_delete: - command.append("--no-delete") - if parsed_args.no_volume: - command.append("--no-volume") - if parsed_args.no_boot_volume: - command.append("--no-boot-volume") - if parsed_args.no_wait: - command.append("--no-wait") - if parsed_args.clean: - command.append("--clean") - - # Add integer parameters - command.extend(["--interval", str(parsed_args.interval)]) - command.extend(["--number", str(parsed_args.number)]) - command.extend(["--parallel", str(parsed_args.parallel)]) - command.extend(["--timeout", str(parsed_args.timeout)]) - command.extend(["--volume-number", str(parsed_args.volume_number)]) - command.extend(["--volume-size", str(parsed_args.volume_size)]) - command.extend(["--boot-volume-size", str(parsed_args.boot_volume_size)]) - - # Add string parameters - command.extend(["--cloud", parsed_args.cloud]) - command.extend(["--flavor", parsed_args.flavor]) - command.extend(["--image", parsed_args.image]) - command.extend(["--subnet-cidr", parsed_args.subnet_cidr]) - command.extend(["--prefix", parsed_args.prefix]) - command.extend(["--compute-zone", parsed_args.compute_zone]) - command.extend(["--storage-zone", parsed_args.storage_zone]) - command.extend(["--affinity", parsed_args.affinity]) - command.extend(["--volume-type", parsed_args.volume_type]) - command.extend(["--mode", parsed_args.mode]) - logger.debug( f"Executing OpenStack stress test with command: {' '.join(command)}" ) @@ -231,9 +78,7 @@ def take_action(self, parsed_args): result = subprocess.run(command, check=False) return result.returncode except FileNotFoundError: - logger.error( - "OpenStack stress tool not found at /openstack-simple-stress/openstack_simple_stress/main.py" - ) + logger.error(f"OpenStack stress tool not found at {STRESS_TOOL}") return 1 except Exception as e: logger.error(f"Error executing OpenStack stress tool: {e}") diff --git a/tests/unit/commands/test_stress.py b/tests/unit/commands/test_stress.py index d97861ce3..497e64881 100644 --- a/tests/unit/commands/test_stress.py +++ b/tests/unit/commands/test_stress.py @@ -8,16 +8,6 @@ STRESS_TOOL = "/openstack-simple-stress/openstack_simple_stress/main.py" -BOOLEAN_FLAGS = [ - "--no-cleanup", - "--debug", - "--no-delete", - "--no-volume", - "--no-boot-volume", - "--no-wait", - "--clean", -] - def _run(args, run_mock=None, setup_success=True): """Drive OpenStackStress.take_action with mocked cloud helpers.""" @@ -33,65 +23,74 @@ def _run(args, run_mock=None, setup_success=True): return_value=(setup, MagicMock(), cleanup), ), patch("osism.commands.stress.subprocess.run", run_mock): result = cmd.take_action(parsed_args) - return result, run_mock, cleanup - - -def _flag_value(command, flag): - return command[command.index(flag) + 1] - - -def test_defaults_build_expected_command(): - result, run_mock, _ = _run([]) - - command = run_mock.call_args[0][0] - assert command[:2] == ["python3", STRESS_TOOL] - for flag in BOOLEAN_FLAGS: - assert flag not in command - - expected = { - "--interval": "10", - "--number": "1", - "--parallel": "1", - "--timeout": "600", - "--volume-number": "1", - "--volume-size": "1", - "--boot-volume-size": "20", - "--cloud": "simple-stress", - "--flavor": "SCS-1V-2", - "--image": "Ubuntu 24.04", - "--subnet-cidr": "10.100.0.0/16", - "--prefix": "simple-stress", - "--compute-zone": "nova", - "--storage-zone": "nova", - "--affinity": "soft-anti-affinity", - "--volume-type": "__DEFAULT__", - "--mode": "rolling", - } - for flag, value in expected.items(): - assert _flag_value(command, flag) == value + return result, run_mock, setup, cleanup + +def test_defaults_pass_only_the_cloud(): + result, run_mock, setup, _ = _run([]) + + assert run_mock.call_args[0][0] == [ + "python3", + STRESS_TOOL, + "--cloud", + "simple-stress", + ] + setup.assert_called_once_with("simple-stress") assert result == 0 -@pytest.mark.parametrize("flag", BOOLEAN_FLAGS) -def test_boolean_flag_appended(flag): - _, run_mock, _ = _run([flag]) - assert flag in run_mock.call_args[0][0] +def test_tool_options_are_forwarded_unchanged(): + args = [ + "--number", + "5", + "--profile", + "acceptance", + "--no-network", + "--clean", + "--yes", + ] + _, run_mock, _, _ = _run(args) + + assert run_mock.call_args[0][0][4:] == args + + +def test_cloud_is_used_for_setup_and_forwarded(): + _, run_mock, setup, _ = _run(["--cloud", "admin", "--number", "2"]) + + setup.assert_called_once_with("admin") + assert run_mock.call_args[0][0][2:] == ["--cloud", "admin", "--number", "2"] + + +def test_separator_is_removed(): + _, run_mock, _, _ = _run(["--number", "3", "--", "--debug", "--help"]) + + assert run_mock.call_args[0][0][4:] == ["--number", "3", "--debug", "--help"] + + +def test_cloud_after_separator_is_rejected(): + cmd = stress.OpenStackStress(MagicMock(), MagicMock()) + with pytest.raises(SystemExit) as exc: + cmd.get_parser("test").parse_args(["--", "--cloud", "admin"]) + assert exc.value.code == 2 + + +def test_cloud_equals_after_separator_is_rejected(): + cmd = stress.OpenStackStress(MagicMock(), MagicMock()) + with pytest.raises(SystemExit) as exc: + cmd.get_parser("test").parse_args(["--number", "2", "--", "--cloud=admin"]) + assert exc.value.code == 2 -def test_custom_values_propagated(): - _, run_mock, _ = _run(["--number", "5", "--flavor", "X", "--volume-size", "10"]) +def test_no_interval_is_imposed(): + _, run_mock, _, _ = _run([]) - command = run_mock.call_args[0][0] - assert _flag_value(command, "--number") == "5" - assert _flag_value(command, "--flavor") == "X" - assert _flag_value(command, "--volume-size") == "10" + assert "--interval" not in run_mock.call_args[0][0] -@pytest.mark.parametrize("returncode", [0, 3]) +@pytest.mark.parametrize("returncode", [0, 1, 2, 130]) def test_returncode_passed_through(returncode): run_mock = MagicMock(return_value=MagicMock(returncode=returncode)) - result, _, cleanup = _run([], run_mock=run_mock) + result, _, _, cleanup = _run([], run_mock=run_mock) assert result == returncode cleanup.assert_called_once_with(["tempfile"], "/cwd") @@ -99,7 +98,7 @@ def test_returncode_passed_through(returncode): def test_tool_not_found_returns_1(loguru_logs): run_mock = MagicMock(side_effect=FileNotFoundError()) - result, _, cleanup = _run([], run_mock=run_mock) + result, _, _, cleanup = _run([], run_mock=run_mock) assert result == 1 assert any( @@ -111,14 +110,14 @@ def test_tool_not_found_returns_1(loguru_logs): def test_generic_exception_returns_1(): run_mock = MagicMock(side_effect=RuntimeError("boom")) - result, _, cleanup = _run([], run_mock=run_mock) + result, _, _, cleanup = _run([], run_mock=run_mock) assert result == 1 cleanup.assert_called_once_with(["tempfile"], "/cwd") def test_setup_failure_returns_1(): - result, run_mock, _ = _run([], setup_success=False) + result, run_mock, _, _ = _run([], setup_success=False) assert result == 1 run_mock.assert_not_called()