Merge "Config fixes" from Calle

"Fixes #2933

Fixes regressions introduced by config restructuring.
Allows "base" config to handle errors by warning, while
other uses can opt otherwise."

[pdziepak: resolved merge conflict]

* 'calle/cfgfix' of github.com:scylladb/seastar-dev:
  config_test: Use error handler (ignore errors) + add error test
  config: Resurrect command line aliases that where lost
  main: Use error handler for config parse
  config_file: Add optional "error_handler" to yaml parse functions

db/config.cc

@@ -112,6 +112,17 @@ void config_file::named_value<db::config::seed_provider_type,
 
 }
 
+boost::program_options::options_description_easy_init&
+db::config::add_options(boost::program_options::options_description_easy_init& init) {
+    config_file::add_options(init);
+
+    data_file_directories.add_command_line_option(init, "datadir", "alias for 'data-file-directories'");
+    rpc_port.add_command_line_option(init, "thrift-port", "alias for 'rpc-port'");
+    native_transport_port.add_command_line_option(init, "cql-port", "alias for 'native-transport-port'");
+
+    return init;
+}
+
 boost::filesystem::path db::config::get_conf_dir() {
     using namespace boost::filesystem;
 

db/config.hh

@@ -726,6 +726,9 @@ public:
 
     seastar::logging_settings logging_settings(const boost::program_options::variables_map&) const;
 
+    boost::program_options::options_description_easy_init&
+    add_options(boost::program_options::options_description_easy_init&);
+
 private:
     template<typename T>
     struct log_legacy_value : public named_value<T, value_status::Used> {

main.cc

@@ -101,7 +101,13 @@ read_config(bpo::variables_map& opts, db::config& cfg) {
         file = relative_conf_dir("scylla.yaml").string();
     }
     return check_direct_io_support(file).then([file, &cfg] {
-        return cfg.read_from_file(file);
+        return cfg.read_from_file(file, [](auto & opt, auto & msg, auto status) {
+            auto level = log_level::warn;
+            if (status.value_or(db::config::value_status::Invalid) != db::config::value_status::Invalid) {
+                level = log_level::error;
+            }
+            startlog.log(level, "{} : {}", msg, opt);
+        });
     }).handle_exception([file](auto ep) {
         startlog.error("Could not read configuration file {}: {}", file, ep);
         return make_exception_future<>(ep);

tests/config_test.cc

@@ -826,7 +826,11 @@ inline std::basic_ostream<Args...> & operator<<(std::basic_ostream<Args...> & os
 SEASTAR_TEST_CASE(test_parse_yaml) {
     config cfg;
 
-    cfg.read_from_yaml(cassandra_conf);
+    cfg.read_from_yaml(cassandra_conf, [](auto& opt, auto& msg, auto status) {
+        if (status != config::value_status::Invalid) {
+            throw std::invalid_argument(msg + " : " + opt);
+        }
+    });
 
     BOOST_CHECK_EQUAL(cfg.cluster_name(), "Test Cluster");
     BOOST_CHECK_EQUAL(cfg.cluster_name.is_set(), true);
@@ -861,4 +865,34 @@ SEASTAR_TEST_CASE(test_parse_yaml) {
     return make_ready_future<>();
 }
 
+SEASTAR_TEST_CASE(test_parse_broken) {
+    config cfg;
 
+    bool ok = false;
+
+    // this should become an "unknown option" kind of error.
+    cfg.read_from_yaml(R"foo(bork_bnork:
+    apa
+    ko
+)foo", [&](auto& opt, auto& msg, auto status) {
+        if (!status) { // unknown
+            ok = true;
+        }
+    });
+
+    BOOST_REQUIRE(ok);
+
+    ok = false;
+
+    // this should be a value parsing error.
+    cfg.read_from_yaml(R"foo(commitlog_segment_size_in_mb: flaska
+)foo", [&](auto& opt, auto& msg, auto status) {
+        if (status && status != config::value_status::Invalid) { // option exists and is valid.
+            ok = true;
+        }
+    });
+
+    BOOST_REQUIRE(ok);
+
+    return make_ready_future<>();
+}

utils/config_file.cc

@@ -241,13 +241,18 @@ utils::config_file::add_options(bpo::options_description_easy_init& init) {
     return init;
 }
 
-void utils::config_file::read_from_yaml(const sstring& yaml) {
-    read_from_yaml(yaml.c_str());
+void utils::config_file::read_from_yaml(const sstring& yaml, error_handler h) {
+    read_from_yaml(yaml.c_str(), std::move(h));
 }
 
-void utils::config_file::read_from_yaml(const char* yaml) {
+void utils::config_file::read_from_yaml(const char* yaml, error_handler h) {
     std::unordered_map<sstring, cfg_ref> values;
 
+    if (!h) {
+        h = [](auto & opt, auto & msg, auto) {
+            throw std::invalid_argument(msg + " : " + opt);
+        };
+    }
     /*
      * Note: this is not very "half-fault" tolerant. I.e. there could be
      * yaml syntax errors that origin handles and still sets the options
@@ -261,7 +266,8 @@ void utils::config_file::read_from_yaml(const char* yaml) {
 
         auto i = std::find_if(_cfgs.begin(), _cfgs.end(), [&label](const config_src& cfg) { return cfg.name() == label; });
         if (i == _cfgs.end()) {
-            throw std::invalid_argument("Unknown option " + label);
+            h(label, "Unknown option", stdx::nullopt);
+            continue;
         }
 
         config_src& cfg = *i;
@@ -272,7 +278,8 @@ void utils::config_file::read_from_yaml(const char* yaml) {
         }
         switch (cfg.status()) {
         case value_status::Invalid:
-            throw std::invalid_argument("Option " + label + " is not applicable");
+            h(label, "Option is not applicable", cfg.status());
+            continue;
         case value_status::Unused:
         default:
             break;
@@ -281,7 +288,13 @@ void utils::config_file::read_from_yaml(const char* yaml) {
             continue;
         }
         // Still, a syntax error is an error warning, not a fail
-        cfg.set_value(node.second);
+        try {
+            cfg.set_value(node.second);
+        } catch (std::exception& e) {
+            h(label, e.what(), cfg.status());
+        } catch (...) {
+            h(label, "Could not convert value", cfg.status());
+        }
     }
 }
 
@@ -305,19 +318,19 @@ utils::config_file::configs utils::config_file::unset_values() const {
     return res;
 }
 
-future<> utils::config_file::read_from_file(file f) {
-    return f.size().then([this, f](size_t s) {
-        return do_with(make_file_input_stream(f), [this, s](input_stream<char>& in) {
-            return in.read_exactly(s).then([this](temporary_buffer<char> buf) {
-               read_from_yaml(sstring(buf.begin(), buf.end()));
+future<> utils::config_file::read_from_file(file f, error_handler h) {
+    return f.size().then([this, f, h](size_t s) {
+        return do_with(make_file_input_stream(f), [this, s, h](input_stream<char>& in) {
+            return in.read_exactly(s).then([this, h](temporary_buffer<char> buf) {
+               read_from_yaml(sstring(buf.begin(), buf.end()), h);
             });
         });
     });
 }
 
-future<> utils::config_file::read_from_file(const sstring& filename) {
-    return open_file_dma(filename, open_flags::ro).then([this](file f) {
-       return read_from_file(std::move(f));
+future<> utils::config_file::read_from_file(const sstring& filename, error_handler h) {
+    return open_file_dma(filename, open_flags::ro).then([this, h](file f) {
+       return read_from_file(std::move(f), h);
     });
 }
 

utils/config_file.hh

@@ -143,10 +143,25 @@ public:
     boost::program_options::options_description_easy_init&
     add_options(boost::program_options::options_description_easy_init&);
 
-    void read_from_yaml(const sstring&);
-    void read_from_yaml(const char *);
-    future<> read_from_file(const sstring&);
-    future<> read_from_file(file);
+    /**
+     * Default behaviour for yaml parser is to throw on
+     * unknown stuff, invalid opts or conversion errors.
+     *
+     * Error handling function allows overriding this.
+     *
+     * error: <option name>, <message>, <optional value_status>
+     *
+     * The last arg, opt value_status will tell you the type of
+     * error occurred. If not set, the option found does not exist.
+     * If invalid, it is invalid. Otherwise, a parse error.
+     *
+     */
+    using error_handler = std::function<void(const sstring&, const sstring&, stdx::optional<value_status>)>;
+
+    void read_from_yaml(const sstring&, error_handler = {});
+    void read_from_yaml(const char *, error_handler = {});
+    future<> read_from_file(const sstring&, error_handler = {});
+    future<> read_from_file(file, error_handler = {});
 
     using configs = std::vector<cfg_ref>;