ansible-hpc
ansible-hpc copied to clipboard
Fix/Improve class With
Current behavior:
One should explicitly provide the second element of With class in order to be able to use any of following ones. Kind of the name of the second element is hard coded.
Improved behavior:
Does not matter any more which of the following elements are provided.
@dylex,
thanks for the great ansible slurm.py plugin!
Do you have an example use case for when this is useful? I think this could be simplified a bit, and maybe things have changed slightly, but sacctmgr doesn't let you specify, say, user=foo cluster=bar without account.
Some simplified versions: for example the following works:
- name: "Ensure that user exists"
slurm:
entity: user
state: present
name: "root"
args:
defaultaccount: "foo"
account: "test"
cluster: "bar"
run_once: true # noqa run-once[task]
This not any more
- name: "Ensure that user exists"
slurm:
entity: user
state: present
name: "root"
args:
defaultaccount: "foo"
cluster: "bar"
run_once: true # noqa run-once[task]
in modified version both work.
or do I miss something here?
Ah, hm, it seems like sacctmgr will infer the account from defaultaccount. If you don't specify defaultaccount, this gives an error Need name of account to add user to. So I don't think this fix is quite right. Instead it should allow defaultaccount to substitute for account, I guess. Though I'd argue it's probably safer to just explicitly specify account, because it's going to infer it anyway.
And the similar statement holds for the rest With class parameters in ENTITIES?
By the way it also works when defaultaccount is not specified but user with defaultaccount already exists!
That is the idea. These were originally derived from the sacctmgr argument parsing source, but from a very old slurm version so some things may be out of date. But in general, yes, sacctmgr only allows you to specify association parameters when you specify a specific association. There are cases where it will infer or just apply to all associations, but you probably don't want you configuration management system changing things without being specific about what they are.
@dylex,
I think that, fixing the second element name explicitly does not match any more to the current sacctmgr behavior. It also does not enforce that all "key" association parameters like user, cluster, partition, and account are all specified.
One should also consider what the ansible check mode reports.
Honestly, I am not pretending that my solution is better, but currently either I should restrict everything explicitly or accept that default values are also there.