DAOS-14532 gurt: Replace environment APIs hook #13483

knard-intel · 2023-12-12T09:46:31Z

Description

This PR is a subset of the PR #13250 allowing thread safe management of environment variables: it has been split into smaller PRs to facilitate the review process.
This PR mainly add thread safe environment variables management functions.
It also remove and replace old non thread safe custom environment management functions.
Finally, it replace the setenv() function with d_setenv().

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

Add thread safe environment variables managment functions. Remove and replace non thread safe custom environment management function. Replace setenv() function with d_setenv() Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

github-actions · 2023-12-12T09:46:51Z

Bug-tracker data:
Ticket title is 'frontera: segfault on mvapich init'
Status is 'In Review'
Labels: 'TACC,TACC_Frontera,triaged'
https://daosio.atlassian.net/browse/DAOS-14532

daosbuild1

LGTM. No errors found by checkpatch.

wangshilong · 2023-12-13T06:31:54Z

src/gurt/misc.c

+	if (!dis_unsigned_str(env)) {
+		rc = -DER_INVAL;
+		goto out;
+	}

-	env_val = getenv(env);
-	if (!env_val)
-		return;
+	errno = 0;
+	val   = strtoul(env, &endptr, 0);
+	if (errno != 0 || endptr == env || *endptr != '\0') {
+		rc = -DER_INVAL;
+		goto out;
+	}

-	if (!dis_integer_str(env_val)) {
-		D_ERROR("ENV %s is not integer.\n", env_val);
-		return;
+#if UINT_MAX != ULONG_MAX
+	assert(sizeof(unsigned) < sizeof(unsigned long));
+	if (val > UINT_MAX) {
+		rc = -DER_INVAL;
+		goto out;
 	}
+#endif


[suggestion] i think for somehow dis_unsigned_str() is duplicated as strtoull()?
we could remove dis_unsigned_str() and just use strtoull()?

The best solution from my point of view should be to remove all the unsigned and to replace them with uint32_t or uint64_t. However, this is beyond the scope of this PR.
For this PR, I would prefer to keep these three functions as the sizeof() of the unsigned is platform dependent: I would like to avoid any unexpected overflow. In a follow-up PR, I could replace the d_getenv_uint() with relevant d_getenv_uintXXt() and update the code accordingly.

To avoid duplicate code, I could factorize the code into d_getenv_uint64_t() function and just check the overflow in d_getenv_uint32t() and d_getenv_uint() functions.
@wangshilong, is this last solution seems acceptable for you ?

I am fine with this, i thought dis_unsigned_str() is unnecessary and might be removed.

Factorize unsigned fonctions.

[suggestion] i think for somehow dis_unsigned_str() is duplicated as strtoull()? we could remove dis_unsigned_str() and just use strtoull()?

My apologies @wangshilong , I did not well understood your question regarding the function dis_unsigned_str(). At the beginning, I also wanted to remove it. However, strtoull() is less restrictive and allow to have spaces before the first digit and even to have a negative integer. To not change the behavior of the d_getenv_uint() functions, I decided to kept the dis_unsigned_str() function.

Is this OK for you ?

The code of the getenv of unsigned integer function has been factorized in commit 193c24c

jolivier23 · 2023-12-14T04:43:18Z

src/gurt/misc.c

+{
+	int rc;
+
+	pthread_rwlock_wrlock(&d_env_lock);


Can you elaborate why this is safer than the hook version? I think the issue we have seen is due to threads calling getenv in other libraries, which in this case would not be locked with respect to calls in daos libraries. Whereas the hooked version would ensure the lock is acquired for all calls, no?

The hook version can have some side effect with other third party libraries such as mvapich and eventually lead to segfault. You could find in the following comment, a detailed description of such scenario https://daosio.atlassian.net/browse/DAOS-14532?focusedCommentId=121770

I tried to overcome this issue with updating hooks or playing with library constructors. At the end, I did not find an acceptable solution which could not have some side effects with other third party libraries.

After some discussion with @johannlombardi and @gnailzenh , it appeared that the least bad solution was to have our own thread safe functions for managing environment. From my side, the only disadvantage of this solution is to be much more intrusive in our code. Happily, we already have some dedicated non thread safe functions for managing the environment.

we see an intermittent issue with fio that manifests as follows
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007d9faabc195d in getenv () from /lib64/libc.so.6
[Current thread is 1 (Thread 0x7d9fad4335c0 (LWP 60560))]
Missing separate debuginfos, use: yum debuginfo-install boost-atomic-1.66.0-13.el8.x86_64 boost-chrono-1.66.0-13.el8.x86_64 boost-date-time-1.66.0-13.el8.x86_64 boost-iostreams-1.66.0-13.el8.x86_64 boost-program-options-1.66.0-13.el8.x86_64 boost-random-1.66.0-13.el8.x86_64 boost-regex-1.66.0-13.el8.x86_64 boost-system-1.66.0-13.el8.x86_64 boost-thread-1.66.0-13.el8.x86_64 daxctl-libs-71.1-4.el8.x86_64 glibc-2.28-225.el8.x86_64 libaio-0.3.112-1.el8.x86_64 libblkid-2.32.1-42.el8_8.x86_64 libgcc-8.5.0-18.el8.x86_64 libibverbs-44.0-2.el8.1.x86_64 libicu-60.3-2.el8_1.x86_64 libmount-2.32.1-42.el8_8.x86_64 libnl3-3.7.0-1.el8.x86_64 libpmem-1.6.1-1.el8.x86_64 libpmemblk-1.6.1-1.el8.x86_64 librados2-12.2.7-9.el8.x86_64 librbd1-12.2.7-9.el8.x86_64 librdmacm-44.0-2.el8.1.x86_64 libselinux-2.9-8.el8.x86_64 libstdc++-8.5.0-18.el8.x86_64 libyaml-0.1.7-5.el8.x86_64 lz4-libs-1.8.3-3.el8_4.x86_64 ndctl-libs-71.1-4.el8.x86_64 nspr-4.34.0-3.el8_6.x86_64 nss-util-3.79.0-11.el8_7.x86_64 numactl-libs-2.0.12-13.el8.x86_64 openssl-libs-1.1.1k-9.el8_7.x86_64 pcre2-10.32-3.el8_6.x86_64 protobuf-c-1.3.0-6.el8.x86_64 systemd-libs-239-74.el8_8.x86_64 zlib-1.2.11-21.el8_7.x86_64
(gdb) bt
#0 0x00007d9faabc195d in getenv () from /lib64/libc.so.6
#1 0x00005b1f2183098c in check_status_file () at stat.c:2372
#2 0x00005b1f21839c25 in check_for_running_stats () at stat.c:2411
#3 0x00005b1f2186cf51 in do_usleep (usecs=10000) at backend.c:2460
#4 run_threads (sk_out=sk_out@entry=0x0) at backend.c:2460
#5 0x00005b1f2186d5b2 in fio_backend (sk_out=0x0) at backend.c:2513
#6 0x00005b1f2181c595 in main (argc=16, argv=0x7ffe8dcfb228, envp=) at fio.c:60

It doesn't show dlopen in the callstack. Do you think it's related to this issue?

There is a high probably that it could be related: however it is hard for me to be 100% sure without a more deep investigation in the core file and the related code source.
As for @daltonbohning issue with mvapich, you could use the full patch #13250 to check if it also solves your issue with fio.

ashleypittman · 2023-12-19T20:00:08Z

src/include/gurt/common.h

+void
+d_free_env(char **str_val);
+int
+d_getenv_bool(bool *bool_val, const char *env);


This is changing the API and ABI isn't it? Existing applications which use this interface will need code changes to work, but if they build with one version and run with another they could also segfault.

I had a similar thought but then kind of assumed that the only user of these APIs are DAOS libraries that will be shipped with the changes.

the danger would be things like mpifileutils and hdf5 etc. At the very least we should bump the SO version with this changes, but that only applies to releases from "before" and "after" and I'd assume that 2.4 and 2.6 will have different SO numbers anyway - perhaps worth checking it though and bumping it with this change if master is still using the same as 2.4.

At this time, the only user of this new APIs are indeed the DAOS libraries and thus should not have any side effects for the client applications linked with DAOS. They will just use the standard non-thread safe getenv() functions.

I was referring to the functions you've changed, not the ones you've added.

…/daos-14532-gurt Required-githooks: true

Integrate reviewers comments: - Factorize d_getenv_uint() functions. Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

Fix clang format. Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

daosbuild1 · 2024-01-02T13:28:57Z

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-13483/3/display/redirect

src/include/gurt/common.h

ashleypittman · 2023-12-19T21:16:09Z

src/include/gurt/common.h

+void
+d_free_env(char **str_val);
+int
+d_getenv_bool(bool *bool_val, const char *env);


the danger would be things like mpifileutils and hdf5 etc. At the very least we should bump the SO version with this changes, but that only applies to releases from "before" and "after" and I'd assume that 2.4 and 2.6 will have different SO numbers anyway - perhaps worth checking it though and bumping it with this change if master is still using the same as 2.4.

ashleypittman · 2024-01-02T15:05:16Z

src/include/gurt/common.h

+void
+d_free_env(char **str_val);
+int
+d_getenv_bool(bool *bool_val, const char *env);


I was referring to the functions you've changed, not the ones you've added.

jolivier23 · 2024-01-04T15:49:39Z

src/bio/bio_xstream.c

-	d_getenv_bool("DAOS_NVME_AUTO_FAULTY_ENABLED", &glb_criteria.fc_enabled);
-	d_getenv_int("DAOS_NVME_AUTO_FAULTY_IO", &glb_criteria.fc_max_io_errs);
-	d_getenv_int("DAOS_NVME_AUTO_FAULTY_CSUM", &glb_criteria.fc_max_csum_errs);
+	d_getenv_bool(&glb_criteria.fc_enabled, "DAOS_NVME_AUTO_FAULTY_ENABLED");


We can't change these APIs. MPI_IO uses d_getenv_bool so we need to maintain the old interfaces.

Revert interfaces update

By any chance, do you know if we have some functional tests using MPI_IO ?

You can use Features: mpiio

In the past where this has come up then what functional testing we have uses a weird system where:

PRs and landings to downstream components test against master daos.

PRs and landings to daos test with the latest built downstream component.

The unfortunate implication here is that we have some testing but it's limited and if we change the API or ABI and it's incompatible then we don't notice until there's a new PR to the downstream component or it gets rebuilt for another reason. If we had test coverage here then it would fail at build time, as it is it'll fail (obscurely) at runtime, but only if the code is called.

It's an area we need to work on.

Revert interfaces update

Fixed with commit ac764d1

jolivier23

Removing my +1 since the API change needs to be reverted.

knard-intel · 2024-01-04T15:57:11Z

@ashleypittman , my apologies for the misunderstanding.
I was thinking that you were talking about the standard getenv() function and not the daos_getenv_xxx() functions already existing. I was expecting that these last ones were only used internaly with DAOS.
For sure, I will revert the interface modification to preserve the compatibility.

…/daos-14532-gurt Required-githooks: true

Integrate reviewers comments: - Revert d_getenv_XXX interfaces update - Rename d_free_env() -> d_free_env_str() Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

Revert useless header file changes. Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

ashleypittman · 2024-01-05T13:51:58Z

src/gurt/misc.c

+{
+	char *env;
+
+	pthread_rwlock_rdlock(&d_env_lock);


These will be coverity warnings unless you either check the return code or use our macros.

Thanks for the info.
Sadly, I will not be able to use the D_RWLOCK_RDLOCK macros as it use the DAOS logging facilities which could lead to infinite recursive calls. Thus I will check the return code is a similar way as this last macros.

Checks return code when using standard function calls such as pthread_rwlock_rdlock()

I did wonder. Does the logging ever call getenv though?

I made the same change to the locking in the logging as that was causing recursion and found out about the coverity issue that way.

From what I remember, I experiment infinite recursion with the function d_logfac_is_enabled() from src/gurt/dlog.c. There is other functions from this file which could also probably lead to the same kind of issues.

Thanks for the info. Sadly, I will not be able to use the D_RWLOCK_RDLOCK macros as it use the DAOS logging facilities which could lead to infinite recursive calls. Thus I will check the return code is a similar way as this last macros.

Checks return code when using standard function calls such as pthread_rwlock_rdlock()

Fixed with commit 2791b93

Integrate reviwers comments: - Checks return code when using standard function calls such as pthread_rwlock_rdlock() Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

Fix clang format coding standards. Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

daosbuild1 · 2024-01-07T21:09:52Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13483/8/execution/node/1453/log

knard-intel · 2024-01-08T07:39:37Z

The only CI issue with the functional test erasurecode_online_rebuild_mdtest_py is a known issue DAOS-14845.

This PR is a subset of the PR #13250 allowing thread safe management of environment variables: it has been split into smaller PRs to facilitate the review process. This PR mainly add thread safe environment variables management functions. It also remove and replace old non thread safe custom environment management functions. Finally, it replace the setenv() function with d_setenv(). Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

This PR is a subset of the PR #13250 allowing thread safe management of environment variables: it has been split into smaller PRs to facilitate the review process. This PR mainly add thread safe environment variables management functions. It also remove and replace old non thread safe custom environment management functions. Finally, it replace the setenv() function with d_setenv(). Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

This PR is a subset of the PR #13250 allowing thread safe management of environment variables: it has been split into smaller PRs to facilitate the review process. This PR mainly add thread safe environment variables management functions. It also remove and replace old non thread safe custom environment management functions. Finally, it replace the setenv() function with d_setenv(). Required-githooks: true Change-Id: Ife6690e2c63dd6c47279a2ac8c3c5a3da5cf8213 Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

daosbuild1 reviewed Dec 12, 2023

View reviewed changes

knard-intel requested review from wangshilong, ashleypittman, gnailzenh and jolivier23 December 12, 2023 09:59

knard-intel marked this pull request as ready for review December 12, 2023 10:49

knard-intel requested review from a team as code owners December 12, 2023 10:49

wangshilong reviewed Dec 13, 2023

View reviewed changes

wangshilong previously approved these changes Dec 13, 2023

View reviewed changes

jolivier23 reviewed Dec 14, 2023

View reviewed changes

knard-intel requested a review from johannlombardi December 14, 2023 06:43

jolivier23 previously approved these changes Dec 19, 2023

View reviewed changes

ashleypittman reviewed Dec 19, 2023

View reviewed changes

kanard38 added 2 commits January 2, 2024 11:59

Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…

1ade13e

…/daos-14532-gurt Required-githooks: true

DAOS-14532 gurt: Replace environment APIs hook

193c24c

Integrate reviewers comments: - Factorize d_getenv_uint() functions. Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

knard-intel dismissed stale reviews from jolivier23 and wangshilong via 193c24c January 2, 2024 12:42

knard-intel requested review from ashleypittman, jolivier23 and wangshilong January 2, 2024 13:20

DAOS-14532 gurt: Replace environment APIs hook

21cc357

Fix clang format. Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

jolivier23 approved these changes Jan 3, 2024

View reviewed changes

wangshilong previously approved these changes Jan 4, 2024

View reviewed changes

knard-intel requested a review from a team January 4, 2024 07:24

ashleypittman reviewed Jan 4, 2024

View reviewed changes

jolivier23 reviewed Jan 4, 2024

View reviewed changes

jolivier23 requested changes Jan 4, 2024

View reviewed changes

kanard38 added 2 commits January 5, 2024 07:50

Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…

92be770

…/daos-14532-gurt Required-githooks: true

DAOS-14532 gurt: Replace environment APIs hook

ac764d1

Integrate reviewers comments: - Revert d_getenv_XXX interfaces update - Rename d_free_env() -> d_free_env_str() Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

knard-intel dismissed wangshilong’s stale review via ac764d1 January 5, 2024 09:59

DAOS-14532 gurt: Replace environment APIs hook

8d8d51d

Revert useless header file changes. Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

ashleypittman reviewed Jan 5, 2024

View reviewed changes

knard-intel self-assigned this Jan 5, 2024

kanard38 added 2 commits January 5, 2024 15:19

DAOS-14532 gurt: Replace environment APIs hook

2791b93

Integrate reviwers comments: - Checks return code when using standard function calls such as pthread_rwlock_rdlock() Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

DAOS-14532 gurt: Replace environment APIs hook

534abe3

Fix clang format coding standards. Features: mpiio Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>

knard-intel requested review from ashleypittman, daosbuild1, jolivier23, daltonbohning and wangshilong January 8, 2024 07:40

wangshilong approved these changes Jan 8, 2024

View reviewed changes

jolivier23 approved these changes Jan 8, 2024

View reviewed changes

mchaarawi approved these changes Jan 8, 2024

View reviewed changes

mchaarawi merged commit 2a9e853 into master Jan 8, 2024
45 of 47 checks passed

mchaarawi deleted the ckochhof/fix/master/daos-14532-gurt branch January 8, 2024 16:50

knard-intel mentioned this pull request Jan 9, 2024

DAOS-14532 gurt: fix environment APIs hook #13579

Merged

18 tasks

DAOS-14532 gurt: Replace environment APIs hook #13483

DAOS-14532 gurt: Replace environment APIs hook #13483

Conversation

knard-intel commented Dec 12, 2023 • edited Loading

Description

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Dec 12, 2023 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

knard-intel Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolivier23 left a comment

Choose a reason for hiding this comment

knard-intel commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

knard-intel Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 7, 2024

knard-intel commented Jan 8, 2024

knard-intel commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

knard-intel Dec 13, 2023 •

edited

Loading

knard-intel Dec 13, 2023 •

edited

Loading

knard-intel Jan 2, 2024 •

edited

Loading

knard-intel Dec 14, 2023 •

edited

Loading

knard-intel Jan 4, 2024 •

edited

Loading

knard-intel commented Jan 4, 2024 •

edited

Loading

knard-intel Jan 5, 2024 •

edited

Loading