Skip to content

Commit 4001097

Browse files
[test] packaging: use shell when running commands (#30852)
When subprocesses are started with ProcessBuilder, they're forked by the java process directly rather than from a shell, which can be surprising for our use case here in the packaging tests which is similar to scripting. This commit changes the tests to run their subprocess commands in a shell, using the bash -c <script> syntax for commands on linux and using the powershell.exe -Command <script> syntax for commands on windows. This syntax on windows is essentially what the tests were already doing.
1 parent ad0dc58 commit 4001097

File tree

5 files changed

+101
-68
lines changed

5 files changed

+101
-68
lines changed

qa/vagrant/README.md

+14-26
Original file line numberDiff line numberDiff line change
@@ -82,38 +82,26 @@ In general it's probably best to avoid running external commands when a good
8282
Java alternative exists. For example most filesystem operations can be done with
8383
the java.nio.file APIs. For those that aren't, use an instance of [Shell](src/main/java/org/elasticsearch/packaging/util/Shell.java)
8484

85-
Despite the name, commands run with this class are not run in a shell, and any
86-
familiar features of shells like variables or expansion won't work.
87-
88-
If you do need the shell, you must explicitly invoke the shell's command. For
89-
example to run a command with Bash, use the `bash -c command` syntax. Note that
90-
the entire script must be in a single string argument
85+
This class runs scripts in either bash with the `bash -c <script>` syntax,
86+
or in powershell with the `powershell.exe -Command <script>` syntax.
9187

9288
```java
9389
Shell sh = new Shell();
94-
sh.run("bash", "-c", "echo $foo; echo $bar");
95-
```
9690

97-
Similary for powershell - again, the entire powershell script must go in a
98-
single string argument
91+
// equivalent to `bash -c 'echo $foo; echo $bar'`
92+
sh.bash("echo $foo; echo $bar");
9993

100-
```java
101-
sh.run("powershell.exe", "-Command", "Write-Host $foo; Write-Host $bar");
94+
// equivalent to `powershell.exe -Command 'Write-Host $foo; Write-Host $bar'`
95+
sh.powershell("Write-Host $foo; Write-Host $bar");
10296
```
10397

104-
On Linux, most commands you'll want to use will be executable files and will
105-
work fine without a shell
106-
107-
```java
108-
sh.run("tar", "-xzpf", "elasticsearch-6.1.0.tar.gz");
109-
```
98+
### Notes about powershell
11099

111-
On Windows you'll mostly want to use powershell as it can do a lot more and
112-
gives much better feedback than Windows' legacy command line. Unfortunately that
113-
means that you'll need to use the `powershell.exe -Command` syntax as
114-
powershell's [Cmdlets](https://msdn.microsoft.com/en-us/library/ms714395.aspx)
115-
don't correspond to executable files and are not runnable by `Runtime` directly.
100+
Powershell scripts for the most part have backwards compatibility with legacy
101+
cmd.exe commands and their syntax. Most of the commands you'll want to use
102+
in powershell are [Cmdlets](https://msdn.microsoft.com/en-us/library/ms714395.aspx)
103+
which generally don't have a one-to-one mapping with an executable file.
116104

117-
When writing powershell commands this way, make sure to test them as some types
118-
of formatting can cause it to return a successful exit code but not run
119-
anything.
105+
When writing powershell commands in this project it's worth testing them by
106+
hand, as sometimes when a script can't be interpreted correctly it will
107+
fail silently.

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Archives.java

+23-22
Original file line numberDiff line numberDiff line change
@@ -64,19 +64,20 @@ public static Installation installArchive(Distribution distribution, Path fullIn
6464
if (distribution.packaging == Distribution.Packaging.TAR) {
6565

6666
if (Platforms.LINUX) {
67-
sh.run("tar", "-C", baseInstallPath.toString(), "-xzpf", distributionFile.toString());
67+
sh.bash("tar -C " + baseInstallPath + " -xzpf " + distributionFile);
6868
} else {
6969
throw new RuntimeException("Distribution " + distribution + " is not supported on windows");
7070
}
7171

7272
} else if (distribution.packaging == Distribution.Packaging.ZIP) {
7373

7474
if (Platforms.LINUX) {
75-
sh.run("unzip", distributionFile.toString(), "-d", baseInstallPath.toString());
75+
sh.bash("unzip " + distributionFile + " -d " + baseInstallPath);
7676
} else {
77-
sh.run("powershell.exe", "-Command",
77+
sh.powershell(
7878
"Add-Type -AssemblyName 'System.IO.Compression.Filesystem'; " +
79-
"[IO.Compression.ZipFile]::ExtractToDirectory('" + distributionFile + "', '" + baseInstallPath + "')");
79+
"[IO.Compression.ZipFile]::ExtractToDirectory('" + distributionFile + "', '" + baseInstallPath + "')"
80+
);
8081
}
8182

8283
} else {
@@ -102,35 +103,35 @@ public static Installation installArchive(Distribution distribution, Path fullIn
102103
private static void setupArchiveUsersLinux(Path installPath) {
103104
final Shell sh = new Shell();
104105

105-
if (sh.runIgnoreExitCode("getent", "group", "elasticsearch").isSuccess() == false) {
106+
if (sh.bashIgnoreExitCode("getent group elasticsearch").isSuccess() == false) {
106107
if (isDPKG()) {
107-
sh.run("addgroup", "--system", "elasticsearch");
108+
sh.bash("addgroup --system elasticsearch");
108109
} else {
109-
sh.run("groupadd", "-r", "elasticsearch");
110+
sh.bash("groupadd -r elasticsearch");
110111
}
111112
}
112113

113-
if (sh.runIgnoreExitCode("id", "elasticsearch").isSuccess() == false) {
114+
if (sh.bashIgnoreExitCode("id elasticsearch").isSuccess() == false) {
114115
if (isDPKG()) {
115-
sh.run("adduser",
116-
"--quiet",
117-
"--system",
118-
"--no-create-home",
119-
"--ingroup", "elasticsearch",
120-
"--disabled-password",
121-
"--shell", "/bin/false",
116+
sh.bash("adduser " +
117+
"--quiet " +
118+
"--system " +
119+
"--no-create-home " +
120+
"--ingroup elasticsearch " +
121+
"--disabled-password " +
122+
"--shell /bin/false " +
122123
"elasticsearch");
123124
} else {
124-
sh.run("useradd",
125-
"--system",
126-
"-M",
127-
"--gid", "elasticsearch",
128-
"--shell", "/sbin/nologin",
129-
"--comment", "elasticsearch user",
125+
sh.bash("useradd " +
126+
"--system " +
127+
"-M " +
128+
"--gid elasticsearch " +
129+
"--shell /sbin/nologin " +
130+
"--comment 'elasticsearch user' " +
130131
"elasticsearch");
131132
}
132133
}
133-
sh.run("chown", "-R", "elasticsearch:elasticsearch", installPath.toString());
134+
sh.bash("chown -R elasticsearch:elasticsearch " + installPath);
134135
}
135136

136137
public static void verifyArchiveInstallation(Installation installation, Distribution distribution) {

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Cleanup.java

+12-12
Original file line numberDiff line numberDiff line change
@@ -59,16 +59,16 @@ public static void cleanEverything() {
5959
if (Platforms.WINDOWS) {
6060

6161
// the view of processes returned by Get-Process doesn't expose command line arguments, so we use WMI here
62-
sh.runIgnoreExitCode("powershell.exe", "-Command",
62+
sh.powershellIgnoreExitCode(
6363
"Get-WmiObject Win32_Process | " +
6464
"Where-Object { $_.CommandLine -Match 'org.elasticsearch.bootstrap.Elasticsearch' } | " +
65-
"ForEach-Object { $_.Terminate() }");
65+
"ForEach-Object { $_.Terminate() }"
66+
);
6667

6768
} else {
6869

69-
sh.runIgnoreExitCode("pkill", "-u", "elasticsearch");
70-
sh.runIgnoreExitCode("bash", "-c",
71-
"ps aux | grep -i 'org.elasticsearch.bootstrap.Elasticsearch' | awk {'print $2'} | xargs kill -9");
70+
sh.bashIgnoreExitCode("pkill -u elasticsearch");
71+
sh.bashIgnoreExitCode("ps aux | grep -i 'org.elasticsearch.bootstrap.Elasticsearch' | awk {'print $2'} | xargs kill -9");
7272

7373
}
7474

@@ -78,8 +78,8 @@ public static void cleanEverything() {
7878

7979
// remove elasticsearch users
8080
if (Platforms.LINUX) {
81-
sh.runIgnoreExitCode("userdel", "elasticsearch");
82-
sh.runIgnoreExitCode("groupdel", "elasticsearch");
81+
sh.bashIgnoreExitCode("userdel elasticsearch");
82+
sh.bashIgnoreExitCode("groupdel elasticsearch");
8383
}
8484

8585
// delete files that may still exist
@@ -95,27 +95,27 @@ public static void cleanEverything() {
9595
// disable elasticsearch service
9696
// todo add this for windows when adding tests for service intallation
9797
if (Platforms.LINUX && isSystemd()) {
98-
sh.run("systemctl", "unmask", "systemd-sysctl.service");
98+
sh.bash("systemctl unmask systemd-sysctl.service");
9999
}
100100
}
101101

102102
private static void purgePackagesLinux() {
103103
final Shell sh = new Shell();
104104

105105
if (isRPM()) {
106-
sh.runIgnoreExitCode("rpm", "--quiet", "-e", "elasticsearch", "elasticsearch-oss");
106+
sh.bashIgnoreExitCode("rpm --quiet -e elasticsearch elasticsearch-oss");
107107
}
108108

109109
if (isYUM()) {
110-
sh.runIgnoreExitCode("yum", "remove", "-y", "elasticsearch", "elasticsearch-oss");
110+
sh.bashIgnoreExitCode("yum remove -y elasticsearch elasticsearch-oss");
111111
}
112112

113113
if (isDPKG()) {
114-
sh.runIgnoreExitCode("dpkg", "--purge", "elasticsearch", "elasticsearch-oss");
114+
sh.bashIgnoreExitCode("dpkg --purge elasticsearch elasticsearch-oss");
115115
}
116116

117117
if (isAptGet()) {
118-
sh.runIgnoreExitCode("apt-get", "--quiet", "--yes", "purge", "elasticsearch", "elasticsearch-oss");
118+
sh.bashIgnoreExitCode("apt-get --quiet --yes purge elasticsearch elasticsearch-oss");
119119
}
120120
}
121121
}

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Platforms.java

+6-6
Original file line numberDiff line numberDiff line change
@@ -28,41 +28,41 @@ public static boolean isDPKG() {
2828
if (WINDOWS) {
2929
return false;
3030
}
31-
return new Shell().runIgnoreExitCode("which", "dpkg").isSuccess();
31+
return new Shell().bashIgnoreExitCode("which dpkg").isSuccess();
3232
}
3333

3434
public static boolean isAptGet() {
3535
if (WINDOWS) {
3636
return false;
3737
}
38-
return new Shell().runIgnoreExitCode("which", "apt-get").isSuccess();
38+
return new Shell().bashIgnoreExitCode("which apt-get").isSuccess();
3939
}
4040

4141
public static boolean isRPM() {
4242
if (WINDOWS) {
4343
return false;
4444
}
45-
return new Shell().runIgnoreExitCode("which", "rpm").isSuccess();
45+
return new Shell().bashIgnoreExitCode("which rpm").isSuccess();
4646
}
4747

4848
public static boolean isYUM() {
4949
if (WINDOWS) {
5050
return false;
5151
}
52-
return new Shell().runIgnoreExitCode("which", "yum").isSuccess();
52+
return new Shell().bashIgnoreExitCode("which yum").isSuccess();
5353
}
5454

5555
public static boolean isSystemd() {
5656
if (WINDOWS) {
5757
return false;
5858
}
59-
return new Shell().runIgnoreExitCode("which", "systemctl").isSuccess();
59+
return new Shell().bashIgnoreExitCode("which systemctl").isSuccess();
6060
}
6161

6262
public static boolean isSysVInit() {
6363
if (WINDOWS) {
6464
return false;
6565
}
66-
return new Shell().runIgnoreExitCode("which", "service").isSuccess();
66+
return new Shell().bashIgnoreExitCode("which service").isSuccess();
6767
}
6868
}

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Shell.java

+46-2
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.util.HashMap;
3030
import java.util.Map;
3131
import java.util.Objects;
32+
import java.util.stream.Stream;
3233

3334
import static java.util.Collections.emptyMap;
3435

@@ -57,15 +58,58 @@ public Shell(Map<String, String> env, Path workingDirectory) {
5758
this.workingDirectory = workingDirectory;
5859
}
5960

60-
public Result run(String... command) {
61+
/**
62+
* Runs a script in a bash shell, throwing an exception if its exit code is nonzero
63+
*/
64+
public Result bash(String script) {
65+
return run(bashCommand(script));
66+
}
67+
68+
/**
69+
* Runs a script in a bash shell
70+
*/
71+
public Result bashIgnoreExitCode(String script) {
72+
return runIgnoreExitCode(bashCommand(script));
73+
}
74+
75+
private static String[] bashCommand(String script) {
76+
return Stream.concat(Stream.of("bash", "-c"), Stream.of(script)).toArray(String[]::new);
77+
}
78+
79+
/**
80+
* Runs a script in a powershell shell, throwing an exception if its exit code is nonzero
81+
*/
82+
public Result powershell(String script) {
83+
return run(powershellCommand(script));
84+
}
85+
86+
/**
87+
* Runs a script in a powershell shell
88+
*/
89+
public Result powershellIgnoreExitCode(String script) {
90+
return runIgnoreExitCode(powershellCommand(script));
91+
}
92+
93+
private static String[] powershellCommand(String script) {
94+
return Stream.concat(Stream.of("powershell.exe", "-Command"), Stream.of(script)).toArray(String[]::new);
95+
}
96+
97+
/**
98+
* Runs an executable file, passing all elements of {@code command} after the first as arguments. Throws an exception if the process'
99+
* exit code is nonzero
100+
*/
101+
private Result run(String[] command) {
61102
Result result = runIgnoreExitCode(command);
62103
if (result.isSuccess() == false) {
63104
throw new RuntimeException("Command was not successful: [" + String.join(" ", command) + "] result: " + result.toString());
64105
}
65106
return result;
66107
}
67108

68-
public Result runIgnoreExitCode(String... command) {
109+
/**
110+
* Runs an executable file, passing all elements of {@code command} after the first as arguments
111+
*/
112+
private Result runIgnoreExitCode(String[] command) {
69113
ProcessBuilder builder = new ProcessBuilder();
70114
builder.command(command);
71115

0 commit comments

Comments
 (0)