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

wreck: add support to set/get scheduler parameters at runtime #1579

Merged
merged 1 commit into from Jul 17, 2018

Conversation

Projects
None yet
6 participants
@tpatki
Copy link
Contributor

tpatki commented Jul 13, 2018

Support for sched/issue337.

Add flux wreck sched-params set/get options to support getting/setting runtime parameters in sched. flux wreck sched-params get, flux wreck sched-params get queue-depth, and `flux-wreck sched-params get queue-depth,delay-sched' should all work.

@tpatki tpatki requested review from grondo , dongahn and SteVwonder Jul 13, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 13, 2018

Coverage Status

Coverage increased (+0.03%) to 79.466% when pulling 20b189b on tpatki:issue337 into ce56834 on flux-framework:master.

@tpatki tpatki force-pushed the tpatki:issue337 branch from 99da183 to 72ed2ef Jul 14, 2018

@tpatki

This comment has been minimized.

Copy link
Contributor Author

tpatki commented Jul 14, 2018

@dongahn: Changed the commit history/msg here as well (as per your suggestion earlier).

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 14, 2018

Codecov Report

Merging #1579 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1579      +/-   ##
==========================================
+ Coverage   79.14%   79.17%   +0.02%     
==========================================
  Files         170      170              
  Lines       31182    31182              
==========================================
+ Hits        24679    24688       +9     
+ Misses       6503     6494       -9
Impacted Files Coverage Δ
src/common/libflux/attr.c 90% <0%> (-4%) ⬇️
src/broker/modservice.c 80.58% <0%> (-0.98%) ⬇️
src/common/libflux/message.c 81.02% <0%> (+0.23%) ⬆️
src/broker/module.c 84.07% <0%> (+0.27%) ⬆️
src/broker/overlay.c 74.13% <0%> (+0.31%) ⬆️
src/common/libutil/base64.c 95.77% <0%> (+0.7%) ⬆️
src/common/libflux/request.c 89.74% <0%> (+1.28%) ⬆️
src/modules/connector-local/local.c 74.39% <0%> (+1.62%) ⬆️
@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Jul 15, 2018

Thanks @tpatki for putting this together. Generally LGTM! (With the caveat that my Lua isn't that great). The only thing I noticed is the merge commit. I believe the normal pattern for flux-framework repos is to rebase to master rather than merge with master, but @garlick or @grondo would know better than me.

@grondo

grondo approved these changes Jul 16, 2018

Copy link
Contributor

grondo left a comment

This seems good, just a couple minor comments inline. I'll also try to do some testing against the pending flux-sched PR. Thanks for doing this @tpatki!


for k,v in pairs(resp) do
result = tostring(k).."="..tostring(v)
print(result)

This comment has been minimized.

@grondo

grondo Jul 16, 2018

Contributor

This would be extra work, but you could add support for flux wreck sched-params get ITEM here. As you iterate the response from the sched.params.get, check for key == ITEM and only print result if there is a match. As far as I'm concerned the current approach is fine for now, I only mean the comment as instructive.

end

for k,v in pairs(resp) do
result = tostring(k).."="..tostring(v)

This comment has been minimized.

@grondo

grondo Jul 16, 2018

Contributor

Not a big deal, but result should be declared local. Or better yet, just use print (k.."="..v), I think both k and v will be automatically promoted to strings, so the explicit use of tostring() is not needed.

end

local resp, err = f:rpc ("sched.params.set", { param = tostring(params) })
if not resp then

This comment has been minimized.

@grondo

grondo Jul 16, 2018

Contributor

A better protocol would be for the JSON payload here to be a dictionary, e.g. instead of

{ "param": "item=42" }

use

{ "item": 42 }

However, looking at the sched project, I see this is because you are using an existing function that parses "item=value" at module load time.

If you end up having to refactor a bit in sched, I would suggest moving the rpc to the more flexible dictionary for the payload. If it is functional, then the current approach seems good enough for now.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 16, 2018

Oh, yes, and please rebase onto master so we don't have the merge commit. Thanks!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jul 16, 2018

If you end up having to refactor a bit in sched, I would suggest moving the rpc to the more flexible dictionary for the payload.

Not to go down a rat hole on this, but if you go that route, it may be a good idea for protocol extensibility to push the diciionary into its own object so that other non-dictionary keys can be added later (for example, a flags parameter):

{ 
  "dict":{ 
    "item":42,
    "foo":"bar",
  },
  "flags":0,
}

but as @grondo said, what you have makes sense for now.

@tpatki tpatki force-pushed the tpatki:issue337 branch from ecbce14 to b2db6e6 Jul 16, 2018

@tpatki

This comment has been minimized.

Copy link
Contributor Author

tpatki commented Jul 16, 2018

@grondo: rebased, and made the change you requested for printing individual parameters. I'm working on refactoring the adjust_for_sched_params in sched.c (as Dong pointed out in the other PR, we need this for the delay-sched argument to to propagate correctly). Please test once I've submitted an updated PR for the same in sched (hopefully will finish it sometime today).

@grondo, @garlick: I will open another issue to refactor the sched end to support the dictionary payload you both suggested.

@tpatki tpatki force-pushed the tpatki:issue337 branch from b2db6e6 to 20b189b Jul 17, 2018

@tpatki

This comment has been minimized.

Copy link
Contributor Author

tpatki commented Jul 17, 2018

@dongahn, @SteVwonder, @grondo:
Addressed your earlier comments, and added a minor update that prints "true" instead of 1 for consistency with the sched side. This can now be tested with the PR submitted to sched.

@garlick garlick changed the title Issue337 wreck: add support to set/get scheduler parameters at runtime Jul 17, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 17, 2018

Thanks @tpatki! Will try to do a quick test early today, but looks good to me.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 17, 2018

Ok, did some quick testing and things look good!

One thing I noticed is that sched-params set allows queue-depth to be set to negative numbers. I actually have no idea if this is a valid value for sched, but if not some checks for valid values should be added. As far as I'm concerned that could wait for a future fix if you'd like this merged asap.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 17, 2018

e.g.:

(flux-69360) grondo@ipa15:~/git/flux-sched.git$ flux wreck sched-params set queue-depth=-1024
(flux-69360) grondo@ipa15:~/git/flux-sched.git$ flux wreck sched-params get
delay-sched=false
queue-depth=-1024

Jobs don't seem to run with a negative queue-depth.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 17, 2018

Actually the fixes will likely be on the sched side since this is just client code.

Merging.

@grondo grondo merged commit 711a8e5 into flux-framework:master Jul 17, 2018

4 checks passed

codecov/patch Coverage not affected when comparing ce56834...20b189b
Details
codecov/project 79.17% (+0.02%) compared to ce56834
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 79.466%
Details
@tpatki

This comment has been minimized.

Copy link
Contributor Author

tpatki commented Jul 17, 2018

@grondo: Thanks! Yes, we need some error checking around queue_depth, I'll open another issue for this.

@tpatki tpatki referenced this pull request Jul 17, 2018

Closed

queue_depth correctness #363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.