Use Cloudflare's zlib in Docker images #81245

pugnascotia · 2021-12-02T09:29:09Z

Closes #81208. Elasticsearch uses zlib for two purposes:

Compression of stored fields with index.codec: best_compression,
which we use for observability and security data.
Request / response compression.

Historically, zlib was packaged within the JDK, so that users wouldn't
have to have zlib installed for basic usage of Java. However, the
original zlib optimizes for portability and misses a number of important
optimizations such as leveraging vectorization support for x86 and ARM
architectures. Several forks have been created in order to address this.

Since version 9, the JDK uses the system's zlib when available and falls
back to the zlib that is packaged within the JDK if a system zlib cannot
be found.

This commit changes the Docker image to install the Cloudflare fork of
zlib, and run Java using the fork instead of the original zlib, so that
users of the Docker image can get better performance.

Other ES distribution types are out-of-scope, since configuring the JVM
to use an alternative zlib requires an environment config as well as
installed another zlib, and Docker is the only distribution type where
we can control both.

Closes elastic#81208. Elasticsearch uses zlib for two purposes: * Compression of stored fields with `index.codec: best_compression`, which we use for observability and security data. * Request / response compression. Historically, zlib was packaged within the JDK, so that users wouldn't have to have zlib installed for basic usage of Java. However, the original zlib optimizes for portability and misses a number of important optimizations such as leveraging vectorization support for x86 and ARM architectures. Several forks have been created in order to address this. Since version 9, the JDK uses the system's zlib when available and falls back to the zlib that is packaged within the JDK if a system zlib cannot be found. This commit changes the Docker image to install the Cloudflare fork of zlib, and run Java using the fork instead of the original zlib, so that users of the Docker image can get better performance. Other ES distribution types are out-of-scope, since configuring the JVM to use an alternative zlib requires an environment config as well as installed another zlib, and Docker is the only distribution type where we can control both.

elasticmachine · 2021-12-02T09:29:13Z

Pinging @elastic/es-delivery (Team:Delivery)

elasticsearchmachine · 2021-12-02T09:29:34Z

Hi @pugnascotia, I've created a changelog YAML for you.

jpountz

Wow, thanks for putting up a PR so quickly! I wonder if there's a way we can test that this cloudflare-zlib is actually being picked up by the bundled JDK, such as running ldd /path/to/java | grep libz and making sure it's the cloudflare zlib and not the original zlib?

jpountz · 2021-12-02T10:11:36Z

distribution/docker/src/docker/bin/docker-entrypoint.sh

@@ -73,6 +73,10 @@ if [[ -n "$ES_LOG_STYLE" ]]; then
  esac
 fi

+if [[ -d /usr/local/cloudflare-zlib/lib ]]; then
+  export LD_LIBRARY_PATH=/usr/local/cloudflare-zlib/lib


should we try to preserve existing values of LD_LIBRARY_PATH, e.g.

Suggested change

export LD_LIBRARY_PATH=/usr/local/cloudflare-zlib/lib

export LD_LIBRARY_PATH=/usr/local/cloudflare-zlib/lib:$LD_LIBRARY_PATH

I wondered about that, but I also struggled to think of another reason why someone else would also be setting LD_LIBRARY_PATH with our image. Any ideas, anyone?

Also, I now think that this isn't the right place for this change, because Cloud do something different with the Docker entrypoint. I'll move this into the elasticsearch script and put a guard on it with $ES_DISTRIBUTION_TYPE.

pugnascotia · 2021-12-02T12:30:09Z

Looks like it gets picked up OK:

$ docker run -u root -it --rm elasticsearch:test bash
root@443cf81c2b16:/usr/share/elasticsearch# export LD_LIBRARY_PATH=/usr/local/cloudflare-zlib/lib
root@443cf81c2b16:/usr/share/elasticsearch# ldd jdk/bin/java
        linux-vdso.so.1 (0x0000ffff851b9000)
        libz.so.1 => /usr/local/cloudflare-zlib/lib/libz.so.1 (0x0000ffff85140000)
        libjli.so => /usr/share/elasticsearch/jdk/bin/../lib/libjli.so (0x0000ffff8511f000)
        libpthread.so.0 => /lib/aarch64-linux-gnu/libpthread.so.0 (0x0000ffff850ef000)
        libdl.so.2 => /lib/aarch64-linux-gnu/libdl.so.2 (0x0000ffff850db000)
        libc.so.6 => /lib/aarch64-linux-gnu/libc.so.6 (0x0000ffff84f68000)
        /lib/ld-linux-aarch64.so.1 (0x0000ffff85189000)

pugnascotia · 2021-12-02T13:47:14Z

@mark-vieira the packaging tests on SLES are reporting failure again, with what looks like a failure to talk to HOMER?

DJRickyB · 2021-12-02T14:14:25Z

A few thoughts on scope:

Other ES distribution types are out-of-scope, since configuring the JVM
to use an alternative zlib requires an environment config as well as
installed another zlib, and Docker is the only distribution type where
we can control both.

Now since the environment config is being applied from within bin/elasticsearch this is slightly less true. Likewise we are adding a build step for a tiny binary, the version being > 8 years old. Would we consider instead building/persisting the platform-specific libraries somewhere (possibly even in this repo) if they are small enough, rather than adding a build step to our process?

If so:

the tarball could package the library, meaning the enhancement comes on every supported platform
our nightly benchmarks could pick up the change, as they do not currently test using the docker distribution
the added library gets scoped into more integrations tests, including those driven by the rest of the stack/solutions
we can simplify the elasticsearch script condition to simply look for the library in $ES_HOME/wherever, or generically add a place for native libraries in ES_HOME and always set this

DJRickyB · 2021-12-02T14:18:35Z

a note on testing in docker, i think at this point we'd want to see the following show cloudflare-zlib in use, given setting the library path has moved out of the entrypoint script:

# bin/elasticsearch -d
# pmap -p $(pidof java)

pugnascotia · 2021-12-02T17:17:56Z

Thanks @DJRickyB, I added a test case to cover this.

jpountz · 2021-12-02T17:47:49Z

qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

+
+        final boolean matches = sh.run("bash -c 'pmap -p $(pidof java)'").stdout.lines().anyMatch(line -> line.contains("cloudflare-zlib"));
+
+        assertTrue("Expect java to be using cloudflare-zlib", matches);


nit: it would be nice to have the output of the command in the error message in case the problem is not fully reproducible?

Good idea @jpountz, I've modified the test.

distribution/docker/src/docker/Dockerfile

distribution/src/bin/elasticsearch

mark-vieira · 2021-12-02T23:53:33Z

@mark-vieira the packaging tests on SLES are reporting failure again, with what looks like a failure to talk to HOMER?

Infra merged a fix for this which might take a day to percolate through CI as the images get rebuilt.

pugnascotia · 2021-12-03T09:09:48Z

@elasticmachine run elasticsearch-ci/packaging-tests-unix

Closes #81208. Elasticsearch uses zlib for two purposes: * Compression of stored fields with `index.codec: best_compression`, which we use for observability and security data. * Request / response compression. Historically, zlib was packaged within the JDK, so that users wouldn't have to have zlib installed for basic usage of Java. However, the original zlib optimizes for portability and misses a number of important optimizations such as leveraging vectorization support for x86 and ARM architectures. Several forks have been created in order to address this. Since version 9, the JDK uses the system's zlib when available and falls back to the zlib that is packaged within the JDK if a system zlib cannot be found. This commit changes the Docker image to install the Cloudflare fork of zlib, and run Java using the fork instead of the original zlib, so that users of the Docker image can get better performance. Other ES distribution types are out-of-scope, since configuring the JVM to use an alternative zlib requires an environment config as well as installed another zlib, and Docker is the only distribution type where we can control both.

pugnascotia · 2021-12-03T10:10:11Z

Backported to 8.0 in e807fb3.

Closes #81208. Elasticsearch uses zlib for two purposes: * Compression of stored fields with `index.codec: best_compression`, which we use for observability and security data. * Request / response compression. Historically, zlib was packaged within the JDK, so that users wouldn't have to have zlib installed for basic usage of Java. However, the original zlib optimizes for portability and misses a number of important optimizations such as leveraging vectorization support for x86 and ARM architectures. Several forks have been created in order to address this. Since version 9, the JDK uses the system's zlib when available and falls back to the zlib that is packaged within the JDK if a system zlib cannot be found. This commit changes the Docker image to install the Cloudflare fork of zlib, and run Java using the fork instead of the original zlib, so that users of the Docker image can get better performance. Other ES distribution types are out-of-scope, since configuring the JVM to use an alternative zlib requires an environment config as well as installed another zlib, and Docker is the only distribution type where we can control both.

pugnascotia · 2021-12-03T11:34:43Z

Backported to 7.16 in 6582acf.

pugnascotia · 2021-12-03T14:00:37Z

Hmm, we may have missed the cut-off for 7.16.0, meaning that we'd either release this in 7.16.1 (which is questionable) or wait until 7.17.0.

mark-vieira · 2021-12-03T17:40:34Z

Would this be considered breaking in any way? What are the user facing implications here?

jpountz · 2021-12-03T17:46:44Z

FWIW my expectation was that this change would make it to 8.1 since it's an enhancement and we're past feature freeze for both 7.16 and 8.0.

mark-vieira · 2021-12-03T17:48:38Z

If we back it out of 7.16 we definitely want it in for 7.17 though. That said, we haven't created a branch for that release yet so we'd have to hold off on the backport until that is done.

This reverts commit 6582acf.

pugnascotia · 2021-12-03T19:27:12Z

OK, I've yanked it from 7.16 in 4eb9667 for now. I can apply it to 7.17 once it exists.

jpountz · 2021-12-09T17:07:03Z

I can apply it to 7.17 once it exists.

Given recent communication that 7.17 should effectively be treated as a patch release, I think we should only target 8.x with this change.

pugnascotia added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 v8.1.0 labels Dec 2, 2021

pugnascotia requested review from jpountz, mark-vieira and DJRickyB December 2, 2021 09:29

elasticmachine added the Team:Delivery Meta label for Delivery team label Dec 2, 2021

Update docs/changelog/81245.yaml

ec62027

jpountz reviewed Dec 2, 2021

View reviewed changes

Move LD_LIBRARY_PATH change to elasticsearch script

1595e6c

Add test case for zlib

08525d9

jpountz reviewed Dec 2, 2021

View reviewed changes

pugnascotia added 2 commits December 2, 2021 18:55

Emit pmap lines on assertion failure

3c6dd97

Don't bother splitting pmap output

8dc0d38

mark-vieira reviewed Dec 2, 2021

View reviewed changes

distribution/docker/src/docker/Dockerfile Show resolved Hide resolved

distribution/src/bin/elasticsearch Outdated Show resolved Hide resolved

pugnascotia added 2 commits December 2, 2021 20:21

Decouple setting LD_LIBRARY_PATH from the value to set

0d52841

Imports

3c7a030

mark-vieira approved these changes Dec 2, 2021

View reviewed changes

pugnascotia added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 3, 2021

elasticsearchmachine merged commit 1f5a0ed into elastic:master Dec 3, 2021

pugnascotia added the v7.16.0 label Dec 3, 2021

pugnascotia deleted the use-cloudflare-zlib-in-docker branch December 3, 2021 11:37

pugnascotia added a commit that referenced this pull request Dec 3, 2021

Revert "Use Cloudflare's zlib in Docker images (#81245)"

4eb9667

This reverts commit 6582acf.

This was referenced Dec 13, 2021

Should we package an alternate zlib implementation in our distributions? #81662

Open

Create a mixin for setting ES_ZLIB_PATH elastic/rally-teams#71

Closed

mark-vieira added v8.0.0-rc1 and removed v8.0.0 labels Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Cloudflare's zlib in Docker images #81245

Use Cloudflare's zlib in Docker images #81245

pugnascotia commented Dec 2, 2021

elasticmachine commented Dec 2, 2021

elasticsearchmachine commented Dec 2, 2021

jpountz left a comment

jpountz Dec 2, 2021

pugnascotia Dec 2, 2021

pugnascotia commented Dec 2, 2021 •

edited

Loading

pugnascotia commented Dec 2, 2021

DJRickyB commented Dec 2, 2021

DJRickyB commented Dec 2, 2021 •

edited

Loading

pugnascotia commented Dec 2, 2021

jpountz Dec 2, 2021

pugnascotia Dec 2, 2021

mark-vieira commented Dec 2, 2021

pugnascotia commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

mark-vieira commented Dec 3, 2021

jpountz commented Dec 3, 2021

mark-vieira commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

jpountz commented Dec 9, 2021

	export LD_LIBRARY_PATH=/usr/local/cloudflare-zlib/lib
	export LD_LIBRARY_PATH=/usr/local/cloudflare-zlib/lib:$LD_LIBRARY_PATH


		final boolean matches = sh.run("bash -c 'pmap -p $(pidof java)'").stdout.lines().anyMatch(line -> line.contains("cloudflare-zlib"));

		assertTrue("Expect java to be using cloudflare-zlib", matches);

Use Cloudflare's zlib in Docker images #81245

Use Cloudflare's zlib in Docker images #81245

Conversation

pugnascotia commented Dec 2, 2021

elasticmachine commented Dec 2, 2021

elasticsearchmachine commented Dec 2, 2021

jpountz left a comment

Choose a reason for hiding this comment

jpountz Dec 2, 2021

Choose a reason for hiding this comment

pugnascotia Dec 2, 2021

Choose a reason for hiding this comment

pugnascotia commented Dec 2, 2021 • edited Loading

pugnascotia commented Dec 2, 2021

DJRickyB commented Dec 2, 2021

DJRickyB commented Dec 2, 2021 • edited Loading

pugnascotia commented Dec 2, 2021

jpountz Dec 2, 2021

Choose a reason for hiding this comment

pugnascotia Dec 2, 2021

Choose a reason for hiding this comment

mark-vieira commented Dec 2, 2021

pugnascotia commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

mark-vieira commented Dec 3, 2021

jpountz commented Dec 3, 2021

mark-vieira commented Dec 3, 2021

pugnascotia commented Dec 3, 2021

jpountz commented Dec 9, 2021

pugnascotia commented Dec 2, 2021 •

edited

Loading

DJRickyB commented Dec 2, 2021 •

edited

Loading