use spaces liberally in integration tests and fix space handling

[rmuir]

By using pathnames with spaces in tests we can kickout all the bugs.
I applied the fix for #12709 but we needed more fixes actually.

TODO: windows

[rjernst]

LGTM

[rmuir]

Everything in the root of our ES dir has a space, so a few parameters for pidfile, repo path, classpath, etc have spaces. ES home itself has a space, and cwd (which is different) has a space. bin/plugin is also fed urls with spaces in the plugin smoketester.

I removed all of ant argline usage (except what is fed by jenkins, which is gc params etc) to reduce the possibility of bugs, it makes the testing clear.

TODO: fix any bat files for windows. Test the ES_GC_LOG_FILE by just setting one up in integ tests. I will look into these tomorrow.

[jaymode]

+1 thank you for doing this

[nik9000]

LGTM

[rmuir]

See my comment, i am not sure this is really 100% correct still. try something like this:

diff --git a/dev-tools/src/main/resources/ant/integration-tests.xml b/dev-tools/src/main/resources/ant/integration-tests.xml
index f67701b..2b45b05 100644
--- a/dev-tools/src/main/resources/ant/integration-tests.xml
+++ b/dev-tools/src/main/resources/ant/integration-tests.xml
@@ -140,6 +140,7 @@
       <attribute name="es.http.port" default="${integ.http.port}"/>
       <attribute name="es.transport.tcp.port" default="${integ.transport.port}"/>
       <attribute name="es.pidfile" default="${integ.pidfile}"/>
+      <attribute name="es.gc.logfile" default="${integ.scratch}/gc@{es.http.port}.log"/>
       <attribute name="jvm.args" default="${tests.jvm.argline}"/>
     <sequential>

@@ -152,6 +153,7 @@
           <env key="JAVA_HOME" value="${java.home}"/>
           <!-- we pass these as gc options, even if they arent, to avoid conflicting gc options -->
           <env key="ES_GC_OPTS" value="@{jvm.args}"/>
+          <env key="ES_GC_LOG_FILE" value="@{es.gc.logfile}"/>
           <arg value="-Des.cluster.name=@{es.cluster.name}"/>
           <arg value="-Des.http.port=@{es.http.port}"/>
           <arg value="-Des.transport.tcp.port=@{es.transport.tcp.port}"/>

Problem is that JAVA_OPTS and ES_JAVA_OPTS are not handled correctly and all folded in one string i think, and need something like the previous 'eval' reparsing that was happening (but ONLY for those two, not the rest of the commandline, and hopefully cleaner).

I am going to go for a long run today and try to catch up on some errands. If someone who actually knows bash wants to take this from me, i would be more than happy. I dont know shell, i just hack until mvn verify passes. And there is still windows to possibly fix :(

But I think we want to fix this soonish, so argument processing is really correct.

[rmuir]

and the fastest way to iterate here is to change files then do ./run.sh which uses all of this logic, but runs in foreground and is easy to debug.

[uschindler]

The usual horror :) it's so simple with ant...

please fix!

[rmuir]

Also the way ES here accepts -D arguments, parses them as es parameters only to call System.setProperty on each one, that's totally bogus. These need to go to the JVM, or it causes very difficult-to-debug issues, like if you set -Djava.io.tmpdir=..., its not really happening until too late, and by then JVM has already initialized e.g. temp file helpers with the default tmpdir in clinits, so your system property does not have the effect you like.

[rmuir]

for the last part, just change Bootstrap parser to require es prefix on all those -D's and fail with an exception otherwise. Then there is no confusion.

[rmuir]

The worst is leniency, $ bin/elasticsearch -Doops.a.typo=4 does nothing but silently work. These settings need to be in a static list somewhere. plugins can register their own lists (could be managed as e.g. properties file, maybe needs wildcard support) and we fail if its an unknown setting.

[jasontedor]

@rmuir There are related issues for that: #6732, and #12657 (the latter being closed in favor of the former).

Setting an unknown setting, or a setting that can't be changed should throw an error.

[rmuir]

That issue is full of confusion and nonsense. It should be done in a simple way with e.g. properties file for checks (and plugins can have them too). each property (prolly needs wildcard/pattern in some case from what i see) should include its type as well (if its being accessed by getAsBoolean its a boolean, etc), and a short description. This also means its easy to do really nice stuff like list all supported properties, "did you mean" in error messages, etc later.

[clintongormley]

Even with this PR, wildcards are still problematic:

bin/elasticsearch --http.cors.enabled=true --http.cors.allow-origin='*'  -d

Fails with:

ERROR: Parameter [-d]does not start with --

If the -d comes before the wildcard, then it works

[spinscale]

@clintongormley looks like a parser issue (independent from this PR), when regular arguments come after the --foo.bar style args.. investigating