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

libutil: add fsd_parse_duration(), support standard duration string in some utilities #2095

Merged
merged 11 commits into from Mar 22, 2019

Conversation

@grondo
Copy link
Contributor

commented Mar 22, 2019

This PR adds a libutil function to parse the proposed RFC 23 Flux Standard Duration, and uses that function to add anoptparse_get_duration() call. Several commands using optparse_get_double() were update to optparse_get_duration() to allow them to take duration strings with s,m,h,d suffixes.

At the same time, all uses of jobspec duration were updated to take duration as a strict float number of seconds as proposed in flux-framework/rfc#177. Unfortunately, this had to go in as one big commit since everything depends on everything else.

In retrospect perhaps the jobspec duration should be a separate PR, but first people could comment on the change. It is best to get this done now, since making the change in the future will likely be more work.

@grondo grondo force-pushed the grondo:fsd branch from f36da86 to 0277ab4 Mar 22, 2019
@grondo grondo changed the title convert some utilities to take RFC 23 duration, make jobspec duration number of seconds libutil: add fsd_parse_duration(), support standard duration string in some utilities Mar 22, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Actually it was pretty messy to include the change of jobspec attributes.system.duration to a number in this PR. I've moved that change to a separate PR, this one now just adds a standard function for parsing (proposed) rfc 23 duration strings.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 22, 2019

Codecov Report

Merging #2095 into master will decrease coverage by <.01%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master    #2095      +/-   ##
==========================================
- Coverage   80.28%   80.27%   -0.01%     
==========================================
  Files         195      196       +1     
  Lines       31291    31318      +27     
==========================================
+ Hits        25121    25140      +19     
- Misses       6170     6178       +8
Impacted Files Coverage Δ
src/cmd/flux-ping.c 86.95% <100%> (ø) ⬆️
src/cmd/builtin/hwloc.c 81.42% <100%> (ø) ⬆️
src/modules/job-exec/job-exec.c 73.2% <100%> (-0.62%) ⬇️
src/cmd/flux-start.c 78.21% <100%> (ø) ⬆️
src/cmd/flux-job.c 85.54% <100%> (ø) ⬆️
src/cmd/flux-aggregate.c 77.77% <100%> (ø) ⬆️
src/common/libutil/fsd.c 100% <100%> (ø)
src/common/liboptparse/optparse.c 90.72% <88.88%> (-0.03%) ⬇️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/common/libflux/ev_flux.c 97.56% <0%> (-2.44%) ⬇️
... and 8 more
@chu11

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

lgtm, just to make sure, you didn't forget these in the broker? Or did you think they shouldn't be adjusted? Obviously not as user facing like the other stuff.

" -H,--heartrate SECS          Set heartrate in seconds (rank 0 only)\n"                                                                                                                                           
" -g,--shutdown-grace SECS     Set shutdown grace period in seconds\n"   
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

just to make sure, you didn't forget these in the broker? Or did you think they shouldn't be adjusted?

I only quickly searched for users of optparse_get_double() that related to duration. Since the broker uses getopt_long() I missed those (and they can't use optparse_get_duration())

Looks like "heartrate" currently takes only an integer with optional "ms" suffix, so maybe that could be adjusted later.

shutdown-grace is a good candidate though because change to fsd_parse_duration will only expand the accepted arguments, not change it. I'll ad that here.

Thanks!

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

This is a nice cleanup IMHO, and as discussed just now, the "ms" suffix on the heartrate is no big deal. There is one test that sets a fractional heartbeat, and it just uses:

t0001-basic.t:	flux start ${ARGS} -o,--heartrate=0.1 /bin/true
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Ok, added updates to switch broker --heartrate and --shutdown-grace to fsd, as well as the hello.timeout and init.rc2_timeout broker attributes.

Also switch cron's sync_epsilon parameter to duration string.

grondo added 11 commits Mar 21, 2019
Add a trivial function for converting a string from Flux Standard
Duration format to double precision seconds.

A function to convert double seconds to human readable FSD is provided
for convenience for tools that report these numbers.
Add a function to process and return an option argument in
Flux Standard Duration (floating point seconds with optional
s,m,h,d suffix).
Add tests for optparse_get_duration operation.
Standardize some commands that take a duration in floating point
seconds to use optparse_get_duration(), allowing a suffix
of 's,m,h,d' to be used.
Update job-exec module to use shared fsd_parse_duration() instead
of internal function for parsing standard duration strings.
Update the broker's --shutdown-grace option to take a standard
duration argument, allowing an optional 's,m,h,d' suffix.
Switch --heartrate option to take a duration string parsed
by fs_parse_duration(). Update test for 'ms' suffix to specify
in floating point seconds instead.
Use fsd_parse_duration () to process "hello.timeout" broker
attribute so that a standard duration string may be used.
Process the init.rc2_timeout broker attribute using fsd_parse_duration,
to allow a standard duration string to be used here.
Process the sync_epsilon cron module parameter with fsd_parse_duration
so that a standard duration string may be used.
@grondo grondo force-pushed the grondo:fsd branch from 501618a to 31695ee Mar 22, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

rebased on master

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Looks like you turned over another rock and more strtod's scampered out! Nice. I'm for merging once travis turns green.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 22, 2019

Codecov Report

Merging #2095 into master will increase coverage by 0.01%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #2095      +/-   ##
==========================================
+ Coverage   80.29%   80.31%   +0.01%     
==========================================
  Files         195      196       +1     
  Lines       31291    31311      +20     
==========================================
+ Hits        25125    25147      +22     
+ Misses       6166     6164       -2
Impacted Files Coverage Δ
src/broker/heartbeat.c 85.29% <100%> (-0.82%) ⬇️
src/broker/broker.c 77.94% <100%> (+0.01%) ⬆️
src/cmd/builtin/hwloc.c 81.42% <100%> (ø) ⬆️
src/cmd/flux-ping.c 86.95% <100%> (ø) ⬆️
src/modules/job-exec/job-exec.c 73.2% <100%> (-0.62%) ⬇️
src/modules/cron/cron.c 79.81% <100%> (-0.1%) ⬇️
src/cmd/flux-start.c 78.21% <100%> (ø) ⬆️
src/cmd/flux-job.c 85.54% <100%> (ø) ⬆️
src/cmd/flux-aggregate.c 77.77% <100%> (ø) ⬆️
src/common/libutil/fsd.c 100% <100%> (ø)
... and 7 more
@chu11

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

lgtm too, merging since @garlick approved too

@chu11 chu11 merged commit 4e01f75 into flux-framework:master Mar 22, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 89.47% of diff hit (target 80.29%)
Details
codecov/project 80.31% (+0.01%) compared to 31fbac6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Thanks!

@grondo grondo deleted the grondo:fsd branch Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.