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

abstract in-tree detection into libutil, remove FLUX_CONF_INTREE environment variable #2351

Merged
merged 7 commits into from Sep 9, 2019

Conversation

@grondo
Copy link
Contributor

commented Sep 5, 2019

This is split off the shell plugin work, since it is fairly independent and may actually generate some discussion on its own.

In the shell we want to load a different default initrc depending on if we're running "in-tree" or not, but the code to detect if an executable was only available in cmd/flux.c. The flux command does export FLUX_CONF_INTREE=1 if it detects it is running out of a build tree, but there are several potential problems with this approach -- including that FLUX_CONF_INTREE is set, but never unset. (Also I'm not sure if that env var would always be exported to flux-shell, e.g. if --standalone is used)

This PR abstracts the in-tree detection into libutil/intree.c, including two new functions:

int executable_is_intree (void);
const char *executable_selfdir (void);

The algorithm for executable_is_intree() is slightly different here. To allow in-tree detection to work with any executable, instead of comparing expected directories directly (and thereby requiring the directory for every executable be stored in the "conf" table), this version just checks to see if the executable_selfdir() is a subdirectory of the $abs_top_builddir. In theory, this should work for any executable run from under the build directory, including flux-broker, flux-shell, and all other flux commands.

Additionally, the new code only computes results on the first call, since both the intree value and executable directory obviously will not change. Since the calls may now be made from broker modules, the critical sections and memoized results are protected by a mutex (since it seems flux commands are already linked with libpthread anyway)

Finally, users of FLUX_CONF_INTREE were converted to use executable_is_intree() and the FLUX_CONF_INTREE environment variable is now removed.

Add a define ABS_TOP_BUILDDIR to CFLAGS to be used by intree
detection function in libutil.
@garlick
garlick approved these changes Sep 9, 2019
Copy link
Member

left a comment

Nice improvement!

Couple minor comments inline.

Is it possible to have a quick and dirty libtap unit test for these functions?

#include <libgen.h> /* dirname(3) */
#include <pthread.h>

#include <flux/core.h>

This comment has been minimized.

Copy link
@garlick

garlick Sep 9, 2019

Member

the <flux/core.h> include probably isn't needed


#include <flux/core.h>

static pthread_mutex_t intree_lock = PTHREAD_MUTEX_INITIALIZER;

This comment has been minimized.

Copy link
@garlick

garlick Sep 9, 2019

Member

if these variables don't need to be shared between the functions, consider moving them to function scope.

This comment has been minimized.

Copy link
@grondo

grondo Sep 9, 2019

Author Contributor

Good point. In another iteration I was using TLS and those variables were shared. I'll make the change.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Is it possible to have a quick and dirty libtap unit test for these functions?

I meant to comment that I wasn't sure of a valid way to add unit tests. I could easily add unit tests to ensure that the unit test itself is detected as running in-tree if you think that would help?

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Sorry, that makes sense, and codecov shows that both cases are being covered in intree.c, so it would probably be redundant to create a test executable and check it from various locations in sharness. Never mind!

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Oops, sorry. Already wrote up a quick unit test -- also exercised threaded access to the function to ensure no issues there (test could be flawed, just starts 16 threads in a barrier and tries to call executable_is_intree() simultaneously from all of them.)

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

LGTM! Not sure if you were planning to squash. Please set merge-when-passing when you're all done.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Yeah, I'll squash.

grondo added 6 commits Sep 5, 2019
Copy functionality from src/cmd/flux.c to detect if flux
components are running "in-tree" into libutil/intree.[ch], with
some differences noted below:

Rename dir_self() and flux_is_installed() to executable_selfdir()
and executable_is_intree().

Rather than comparing directories directly, this version of
executable_is_intree() works by comparing the discovered directory to
the current executable to the absolute top builddir ($abs_top_buildir
as exported by Makfele.am) to determine if the executable was run
from *any* subdirectory of the top build dir.

Results from both executable_is_intree() and executable_selfdir()
are memoized on the first call so that multiple calls don't have
to open "/proc/self/exec", call realpath(3), etc, and it is known
that the results will never change for a given executable.

However, since broker modules may call into these functions, the
calls are made thread safe by wrapping critical sections in a
mutex.
Add simple functionality unit test for libutil/intree.c
Switch flux_is_installed() to use the provided libutil
executable_is_intree() interface.

Similarly, replace dir_self() with executable_selfdir().
Since executable_selfdir() returns a const char *, simplify
the call and remove the free() of the result.
Update job-exec module to use executable_is_intree() instead of
FLUX_CONF_INTREE environment variable.

Also moves default_job_shell to a static global so it doesn't
have to be recomputed each time a job is launched.
Drop use of FLUX_CONF_INTREE environment variable for in-tree
detection and use libutil/intree executable_is_intree() instead.
Problem: The FLUX_CONF_INTREE environment variable may be somewhat
brittle as a method to determine if flux components are running in-tree
or not. For example, the variable is exported, but never unset,
and it is unclear what effect this might have on jobs, etc.

Now that there are no users of FLUX_CONF_INTREE, just drop it.
@grondo grondo force-pushed the grondo:in-tree-detect branch from 0d8a36d to 7bb4ff9 Sep 9, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Sep 9, 2019

Codecov Report

Merging #2351 into master will decrease coverage by <.01%.
The diff coverage is 92.45%.

@@            Coverage Diff             @@
##           master    #2351      +/-   ##
==========================================
- Coverage   80.82%   80.81%   -0.01%     
==========================================
  Files         217      218       +1     
  Lines       34518    34530      +12     
==========================================
+ Hits        27898    27907       +9     
- Misses       6620     6623       +3
Impacted Files Coverage Δ
src/modules/job-ingest/validate.c 83.92% <100%> (ø) ⬆️
src/modules/job-exec/exec.c 78.62% <100%> (+0.5%) ⬆️
src/cmd/flux.c 82.91% <77.77%> (-1.23%) ⬇️
src/common/libutil/intree.c 94.59% <94.59%> (ø)
src/common/libutil/environment.c 90% <0%> (-3%) ⬇️
src/common/libflux/message.c 80.7% <0%> (-0.26%) ⬇️
src/modules/connector-local/local.c 73.41% <0%> (+0.14%) ⬆️
src/common/libutil/dirwalk.c 94.4% <0%> (+0.69%) ⬆️
@mergify mergify bot merged commit a3645e6 into flux-framework:master Sep 9, 2019
2 checks passed
2 checks passed
Summary 1 rule matches
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo grondo deleted the grondo:in-tree-detect branch Sep 9, 2019
@garlick garlick referenced this pull request Sep 30, 2019
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.