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

Minor cleanups to docker image #8925

Merged
merged 4 commits into from
Feb 11, 2024
Merged

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Feb 1, 2024

Pull Request Description

Added some sensible defaults which all can be overriden from the outside.
Most importantly, we want to use MaxRAMPercentage java option rather than mx because the former plays better in the container setting.

Important Notes

I wasn't able to test it with cloud's project manager, just as a standalone run so this will need more testing. Also not sure how the image is currently invoked.

from a desired _edition_. The root directory of the docker build context can be
provided in the `docker build` command:

```bash
Copy link
Member

Choose a reason for hiding this comment

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

certain number of directories to be present

What's the command to invoke before invoking docker build to get the "certain number of directories"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's this argument built-distribution/enso-engine-$VERSION-linux-amd64/enso-$VERSION

docker build -t <my-custom-name> -f tools/ci/docker/DockerFile --build-context docker-tools=tools/ci/docker built-distribution/enso-engine-$VERSION-linux-amd64/enso-$VERSION
```

where for a locally built distribution on Linux it would be `VERSION=0.0.0-dev`.
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind having sbt buildDockerImage that would invoke buildXyz and then passed in the right VERSION variable...

Copy link
Member

Choose a reason for hiding this comment

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

Another advantage of sbt buildDockerImage would be that it could depend on buildEngineDistribution, from which it copies the directories.

ARG RPC_PORT=30001
ARG DATA_PORT=30002
ARG PRINT_VERSION=0
ARG JAVA_OPTS="-XX:MaxRAMPercentage=90.0 -XX:InitialRAMPercentage=90.0"
Copy link
Member

Choose a reason for hiding this comment

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

How did you find these are the right options? Do you have any reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Past experience but not tested in battlefield with Enso. Gives enough room when JVM decides to go over the limit (which it occasionally can do).

@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 2, 2024
@hubertp hubertp merged commit b00dc9e into develop Feb 11, 2024
28 of 29 checks passed
@hubertp hubertp deleted the wip/hubert/8897-docker-cleanup branch February 11, 2024 20:52
mergify bot pushed a commit that referenced this pull request Feb 13, 2024
Since #8925 `--build-context` flag must be given to build the runtime image. However, it can be used only with BuildKit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants