Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

policy: refactor options to allow custom #1268

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

policy: refactor options to allow custom #1268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions resource/policies/base/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
)
Expand Down
63 changes: 63 additions & 0 deletions resource/policies/base/test/matcher_policy_factory_test02.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*****************************************************************************\
* 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 <config.h>
#endif
}

#include <string>
#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::string policy_opts = policies.find ("first")->second;
bool first = Flux::resource_model::parse_bool_match_options ("high", policy_opts);
ok (first == true, "policy first uses option high");

policy_opts = policies.find ("high")->second;
bool second = Flux::resource_model::option_exists ("stop_on_k_matches", policy_opts);
ok (second == false, "policy high does not have stop_on_k_matches as substring");

policy_opts = policies.find ("firstnodex")->second;
bool third = Flux::resource_model::parse_bool_match_options ("node_exclusive", policy_opts);
ok (third == true, "policy first uses option node_centric");

bool fourth = Flux::resource_model::option_exists ("stop_on_k_matches", policy_opts);
ok (fourth == true, "policy firstnodex has stop_on_k_matches as substring");

int fifth = Flux::resource_model::parse_int_match_options ("stop_on_k_matches", policy_opts);
ok (fifth == 1, "policy firstnodex stops on first match");

return 0;
}

int main (int argc, char *argv[])
{
plan (5);
test_parsers ();
done_testing ();
return 0;
}

/*
* vi: ts=4 sw=4 expandtab
*/
113 changes: 80 additions & 33 deletions resource/policies/dfu_match_policy_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include <config.h>
#endif
}

#include <string>
#include "resource/policies/dfu_match_policy_factory.hpp"

Expand All @@ -22,50 +21,98 @@

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;
if (policies.contains (policy)) {
return true;
}
return false;
}

std::shared_ptr<dfu_match_cb_t> create_match_cb (const std::string &policy)
bool parse_bool_match_options (const std::string match_option, const std::string policy_options)
{
std::shared_ptr<dfu_match_cb_t> matcher = nullptr;
// Return anything from after the = and before space or newline
size_t spot = policy_options.find (match_option, 0);
size_t start_pos = policy_options.find ("=", spot);
size_t end_pos = policy_options.find (" ", spot);
size_t end_str = policy_options.length ();
std::string return_opt;
if ((end_pos == std::string::npos) && (start_pos != std::string::npos)) {
return_opt = policy_options.substr ((start_pos + 1), (end_str - start_pos - 1));
} else {
return_opt = policy_options.substr ((start_pos + 1), (end_pos - start_pos - 1));
}
if (return_opt == "true") {
return true;
}
return false;
}

bool option_exists (const std::string match_option, const std::string policy_options)
{
size_t found = policy_options.find (match_option, 0);
if (found == std::string::npos) {
return false;
}
return true;
}

int parse_int_match_options (const std::string match_option, const std::string policy_options)
{
size_t spot = policy_options.find (match_option, 0);
size_t start_pos = policy_options.find ("=", spot);
size_t end_pos = policy_options.find (" ", spot);
size_t end_str = policy_options.length ();
int return_opt;
if ((end_pos == std::string::npos) && (start_pos != std::string::npos)) {
return_opt = stoi (policy_options.substr ((start_pos + 1), (end_str - start_pos - 1)));
} else {
return_opt = stoi (policy_options.substr ((start_pos + 1), (end_pos - start_pos - 1)));

Check warning on line 68 in resource/policies/dfu_match_policy_factory.cpp

View check run for this annotation

Codecov / codecov/patch

resource/policies/dfu_match_policy_factory.cpp#L68 

Added line #L68 was not covered by tests
}
return return_opt;
}

resource_type_t node_rt ("node");
std::shared_ptr<dfu_match_cb_t> create_match_cb (const std::string &policy_requested)
{
std::string policy = policies.find (policy_requested)->second;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this .find () call have any sort of error checking after the lookup of policy_requested? Maybe this is always guaranteed to find something successfully. If not, maybe you can check the contents of policy before moving on.

if (policy == "")
    // return an error?
    // set matcher to something default?
    // sorry that idk what the right behavior should be here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm good catch. It should call known_match_policy first to see if the policy exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually, known_match_policy is called when the config is validated in resource/modules/resource_match_opts.cpp so it's already been validated before it gets here.

This will need additional work to support custom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, a comment making note that it is already validated by the time it gets to that lookup might be helpful here!

std::shared_ptr<dfu_match_cb_t> matcher = nullptr;
try {
if (policy == FIRST_MATCH || policy == FIRST_NODEX_MATCH) {
if (policy_requested == "locality") {
matcher = std::make_shared<greater_interval_first_t> ();
}
if (policy_requested == "variation") {
matcher = std::make_shared<var_aware_t> ();
}

if (parse_bool_match_options ("high", policy)) {
std::shared_ptr<high_first_t> ptr = std::make_shared<high_first_t> ();
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 (option_exists ("stop_on_k_matches", policy)) {
ptr->set_stop_on_k_matches (parse_int_match_options ("stop_on_k_matches", policy));
}
matcher = ptr;
} else if (policy == HIGH_ID_FIRST) {
matcher = std::make_shared<high_first_t> ();
} else if (policy == LOW_ID_FIRST) {
matcher = std::make_shared<low_first_t> ();
} else if (policy == LOW_NODE_FIRST || policy == LOW_NODEX_FIRST) {

} else if (parse_bool_match_options ("low", policy)) {
std::shared_ptr<low_first_t> ptr = std::make_shared<low_first_t> ();
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<high_first_t> ptr = std::make_shared<high_first_t> ();
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 (option_exists ("stop_on_k_matches", policy)) {
ptr->set_stop_on_k_matches (parse_int_match_options ("stop_on_k_matches", policy));

Check warning on line 111 in resource/policies/dfu_match_policy_factory.cpp

View check run for this annotation

Codecov / codecov/patch

resource/policies/dfu_match_policy_factory.cpp#L111 

Added line #L111 was not covered by tests
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this if-else branch need an else statement? Excuse my ignorance on this - I'm not sure if the addition of custom policies allows for something other than low or high

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK low and high are the only options, unless locality or variation are chosen.

matcher = ptr;
} else if (policy == LOCALITY_AWARE) {
matcher = std::make_shared<greater_interval_first_t> ();
} else if (policy == VAR_AWARE) {
matcher = std::make_shared<var_aware_t> ();
}

} catch (std::bad_alloc &e) {
errno = ENOMEM;
matcher = nullptr;
Expand Down
32 changes: 21 additions & 11 deletions resource/policies/dfu_match_policy_factory.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <string>
#include <memory>
#include <map>
#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"
Expand All @@ -24,19 +25,28 @@
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";

bool known_match_policy (const std::string &policy);

const std::map<std::string, std::string> policies =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a deeper look over this, but first thing this should be split. Put the declaration here with extern and the definition in the cpp file so this variable doesn't end up getting defined in all includers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, that's what the old code did, but it shouldn't 😬

{{"first", "name=FIRST_MATCH high=true node_centric=true stop_on_k_matches=1"},
{"firstnodex",
"name=FIRST_NODEX_MATCH high=true node_centric=true node_exclusive=true stop_on_k_matches=1"},
{"high", "name=HIGH_ID_FIRST high=true"},
{"low", "name=LOW_ID_FIRST low=true"},
{"lonode", "name=LOW_NODE_FIRST low=true node_centric=true"},
{"hinode", "name=HIGH_NODE_FIRST high=true node_centric=true"},
{"lonodex", "name=LOW_NODEX_FIRST low=true node_centric=true node_exclusive=true"},
{"hinodex", "name=HIGH_NODEX_FIRST high=true node_centric=true node_exclusive=true"},
{"locality", "name=LOCALITY_AWARE"},
{"variation", "name=VAR_AWARE"},
{"custom", ""}};

bool parse_bool_match_options (const std::string match_option, const std::string policy_options);

bool option_exists (const std::string match_option, const std::string policy_options);

int parse_int_match_options (const std::string match_option, const std::string policy_options);

/*! Factory method for creating a matching callback
* object, representing a matching policy.
*/
Expand Down
Loading