improve mktemp compatibility

[ademariag]

@tlrx with reference to #30999, I appreciate and respect your concerns.

The following change makes mktemp compatible with more versions of the command without changing the logic or affecting your altering the testing you currently do on your infrastructure.

Please take a look. @ndegory FYI

[elasticmachine]

Pinging @elastic/es-core-infra

[jasontedor]

Ultimately the problem with trying to make a change like this without us having the platforms this is targeted for being in our CI is that we can later refactor this and possibly break those platforms again. We do not want to take on this burden. As mentioned in one of the previously linked issues: this issue on such platforms can be worked around by setting ES_TMPDIR explicitly before starting Elasticsearch. That skips execution of this block of code to begin with.

This also has a consequence on macOS where the temporary directory will now be elasticsearch.XXXXXXX.<eight random characters>. That's ugly, and macOS is so popular a development platform I would not want to do that.

Given a workaround exists, and we don't want to take on code for unsupported platforms, I am going to close this pull request.

[kzalewski]

I disagree with closing this issue. The fix posted by @ademariag was correct and fixes a bug in the elasticsearch-env startup script that contains a broken usage of the mktemp command. On many Linux systems, mktemp -d -t elasticsearch produces an "Invalid argument" error. However, mktemp -d -t elasticsearch.XXXXXXXX works fine. You mention that on MacOS, the filename ends up being longer than the template. So what? It's a temporary directory, and the guiding principle here should be having the default startup work on the maximum number of systems. The simple fix from @ademariag achieves that. I've already spent over 3 hours on this ridiculous startup issue.

Another solution would be to add a bit more logic that can detect the MacOS version of mktemp, similar to how the script currently detects the GNU version (via a grep for "coreutils") versus the older BSD version.

[ademariag]

Thanks @kzalewski!

[hustshawn]

Hi I downloaded the ES source of 6.4.3, the mktemp issue still occurs on my alpine OS.

/usr/share/elasticsearch # elasticsearch -V
mktemp: Invalid argument
/usr/share/elasticsearch # mktemp --help
BusyBox v1.28.4 (2018-07-17 15:21:40 UTC) multi-call binary.

Usage: mktemp [-dt] [-p DIR] [TEMPLATE]

Create a temporary file with name based on TEMPLATE and print its name.
TEMPLATE must end with XXXXXX (e.g. [/dir/]nameXXXXXX).
Without TEMPLATE, -t tmp.XXXXXX is assumed.

	-d	Make directory, not file
	-q	Fail silently on errors
	-t	Prepend base directory name to TEMPLATE
	-p DIR	Use DIR as a base directory (implies -t)
	-u	Do not create anything; print a name

Base directory is: -p DIR, else $TMPDIR, else /tmp

[kzalewski]

It has not been fixed in Elasticsearch 6.5.1 either. This is literally a 30-second fix, which has already been provided by @ademariag, yet the developers do not seem motivated to implement it.

Since I build my own Slackware package for Elasticsearch, I modified my build script to perform the following fix-up:

# Add XXXXXX to end of mktemp command to fix "Invalid argument" error
sed -i 's;\(`mktemp -d -t elasticsearch\)`;\1.XXXXXX`;' bin/elasticsearch-env

[ghost]

I don't need the fix anymore, but I find the whole attitude ridiculous..
I believe ultimately Elastic wants to disincentive people from running non-approved docker images.

[jasontedor]

@ademaria Thank you for your feedback. I am sorry to hear that you feel that way.

I think we have been transparent in why we are approaching this the way that we are, and have offered a workaround that does not require us to make any changes at all.

I assure you that we do not have any other motive than remaining consistent with our development philosophy. We do not make changes in favor of platforms that we do not support. We do run our tests only on platforms that we support, so we can not carry any guarantees that we would not inadvertently break this functionality in the future. Since we offer no guarantees here, we prefer to not leave any room for doubt (implicitly or otherwise) by making changes in support of a platform that we do not support, and we do not want to carry any maintenance burden should we break this functionality in the future. Rather, we prefer to set clear expectations: there are some platforms that we do not support, and we do not make changes in support of said platforms.

Maintaining a system like Elasticsearch on the platforms that we support carries a heavy weight and we take the responsibility seriously. We can not support every platform. We dedicate our limited time and capacity to fully supporting our users on a specific set of platforms.

We have seen other communities take a different approach to these sorts of problems. For example, some communities will offer guidance along the lines of we do not support X, or we are not experts in X, we rely on the community to help us maintain in support of X. I think these approaches are admirable and noble, but we do not like the experience that it gives to users on X. Since there is no testing, effort, or experts to sustain support of X, users on X end up getting broken at unexpected times. And then they are left waiting for someone in the community to bring forward a patch to fix the problem on X. Until the next time.... This is hard on such users, hard on the community, and hard on the maintainers.

I believe ultimately Elastic wants to disincentive people from running non-approved docker images.

I assure you that this not the case at all. I encourage you to be more deliberate in assigning motives.

[kzalewski]

@jasontedor Yes, you have been transparent about your reasoning, but that doesn't make it correct. Elasticsearch should work "out of the box" with as many platforms as possible. In the earlier days of ES, this was always the case. Now, it's not.

The change that @ademaria suggested is very simple, and works with a greater number of platforms (including all platforms that you currently support) than the current startup script. There is no reason not to utilize it. Also, regarding platforms, ES is a Java service, at its core. It is already compatible with any platform that runs Java. The startup scripts are Bash and Windows batch files, which are also compatible with every platform where a user would run ES. In fact, I can't find any binary executables in the ES tar file other than some Windows and Darwin 64-bit files in the x-pack-ml module.

Platform compatibility, therefore, is not a heavy lift when 99% of the project is already cross-platform compatible by definition.

In the time that it took you to write your comment, this long-standing issue could have been fixed, once and for all.

[ademariag]

@jasontedor you make it sound super complicated but we are talking about rejecting a 9 ASCII character change for "cosmetic reasons" (on a temp dir!)

This also has a consequence on macOS where the temporary directory will now be elasticsearch.XXXXXXX.. That's ugly, and macOS is so popular a development platform I would not want to do that.

Ultimately, to me your comments, including

We do not make changes in favor of platforms that we do not support.

reads exactly what I said. Not supporting a platform (Alpine docker) especially in such a brutal, strict way is a disincentive for people that want to run ES on it. There is absolutely no shame in that, provided that you are also transparent to admit it.

[jasontedor]

Sorry folks, I have explained our development philosophy. That is the reason that we will not accept the change proposed here, the size of the change is irrelevant, and it has nothing to do with serving as a disincentive to users using non-official Docker images. We are not going to support every platform, or even "as many platforms as possible". We have to make trade-offs on where we focus our efforts. There is more to supporting a change than the lines of code that are behind it.

There is another approach here, which is to completely remove the use of mktemp. We have a long-standing goal to minimize our startup scripts to the bare-minimum, instead executing startup in Java. This will remove a lot of platform-specific code (Windows versus Linux/macOS), instead relying on the JVM to handle cross-platform concerns for us. We have taken steps previously towards this with the Java version checker, and the JVM options parser, and @rjernst is currently working on taking this even further. As part of that effort, we can take mktemp out of the startup scripts, and instead do it in Java. Then it would run on Alpine without any problems (I have tested this). See #36002 which will be merged later today.

I hope that this solution is acceptable to you.

[ghost]

That is a fantastic answer and solution. Thank you so much.

[jasontedor]

You're welcome.