Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/Improve class With #2

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix/Improve class With #2

wants to merge 1 commit into from

Conversation

msekania
Copy link

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
Copy link
Author

@dylex,

thanks for the great ansible slurm.py plugin!

@dylex
Copy link
Owner

dylex commented Aug 17, 2023

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.

@msekania
Copy link
Author

msekania commented Aug 17, 2023

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?

@dylex
Copy link
Owner

dylex commented Aug 17, 2023

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.

@msekania
Copy link
Author

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

@msekania
Copy link
Author

msekania commented Aug 17, 2023

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

@dylex
Copy link
Owner

dylex commented Aug 17, 2023

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.

@msekania
Copy link
Author

@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 msekania marked this pull request as draft August 18, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants