priority plugin: add support for unpacking priorities for queues in TOML config #541
Conversation
cmoussa1
added
new feature
src/plugins/mf_priority.cpp
Outdated
| Queue new_queue; | ||
| new_queue.min_nodes_per_job = 1; | ||
| new_queue.max_nodes_per_job = 1; | ||
| new_queue.max_time_per_job = 60; | ||
| new_queue.priority = priority; | ||
| queues[queue_name] = new_queue; |
There was a problem hiding this comment.
Can you remind me if it is an error if a queue exists in the Flux broker config, but does not exist in the flux-accounting database?
I'm wondering if these defaults make sense if, say, an admin wanted to configure a temporary queue with a high priority. The defaults of max nodes (1) and max time (60m) would not necessarily make sense in this case...
There was a problem hiding this comment.
Good question. I don't think it is an error - I think the plugin will just return UNKNOWN_QUEUE, meaning it can't find any information about the queue, so it will just apply a priority of 0 for that queue. Perhaps I should just leave out the initialization of max nodes and max time when adding this queue to the map?
There was a problem hiding this comment.
That's what I was thinking (just leave out initialization of maximums here). However, we should ask @kkier and @ryanday36 if there is a use case for adding queue priorities for queues not in the accounting db.
There was a problem hiding this comment.
Cool. FWIW, those maximums are not currently enforced anywhere in the plugin (yet). But probably good to get @ryanday36 and @kkier's thoughts on it too.
There was a problem hiding this comment.
I can confirm that it is not an error for queues to exist in the broker config but not the accounting database. I've generally only added queues to the accounting database when I have a specific need to do so (i.e. I need to limit access to the queue to specific users). The use case that I can see for queue priorities without the accounting db would be pre-production systems where we're not otherwise worried about banks / fairshare / per-user limits / etc. It might be useful to be able to configure what the accounting plugin is enforcing a bit more (e.g. enforce queue limits, but don't require banks or calculate fairshare). It's pretty easy to set up a dummy-ish bank (guests) and add everyone to it though, which is what we're usually actually doing.
Problem: The priority plugin cannot unpack any defined integer priorities for queues in a configuration file, but there is at least one use case to be able to set these priorities in a TOML config file. Add an unpack of a "accounting.queue-priorities" table in the callback for conf.update. The table will have any queues and their associated integer priorities. If the queue already exists in the plugin's internal queues map, update the associated priority of the queue. If it does not exist, add the queue to the plugin's internal map.
Problem: The test suite does not contain any tests for configuring queues in a TOML config file and ensuring a job's priority is affected by the new configuration. Add some tests that go through changing the priority for a queue and ensuring that a job's priority submitted to that queue will change depending on the configuration.
Problem: The man page for flux-config-accounting is out-of-date and needs to be updated to include how to define priorities for queues to be read and used by the priority plugin. Add a section to the man page explaining how to set integer priorities for queues in a TOML configuration file.
|
Just force-pushed this PR to get rid of the initialization of the limits for a queue. So, if a queue is defined in a TOML config file that flux-accounting does not know about, the only thing it is adding is its associated integer priority. |
Problem
The priority plugin cannot unpack any defined integer priorities for queues in a configuration file, but it was brought up in an offline conversation that it would be nice to be able to set these priorities in a TOML config file.
This PR adds an unpack of an
accounting.queue-prioritiestable in the callback forconf.updatein the priority plugin. The table can have any queues and their associated integer priorities. If the queue already exists in the plugin's internalqueuesmap, it will update the associated priority of the queue. If the queue does not exist, it will add the queue to the plugin's internalqueuesmap.I've added some basic tests that go through defining and changing a priority for a specific queue in a TOML file as well as updated the
flux-config-accountingman page to explain how to set up this table.