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

establish single run directory and default URI for system instance #992

Merged
merged 7 commits into from Feb 25, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Feb 24, 2017

This PR adds a new broker attribute broker.rundir which replaces both scratch-directory and scratch-directory-rank. The intent is to allow systemd to manage a single /run directory for a flux broker started as part of the system instance.

flux-start and sideflux.py were updated to match the simplified directory scheme.

In addition, the local connector was changed so that it no longer tries to send a signal to the broker as discussed in #987.

Finally, flux_open() was modified so that if a URI is not specified, and FLUX_URI is not set, it will try the local connector of the system instance (using $localstatedir/flux as the path).

This PR was split off of #980, which is focused on message security credentials.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.2%) to 76.082% when pulling dc7a639 on garlick:rundir2 into 3d0fa0f on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 24, 2017

Codecov Report

Merging #992 into master will decrease coverage by -0.02%.
The diff coverage is 76.59%.

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
- Coverage   75.94%   75.92%   -0.02%     
==========================================
  Files         152      152              
  Lines       25954    25920      -34     
==========================================
- Hits        19712    19681      -31     
+ Misses       6242     6239       -3
Impacted Files Coverage Δ
src/common/libflux/conf.c 100% <ø> (ø)
src/connectors/local/local.c 88.04% <100%> (+0.16%)
src/modules/content-sqlite/content-sqlite.c 54.84% <50%> (ø)
src/common/libflux/handle.c 86.2% <60%> (+0.83%)
src/broker/broker.c 65.83% <77.77%> (-1.92%)
src/cmd/flux-start.c 74% <83.33%> (+0.12%)
src/broker/overlay.c 64.46% <ø> (-4.72%)
src/common/libflux/message.c 83.84% <ø> (+0.13%)
src/common/libflux/dispatch.c 82.71% <ø> (+0.28%)
... and 7 more

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 fd3ebe0...950f011. Read the comment docs.

garlick added some commits Feb 23, 2017

config: add "rundir" to static configuration
Allow configured rundir based on $localstatedir
to be fetched with flux_conf_get().
libflux/handle: add system default URI for flux_open()
If FLUX_URI is not set, provide a default URI for flux_open()
based on the compiled-in "rundir" path.
cmd/flux-start: add --scratchdir option
Add an option to set the temp directory that
will be used for an instance launched locally by
flux-start.

This will be useful for sideflux.py once the
scratch-directory broker attribute is deprecated
and flux-start becomes responsible for creating
broker runtime directories.
broker: rename scratch-directory-rank to broker.rundir
In preparation for changes that will make broker.rundir
the primary location of all broker-specific runtime files,
rename the 'scratch-directory-rank' attribute to broker.rundir
and update users.

Update broker code as well as users, docs, and tests.

Cosmetic changes only.
connectors/local: remove broker pid check
Problem: users other than the instance owner cannot connect
to the local connector since the kill -0 on the broker pid
cannot succeed.

Remove the broker pid check.  It serves no real purpose
for the client to check this anyway.

Fixes #987
broker: eliminate scratch-directory attr
Use broker.rundir to locate all broker runtime sockets,
and temporary files, deprecating scratch-directory.

Change flux-start so rather than passing all ranks
the same initial value for scratch-directory, it creates
a subdirectory (named for the rank) under the shared scratch
directory and passes each rank a different value for
broker.rundir.

Change sideflux.py to pass a the --scratchdir option
to flux-start rather than the scratch-directory attribute
to the broker.

Update docs and tests accordingly.

@garlick garlick force-pushed the garlick:rundir2 branch from dc7a639 to a22608d Feb 25, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Feb 25, 2017

Just rebased on current master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 25, 2017

This looks like really good cleanup.

Could the connector use of rundir be somehow simulated for the automated tests? (sorry if that was done already)

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Feb 25, 2017

Could the connector use of rundir be somehow simulated for the automated tests? (sorry if that was done already)

Not sure how to do that as it would mean that the connector would need to find something at the path configured for $localstatedir/flux which might not be under the test suite's control... (Pondering though)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage decreased (-0.1%) to 76.095% when pulling 950f011 on garlick:rundir2 into fd3ebe0 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 25, 2017

Not sure how to do that as it would mean that the connector would need to find something at the path configured for $localstatedir/flux which might not be under the test suite's control... (Pondering though)

No big deal if not, but it would be nice to have automated tests for the intended behavior. However, don't put any great effort into it.

Maybe it would be enough to unset FLUX_URI and make sure the $localstatedir/flux is attempted, but fails in an expected way? We know that other parts of the connector work, we're just testing the fallback to system instance?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 25, 2017

Oh, yeah nevermind. I see what you are saying -- this idea would totally fail on a system that had an actual system instance running. Feel free to ignore the whining from the peanut gallery...

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage decreased (-0.03%) to 76.192% when pulling 950f011 on garlick:rundir2 into fd3ebe0 on flux-framework:master.

@grondo grondo merged commit 8ff2115 into flux-framework:master Feb 25, 2017

4 checks passed

codecov/patch 76.59% of diff hit (target 75.94%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +0.64% compared to fd3ebe0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 76.192%
Details

@garlick garlick referenced this pull request Feb 28, 2017

Closed

start flux broker from systemd #967

6 of 6 tasks complete

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

@garlick garlick deleted the garlick:rundir2 branch Sep 6, 2017

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.