Issue337 #362

tpatki · 2018-07-13T22:25:51Z

Added request/reponse functionality to update scheduler parameters at runtime. Added a test case for the same. Needs the flux wreck sched-params get/set subcommand to be present in flux-core (separate PR).
Also fixes #364.

dongahn · 2018-07-13T22:41:48Z

@tpatki: great start!

It seems a few minor changes can help me review your PR. You may want to:

Put all the C code changes within sched.c into the first commit
Put all the tests into the second commit;
And remove intermediate commits (you may find git rebase's squash useful)

Also for commit messages. The convention has been:

Put a particular component name that the commit affects as "component:" in the commit title
Then, have a short summary. Keep it short so that the fully title will show up full in the PR with no elision. "sched: Add support to update sched params at runtime". "test: Add cases for ..."
Add some details in the body of the commit. Typically, one uses imperative like "Add request handers for..." instead of "This adds request handers for..." in here. But we can be flexible in the commit body message format.

dongahn · 2018-07-13T22:42:34Z

@tpatki: Sorry I just updated my comments above.

SteVwonder

Thanks @tpatki! Generally LGTM, just minor nits on the tests.

SteVwonder · 2018-07-13T22:32:41Z

t/t1008-runtime-sched-params.t

+ test_expect_success 'sched-params: getting sched params at runtime works' '
+    adjust_session_info 4 &&
+    flux hwloc reload ${excl_4N4B} &&
+    flux module load sched  sched-once=true &&


I recommend explicitly setting the delay-sched and queue-depth arguments on load so that if the defaults ever change, that doesn't affect this test.

@SteVwonder: Will do.

SteVwonder · 2018-07-13T22:34:53Z

t/t1008-runtime-sched-params.t

+    flux wreck sched-params set queue-depth=4 &&
+    timed_sync_wait_job 10 &&
+    verify_1N_nproc_sleep_jobs ${excl_4N4B_nc} 
+'


I recommend adding a flux wreck sched-params get queue-depth here to ensure that the value was actually set correctly. Right now, I believe this test just ensures that Flux doesn't crash when you call flux wreck sched-params get.

I think you can avoid writing to a file too by just using test flux wreck sched-params get queue-depth -eq 4

For now, I only added support for flux wreck sched-params get, which returns all the scheduler parameters.

I didn't add support for flux wreck sched-params get queue-depth or flux wreck sched-params get queue-depth,delay-sched. This would require me to introduce redundant code (similar to sched_params_args, but one that "gets" instead of "sets"), and I didn't want to do that. I think this is fine as a first cut, let me know if you think otherwise.

To support something like flux wreck sched-params get queue-depth,delay-sched, we will
(1) Either have to change sched_params_args to have a flag for set/get and have the latter return the sched_params_t object, which can then be packed and sent back to the requester, or,
(2)Add similiar parsing code in either the subcommand in wreck or the callback in sched.c

I was trying to avoid both of those things.

This would require me to introduce redundant code (similar to sched_params_args, but one that "gets" instead of "sets"), and I didn't want to do that. I think this is fine as a first cut, let me know if you think otherwise.

This makes sense. Thanks @tpatki!

Could you use flux wreck sched-params get | grep queue-depth to simulate a "get" of a single value? It would be nice to verify the set worked in the tests.

SteVwonder · 2018-07-13T22:41:47Z

sched/sched.c

+
+    flux_log (h, LOG_DEBUG, "return values of sched_params ");
+
+    msg2 = flux_event_encode ("sched.params.returned", NULL);


Initially, it seems like the only client that cares about a parameter query being answered is the client that made the original query. And that client is about to get an RPC response, so this event seems redundant.

Can you elaborate on the use-cases that you had in mind for this event?

I was a bit confused about this myself, and was following the convention used by the existing include/exclude/cancel callbacks.

This event (msg2) is used in the flux_send call that goes out to the broker, which is different than the flux_respond_pack which contains the data requested by the client. I wasn't sure why the other callbacks had it, and if the response to the client was different than the information sent out to the broker. Maybe @dongahn or @grondo have some clarity here?

In the case of exclude and include request, an extra event needs to be generated so that the scheduler will get an resource event and invoke a scheduler loop. For this particular case, we probably won' t need an event. Sorry if it wasn't obvious in the code. (Should have had a comment.)

Ah, that makes sense. Thanks Dong! Yes it wasn't clear to me, and didn't want to break anything or cause a race condition. I'll remove the event/flux_send from both the callbacks I added (and add a comment to capture the above discussion as well).

tpatki · 2018-07-13T23:02:15Z

@dongahn: Shall fix the PR per your suggestions soon (need to head out for a bit but will revisit later today evening).

dongahn · 2018-07-13T23:04:11Z

Thanks @tpatki

coveralls · 2018-07-13T23:05:34Z

Coverage decreased (-0.01%) to 75.408% when pulling 58de2de on tpatki:issue337 into 371ae9d on flux-framework:master.

tpatki · 2018-07-13T23:06:42Z

@dongahn, @SteVwonder: I submitted a separate PR to flux-core for the new flux wreck sched-params subcommand, please take a look at that as well.

codecov-io · 2018-07-14T05:00:08Z

Codecov Report

Merging #362 into master will decrease coverage by <.01%.
The diff coverage is 77.19%.

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
- Coverage   73.96%   73.96%   -0.01%     
==========================================
  Files          61       61              
  Lines       10211    10263      +52     
==========================================
+ Hits         7553     7591      +38     
- Misses       2658     2672      +14

Impacted Files	Coverage Δ
sched/sched.c	`73.4% <77.19%> (-0.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 371ae9d...58de2de. Read the comment docs.

tpatki · 2018-07-14T05:02:04Z

@dongahn: updated as per your suggestion. Let me know if I missed anything. Thanks!

dongahn · 2018-07-14T05:28:51Z

sched/sched.c

+
+    if (sprms)
+        rc = sched_params_args (sprms, &(ctx->arg.s_params));
+


I don't think calling sched_params_args () alone will lead to scheduling behavior changes for all parameters. queue-depth would be okay. But delay-sched would not. If the sched load option was delay-sched=false, and the user executes flux wreck sched-param set delay-sched=true to change it. This alone will not change the scheduler to start delay scheduling.

Generally speaking, as we will add more parameters, we can't expect only changing a field in the sched context will effect the requested behavior.

I think this can be addressed if we refactor adjust_for_sched_params () into effect_sched_params () Here, you can have a logic to dynamically adjust the behavior based on the given scheduling parameters.

I have to believe you can make it so that the function will work for both load time options as well as dynamically requested options.

Noted, I will work on fixing this and also on testing if the change took effect. I was thinking about this as well, thanks for the pointers!

dongahn · 2018-07-14T05:29:56Z

sched/sched.c

+    if (flux_msg_get_userid (msg, &userid) < 0)
+        goto error;
+
+    flux_log (h, LOG_INFO, "sched_params change requested by user at runtime (%u).", userid);


Minor nit: Maybe (%u) can be next to user?

dongahn · 2018-07-14T05:31:49Z

sched/sched.c

+        goto error;
+
+    flux_log (h, LOG_INFO, "sched_param values requested by user (%u).", userid);
+    if (flux_request_unpack (msg, NULL, "{}") < 0)


Did you want to have bulk get AND individual get?

Only bulk get for now, lets discuss individual get in person. Take a look at the comment I added to Stephen's question above regarding that. I'm not sure how to avoid duplicate code there.

Ah. OK. Yeah I'm ok with bulk get only since this will have be refactored later anyway within flux sched command. Thanks @tpatki.

We should also think about how to refactor sched_params_args so we don't write duplicate code for the individual get. I'll add an issue corresponding to this once this PR is done.

@dongahn, @SteVwonder: PRs now support individual get as well. Please see test cases.

dongahn · 2018-07-14T05:33:07Z

t/t1008-runtime-sched-params.t

+#set -x
+
+test_description='Test dynamic updation of queue depth. 
+First cut. Ensure jobs are correctly scheduled under different values of


Remove "First cut."

Very minor nit. I don't think you need "Frist cut" here.

dongahn · 2018-07-14T05:37:40Z

t/t1008-runtime-sched-params.t

+    timed_sync_wait_job 10 &&
+    verify_1N_nproc_sleep_jobs ${excl_4N4B_nc} 
+'
+test_expect_success 'sched-params: unloaded sched module' '


Great you have added many tests! You think it will be possible to test whether "set" has led to scheduler behavior changes as well?

dongahn · 2018-07-14T05:38:37Z

@tpatki: Thank you for putting this together so quickly. I have just added some comments inline.

tpatki · 2018-07-17T08:42:45Z

@dongahn, @SteVwonder, @grondo:

Please see the modified PR that addresses the changes requested. I have added a few test cases, and have manually verified each of the cases for adjust_for_sched_params through logging/stepping, but I can't ensure the correctness of delay_sched (I can only match existing workflow). If you can think of better test cases, please let me know.

SteVwonder · 2018-07-17T15:09:56Z

LGTM. It just needs a rebase to master. Thanks, @tpatki! This was more work than I thought it would be, but this should come in handy.

tpatki · 2018-07-17T16:58:38Z

@SteVwonder: rebased.

dongahn · 2018-07-17T17:40:08Z

@tpatki and I looked at the code together a bit after our meeting this morning. There will be some minor changes.

tpatki · 2018-07-17T18:19:41Z

@dongahn: updated as per our discussion.

dongahn · 2018-07-17T18:43:15Z

sched/sched.c

+
+        /* If one of the watchers is not available, send error */
+        if ((!ctx->before && ctx->after) || (!ctx->after && ctx->before)) {
+            rc = EINVAL;


Thank you for following up. I would actually go with:

rc = -1; errno = EINVAL;

dongahn · 2018-07-17T18:44:00Z

sched/sched.c

-    }
+        /* If one of the watchers is not available, send error */
+        if ((!ctx->before && ctx->after) || (!ctx->after && ctx->before)) {
+            rc = EINVAL;


Thank you for following up. I would actually go with:

rc = -1; errno = EINVAL;

dongahn · 2018-07-17T18:46:14Z

sched/sched.c

+    if (flux_msg_get_userid (msg, &userid) < 0)
+        goto error;
+
+    flux_log (h, LOG_INFO, "sched_params change requested by user (%u) at runtime.", userid);


This line seems long. Does it fit 80 characters? If not, I'd suggest to break the line into two.

dongahn · 2018-07-17T18:52:05Z

sched/sched.c

+    if (flux_msg_get_userid (msg, &userid) < 0)
+        goto error;
+
+    flux_log (h, LOG_INFO, "sched_param values requested by user (%u).", userid);


Same comment as above.

tpatki · 2018-07-17T18:54:58Z

@dongahn, @SteVwonder: Looks like my testcase fails with certain gcc/clang versions, not sure why. Will debug further in the evening/tmrw. Any thoughts on why this may be happening?

dongahn · 2018-07-17T18:53:18Z

t/t1008-runtime-sched-params.t

+#set -x
+
+test_description='Test dynamic updation of queue depth. 
+First cut. Ensure jobs are correctly scheduled under different values of


Very minor nit. I don't think you need "Frist cut" here.

dongahn · 2018-07-17T18:55:15Z

t/t1008-runtime-sched-params.t

+    echo ${excl_4N4B_nc}
+'
+
+ test_expect_success 'sched-params: getting sched params (no args) at runtime works' '


Any way we can try to shorten the test messages here and below to fit into 80 characters. I don't think it is required but it would be generally good for readability.

dongahn · 2018-07-17T18:56:41Z

Looks like my testcase fails with certain gcc/clang versions, not sure why. Will debug further in the evening/tmrw.

Sounds good. I also added some minor comments above. I believe we are close. Thanks!

dongahn · 2018-07-17T21:06:03Z

We use `errno` for that purpose. Hopefully it answers your question. http://man7.org/linux/man-pages/man3/errno.3.html Dong

…

________________________________ From: Tapasya Patki <notifications@github.com> Sent: Tuesday, July 17, 2018 12:08:19 PM To: flux-framework/flux-sched Cc: Ahn, Dong H.; Mention Subject: Re: [flux-framework/flux-sched] Issue337 (#362) @dongahn<https://github.com/dongahn>: I think some of the tests might be failing because rc isn't explicitly set to -1. Regarding, error=EINVAL, should I be setting that to a global error variable somewhere? I don't see one in sched (setting it locally doesn't make sense as it won't propagate through the callstack). Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#362 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AA0nqxaU4E2edrdSZptEQyllNdU0eiEyks5uHjYjgaJpZM4VPlQh>.

tpatki · 2018-07-18T03:26:55Z

@dongahn, @SteVwonder, @grondo:

I think this is ready to go, please let me know if I missed anything. It also fixes #364, I had to add "-libevent-dev" to .travis.yml in sched after the pmix update in flux-core.

dongahn · 2018-07-18T16:29:50Z

@tpatki: This looks great. Merging.

tpatki requested review from SteVwonder, dongahn and grondo July 13, 2018 22:26

SteVwonder reviewed Jul 13, 2018

View reviewed changes

dongahn requested changes Jul 14, 2018

View reviewed changes

SteVwonder approved these changes Jul 17, 2018

View reviewed changes

dongahn requested changes Jul 17, 2018

View reviewed changes

dongahn reviewed Jul 17, 2018

View reviewed changes

sched: add request/response to update sched params at runtime

19e0e7f

tpatki added 2 commits July 17, 2018 15:50

test: check get/set of sched parameters at runtime

465a0c0

travis: add libevent-dev for pmix

58de2de

dongahn merged commit e90be79 into flux-framework:master Jul 18, 2018

dongahn mentioned this pull request Jul 18, 2018

Need a runtime method to change scheduling parameters #337

Closed


		flux_log (h, LOG_DEBUG, "return values of sched_params ");

		msg2 = flux_event_encode ("sched.params.returned", NULL);


		if (sprms)
		rc = sched_params_args (sprms, &(ctx->arg.s_params));

Issue337 #362

Issue337 #362

Conversation

tpatki commented Jul 13, 2018 • edited

dongahn commented Jul 13, 2018 • edited

dongahn commented Jul 13, 2018

SteVwonder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpatki Jul 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpatki commented Jul 13, 2018

dongahn commented Jul 13, 2018

coveralls commented Jul 13, 2018 • edited

tpatki commented Jul 13, 2018 • edited

codecov-io commented Jul 14, 2018 • edited

Codecov Report

tpatki commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongahn commented Jul 14, 2018

tpatki commented Jul 17, 2018

SteVwonder commented Jul 17, 2018

tpatki commented Jul 17, 2018

dongahn commented Jul 17, 2018

tpatki commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpatki commented Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongahn commented Jul 17, 2018

dongahn commented Jul 17, 2018 via email

tpatki commented Jul 18, 2018

dongahn commented Jul 18, 2018

tpatki commented Jul 13, 2018 •

edited

dongahn commented Jul 13, 2018 •

edited

tpatki Jul 13, 2018 •

edited

coveralls commented Jul 13, 2018 •

edited

tpatki commented Jul 13, 2018 •

edited

codecov-io commented Jul 14, 2018 •

edited

tpatki commented Jul 17, 2018 •

edited