From 28e7eecf0b29821077205ec5b5c00a3891e889d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Mon, 5 Feb 2024 07:53:04 -0500 Subject: [PATCH 1/5] tools: add constant with current help command-line arguments Unfortunately, we have code in scylla-nodetool.cc which needs to know what are the current help options available. Soon, there will be more code like this in tools/utils.cc, so centralize this list in a const static tool_app_template member. --- tools/scylla-nodetool.cc | 12 +++--------- tools/utils.cc | 6 ++++++ tools/utils.hh | 3 +++ 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/tools/scylla-nodetool.cc b/tools/scylla-nodetool.cc index c697aea32d..d06201b5bb 100644 --- a/tools/scylla-nodetool.cc +++ b/tools/scylla-nodetool.cc @@ -1170,15 +1170,9 @@ void help_operation(const tool_app_template::config& cfg, const bpo::variables_m // goes in. bpo::options_description opts_desc(fmt::format("scylla-{} options", app_name)); - opts_desc.add_options() - ("help,h", "show help message") - ; - opts_desc.add_options() - ("help-seastar", "show help message about seastar options") - ; - opts_desc.add_options() - ("help-loggers", "print a list of logger names and exit") - ; + for (const auto& [option, help] : tool_app_template::help_arguments) { + opts_desc.add_options()(option, help); + } if (cfg.global_options) { for (const auto& go : *cfg.global_options) { go.add_option(opts_desc); diff --git a/tools/utils.cc b/tools/utils.cc index 56f09663b2..03e87b2251 100644 --- a/tools/utils.cc +++ b/tools/utils.cc @@ -92,6 +92,12 @@ db_config_and_extensions::db_config_and_extensions() db_config_and_extensions::db_config_and_extensions(db_config_and_extensions&&) = default; db_config_and_extensions::~db_config_and_extensions() = default; +const std::vector> tool_app_template::help_arguments{ + {"help,h", "show help message"}, + {"help-seastar", "show help message about seastar options"}, + {"help-loggers", "print a list of logger names and exit"}, +}; + tool_app_template::tool_app_template(config cfg) : _cfg(std::move(cfg)) {} diff --git a/tools/utils.hh b/tools/utils.hh index 8869760532..e90a8dd2e6 100644 --- a/tools/utils.hh +++ b/tools/utils.hh @@ -140,6 +140,9 @@ struct db_config_and_extensions { }; class tool_app_template { +public: + static const std::vector> help_arguments; + public: struct config { sstring name; From 7ae98c586a473bf4d68c770e9185392ab4b71eb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Mon, 5 Feb 2024 06:36:45 -0500 Subject: [PATCH 2/5] tools/utils: get_selected_operation(): remove alias param This method has a single caller, who always passes "operation". Just hard-code this into the method, no need to keep a param for it. --- tools/utils.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/utils.cc b/tools/utils.cc index 03e87b2251..1bef88f235 100644 --- a/tools/utils.cc +++ b/tools/utils.cc @@ -29,9 +29,9 @@ namespace { // The operation is expected to be at argv[1].If found, it is shifted out to the // end (effectively removed) and the corresponding operation* is returned. // If not found or unrecognized an error is logged and exit() is called. -const operation& get_selected_operation(int& ac, char**& av, const std::vector& operations, std::string_view alias) { +const operation& get_selected_operation(int& ac, char**& av, const std::vector& operations) { if (ac < 2) { - fmt::print(std::cerr, "error: missing mandatory {} argument\n", alias); + fmt::print(std::cerr, "error: missing mandatory operation argument\n"); exit(1); } @@ -53,7 +53,7 @@ const operation& get_selected_operation(int& ac, char**& av, const std::vector Date: Mon, 5 Feb 2024 07:40:57 -0500 Subject: [PATCH 3/5] tools/utils: make finding the operation command line option more flexible Currently all scylla-tools assume that the operation/command is in argv[1]. This is not very flexible, because most programs allow global options (that are not dependent on the current operation/command) to be passed before the operation name on the command line. Notably C*'s nodetool is one such program and indeed scripts and tests using nodetool do utilize this. This patch makes this more flexible. Instead of looking at argv[1], do an initial option parsing with boost::program_options to locate the operation parameter. This initial parser knows about the global options, and the operation positional argument. It allows for unrecognized positional and non-positional arguments, but only after the command. With this, any combination of global options + operation is allowed, in any order. --- test/nodetool/test_nodetool.py | 18 +++++++- tools/utils.cc | 76 ++++++++++++++++++++++++++++------ 2 files changed, 80 insertions(+), 14 deletions(-) diff --git a/test/nodetool/test_nodetool.py b/test/nodetool/test_nodetool.py index 38bd0e14a8..eaa13c9643 100644 --- a/test/nodetool/test_nodetool.py +++ b/test/nodetool/test_nodetool.py @@ -4,7 +4,7 @@ # SPDX-License-Identifier: AGPL-3.0-or-later # -from rest_api_mock import expected_request +from rest_api_mock import expected_request, set_expected_requests import subprocess import utils @@ -67,3 +67,19 @@ def test_nodetool_nonexistent_command(nodetool, scylla_only): ("non-existent-command",), {}, ["error: unrecognized operation argument: expected one of"]) + + +def test_global_options_order(nodetool_path, rest_api_mock_server, scylla_only): + set_expected_requests(rest_api_mock_server, [ + expected_request("POST", "/storage_service/compact", multiple=expected_request.MULTIPLE)]) + + ip, port = rest_api_mock_server + port = str(port) + + subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, "-p", port], check=True) + subprocess.run([nodetool_path, "nodetool", "-h", ip, "compact", "-p", port], check=True) + subprocess.run([nodetool_path, "nodetool", "-h", ip, "-p", port, "compact"], check=True) + + # Also add some compatibility args to the mix + subprocess.run([nodetool_path, "nodetool", "-h", ip, "-p", port, "-u", "us3r", "compact"], check=True) + subprocess.run([nodetool_path, "nodetool", "-h", ip, "-p", port, "compact", "-u", "us3r"], check=True) diff --git a/tools/utils.cc b/tools/utils.cc index 1bef88f235..841618b8b0 100644 --- a/tools/utils.cc +++ b/tools/utils.cc @@ -8,6 +8,7 @@ #include #include +#include #include #include "db/config.hh" @@ -16,6 +17,8 @@ #include "utils/logalloc.hh" #include "init.hh" +namespace bpo = boost::program_options; + namespace tools::utils { bool operation::matches(std::string_view name) const { @@ -26,23 +29,73 @@ namespace { // Extract the operation from the argv. // -// The operation is expected to be at argv[1].If found, it is shifted out to the -// end (effectively removed) and the corresponding operation* is returned. -// If not found or unrecognized an error is logged and exit() is called. -const operation& get_selected_operation(int& ac, char**& av, const std::vector& operations) { - if (ac < 2) { +// Do an initial parsing of command-line options to identify the operation +// command-line argument. +// If found, it is shifted out to the end (effectively removed) and the +// corresponding operation* is returned. +// If not found: +// * If any of the tool_app_template::help_arguments was used, nullptr is returned, to allow the tool to produce a proper help. +// * If no help was invoked, the application exits with return-code 1. +// If the operation is not recognized, the application exits with return-code 100. +// If parsing the command-line arguments fails, the application exits with return-code 2. +const operation* get_selected_operation(int& ac, char**& av, const std::vector& operations, const std::vector* global_options) { + bpo::positional_options_description pos_opts; + bpo::options_description opts("scylla-tool options"); + for (const auto& [option, help] : tool_app_template::help_arguments) { + opts.add_options()(option, help); + } + opts.add(boost::make_shared("operation", bpo::value(), "Operation")); + pos_opts.add("operation", 1); + opts.add(boost::make_shared("operation_options", bpo::value>(), "Operation specific options")); + pos_opts.add("operation_options", -1); + + if (global_options) { + for (const auto& go : *global_options) { + go.add_option(opts); + } + } + + bpo::variables_map vm; + try { + bpo::store(bpo::command_line_parser(ac, av) + .options(opts) + .positional(pos_opts) + .allow_unregistered() + .run() + , vm); + } catch (bpo::error& e) { + fmt::print("error: {}\n\nTry --help.\n", e.what()); + exit(2); + } + + if (!vm.count("operation")) { + // We allow no operation only when help was requested. + for (const auto& [op, _] : tool_app_template::help_arguments) { + std::string option(op); + if (auto comma_pos = option.find(","); comma_pos != sstring::npos) { + option = option.substr(0, comma_pos); + } + if (vm.count(option)) { + return nullptr; + } + } fmt::print(std::cerr, "error: missing mandatory operation argument\n"); exit(1); } - const char* op_name = av[1]; + sstring op_name = vm["operation"].as(); if (auto found = std::ranges::find_if(operations, [op_name] (auto& op) { return op.matches(op_name); }); found != operations.end()) { - std::shift_left(av + 1, av + ac, 1); - --ac; - return *found; + for (int i = 0; i < ac; ++i) { + if (op_name == av[i]) { + std::shift_left(av + i, av + ac, 1); + --ac; + break; + } + } + return &*found; } std::vector all_operation_names; @@ -110,10 +163,7 @@ int tool_app_template::run_async(int argc, char** argv, noncopyable_function Date: Mon, 5 Feb 2024 09:43:28 -0500 Subject: [PATCH 4/5] tools/scylla-nodetool: ignore JVM args Legacy scripts and tests for nodetool, might pass JVM args like -Dcom.sun.jndi.rmiURLParsing=legacy. Ignore these, by dropping anything that starts with -D from the command line args. --- test/nodetool/test_nodetool.py | 14 ++++++++++++++ tools/scylla-nodetool.cc | 5 +++++ 2 files changed, 19 insertions(+) diff --git a/test/nodetool/test_nodetool.py b/test/nodetool/test_nodetool.py index eaa13c9643..1285f7b76a 100644 --- a/test/nodetool/test_nodetool.py +++ b/test/nodetool/test_nodetool.py @@ -83,3 +83,17 @@ def test_global_options_order(nodetool_path, rest_api_mock_server, scylla_only): # Also add some compatibility args to the mix subprocess.run([nodetool_path, "nodetool", "-h", ip, "-p", port, "-u", "us3r", "compact"], check=True) subprocess.run([nodetool_path, "nodetool", "-h", ip, "-p", port, "compact", "-u", "us3r"], check=True) + + +def test_jvm_options(nodetool_path, rest_api_mock_server, scylla_only): + set_expected_requests(rest_api_mock_server, [ + expected_request("POST", "/storage_service/compact", multiple=expected_request.MULTIPLE)]) + + ip, port = rest_api_mock_server + port = str(port) + + jvm_opt = "-Dcom.sun.jndi.rmiURLParsing=legacy" + + subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, "-p", port, jvm_opt], check=True) + subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, jvm_opt, "-p", port], check=True) + subprocess.run([nodetool_path, "nodetool", jvm_opt, "compact", "-h", ip, "-p", port], check=True) diff --git a/tools/scylla-nodetool.cc b/tools/scylla-nodetool.cc index d06201b5bb..78eafc0545 100644 --- a/tools/scylla-nodetool.cc +++ b/tools/scylla-nodetool.cc @@ -3495,6 +3495,11 @@ std::vector massage_argv(int argc, char** argv) { continue; } + // Java JVM options, they look like -Dkey=value, or just -Dkey + if (argv[i][1] == 'D') { + continue; + } + std::string arg = argv[i]; std::string arg_key; std::optional arg_value; From c2425ca1354072ec733a369edd2402b33dadae2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Fri, 16 Feb 2024 08:19:27 -0500 Subject: [PATCH 5/5] tools/scylla-nodetool: add --rest-api-port option This option is an alternative to --port|-p and takes precedence over it. This is meant to aid the switch from the legacy nodetool to the native one. Users of the legacy nodetool pass the port of JMX to --port. We need a way to provide both the JMX port (via --port) and also the REST API port, which only the native nodetool will interpret. So we add this new --rest-api-port, which when provided, overwrites the --port|-p option. To ensure the legacy nodeotol doesn't try to interpret this, this option can also be provided as -Dcom.scylladb.apiPort (which is substituted to --rest-api-port behind the scenes). --- test/nodetool/test_nodetool.py | 13 +++++++++++++ tools/scylla-nodetool.cc | 16 +++++++++++++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/test/nodetool/test_nodetool.py b/test/nodetool/test_nodetool.py index 1285f7b76a..d76c598aa9 100644 --- a/test/nodetool/test_nodetool.py +++ b/test/nodetool/test_nodetool.py @@ -97,3 +97,16 @@ def test_jvm_options(nodetool_path, rest_api_mock_server, scylla_only): subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, "-p", port, jvm_opt], check=True) subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, jvm_opt, "-p", port], check=True) subprocess.run([nodetool_path, "nodetool", jvm_opt, "compact", "-h", ip, "-p", port], check=True) + + +def test_alternative_api_port(nodetool_path, rest_api_mock_server, scylla_only): + set_expected_requests(rest_api_mock_server, [ + expected_request("POST", "/storage_service/compact", multiple=expected_request.MULTIPLE)]) + + ip, port = rest_api_mock_server + port = str(port) + + subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, "-p", "1", "--rest-api-port", port], check=True) + subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, "-p", "1", f"--rest-api-port={port}"], check=True) + subprocess.run([nodetool_path, "nodetool", "compact", "-h", ip, "-p", "1", f"-Dcom.scylladb.apiPort={port}"], + check=True) diff --git a/tools/scylla-nodetool.cc b/tools/scylla-nodetool.cc index 78eafc0545..0694773de5 100644 --- a/tools/scylla-nodetool.cc +++ b/tools/scylla-nodetool.cc @@ -2646,6 +2646,7 @@ void version_operation(scylla_rest_client& client, const bpo::variables_map& vm) const std::vector global_options{ typed_option("host,h", "localhost", "the hostname or ip address of the ScyllaDB node"), typed_option("port,p", 10000, "the port of the REST API of the ScyllaDB node"), + typed_option("rest-api-port", "the port of the REST API of the ScyllaDB node; takes precedence over --port|-p"), typed_option("password", "Remote jmx agent password (unused)"), typed_option("password-file", "Path to the JMX password file (unused)"), typed_option("username,u", "Remote jmx agent username (unused)"), @@ -2654,6 +2655,7 @@ const std::vector global_options{ const std::map option_substitutions{ {"-h", "--host"}, + {"-Dcom.scylladb.apiPort", "--rest-api-port"}, {"-pw", "--password"}, {"-pwf", "--password-file"}, {"-pp", "--print-port"}, @@ -3495,12 +3497,14 @@ std::vector massage_argv(int argc, char** argv) { continue; } + std::string arg = argv[i]; + // Java JVM options, they look like -Dkey=value, or just -Dkey - if (argv[i][1] == 'D') { + // We leave -Dcom.scylladb.apiPort, it will be substituted to --rest-api-port + if (argv[i][1] == 'D' && !arg.starts_with("-Dcom.scylladb.apiPort=")) { continue; } - std::string arg = argv[i]; std::string arg_key; std::optional arg_value; @@ -3573,7 +3577,13 @@ For more information, see: https://opensource.docs.scylladb.com/stable/operating if (operation.name() == "help") { help_operation(app.get_config(), app_config); } else { - scylla_rest_client client(app_config["host"].as(), app_config["port"].as()); + uint16_t port{}; + if (app_config.count("rest-api-port")) { + port = app_config["rest-api-port"].as(); + } else { + port = app_config["port"].as(); + } + scylla_rest_client client(app_config["host"].as(), port); get_operations_with_func().at(operation)(client, app_config); } } catch (std::invalid_argument& e) {