From 98ba3311fe871bb3217f157ac6f6baed91fd1cc0 Mon Sep 17 00:00:00 2001 From: William Hobbs Date: Mon, 12 Aug 2024 17:53:46 -0700 Subject: [PATCH] policy: refactor options to allow custom Problem: the current fluxion policy matching factory doesn't allow for customizable policies. Add a custom option and refactor existing policies to behave similarly. Add unit testing for the string parsing convenience functions. --- resource/policies/base/test/CMakeLists.txt | 6 + .../test/matcher_policy_factory_test02.cpp | 75 +++++++++ .../policies/dfu_match_policy_factory.cpp | 153 ++++++++++++++---- .../policies/dfu_match_policy_factory.hpp | 26 +-- 4 files changed, 217 insertions(+), 43 deletions(-) create mode 100644 resource/policies/base/test/matcher_policy_factory_test02.cpp diff --git a/resource/policies/base/test/CMakeLists.txt b/resource/policies/base/test/CMakeLists.txt index 4f19c04cb..bf3661bf0 100644 --- a/resource/policies/base/test/CMakeLists.txt +++ b/resource/policies/base/test/CMakeLists.txt @@ -1,3 +1,9 @@ +add_executable(matcher_policy_factory_test + ${CMAKE_CURRENT_SOURCE_DIR}/matcher_policy_factory_test02.cpp + ) +target_link_libraries(matcher_policy_factory_test PRIVATE libtap resource) +add_sanitizers(matcher_policy_factory_test) +flux_add_test(NAME matcher_policy_factory_test COMMAND matcher_policy_factory_test) add_executable(matcher_util_api_test ${CMAKE_CURRENT_SOURCE_DIR}/matcher_util_api_test01.cpp ) diff --git a/resource/policies/base/test/matcher_policy_factory_test02.cpp b/resource/policies/base/test/matcher_policy_factory_test02.cpp new file mode 100644 index 000000000..8462c660b --- /dev/null +++ b/resource/policies/base/test/matcher_policy_factory_test02.cpp @@ -0,0 +1,75 @@ +/*****************************************************************************\ + * Copyright 2024 Lawrence Livermore National Security, LLC + * (c.f. AUTHORS, NOTICE.LLNS, LICENSE) + * + * This file is part of the Flux resource manager framework. + * For details, see https://github.com/flux-framework. + * + * SPDX-License-Identifier: LGPL-3.0 +\*****************************************************************************/ + +/* + * Test for the dfu_match_policy class(es). Compares shared pointers + * expected values to string inputs to these classes. Strings can be + * specified in config for custom policies, or some are provided. + */ + +extern "C" { +#if HAVE_CONFIG_H +#include +#endif +} + +#include +#include +#include "resource/policies/dfu_match_policy_factory.hpp" +#include "src/common/libtap/tap.h" + +using namespace Flux; +using namespace Flux::resource_model; + +int test_parsers () +{ + std::map container; + bool first = Flux::resource_model::parse_custom_match_policy("high=true node_centric=true node_exclusive=true stop_on_1_matches=false blah=4", container); + ok (first == false, "blah is an unrecognized policy option"); + + std::map container2; + bool second = Flux::resource_model::parse_custom_match_policy("high=1 node_centric=true node_exclusive=true stop_on_1_matches=false", container2); + ok (second == false, "1 is an invalid option, must be true or false"); + + std::map container3; + bool third = Flux::resource_model::parse_custom_match_policy("high=true node_centric=true stop_on_1_matches=true", container3); + ok (third == true, "first is a valid policy"); + + // little debugging helper for printing the container + // std::map::iterator it; + // for (it=container.begin(); it != container.end(); it++) { + // std::cout << it->first << ": " << it->second << std::endl; + // } + bool fourth = Flux::resource_model::parse_bool_match_options ("high", container3); + ok (fourth == true, "policy first uses option high"); + + bool fifth = Flux::resource_model::parse_bool_match_options ("node_centric", container3); + ok (fifth == true, "policy first uses option node_centric"); + + bool sixth = Flux::resource_model::parse_bool_match_options ("stop_on_1_matches", container3); + ok (sixth == true, "policy first uses option stop_on_1_matches"); + + bool seventh = Flux::resource_model::parse_bool_match_options ("low", container3); + ok (seventh == false, "policy first does not use option low"); + + return 0; +} + +int main (int argc, char *argv[]) +{ + plan (7); + test_parsers (); + done_testing (); + return 0; +} + +/* + * vi: ts=4 sw=4 expandtab + */ diff --git a/resource/policies/dfu_match_policy_factory.cpp b/resource/policies/dfu_match_policy_factory.cpp index 8e7a4f645..9dc992235 100644 --- a/resource/policies/dfu_match_policy_factory.cpp +++ b/resource/policies/dfu_match_policy_factory.cpp @@ -13,59 +13,146 @@ extern "C" { #include #endif } - +#include #include +#include #include "resource/policies/dfu_match_policy_factory.hpp" namespace Flux { namespace resource_model { +policies = {{"first", "high=true node_centric=true stop_on_1_matches=true"}, + {"firstnodex", + "high=true node_centric=true node_exclusive=true stop_on_1_matches=true"}, + {"high", "high=true"}, + {"low", "low=true"}, + {"lonode", "low=true node_centric=true"}, + {"hinode", "high=true node_centric=true"}, + {"lonodex", "low=true node_centric=true node_exclusive=true"}, + {"hinodex", "high=true node_centric=true node_exclusive=true"}, + {"locality", ""}, + {"variation", ""}}; + +const std::vector policy_options = + {"name", "high", "node_centric", "stop_on_1_matches", "node_exclusive", "low"}; + +/* This takes a string of match policy options, deliniated by + * spaces and equals signs, and turns them into a std::map. + * It validates that each option is set to either "true" or "false" + * and puts the corresponding boolean in the map. + */ +bool parse_custom_match_policy (const std::string long_string, std::map &split) +{ + std::vector temp; + boost::split (temp, long_string, boost::is_any_of (" ")); + /* After splitting based on term, validate the terms. If invalid, + * abort. + */ + for (int i = 0; i < temp.size (); i++) { + std::vector temp_two; + /* This split should take an entire string and split it + * on the equals sign. If there's no equals sign, maybe it will + * get caught later? + */ + boost::split (temp_two, temp.at (i), boost::is_any_of ("=")); + /* Place the terms in a map for ease of access. Lots of gross + * temporary structures needed here, hopefully they'll get + * garbage collected soon. + */ + if (std::find (policy_options.begin (), policy_options.end (), temp_two.at (0)) + == policy_options.end ()) { + // better way to log something? + std::cout << "invalid policy option: " << temp_two.at (0) << std::endl; + return false; + } else { + if (temp_two.at (1).compare ("true") == 0) { + const auto [it, success] = split.insert ({temp_two.at (0), true}); + if (success == false) { + std::cout << "Insertion of " << it->first << " failed" << std::endl; + } + } else if (temp_two.at (1).compare ("false") == 0) { + const auto [it, success] = split.insert ({temp_two.at (0), false}); + if (success == false) { + std::cout << "Insertion of " << it->first << " failed" << std::endl; + return false; + } + } else { + std::cout << "Policy option " << temp_two.at (0) << " requires true or false, got " + << temp_two.at (1) << std::endl; + return false; + } + } + } + return true; +} + bool known_match_policy (const std::string &policy) { - bool rc = true; - if (policy != FIRST_MATCH && policy != FIRST_NODEX_MATCH && policy != HIGH_ID_FIRST - && policy != LOW_ID_FIRST && policy != LOW_NODE_FIRST && policy != HIGH_NODE_FIRST - && policy != LOW_NODEX_FIRST && policy != HIGH_NODEX_FIRST && policy != LOCALITY_AWARE - && policy != VAR_AWARE) - rc = false; - - return rc; + std::map throw_away; + if (policies.contains (policy)) { + return true; + } else if (parse_custom_match_policy (policy, throw_away) == true) { + return true; + } + return false; } -std::shared_ptr create_match_cb (const std::string &policy) +bool parse_bool_match_options (const std::string match_option, std::map &policy) { - std::shared_ptr matcher = nullptr; + if ((policy.find (match_option) != policy.end ()) && (policy[match_option] == true)) { + return true; + } + return false; +} - resource_type_t node_rt ("node"); +std::shared_ptr create_match_cb (const std::string &policy_requested) +{ + std::map policy; + if (policies.contains (policy_requested)) { + parse_custom_match_policy (policies.find (policy_requested)->second, policy); + } else { + parse_custom_match_policy (policy_requested, policy); + } + std::shared_ptr matcher = nullptr; try { - if (policy == FIRST_MATCH || policy == FIRST_NODEX_MATCH) { + if (policy_requested == "locality") { + matcher = std::make_shared (); + } + if (policy_requested == "variation") { + matcher = std::make_shared (); + } + + if (parse_bool_match_options ("high", policy)) { std::shared_ptr ptr = std::make_shared (); - ptr->add_score_factor (node_rt, 1, 10000); - ptr->set_stop_on_k_matches (1); - if (policy == FIRST_NODEX_MATCH) + if (parse_bool_match_options ("node_centric", policy)) { + ptr->add_score_factor (node_rt, 1, 10000); + } + + if (parse_bool_match_options ("node_exclusive", policy)) { ptr->add_exclusive_resource_type (node_rt); + } + + if (parse_bool_match_options ("stop_on_1_matches", policy)) { + ptr->set_stop_on_k_matches (1); + } matcher = ptr; - } else if (policy == HIGH_ID_FIRST) { - matcher = std::make_shared (); - } else if (policy == LOW_ID_FIRST) { - matcher = std::make_shared (); - } else if (policy == LOW_NODE_FIRST || policy == LOW_NODEX_FIRST) { + + } else if (parse_bool_match_options ("low", policy)) { std::shared_ptr ptr = std::make_shared (); - ptr->add_score_factor (node_rt, 1, 10000); - if (policy == LOW_NODEX_FIRST) - ptr->add_exclusive_resource_type (node_rt); - matcher = ptr; - } else if (policy == HIGH_NODE_FIRST || policy == HIGH_NODEX_FIRST) { - std::shared_ptr ptr = std::make_shared (); - ptr->add_score_factor (node_rt, 1, 10000); - if (policy == HIGH_NODEX_FIRST) + if (parse_bool_match_options ("node_centric", policy)) { + ptr->add_score_factor (node_rt, 1, 10000); + } + + if (parse_bool_match_options ("node_exclusive", policy)) { ptr->add_exclusive_resource_type (node_rt); + } + + if (parse_bool_match_options ("stop_on_1_matches", policy)) { + ptr->set_stop_on_k_matches (1); + } matcher = ptr; - } else if (policy == LOCALITY_AWARE) { - matcher = std::make_shared (); - } else if (policy == VAR_AWARE) { - matcher = std::make_shared (); } + } catch (std::bad_alloc &e) { errno = ENOMEM; matcher = nullptr; diff --git a/resource/policies/dfu_match_policy_factory.hpp b/resource/policies/dfu_match_policy_factory.hpp index 5fbbed0f8..24af42ac2 100644 --- a/resource/policies/dfu_match_policy_factory.hpp +++ b/resource/policies/dfu_match_policy_factory.hpp @@ -13,6 +13,7 @@ #include #include +#include #include "resource/policies/base/dfu_match_cb.hpp" #include "resource/policies/dfu_match_high_id_first.hpp" #include "resource/policies/dfu_match_low_id_first.hpp" @@ -24,19 +25,24 @@ namespace Flux { namespace resource_model { -const std::string FIRST_MATCH = "first"; -const std::string FIRST_NODEX_MATCH = "firstnodex"; -const std::string HIGH_ID_FIRST = "high"; -const std::string LOW_ID_FIRST = "low"; -const std::string LOW_NODE_FIRST = "lonode"; -const std::string HIGH_NODE_FIRST = "hinode"; -const std::string LOW_NODEX_FIRST = "lonodex"; -const std::string HIGH_NODEX_FIRST = "hinodex"; -const std::string LOCALITY_AWARE = "locality"; -const std::string VAR_AWARE = "variation"; +extern const std::map policies; bool known_match_policy (const std::string &policy); +bool get_match_options (const std::string match_option, std::map &options); + +/* Returns true if the custom match policy is valid. + * Returns false if it is invalid. + * Stores all of the options requested in the map under + * key-value pairs. + */ +bool parse_custom_match_policy (const std::string long_string, std::map &split); + +/* Included in the header for unit testing. + * This is how the map keys get accessed and returned easily. + */ +bool parse_bool_match_options (const std::string match_option, std::map &policy); + /*! Factory method for creating a matching callback * object, representing a matching policy. */