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

Allow specifying VM options when running RSKj in docker container #1998

Merged
merged 4 commits into from Jun 20, 2023

Conversation

rmoreliovlabs
Copy link
Contributor

@rmoreliovlabs rmoreliovlabs commented Mar 24, 2023

Description

Allow JAVA_OPTS env variable when running docker container.

Motivation and Context

In the rskj repo we have a GitHub workflow that builds and pushes a docker image to DockerHub. Here’s a docker file for that

As of now, only a specific set of sys args is specified in the entrypoint statement and there’s no way to override or add extra vm args, eg. -Xmx or -Xss.

The idea is to define an env. var. in the Dockerfile, which would allow us to do that.

How Has This Been Tested?

Running the follwoing commands:

docker build -t rskj-node -f Dockerfile . 
docker run --name rskj-node -e DEFAULT_JVM_OPTS="-Xms3G -Xmx5G" rskj-node

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

@rmoreliovlabs rmoreliovlabs marked this pull request as ready for review March 24, 2023 21:16
@rmoreliovlabs rmoreliovlabs force-pushed the setup-vm-args-in-docker-file branch 9 times, most recently from fb3eab7 to 13f084a Compare March 25, 2023 01:25
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Vovchyk
Vovchyk previously approved these changes Mar 29, 2023
@Vovchyk
Copy link
Contributor

Vovchyk commented Mar 29, 2023

pipeline:run

@Vovchyk Vovchyk changed the title Allow JAVA_OPTS when running Dockerfile Allow specifying VM options when running RSKj in docker container Apr 12, 2023
@lucasvuotto
Copy link
Contributor

lucasvuotto commented Apr 13, 2023

Hi! In principle, I'm against the PR. The idea I had in mind when I originally added this was to provide an easy way for users to run a fullnode. It wasn't intended as a dev tool.

Regarding what you want to achieve, there are several ways:

  1. Rewrite the entrypoint in run invocation: docker run --name fullnode --entrypoint java rskj-node -Xms3G -Xmx5G -Dfoo=bar -cp rsk.jar co.rsk.Start --some --other=options
  2. Rewrite the entrypoint to just java (ENTRYPOINT java) and then the user has to provide all the flags like in the previous command, except for --entrypoint java: docker run --name fullnode rskj-node -Xms3G -Xmx5G -Dfoo=bar -cp rsk.jar co.rsk.Start --some --other=options

That being said, if this approach is insisted on, I'd like to see it working for all the examples in the README at https://hub.docker.com/r/rsksmart/rskj , and a new README will be needed for the new options: not just JAVA_OPTS, but DEFAULT_JVM_OPTS, RSKJ_SYS_PROPS and RSKJ_CLASS.

Finally, if -Xms3G -Xmx5G should be the default (haven't found any document suggesting that in a very quick search), I'm fine with adding it to the current Dockerfile.

@Vovchyk
Copy link
Contributor

Vovchyk commented Apr 13, 2023

Hi! In principle, I'm against the PR. The idea I had in mind when I originally added this was to provide an easy for users to run a fullnode. It wasn't intended as a dev tool.

Regarding what you want to achieve, there are several ways:

  1. Rewrite the entrypoint in run invocation: docker run --name fullnode --entrypoint java rskj-node -Xms3G -Xmx5G -Dfoo=bar -cp rsk.jar co.rsk.Start --some --other=options
  2. Rewrite the entrypoint to just java and then the user has to provide all the flags like in the previous command, except for --entrypoint java: docker run --name fullnode --entrypoint java rskj-node -Xms3G -Xmx5G -Dfoo=bar -cp rsk.jar co.rsk.Start --some --other=options

That being said, if this approach is insisted on, I'd like to see it working for all the examples in the README at https://hub.docker.com/r/rsksmart/rskj , and a new README will be needed for the new options: not just JAVA_OPTS, but DEFAULT_JVM_OPTS, RSKJ_SYS_PROPS and RSKJ_CLASS.

Finally, if -Xms3G -Xmx5G should be the default (haven't found any document suggesting that in a very quick search), I'm fine with adding it to the current Dockerfile.

@lucasvuotto , hmm, I guess this is a good point.

What we're trying to achieve is having minimum heap size allocated, but probably it's no need to have an upper boundary. Generally speaking, size of heap memory being allocated to a java process is underlaying system dependant, so if no vm options overwrite that the java process can get quite small amount of ram to operate with, eg. 1gb or alike. For example, ubuntu with java11 and 8gb of RAM on board allocates by default just 1.88gb to a java process, which may not be enough for RSKj to operate.

btw. there are min hardware requirements here.

Dockerfile Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Apr 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Vovchyk
Vovchyk previously approved these changes Apr 19, 2023
@rmoreliovlabs
Copy link
Contributor Author

pipeline:run

@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@Vovchyk Vovchyk merged commit 2bda949 into master Jun 20, 2023
10 checks passed
@Vovchyk Vovchyk deleted the setup-vm-args-in-docker-file branch June 20, 2023 16:14
@aeidelman aeidelman added this to the Fingerroot 5.1.0 milestone Aug 14, 2023
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.

None yet

4 participants