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

t: add sharness test for multiple user jobs with different urgencies #236

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

cmoussa1
Copy link
Member

Background

This was opened in an issue a little while ago (#134), but it was noted that it might be useful to have a sharness test that simulated multiple users submitting jobs with different urgency values.


This is a [WIP] PR that adds a sharness test which has multiple fake users submit jobs with different urgency values. For the purpose of checking ordering of the job queue, I set the same fairshare value for all users and had them submit jobs to the same default queue, which would leave urgency as the one factor to affect a user's job priority.

Since I am using the submit_as.py script that @grondo added a while ago for this test, I think I noticed that the urgency option would not be set in the actual job submit command even if it was passed on the command line, so I made a small change in the script to check explicitly for an optional urgency argument, and if passed, add it to submitcmd. @grondo: I don't know if this is the best approach or change to make to your submit script, so if you have a better idea or solution for your script, I am completely open to any feedback you might have. Let me know if there is a better workaround for my proposed solution. :-)

Fixes #134

@cmoussa1 cmoussa1 added new feature new feature testing issues that deal with testing low priority items that can be worked on at a later date labels May 10, 2022
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

This LGTM! I made some notes on the submit_as.py script inline.

@@ -34,6 +34,8 @@ def main():
userid = int(sys.argv[1])
os.environ["FLUX_HANDLE_USERID"] = str(userid)

urgency = sys.argv[2] if "urgency" in sys.argv[2] else None
Copy link
Contributor

Choose a reason for hiding this comment

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

Since submit-as.py takes any flux-mini run option, it won't be guaranteed that --urgency will be the second option. Therefore, I think it might be better (i.e. less surprising) if sys.argv[2:] was iterated and any option matching --urgency was passed down to flux job submit.

Also, the code could be simplified if you just append to the submitcmd list if needed.

e.g. something like (untested):

diff --git a/t/scripts/submit_as.py b/t/scripts/submit_as.py
index 4b22c92..6bd45b9 100644
--- a/t/scripts/submit_as.py
+++ b/t/scripts/submit_as.py
@@ -34,7 +34,10 @@ def main():
     userid = int(sys.argv[1])
     os.environ["FLUX_HANDLE_USERID"] = str(userid)
 
-    urgency = sys.argv[2] if "urgency" in sys.argv[2] else None
+    submitcmd = [ "flux", "job", "submit", "--flags=signed" ]
+    for arg in sys.argv[2:]:
+        if "urgency" in arg:
+            submitcmd.append(arg)
 
     minicmd = [
         "flux",
@@ -48,10 +51,6 @@ def main():
 
     signedJ = SecurityContext().sign_wrap_as(userid, jobspec, mech_type="none")
 
-    if urgency is not None:
-        submitcmd = ["flux", "job", "submit", urgency, "--flags=signed"]
-    else:
-        submitcmd = ["flux", "job", "submit", "--flags=signed"]
     jobid = run_process(submitcmd, input=signedJ)
     print(jobid)

Good catch by the way!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @grondo for reviewing and for the suggested improvement! I didn't know you could just append the urgency argument to the end of submitcmd, so thank you for pointing that out. This solution is much cleaner and did result in passing tests, so I just force pushed a change that implemented this suggestion and rebased to catch up after some of the PR's that have landed before this one. I've also taken it out of [WIP].

Problem: The urgency attribute does not get passed to the priority plugin
when passed to minicmd.

Add a new arg to the submit_as.py helper script that checks to see if urgency
is passed on the command line; if it is, add it to submitcmd.
Add a new sharness test to the testsuite that has multiple users submit jobs
with different urgency values to ensure the priority for their jobs are
correctly adjusted.
@cmoussa1 cmoussa1 changed the title [WIP] t: add sharness test for multiple user jobs with different urgencies t: add sharness test for multiple user jobs with different urgencies Jun 9, 2022
@cmoussa1 cmoussa1 marked this pull request as ready for review June 9, 2022 19:57
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #236 (900bc8b) into master (351239a) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 900bc8b differs from pull request most recent head 2fc37ad. Consider uploading reports for the commit 2fc37ad to get more accurate results

@@           Coverage Diff           @@
##           master     #236   +/-   ##
=======================================
  Coverage   84.26%   84.27%           
=======================================
  Files          23       23           
  Lines        1176     1170    -6     
=======================================
- Hits          991      986    -5     
+ Misses        185      184    -1     
Impacted Files Coverage Δ
src/plugins/mf_priority.cpp 87.31% <0.00%> (+0.08%) ⬆️

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

LGTM!

@cmoussa1
Copy link
Member Author

Thanks! Setting MWP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low priority items that can be worked on at a later date merge-when-passing new feature new feature testing issues that deal with testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

t: add priority plugin tests for multiple users with different urgency values
2 participants