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

get all available accounts for a user from the scheduler #783

Merged
merged 12 commits into from
Jan 18, 2023
Merged

Conversation

johrstrom
Copy link
Contributor

@johrstrom johrstrom commented Dec 5, 2022

Fixes upstream ood issue below. The idea here is that the scheduler (Slurm in this case) knows all the valid accounts that a user is able to use. So instead of pulling accounts from Unix file systems, we can query the scheduler itself. A Unix group may or may not be a valid account as the account can suspended/closed due to budgets but the Unix group still exists.

OSC/ondemand#1970

┆Issue is synchronized with this Asana task by Unito

Copy link
Contributor

@treydock treydock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments for requested changes

Oglopf
Oglopf previously approved these changes Dec 12, 2022
Copy link
Contributor

@Oglopf Oglopf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@johrstrom
Copy link
Contributor Author

I don't believe this will work as is. We're likely going to need at least qos associated with the account. So passing just strings back is not likely enough.

With some sites, partition will show up in this output. On other sites, I believe we'll need another query to stitch together with this data.

sacctmgr show association 'format=account,partition,qos%100,' where "user=$(whoami)"

@treydock
Copy link
Contributor

If you use --parsable or --parsable2 flags you don't need format length like %100. Just FYI as that would simplify the logic.

As far as whether this works, I think it's a good starting place. Having a list of available accounts irregardless of partition is good. However I don't think you will need to query QOS since an association in the database is either "cluster + account + user" or "cluster + account + user + partition". The QOS is an attribute on the association. I think it's a bridge too far for OOD to try and work out how a QOS is used because a QOS can be used to limit resources or just do things like set priority or preemption. A QOS is not typically how you grant someone access to resources, that's usually done through the association in one of the two defining ways previously mentioned.

@treydock treydock self-requested a review January 11, 2023 12:52
treydock
treydock previously approved these changes Jan 11, 2023
@johrstrom
Copy link
Contributor Author

johrstrom commented Jan 11, 2023

Thanks for the reivew, I've added a bit. Namely the AccountInfo class so it can hold extra information for all the use cases we need.

Check out this Utah sacctmgr output. Some accounts only exists on 1 cluster (smithp-guest on ahs or dtn on notchpeak). So for this to be useful every account has to be sort of cluster aware so that upper layers can hide/show options as needed.

[u6031686@kingspeak1:~]$ sacctmgr -np show user withassoc format=account,qos,cluster where user=$(whoami)
smithp-guest|ash-guest,ash-guest-res|ash|
chpcservice|kingspeak|kingspeak|
owner-guest|kingspeak-guest|kingspeak|
chpcservice|lonepeak|lonepeak|
owner-guest|lonepeak-guest|lonepeak|
dtn|notchpeak-dtn|notchpeak|
notchpeak-shared-short|notchpeak-shared-short|notchpeak|
chpcservice|notchpeak-freecycle|notchpeak|
owner-guest|notchpeak-guest|notchpeak|

I'm quite sure this is not enough as the upper layers (OOD) is going to have to stitch a lot of information together. But as an initial luanch maybe it can just pull adapter.accounts.map(&:to_s) initially.

In any case, I know for sure these are going to be more complex than just a string, so we may as well start with objects.

@@ -325,7 +342,7 @@ def call(cmd, *args, env: {}, stdin: "")
cmd = OodCore::Job::Adapters::Helper.bin_path(cmd, bin, bin_overrides)

args = args.map(&:to_s)
args.concat ["-M", cluster] if cluster
args.concat ["-M", cluster] if cluster && cmd != 'sacctmgr'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I can't seem to get sacctmgr to query per cluster, which is going to be troublesome for upper layers as each adapter object is meant to represent 1 cluster, but here we are returning information for all clusters.

@johrstrom johrstrom dismissed stale reviews from treydock and Oglopf via 8b72830 January 12, 2023 17:58
@johrstrom johrstrom mentioned this pull request Jan 12, 2023
@johrstrom johrstrom merged commit 005ca58 into master Jan 18, 2023
@johrstrom johrstrom deleted the accounts branch January 18, 2023 16:09
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

3 participants