Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to zk 3.8, upgrade to temurin java-11, delete apt cache #515

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

janhoy
Copy link

@janhoy janhoy commented Nov 23, 2022

Closes #514

Change log description

Upgrade to zookeeper 3.8. Upgrade to Temurin java-11. Delete apt cache

Purpose of the change

See #514

What the code does

Builds a better docker image

How to verify it

Build image, verify that it uses newer Zookeeper (3.8), newer Java release (Eclipse Temurin Openjdk-11), newer Linux release (Ubuntu Focal)

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (a4a345f) 85.12% compared to head (3458769) 85.08%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
- Coverage   85.12%   85.08%   -0.04%     
==========================================
  Files          12       12              
  Lines        1613     1609       -4     
==========================================
- Hits         1373     1369       -4     
  Misses        155      155              
  Partials       85       85              
Impacted Files Coverage Δ
controllers/zookeepercluster_controller.go 65.18% <0.00%> (-0.28%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

docker/Dockerfile Outdated Show resolved Hide resolved
@anishakj
Copy link
Contributor

anishakj commented Dec 2, 2022

@janhoy Could you please try scaling down of zookeeper cluster with the new image and see if dynamic config is generated. we have seen issue with 3.8.0 image. ZOOKEEPER-4530

@janhoy
Copy link
Author

janhoy commented Dec 2, 2022

@janhoy Could you please try scaling down of zookeeper cluster with the new image and see if dynamic config is generated. we have seen issue with 3.8.0 image. ZOOKEEPER-4530

I'm not able to test this now, but the zk JIRA is not closed, so why would it work? Better try to get traction on that JIRA to find the root cause? I'm afraid I don't have spare cycles to help with that.

@janhoy
Copy link
Author

janhoy commented Dec 13, 2022

So what can we do to move this forward?

@janhoy
Copy link
Author

janhoy commented Feb 28, 2023

I believe we should still publish a docker image for ZK 3.8, even if we believe ZK to have a bug with dynamic config after down-scaling. Downscaling is not a very common operation, and we should still give users of zookeeper-operator access to 3.8 features / bugfixes. Then if anyone feels down-scaling of a cluster is a blocker for them, they can override tag to continue using 3.7.x or they can help fix the root cause.

I'm going to update my PR branch to 3.8.1

@janhoy
Copy link
Author

janhoy commented Mar 8, 2023

For those who want to test the image produced by this dockerfile, here is one I built on my account:

docker run --rm janhoy/pravega-zookeeper:3.8.1-temurin

https://hub.docker.com/layers/janhoy/pravega-zookeeper/3.8.1-temurin/images/sha256-bc7c36fdedc231e99556d62bb1ef67acba1703258227ab90ab5be3f73e206ba9?context=repo

I had an issue building the amd64-image from my M1 mac with docker buildx, it was hanging during gradlew, so I had to pre-build the jar-file and just copy it when making this test repo.

Copy link

@aignatov aignatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change. Checked ZK running from this Dockerfile. It works well

@alewis001
Copy link

Hi. What needs to happen to have this change merged and a new release to include it? With the various managed Kubernetes offerings (EKS, AKS, LKE, etc.) they are all moving to a version of Kubernetes that has switched to cgroups v2 (EKS in 1.26, AKS in 1.25, etc.). That change causes Java below the latest builds of Java 8 and Java 11 to misread container CPU and memory. Java 15+ already supports cgroups v2. With this change to the Temurin based image of 11, this build would include 11.0.16 which has the cgroups v2 backported fixes/changes.

Without this change, users will see odd memory/CPU behaviour. (I.e. Java thinks it has the node's memoery, not what's allocated to the container).

@diegs
Copy link

diegs commented Jan 23, 2024

@alewis001 (and others who end up here), fwiw openjdk 11 should also have cgroups v2 after 11.0.16 (https://bugs.openjdk.org/browse/JDK-8283559).

i haven't verified but the openjdk base image has 11.0.16 (https://hub.docker.com/layers/library/openjdk/11-jdk/images/sha256-e81b7f317654b0f26d3993e014b04bcb29250339b11b9de41e130feecd4cd43c?context=explore), and a new image was built in #586

still, it would be nice to switch to a more modern image. the openjdk base image is officially deprecated per the above link

@alewis001
Copy link

@alewis001 (and others who end up here), fwiw openjdk 11 should also have cgroups v2 after 11.0.16 (https://bugs.openjdk.org/browse/JDK-8283559).

i haven't verified but the openjdk base image has 11.0.16 (https://hub.docker.com/layers/library/openjdk/11-jdk/images/sha256-e81b7f317654b0f26d3993e014b04bcb29250339b11b9de41e130feecd4cd43c?context=explore), and a new image was built in #586

still, it would be nice to switch to a more modern image. the openjdk base image is officially deprecated per the above link

Hi @diegs, thanks for the response. As it was a long time ago I can't quite remember what we did but I think we were able to bump the 11 version on what we deploy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release newer zookeeper docker image
5 participants