-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[native] Add dockerfiles for dependencies and runtime and remove existing dockerfiles #21929
Conversation
c36a5d1
to
697ecc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@majetideepak This is nice.
### dependency.dockefile | ||
This dockerfile installs all the dependencies including those needed for testing. | ||
By default, the dependencies are built in Release mode. | ||
This images needs to be built only when some dependency is updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "This images needs" -> "... image ..."
This dockerfile installs all the dependencies including those needed for testing. | ||
By default, the dependencies are built in Release mode. | ||
This images needs to be built only when some dependency is updated. | ||
Prestissimo dependencies change very infrequently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: drop "very"
centos-container: #: Build the linux container for CircleCi | ||
$(MAKE) linux-container CONTAINER_NAME=centos | ||
|
||
linux-container: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you update presto-native-execution/README.md since it has references to these deleted targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! done!
@@ -1,3 +0,0 @@ | |||
presto.version=0.279 | |||
discovery.uri=http://127.0.0.1:8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering why we don't need something like this anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configs and deployment are part of the orchestration layer. These will be added on top of the native runtime image. These configs are dynamic and cannot be hard-coded.
We have a helm project that could add these templates.
I will ask the release team to publish another image with the Java package (coordinator) added to the native runtime image along with these configs so that users can evaluate Prestissimo.
8b2a749
to
92f7f56
Compare
Is it possible to have a default |
presto-native-execution/Makefile
Outdated
# Possible values are centos, ubuntu | ||
OSNAME ?= centos | ||
ARCH ?= $(shell uname -p) | ||
DEPENDENCY_IMAGE_NAME ?= prestissimo/dependency-$(ARCH)-$(OSNAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Velox has a docker-compose.yml that allows us to have multiple images with their base image names etc. That way you can just have on command to build the dependency image and runtime image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe docker-compose is not being used on the Java packaging side. But I like the docker-compose API.
@wanglinsong Any thoughts on this?
https://github.com/facebookincubator/velox/blob/main/docker-compose.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some examples for both Prestissimo and Presto Java in the Prestorials repo: https://github.com/prestodb/prestorials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use docker compose instead of Makefile targets. I will fix this.
presto-native-execution/README.md
Outdated
Run `make runtime-container` in the presto-native-execution root directory | ||
to build run-ready containerized version of Prestissimo. Information on available | ||
configuration options can be found in [scripts/release-centos-dockerfile/README.md](scripts/release-centos-dockerfile/README.md) | ||
Run `make dependency-image && make runtime-image` in the presto-native-execution root directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe have a target called make images that is dependent on both ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do this but decided not to since Docker was rebuilding the runtime-image every time even without any changes. Likely because of some timestamp changes when it invokes make dependency-image
.
RUN mkdir build && (cd build && \ | ||
if [ "${OSNAME}" = "centos" ] ; then \ | ||
../velox/scripts/setup-centos8.sh ; \ | ||
export CC=/opt/rh/gcc-toolset-9/root/bin/gcc ; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the export env variables to just use ENV above ; I think these are not limited to just centos .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are limited to rhel(centos).
export CXX=/opt/rh/gcc-toolset-9/root/bin/g++ ; \ | ||
fi && EXTRA_CMAKE_FLAGS=${EXTRA_CMAKE_FLAGS} \ | ||
make -j$(nproc) --directory="/prestissimo/" cmake-and-build BUILD_TYPE=${BUILD_TYPE} BUILD_DIR=${BUILD_DIR} BUILD_BASE_DIR=${BUILD_BASE_DIR} | ||
RUN ldd /prestissimo/${BUILD_BASE_DIR}/${BUILD_DIR}/presto_cpp/main/presto_server | awk 'NF == 4 { system("cp " $3 " /runtime-libraries") }' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command copies the shared (dynamic) libraries required by the presto_server.
@wanglinsong Any reason for this particular file? Should we put other default |
velox.properties file is only needed by native workers to work. This is the one we use now - https://github.com/prestodb/presto/blob/native-worker-image/presto-native-execution/velox.properties. |
@wanglinsong velox.properties should be optional and limited to advanced users.
|
This script is out of date and is broken. We should seperate this work into dependency image, runtime image, and orchestration.
This script is out of date. It uses a Velox circle ci image as the base image. Presto release team should build and manage the base image.
92f7f56
to
3d42000
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks! A few nits and a small formatting suggestion, looks good.
3d42000
to
d9f9c86
Compare
@steveburnett thanks for the corrections! |
d9f9c86
to
bb3eadd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
I focused on presto-native-execution/scripts/dockerfiles/README.md.
All my requested changes from my review yesterday are incorporated now.
I reviewed the full document and find nothing else to comment about today. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I have only one question.
ENV BUILD_BASE_DIR=_build | ||
ENV BUILD_DIR="" | ||
|
||
COPY --chown=186:root --chmod=0775 --from=prestissimo-image /prestissimo/${BUILD_BASE_DIR}/${BUILD_DIR}/presto_cpp/main/presto_server /usr/local/bin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was chown necessary here to user id 186 of the build result? Do we need a comment to explain this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! We don't need chown here. I removed it.
bb3eadd
to
4e2cb0c
Compare
These dockerfiles allow building container images for dependencies and Prestissimo runtime.
4e2cb0c
to
cfff317
Compare
Description
Remove the existing dockerfiles for Prestissimo as they are out of date.
We split the release-centos-dockerfile into dependency image dockerfile
and runtime image dockerfile.
This split is helpful since the dependencies are rarely updated. Prestissimo can
now build faster from the dependency image.
The orchestration will be moved to the helm project.
The release team will also use the new dockerfiles to release official dependency
and runtime Prestissimo images.
Motivation and Context
There are no working dockerfiles for Prestissimo. This PR adds these files.
Impact
Enable official Prestissimo dependency and runtime images.
Test Plan
Approval from the release team.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.