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

flux-version: show broker, library, and command version, and add API version access #1817

Merged
merged 8 commits into from Nov 9, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Nov 8, 2018

This adds a new broker attribute version which reports the broker's compiled in version, e.g.

$ flux getattr version
0.10.0-469-g0605e1a0

Then it adds a --remote option to flux version so that, in addition to reporting its compiled in version, it can fetch the broker's version if desired:

$ flux version --remote
flux-core-0.10.0-469-g0605e1a0

We mused in #1810 that this might come in handy as our flux deployments get more complicated, and it was super simple to add, so here it is.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 8, 2018

Codecov Report

Merging #1817 into master will decrease coverage by <.01%.
The diff coverage is 88%.

@@            Coverage Diff            @@
##           master   #1817      +/-   ##
=========================================
- Coverage   79.71%   79.7%   -0.01%     
=========================================
  Files         188     189       +1     
  Lines       34587   34611      +24     
=========================================
+ Hits        27570   27587      +17     
- Misses       7017    7024       +7
Impacted Files Coverage Δ
src/common/libflux/version.c 100% <100%> (ø)
src/cmd/flux-start.c 75.6% <100%> (ø) ⬆️
src/broker/broker.c 76.97% <50%> (-0.04%) ⬇️
src/cmd/builtin/version.c 88.23% <83.33%> (-11.77%) ⬇️
src/modules/connector-local/local.c 73.62% <0%> (-1.19%) ⬇️
src/modules/kvs/kvs.c 65.73% <0%> (-0.16%) ⬇️
src/cmd/flux-event.c 77.97% <0%> (ø) ⬆️
src/common/libflux/message.c 81.64% <0%> (+0.24%) ⬆️
... and 2 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 8, 2018

Cool!

I think the --remote option could be confusing (especially if you are connected via a URI explicitly called local://), though I don't have a better idea (maybe --parent?)

My thought was that users may not know to run two separate commands in case the enclosing instance is a different version than the local flux command. What if flux version just looks for FLUX_URI set in environment and queries the "remote" version automatically, and reports both versions like:

commands: flux-core-x.y.z
broker: flux-core-x.y.z

It might also be useful for flux version to eventually report the version of flux-security to which it is linked, which might lend itself nicely to this extensible format or something similar.

We also have the open request for a --verbose flag in #1632. I'm not sure the best way to incorporate that into an expanded flux version output format.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 8, 2018

What if it queried the broker by default, but if FLUX_URI is not set, it issues an error message like

flux-version: no Flux instance to connect to, try --command-version

And also, if it succeeds in getting the broker version, but the command version is different, it could issue a warning on stderr before printing the broker version e.g.

flux-version: warning: command version flux-core-0.10.0 differs from Flux instance version
flux-core-0.10.0-469-g0605e1a0

That way there's always just a version on stdout, which might have scripting advantages over a more complicated output.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 8, 2018

That way there's always just a version on stdout, which might have scripting advantages over a more complicated output.

Yeah, I was thinking about machine readable output as well, but then I realized that maybe there is a better way to get the various versions if you need it in a script, and that the basic flux version output should be as helpful as possible for users, e.g.

Version of flux commands:
flux-core-x.y.z
Version of flux instance at URI=local:///tmp/flux-ftv4mnM:
flux-core-x.y.z +security

For machine readable output you could add a separate command or option. Since scriptable output would be run via scripts, it might make sense to make that invocation require the extra option, instead of the other way around.

BTW, for an example of a helpful, very non-machine-readable version output, try vim --version ;-)

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 8, 2018

Those are good points!

Uh, we are getting the command version from a preprocessor symbol set in config.h. What if we are dynamically linking against a different flux library? Do we need a way to fetch that version as well?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 8, 2018

Maybe the ideal output would be:

libflux-core: version
libflux-security: version
commands: version
broker at (uri): version
@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 8, 2018

It might also be useful for flux version to eventually report the version of flux-security to which it is linked, which might lend itself nicely to this extensible format or something similar.

Looking even farther out, versions of loaded modules in the broker? i.e. what version of flux-sched is loaded?

@garlick garlick referenced this pull request Nov 8, 2018

Closed

API access to flux version #1819

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 8, 2018

Looking even farther out, versions of loaded modules in the broker? i.e. what version of flux-sched is loaded?

Excellent point. Maybe that should be an addition to flux module list with some well known (RFC) interface for obtaining the version of a loaded module?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 8, 2018

Excellent point. Maybe that should be an addition to flux module list with some well known (RFC) interface for obtaining the version of a loaded module?

Yes, I like that idea, and module list protocol needs a rework anyway.

garlick added some commits Nov 8, 2018

libflux/version: add version accessors
Add <flux/core/version.h> which defines the following
macros for accessing the flux-core source version

FLUX_CORE_VERSION_STRING // string, e.g. "0.10.0"

FLUX_CORE_VERSION_MAJOR	 // numeric e.g. 0
FLUX_CORE_VERSION_MINOR	 // numeric e.g. 10
FLUX_CORE_VERSION_PATCH  // numeric e.g. 0
FLUX_CORE_VERSION_HEX	 // 3 byte numeric, e.g. 160 (0x0A0)

and the following functions which return:

flux_core_version_string ()  // string value above
flux_core_version ()         // all four numeric values above

The version.h header is generated from version.h.in
at configure time.  Also add version.h to .gitignore.
build: add ax_split_version.m4
Add ax_split_version.m4 from current master of
the autoconf archive:

https://github.com/autoconf-archive/autoconf-archive
build: substitute major,minor,patch versions
Load the AX_SPLIT_VERSION macro, and then AC_SUBST
AX_MAJOR_VERSION, AX_MINOR_VERSION, AX_POINT_VERSION
for use in generated version.h.
broker: add 'version' attribute
Add an attribute 'version', which returns
FLUX_CORE_VERSION_STRING, so that the version of
a possibly remote broker can be queried.

@garlick garlick force-pushed the garlick:flux_version branch from 3496d0f to 638ea82 Nov 8, 2018

@garlick garlick changed the title add flux version --remote flux-version: show broker, library, and command version, and add API version access Nov 9, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 9, 2018

OK, I added the macros and functions we discussed, and changed flux-version output so it looks like this

$ flux version
commands:    	0.10.0-506-g34e9b29a
libflux-core:	0.10.0-506-g34e9b29a
broker:  	0.10.0-506-g34e9b29a
FLUX_URI:	local:///tmp/flux-o3OqOQ

If FLUX_URI isn't set then broker and FLUX_URI are omitted from the output.

@grondo
Copy link
Contributor

grondo left a comment

Awesome! Should we add manpages for flux_core_version_*? I'm fine with delaying that and merging this before documentation.

Just a couple other comments inline.

@@ -336,7 +336,7 @@ static void state_cb (flux_subprocess_t *p, flux_subprocess_state_t state)
log_msg ("%d (pid %d) %s", cli->rank, pid, strsignal (SIGCONT));
else if (WIFSIGNALED (status))
log_msg ("%d (pid %d) %s", cli->rank, pid, strsignal (WTERMSIG (status)));
else if (WIFEXITED (status))
else if (WIFEXITED (status) && WEXITSTATUS (status) != 0)
log_msg ("%d (pid %d) exited with rc=%d", cli->rank, pid, WEXITSTATUS (status));

This comment has been minimized.

@grondo

grondo Nov 9, 2018

Contributor

Hm, I had thought you'd already fixed this. Maybe in an existing PR that hasn't been merged?

While you are in here, the tests for WIFSTOPPED() (actually erroneously written here as WIFSIGNALED()) and WIFCONTINUED() should be removed. This code is only executed under FLUX_SUBPROCESS_EXITED so those states will never be present.

This comment has been minimized.

@garlick

garlick Nov 9, 2018

Author Member

Oh, hmm, I thought I had fixed it the way you asked before too.... I cherry picked that from another PR thinking it would get in quicker if I tacked it on here. I'll revist, thanks for catching.

flux_t *h = builtin_get_flux_handle (p);
if (!h)
log_err_exit ("flux_open");
if (!(version = flux_attr_get (h, "version", NULL)))

This comment has been minimized.

@grondo

grondo Nov 9, 2018

Contributor

Rather than exiting with error on an invalid FLUX_URI, would it be better to either

  • silently ignore error from flux_open and act as if FLUX_URI wasn't set
  • or report a more user-friendly error Failed to connect to broker at FLUX_URI=%s
    ?

This comment has been minimized.

@garlick

garlick Nov 9, 2018

Author Member

I think at minimum a user friendly error should be reported since other things aren't going to do well if you have FLUX_URI set to a dead instance or whatever.

This comment has been minimized.

@grondo

grondo Nov 9, 2018

Contributor

Thanks, flux start -s will still work, but going forward that will not be a very common case, so I agree with your assesment...

garlick added some commits Oct 26, 2018

cmd/flux-start: suppress exited with rc=0
Problem: even when brokers exit normally, flux-start
(with size > 1) reports the broker exit codes.

Only report if exit code != 0.

Fixes #1749
testsuite: look for new output from flux-version
Verify that flux-version runs with or without
FLUX_URI set, and produces the expected output.

@garlick garlick force-pushed the garlick:flux_version branch from 5d82a85 to f09f39d Nov 9, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 9, 2018

OK, I addressed those two comments and squashed.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 9, 2018

Excellent!

@grondo grondo merged commit 4b04fdf into flux-framework:master Nov 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 9, 2018

Thanks!

@garlick garlick deleted the garlick:flux_version branch Nov 9, 2018

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.