policy: refactor options to allow custom

[wihobbs]

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.

Outstanding to-do:

• add an option in the scheduler config to actually pass a custom string in, and probably some validation too

[wihobbs]

And I know the testing and the code should be in separate commits... sigh (will fix)

[cmoussa1]

Thanks for opening this @wihobbs! I have just some preliminary questions/comments on a first pass. Apologies in advance that I'm not very familiar with the sched code at all so I could definitely be misunderstanding how this works. :-)

[cmoussa1]

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

[wihobbs]

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

[wihobbs]

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.

[cmoussa1]

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

[cmoussa1]

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

[wihobbs]

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

[codecov]

Codecov Report

Attention: Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.5%. Comparing base (8b2cb13) to head (ba80f7b).
Report is 5 commits behind head on master.

Files	Patch %	Lines
resource/policies/dfu_match_policy_factory.cpp	95.6%	2 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1268   +/-   ##
======================================
  Coverage    75.4%   75.5%           
======================================
  Files         107     108    +1     
  Lines       15219   15254   +35     
======================================
+ Hits        11487   11520   +33     
- Misses       3732    3734    +2     

Files	Coverage Δ
...licies/base/test/matcher_policy_factory_test02.cpp	100.0% <100.0%> (ø)
resource/policies/dfu_match_policy_factory.cpp	91.3% <95.6%> (-1.5%)	⬇️

[jameshcorbett]

Just did a quick look-through, it looks reasonable to me. I think the commits could be broken up better though (as you noted already)

[trws]

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.

[trws]

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

[garlick]

PR title seems truncated?

[wihobbs]

Well, custom is a new type of policy that this PR is enabling. Or will enable eventually. Maybe "resource: refactor policy options to allow a user-defined policy" is more descriptive?

Note that this is still WIP and needs some config work and testing for the custom options. Happy to incorporate any feedback on the initial draft (and thanks to those who have already reviewed!), but it's going to stay in draft state until the code can:

• accept a custom string with policy options
• reject certain strings that are requesting invalid custom options, such as set_stop_on_k_matches=-100 and such
• test some custom options and make sure they behave as expected
• break out the commit history in a better way

[wihobbs]

Per discussion with Tom:

• To handle custom policies, "don't validate, parse" -- parse the input on a delimiter first and then verify that each key is valid, give an error message if it isn't, possibly demote this to a warning based on a config option
• Fluxion should issue a fatal warning if a policy named is not valid -- don't fall back on first (might already be done?)
• set_stop_on_k_matches probably only takes a value of 1, however, we should test this
• we should also think about having "node_centric" have a score factor of 10000, maybe we don't want to allow users to configure this, but maybe that number doesn't cover all our bases