ingest: set configured queue constraints #4587

garlick · 2022-09-19T19:08:14Z

This adds an ingest "frobnicator" plugin (thanks @grondo) that works with the RFC 33 queues.NAME.requires spec proposed in flux-framework/rfc#342. The broker policy/queue config validator is also modified to allow queues.NAME.requires.

This was sufficient to get my test system queues working again with this config

[ingest.validator]
plugins = ["feasibility", "jobspec" ]

[ingest.frobnicator]
plugins = [ "defaults", "constraints" ]

[[resource.config]]
hosts = "picl[0-7]"
cores = "0-3"

[[resource.config]]
hosts = "picl[1-5]"
properties = ["batch"]

[[resource.config]]
hosts = "picl[6-7]"
properties = ["debug"]

[queues.debug]
requires = [ "debug" ]

[queues.batch]
requires = [ "batch" ]

[policy.jobspec.defaults.system]
queue = "debug"

Marked as WIP pending

more testing
add flux-config-queues(5)

grondo

LGTM so far. I had just one thought while doing a first pass (sorry the comment was meant to be an individual comment, not a review, but I had already clicked "Start a Review")

grondo · 2022-09-19T19:24:44Z

src/broker/brokercfg.c

            }
            if (policy) {
                char key[1024];
                snprintf (key, sizeof (key), "queues.%s.policy", name);
                if (validate_policy_json (policy, key, error) < 0)
                    return -1;
            }
+            if (requires) {


One thing we might want to do here is validate that each configured property is valid according to RFC 20, i.e. does not contain whitespace or any characters from the set ! & ' " ^ `` | ( ).

Ah, I was wrong! Actually ^ should be allowed since that is currently our shorthand for not, e.g. a sysadmin should be able to configure a queue.NAME.requires of [ "^batch" ] which means that queue NAME excludes all resources with the batch property.

I'm sorry for messing that one up.

garlick · 2022-09-19T23:23:26Z

Force pushed with a new flux-config-queues(5) man page and changes to validate property names, per @grondo's comments.

grondo

LGTM. I unfortunately gave bad advice in my previous comment (the ^ character at least should be allowed in requires (sorry about that), and had one trivial suggestion for the flux-config-queues(5) man page.

grondo · 2022-09-20T00:59:10Z

doc/man5/flux-config-queues.rst

+The ``queues`` table configures job queues, as described in RFC 33.
+Normally, Flux has a single anonymous queue, but when queues are configured,
+all queues are named, and a job submitted without a queue name is rejected,
+unless a default queue is configured.


Very small suggestion, this might read better if the last comma is removed:

Normally, Flux has a single anonymous queue, but when queues are configured,
all queues are named, and a job submitted without a queue name is rejected
unless a default queue is configured.

grondo · 2022-09-20T01:02:53Z

src/broker/brokercfg.c

            }
            if (policy) {
                char key[1024];
                snprintf (key, sizeof (key), "queues.%s.policy", name);
                if (validate_policy_json (policy, key, error) < 0)
                    return -1;
            }
+            if (requires) {


Ah, I was wrong! Actually ^ should be allowed since that is currently our shorthand for not, e.g. a sysadmin should be able to configure a queue.NAME.requires of [ "^batch" ] which means that queue NAME excludes all resources with the batch property.

I'm sorry for messing that one up.

garlick · 2022-09-20T01:16:23Z

NP! I should have caught it - I did skim RFC 20 after your comment but missed it too.

Any thoughts for further testing of the constraints plugin before we merge?

grondo · 2022-09-20T01:34:21Z

Any thoughts for further testing of the constraints plugin before we merge?

Yeah, here's some ideas:

ensure the constraints plugin can be specified even if no queues are configured
ensure the constraints plugin can be specified when the queue does not have a requires key

Also, I noticed that the constraints plugin I gave you allows for a non-list comma separated string to be specified for requires I think. That code could be removed.

Edit: Also if you drive the constraints plugin from flux job-frobnicator directly you could test that some of the invalid configuration is handled correctly there as well (e.g. requires not a list, invalid queue specified...

garlick · 2022-09-20T14:50:40Z

Also if you drive the constraints plugin from flux job-frobnicator directly you could test that some of the invalid configuration is handled correctly there as well (e.g. requires not a list, invalid queue specified...

Since the config comes from the broker, and the broker now checks the config, this is not really possible.

Just squashed the following changes

fix up a missed check in the broker queues validation
add some more tests based on your suggestions
tweak man page opening paragraph per suggestion
allow ^ in property names
eliminate support for spitting comma-separated properties in constraints plugin
rebase on current master

garlick · 2022-09-20T15:05:12Z

oops I need to try that again - I accidentally squashed the wrong pair of commits

grondo · 2022-09-20T15:46:03Z

Since the config comes from the broker, and the broker now checks the config, this is not really possible.

Ah, yes, then I suppose the code to check validity of the config in the plugin is actually dead code and can be removed?

codecov · 2022-09-20T15:55:04Z

Codecov Report

Merging #4587 (ad7525b) into master (5714d9d) will increase coverage by 0.02%.
The diff coverage is 94.64%.

❗ Current head ad7525b differs from pull request most recent head 02d6ab8. Consider uploading reports for the commit 02d6ab8 to get more accurate results

@@            Coverage Diff             @@
##           master    #4587      +/-   ##
==========================================
+ Coverage   83.35%   83.37%   +0.02%     
==========================================
  Files         409      410       +1     
  Lines       68548    68599      +51     
==========================================
+ Hits        57135    57196      +61     
+ Misses      11413    11403      -10

Impacted Files	Coverage Δ
...python/flux/job/frobnicator/plugins/constraints.py	`91.89% <91.89%> (ø)`
src/broker/brokercfg.c	`87.81% <100.00%> (+1.02%)`	⬆️
...gs/python/flux/job/frobnicator/plugins/defaults.py	`88.88% <0.00%> (-1.86%)`	⬇️
src/modules/cron/cron.c	`82.47% <0.00%> (-0.45%)`	⬇️
src/common/libterminus/terminus.c	`85.82% <0.00%> (-0.25%)`	⬇️
src/cmd/flux-job.c	`87.53% <0.00%> (+0.12%)`	⬆️
src/broker/overlay.c	`86.46% <0.00%> (+0.21%)`	⬆️
src/common/libflux/handle.c	`83.87% <0.00%> (+0.38%)`	⬆️
src/common/libsubprocess/local.c	`84.39% <0.00%> (+0.48%)`	⬆️
src/common/libsdprocess/sdprocess.c	`69.76% <0.00%> (+0.62%)`	⬆️
... and 2 more

grondo

LGTM!

Problem: broker policy/queues config validator does not allow 'requires' queues subtable, but this is defined in RFC 33. Allow the following, per RFC 33: queues.NAME.requires = [ string array ]. Add coverage to t2241-policy-config.t.

Problem: a malformed queues config such as queues=42 is allowed by the broker policy/queues config validator. Check that queues is a table. Add test to t2241-policy-config.t.

Problem: configuration can specify a queue-specific default queue, or an unknown default queue. Validate the [policy] default queue. Forbid [queues] from specifying a queue-specific default queue. Update t2241-policy-config.t. Drop config tests from t2112-job-ingest-frobnicator.t. Update the now invalid example in flux-config-policy(5). drop frobnicator test on missing queue config

Problem: RFC 33 specifies that a queue may define resource constraints to identify partitioned resources, but that is not currently supported by the frobnicator. Add `constraints` plugin.

Problem: frobnicator constraints plugin has no test coverage. Add tests to t2112-job-ingest-frobnicator.t to cover the plugin.

Problem: there is no manual page for the [queues] config. Add man page.

garlick · 2022-09-20T20:02:31Z

Ah, yes, then I suppose the code to check validity of the config in the plugin is actually dead code and can be removed?

Well, that prompted me to look through what was being checked and discover a couple of cases that were missed by the broker check (default queue not in [queues], queue policy contains default queue) and cover those. A few tests in t2112-job-ingest-frobnicator.t were replaced with tests in t2241-policy-config.t (which checks the broker config check).

I also discovered that the man page example didn't have the default queue in [queues] and fixed that (doh!)

However, the checks in the frobnicator plugins are pretty minimal and do serve as documentation of expectations, if nothing else (like an assertion). Maybe OK to leave them in?

grondo · 2022-09-20T20:22:30Z

doc/man5/flux-config-queues.rst

+CAVEATS
+=======
+
+Queue resources should not overlap.


Oh, I was going to ask about this caveat. Why can't queue resources overlap (is this a Fluxion constraint?).

Fluxion allows it, but some issues were identified in flux-framework/flux-sched#939.

Some use cases might work with Fluxion as is, like opening up a stopped queue to select users before a DAT, then when the DAT comes around, stop/drain the normal queues and start the DAT queue. That particular one would require the job manager to support multiple queues though.

Maybe there are other cases, but I thought it best to not advertise it at this point since the near term goal is partitioned resources.

Ah, thanks, I had forgotten about that. I do know we were talking about allowing a queue with access to all resources (and we do have a pall partition on many clusters), and this caveat prevents that option. I agree we can ignore it for now as long as the concept of non-overlapping resource sets isn't baked into the design.

garlick · 2022-09-20T21:44:42Z

Setting MWP, thanks!

grondo reviewed Sep 19, 2022

View reviewed changes

garlick force-pushed the queue_constraint branch from 908c496 to 085a063 Compare September 19, 2022 23:22

grondo approved these changes Sep 20, 2022

View reviewed changes

garlick force-pushed the queue_constraint branch from 085a063 to ad7525b Compare September 20, 2022 14:50

garlick force-pushed the queue_constraint branch from ad7525b to 02d6ab8 Compare September 20, 2022 15:08

garlick changed the title ~~WIP: ingest: set configured queue constraints~~ ingest: set configured queue constraints Sep 20, 2022

grondo approved these changes Sep 20, 2022

View reviewed changes

garlick and others added 6 commits September 20, 2022 12:57

broker: validate queues.NAME.requires

5f787bf

Problem: broker policy/queues config validator does not allow 'requires' queues subtable, but this is defined in RFC 33. Allow the following, per RFC 33: queues.NAME.requires = [ string array ]. Add coverage to t2241-policy-config.t.

broker: check type of [queues] table

78ed135

Problem: a malformed queues config such as queues=42 is allowed by the broker policy/queues config validator. Check that queues is a table. Add test to t2241-policy-config.t.

python: add constraints frobnicator plugin

b0aa547

Problem: RFC 33 specifies that a queue may define resource constraints to identify partitioned resources, but that is not currently supported by the frobnicator. Add `constraints` plugin.

testsuite: add coverage for constraints plugin

ca5607b

Problem: frobnicator constraints plugin has no test coverage. Add tests to t2112-job-ingest-frobnicator.t to cover the plugin.

flux-config-queues(5): add man page

279348b

Problem: there is no manual page for the [queues] config. Add man page.

garlick force-pushed the queue_constraint branch from 02d6ab8 to 279348b Compare September 20, 2022 19:58

grondo reviewed Sep 20, 2022

View reviewed changes

garlick added the merge-when-passing label Sep 20, 2022

mergify bot merged commit dd58143 into flux-framework:master Sep 20, 2022

garlick deleted the queue_constraint branch September 20, 2022 21:56

garlick mentioned this pull request Sep 22, 2022

add queue specific resource constraint/property for partitioned resources #4438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingest: set configured queue constraints #4587

ingest: set configured queue constraints #4587

garlick commented Sep 19, 2022 •

edited

Loading

grondo left a comment

grondo Sep 19, 2022 •

edited

Loading

grondo Sep 20, 2022

garlick commented Sep 19, 2022

grondo left a comment

grondo Sep 20, 2022

grondo Sep 20, 2022

garlick commented Sep 20, 2022

grondo commented Sep 20, 2022 •

edited

Loading

garlick commented Sep 20, 2022

garlick commented Sep 20, 2022

grondo commented Sep 20, 2022

codecov bot commented Sep 20, 2022

grondo left a comment

garlick commented Sep 20, 2022 •

edited

Loading

grondo Sep 20, 2022

garlick Sep 20, 2022

grondo Sep 20, 2022

garlick commented Sep 20, 2022

ingest: set configured queue constraints #4587

ingest: set configured queue constraints #4587

Conversation

garlick commented Sep 19, 2022 • edited Loading

grondo left a comment

Choose a reason for hiding this comment

grondo Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

grondo Sep 20, 2022

Choose a reason for hiding this comment

garlick commented Sep 19, 2022

grondo left a comment

Choose a reason for hiding this comment

grondo Sep 20, 2022

Choose a reason for hiding this comment

grondo Sep 20, 2022

Choose a reason for hiding this comment

garlick commented Sep 20, 2022

grondo commented Sep 20, 2022 • edited Loading

garlick commented Sep 20, 2022

garlick commented Sep 20, 2022

grondo commented Sep 20, 2022

codecov bot commented Sep 20, 2022

Codecov Report

grondo left a comment

Choose a reason for hiding this comment

garlick commented Sep 20, 2022 • edited Loading

grondo Sep 20, 2022

Choose a reason for hiding this comment

garlick Sep 20, 2022

Choose a reason for hiding this comment

grondo Sep 20, 2022

Choose a reason for hiding this comment

garlick commented Sep 20, 2022

garlick commented Sep 19, 2022 •

edited

Loading

grondo Sep 19, 2022 •

edited

Loading

grondo commented Sep 20, 2022 •

edited

Loading

garlick commented Sep 20, 2022 •

edited

Loading