ansible-hpc icon indicating copy to clipboard operation
ansible-hpc copied to clipboard

Fix/Improve class With

Open msekania opened this issue 2 years ago • 8 comments

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.

msekania avatar Aug 17 '23 17:08 msekania

@dylex,

thanks for the great ansible slurm.py plugin!

msekania avatar Aug 17 '23 17:08 msekania

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.

dylex avatar Aug 17 '23 18:08 dylex

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?

msekania avatar Aug 17 '23 18:08 msekania

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.

dylex avatar Aug 17 '23 18:08 dylex

And the similar statement holds for the rest With class parameters in ENTITIES?

msekania avatar Aug 17 '23 18:08 msekania

By the way it also works when defaultaccount is not specified but user with defaultaccount already exists!

msekania avatar Aug 17 '23 18:08 msekania

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 avatar Aug 17 '23 18:08 dylex

@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.

msekania avatar Aug 17 '23 18:08 msekania