diff --git a/test/nodetool/test_nodetool.py b/test/nodetool/test_nodetool.py index 38bd0e14a8..d76c598aa9 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,46 @@ 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) + + +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) + + +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 c697aea32d..0694773de5 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); @@ -2652,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)"), @@ -2660,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"}, @@ -3502,6 +3498,13 @@ std::vector massage_argv(int argc, char** argv) { } std::string arg = argv[i]; + + // Java JVM options, they look like -Dkey=value, or just -Dkey + // 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_key; std::optional arg_value; @@ -3574,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) { diff --git a/tools/utils.cc b/tools/utils.cc index 56f09663b2..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, std::string_view alias) { - if (ac < 2) { - fmt::print(std::cerr, "error: missing mandatory {} argument\n", alias); +// 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; @@ -53,7 +106,7 @@ const operation& get_selected_operation(int& ac, char**& av, 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)) {} @@ -104,10 +163,7 @@ int tool_app_template::run_async(int argc, char** argv, noncopyable_function> help_arguments; + public: struct config { sstring name;