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.

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

[WIP] policy: refactor of policy options and matching #1192

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
54 changes: 54 additions & 0 deletions resource/policies/base/test/matcher_policy_factory_test02.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*****************************************************************************\
* Copyright 2021 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
\*****************************************************************************/

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_match_policy()
{
for (auto i = policies.begin(); i != policies.end(); i++) {
const std::string policy = i->first;
bool check = known_match_policy(policy);
if (!check) {
std::cout << "failed on policy " << policy << std::endl;
return -1;

Check warning on line 31 in resource/policies/base/test/matcher_policy_factory_test02.cpp

View check run for this annotation

Codecov / codecov/patch

resource/policies/base/test/matcher_policy_factory_test02.cpp#L30-L31

Added lines #L30 - L31 were not covered by tests
}
}
bool check = known_match_policy("asdf");
if (!check) {
std::cout << "failed, as it should" << std::endl;
}
return 0;
}

int test_policy_settings()

Check warning on line 41 in resource/policies/base/test/matcher_policy_factory_test02.cpp

View check run for this annotation

Codecov / codecov/patch

resource/policies/base/test/matcher_policy_factory_test02.cpp#L41

Added line #L41 was not covered by tests
{
return 0;

Check warning on line 43 in resource/policies/base/test/matcher_policy_factory_test02.cpp

View check run for this annotation

Codecov / codecov/patch

resource/policies/base/test/matcher_policy_factory_test02.cpp#L43

Added line #L43 was not covered by tests
}

int main(int argc, char* argv[]) {
int ret = test_match_policy();
done_testing();
return ret;
}

/*
* vi: ts=4 sw=4 expandtab
*/
39 changes: 17 additions & 22 deletions resource/policies/dfu_match_policy_factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,53 +20,48 @@ extern "C" {
namespace Flux {
namespace resource_model {

bool known_match_policy (const std::string &policy)
bool known_match_policy (const std::string &policy_req)
{
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_req)) {
return true;
}
return false;
}

std::shared_ptr<dfu_match_cb_t> create_match_cb (const std::string &policy)
std::shared_ptr<dfu_match_cb_t> create_match_cb (const std::string &policy_requested)
{
std::string policy = policies.find(policy_requested)->second;
std::shared_ptr<dfu_match_cb_t> matcher = nullptr;

try {
if (policy == FIRST_MATCH || policy == FIRST_NODEX_MATCH) {
if (policy == "FIRST_MATCH" || policy == "FIRST_NODEX_MATCH") {
std::shared_ptr<high_first_t> ptr
= std::make_shared<high_first_t> ();
ptr->add_score_factor (std::string ("node"), 1, 10000);
ptr->set_stop_on_k_matches (1);
if (policy == FIRST_NODEX_MATCH)
if (policy == "FIRST_NODEX_MATCH")
ptr->add_exclusive_resource_type ("node");
matcher = ptr;
} else if (policy == HIGH_ID_FIRST) {
} else if (policy == "HIGH_ID_FIRST") {
matcher = std::make_shared<high_first_t> ();
} else if (policy == LOW_ID_FIRST) {
} 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 (policy == "LOW_NODE_FIRST" || policy == "LOW_NODEX_FIRST") {
std::shared_ptr<low_first_t> ptr
= std::make_shared<low_first_t> ();
ptr->add_score_factor (std::string ("node"), 1, 10000);
if (policy == LOW_NODEX_FIRST)
if (policy == "LOW_NODEX_FIRST")
ptr->add_exclusive_resource_type ("node");
matcher = ptr;
} else if (policy == HIGH_NODE_FIRST || policy == HIGH_NODEX_FIRST) {
} 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 (std::string ("node"), 1, 10000);
if (policy == HIGH_NODEX_FIRST)
if (policy == "HIGH_NODEX_FIRST")
ptr->add_exclusive_resource_type ("node");
matcher = ptr;
} else if (policy == LOCALITY_AWARE) {
} else if (policy == "LOCALITY_AWARE") {
matcher = std::make_shared<greater_interval_first_t> ();
} else if (policy == VAR_AWARE) {
} else if (policy == "VAR_AWARE") {
Copy link
Member

@jameshcorbett jameshcorbett May 1, 2024

Choose a reason for hiding this comment

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

I think it's usually discouraged to have lots of string literals lying around. The more times you type out the string, the greater the chance that you'll mistype it once and it can sometimes be pretty annoying to debug. If you refer to the string using a variable (like VAR_AWARE here), at least the compiler will tell you if you mistype the name. So in this switchyard I would probably go for something like if policy == policies["firstnodex"] rather than if policy == "FIRST_NODEX_MATCH".

matcher = std::make_shared<var_aware_t> ();
}
} catch (std::bad_alloc &e) {
Expand Down
23 changes: 13 additions & 10 deletions resource/policies/dfu_match_policy_factory.hpp
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,16 +25,18 @@
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";
const std::map<std::string, std::string> policies = {{"first", "FIRST_MATCH"},
{"firstnodex", "FIRST_NODEX_MATCH"},
{"high", "HIGH_ID_FIRST"},
{"low", "LOW_ID_FIRST"},
{"lonode", "LOW_NODE_FIRST"},
{"hinode", "HIGH_NODE_FIRST"},
{"lonodex", "LOW_NODEX_FIRST"},
{"hinodex", "HIGH_NODEX_FIRST"},
{"locality", "LOCALITY_AWARE"},
{"variation", "VAR_AWARE"}};



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

Expand Down
Loading