Preserve the environment when running cli tools #13689

anti-social · 2024-05-15T19:30:04Z

Description

In some operating systems (for example NixOS) there is no default PATH variable for a shell (in NixOS they compile it with an option -DDEFAULT_PATH_VALUE="/no-such-path"). As a result it is not possible to run integration tests. They fail with weird error:

> Task :integTest FAILED
Exec output and error:
| Output for ./bin/opensearch-plugin:./bin/opensearch-plugin: line 7: dirname: command not found
| ./bin/opensearch-plugin: line 7: /opensearch-cli: No such file or directory

The PR solves it explicitly passing PATH variable to the environment of CLI tools.

Related Issues

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
API changes companion pull request created.
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2024-05-15T20:02:35Z

❌ Gradle check result for 0eb90c9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

dblock · 2024-05-16T13:00:57Z

Why does PATH matter? Shouldn't ./bin/opensearch-plugin be relative and always resolved?

anti-social · 2024-05-16T14:25:11Z

PATH is needed because opensearch-plugin and other cli tools are shell scripts and they run another programs which somehow should be found by a shell.

In the output that I provided a dirname program had not been found.

anti-social · 2024-05-16T14:29:19Z

I wonder should we pass all the environment? I think so because other environment variables also may affect on script execution.

dblock · 2024-05-16T14:35:08Z

I wonder should we pass all the environment? I think so because other environment variables also may affect on script execution.

Maybe. It also looks like something very non-standard is going on this particular system? Cause it has been working pretty much for everyone, no?

reta · 2024-05-16T14:39:52Z

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

@@ -782,6 +782,13 @@ private Map<String, String> getOpenSearchEnvironment() {
        defaultEnv.put("HOSTNAME", HOSTNAME_OVERRIDE);
        defaultEnv.put("COMPUTERNAME", COMPUTERNAME_OVERRIDE);

+        // Pass PATH environment variable to be able to run OpenSearch cli tools


I believe we may not be fixing the right place, the shell scripts ./bin/opensearch-plugin should handle that gracefully? (since this is where the failure happens)

When opensearch-plugin script is executed it does not have any information about an environment of its parent process as the environment was cleared.

I think we should preserve all the environment like a shell do it for us when we run programs.

I think we should preserve all the environment like a shell do it for us when we run programs.

Correct, sorry if it was not clear, I meant not messing up with path but check if dirname is available, and if not use alternative means ?

By default Exec task keeps the environment if we do not provide ours:

The environment variables to use for the process. Defaults to the environment of this process.

When we call a setEnvironment method we should merge our environment with the environment of the process (if we need it of course).

Correct, sorry if it was not clear, I meant not messing up with path but check if dirname is available, and if not use alternative means ?

How it should guess where to look for a program? PATH variable is designed especially for this.

User may have their own standard utility as dirname somewhere in their home directory, for example inside ~/.bin. And they want all scripts and programs use it so they add ~/.bin into a PATH variable. And that's it. Now all the programs should use their version of dirname including OpenSearch cli tools.

anti-social · 2024-05-16T15:22:37Z

Maybe. It also looks like something very non-standard is going on this particular system? Cause it has been working pretty much for everyone, no?

Yeah. NixOS is a very specific Linux distro. Its /usr/bin directory contains only a single program: env. So it does not make sense to have default PATH value. All programs are located inside a /nix/store directory. And they collect links to the installed programs at runtime into a /run/current-system/sw/bin directory.

But anyway I believe that correct behavior is to rely on a PATH variable that user has in their shell.

anti-social · 2024-05-16T15:38:03Z

Cause it has been working pretty much for everyone, no?

It works on other systems because a shell has a default PATH compiled into a binary. Taken from bash sources:

/* The default value of the PATH variable. */
#ifndef DEFAULT_PATH_VALUE
#define DEFAULT_PATH_VALUE \
  "/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin:."
#endif

github-actions · 2024-05-16T15:53:09Z

❌ Gradle check result for b820d3d: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

reta · 2024-05-16T16:01:45Z

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

@@ -742,7 +742,8 @@ private void runOpenSearchBinScript(String tool, CharSequence... args) {
    }

    private Map<String, String> getOpenSearchEnvironment() {
-        Map<String, String> defaultEnv = new HashMap<>();
+        Map<String, String> defaultEnv = System.getenv().entrySet().stream()


This seems to not go into right direction, see please reasoning here https://github.com/opensearch-project/OpenSearch/pull/13689/files#diff-0eecf581460b128c3d70a559dfdabece54a006048cc0a4e773d020ce1f04fc9bL814

I see. Then we should keep only a PATH variable.

Do you agree that we should preserve at least PATH here?

Are you agree that we should preserve at least PATH here?

So any environment injection (as per https://github.com/opensearch-project/OpenSearch/pull/13689/files#diff-0eecf581460b128c3d70a559dfdabece54a006048cc0a4e773d020ce1f04fc9bL814) risks to harm reproducibility, but I don't really know the background of why things were done this way, @dblock do you?

The original comment though is pretty clear. It just wants a clean slate of an environment to make things consistent.

Correct, so we should not pursue the route of injecting env variables @anti-social

github-actions · 2024-05-16T16:08:56Z

❌ Gradle check result for fbcc913: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

dblock · 2024-05-16T17:09:21Z

~~I think I can live with this change.~~ Is this something we (you) can add tests for?

What if the change was something more directed, like an actual setting that defines PATH in the opensearch.yml? Would that work around the problem on NixOS?

anti-social · 2024-05-16T20:48:30Z

What if the change was something more directed, like an actual setting that defines PATH in the opensearch.yml? Would that work around the problem on NixOS?

We could explicitly set PATH to something like:

/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/run/current-system/sw/bin

But to be honest I don't like this solution.

Not passing PATH variable does not guarantee reproducibility, as user can install some programs into /usr/local/bin between running tests, which can break reproducibility. And also different operating systems have different programs inside their PATH. I understand that we do not want to take into account variables that change OpenSearch behavior but PATH is not that one.

anti-social · 2024-05-16T20:58:30Z

I think acceptable solution could be taking into account an /etc/profile file. We can run source /etc/profile in the beginning of the cli tools.

github-actions · 2024-05-16T21:15:34Z

❌ Gradle check result for 1a92df2: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Some operating systems may have a shell without default PATH variable compiled in, so we should take into account system PATH Signed-off-by: Alexander Koval <kovalidis@gmail.com>

github-actions · 2024-05-16T22:05:35Z

❌ Gradle check result for 3f8e7d4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-05-16T22:12:08Z

❌ Gradle check result for 3f8e7d4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-05-16T22:24:27Z

❌ Gradle check result for f217270: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

reta · 2024-05-16T22:51:58Z

I think acceptable solution could be taking into account an /etc/profile file. We can run source /etc/profile in the beginning of the cli tools.

I think you could set env variables while running the command line:

PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/run/current-system/sw/bin" ./gradlew :integTest

anti-social · 2024-05-17T07:27:36Z

I think acceptable solution could be taking into account an /etc/profile file. We can run source /etc/profile in the beginning of the cli tools.

I think you could set env variables while running the command line:
PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/run/current-system/sw/bin" ./gradlew :integTest

Of course I have PATH in my shell. The problem is that it is not passed when gradle runs the task.

dblock · 2024-05-17T11:56:06Z

First, thank you for hanging with us here @anti-social. I have to say, I am very uncomfortable with this change, it has a lot of side effects that can break things for developers on other platforms. Passing PATH, doing source .profile, all that looks like very intrusive workarounds. I'm not sure what's best here, but I would say something that the developer configures explicitly only on NixOS would probably be acceptable.

github-actions · 2024-05-17T15:51:05Z

❌ Gradle check result for f217270: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

anti-social requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami, tlfeng and VachaShah as code owners May 15, 2024 19:30

dblock added the skip-changelog label May 16, 2024

reta reviewed May 16, 2024

View reviewed changes

anti-social force-pushed the pass-path-env-var branch from 0eb90c9 to b820d3d Compare May 16, 2024 15:43

anti-social changed the title ~~Pass PATH environment variable when running cli tools~~ Preserve the environment when running cli tools May 16, 2024

anti-social force-pushed the pass-path-env-var branch from b820d3d to fbcc913 Compare May 16, 2024 15:58

reta reviewed May 16, 2024

View reviewed changes

anti-social force-pushed the pass-path-env-var branch from fbcc913 to 1a92df2 Compare May 16, 2024 21:12

anti-social closed this May 16, 2024

anti-social force-pushed the pass-path-env-var branch from 1a92df2 to f217270 Compare May 16, 2024 21:13

Apply /etc/profile when running CLI tools before integration tests

3f8e7d4

Some operating systems may have a shell without default PATH variable compiled in, so we should take into account system PATH Signed-off-by: Alexander Koval <kovalidis@gmail.com>

anti-social reopened this May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve the environment when running cli tools #13689

Preserve the environment when running cli tools #13689

anti-social commented May 15, 2024 •

edited

github-actions bot commented May 15, 2024

dblock commented May 16, 2024

anti-social commented May 16, 2024 •

edited

anti-social commented May 16, 2024

dblock commented May 16, 2024

reta May 16, 2024

anti-social May 16, 2024

reta May 16, 2024

anti-social May 16, 2024 •

edited

anti-social May 16, 2024

anti-social commented May 16, 2024

anti-social commented May 16, 2024 •

edited

github-actions bot commented May 16, 2024

reta May 16, 2024

anti-social May 16, 2024

anti-social May 16, 2024 •

edited

reta May 16, 2024

dblock May 16, 2024

dblock May 16, 2024

reta May 16, 2024

github-actions bot commented May 16, 2024

dblock commented May 16, 2024 •

edited

anti-social commented May 16, 2024

anti-social commented May 16, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

reta commented May 16, 2024 •

edited

anti-social commented May 17, 2024

dblock commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024

Preserve the environment when running cli tools #13689

Are you sure you want to change the base?

Preserve the environment when running cli tools #13689

Conversation

anti-social commented May 15, 2024 • edited

Description

Related Issues

Check List

github-actions bot commented May 15, 2024

dblock commented May 16, 2024

anti-social commented May 16, 2024 • edited

anti-social commented May 16, 2024

dblock commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anti-social May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anti-social commented May 16, 2024

anti-social commented May 16, 2024 • edited

github-actions bot commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anti-social May 16, 2024 • edited

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

github-actions bot commented May 16, 2024

dblock commented May 16, 2024 • edited

anti-social commented May 16, 2024

anti-social commented May 16, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

reta commented May 16, 2024 • edited

anti-social commented May 17, 2024

dblock commented May 17, 2024 • edited

github-actions bot commented May 17, 2024

anti-social commented May 15, 2024 •

edited

anti-social commented May 16, 2024 •

edited

anti-social May 16, 2024 •

edited

anti-social commented May 16, 2024 •

edited

anti-social May 16, 2024 •

edited

dblock commented May 16, 2024 •

edited

reta commented May 16, 2024 •

edited

dblock commented May 17, 2024 •

edited