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

broker: don't read FLUX_RCX_PATH to set rc1,rc3 paths #2431

Merged
merged 5 commits into from Oct 1, 2019

Conversation

@grondo
Copy link
Contributor

commented Oct 1, 2019

This is an attempted fix for #2304. The use of FLUX_RC{1,3}_PATH is deprecated in favor of setting the broker attribute directly. The broker now sets the default for these paths from the static "conf", and allows them to be overridden on the command line.

All tests were updated to use the new scheme. Still need to test with flux-under-flux, but this should solve that issue in theory.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Verified this fixes the issue I was seeing on fluke:

t$ src/cmd/flux start -s2
flux-start: warning: setting --bootstrap=selfpmi due to --size option
(tmp/grondo/flux-9139-RIZEDq/0) grondo@fluke64:~/git/f.git$ flux version
commands:    		0.12.0-400-gd91cedc
libflux-core:		0.12.0-400-gd91cedc
broker:  		0.12.0-400-gd91cedc
FLUX_URI:		local:///var/tmp/grondo/flux-9139-RIZEDq/0
(tmp/grondo/flux-9139-RIZEDq/0) grondo@fluke64:~/git/f.git$ flux --parent version
commands:    		0.12.0-400-gd91cedc
libflux-core:		0.12.0-400-gd91cedc
broker:  		0.11.1
FLUX_URI:		local:///var/tmp/grondo/flux-quWkmA/0

Previously it was all segfaults, all the time.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

One missing piece here. The python subflux code still uses FLUX_RCX_PATH, though no tests actually fail with the changes above. subflux should be updated to use broker attributes though, but it will take a little bit to figure that out in Python.

grondo added 2 commits Sep 30, 2019
FLUX_RC{1,3}_PATH environment variable is being deprecated.

Use the broker attribute for testing instead. (where necessary).
The use of FLUX_RC1_PATH and FLUX_RC3_PATH to control rc scripts
is being deprecated. In test_under_flux() use broker attributes
instead.
@grondo grondo force-pushed the grondo:issue#2304 branch 2 times, most recently from ef038b8 to 86d14ce Oct 1, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Ok, I think I've made the appropriate corresponding changes to subflux.py. This should be ready for consideration.

grondo added 3 commits Oct 1, 2019
FLUX_RC{1,3}_PATH is deprecated, so avoid using it in subflux.py.
Switch to setting broker attributes instead.
Problem: The inheritance of rc1 and rc3 paths through the environment
has caused some problems when running an alternate version of Flux
under another running instance. The new Flux instance inherits the
wrong rc paths, which possibly load invalid modules and can cause
crashes.

Instead of using an environment variable to set a different rc1
and/or rc3 path, force users to use broker attributes directly.
The default rc1,rc3 paths are now initialized from the static flux
"conf" instead of the environment.

Fixes #2304
The broker no longer looks for FLUX_RC1_PATH and FLUX_RC3_PATH,
so ignore them in flux(1) command driver.
@grondo grondo force-pushed the grondo:issue#2304 branch from 86d14ce to 65f3775 Oct 1, 2019
@garlick
garlick approved these changes Oct 1, 2019
Copy link
Member

left a comment

Good this looks to me.

@codecov-io

This comment has been minimized.

Copy link

commented Oct 1, 2019

Codecov Report

Merging #2431 into master will increase coverage by <.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2431      +/-   ##
==========================================
+ Coverage   81.12%   81.12%   +<.01%     
==========================================
  Files         225      225              
  Lines       36110    36116       +6     
==========================================
+ Hits        29293    29299       +6     
  Misses       6817     6817
Impacted Files Coverage Δ
src/cmd/flux.c 83% <ø> (-0.17%) ⬇️
src/broker/broker.c 73.83% <87.5%> (+0.11%) ⬆️
src/common/libsubprocess/local.c 79.86% <0%> (-0.35%) ⬇️
src/shell/shell.c 85.75% <0%> (-0.27%) ⬇️
src/cmd/flux-module.c 83.96% <0%> (ø) ⬆️
src/common/libflux/message.c 80.36% <0%> (ø) ⬆️
src/cmd/flux-kvs.c 81.93% <0%> (+0.11%) ⬆️
src/modules/job-info/guest_watch.c 74.71% <0%> (+0.56%) ⬆️
@mergify mergify bot merged commit 7749e8c into flux-framework:master Oct 1, 2019
4 checks passed
4 checks passed
Summary 1 rule matches
Details
codecov/patch 87.5% of diff hit (target 81.12%)
Details
codecov/project 81.12% (+<.01%) compared to d91cedc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo grondo deleted the grondo:issue#2304 branch Oct 1, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Thank you kindly, sir!

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