Add a docker-compose file and dockerfiles #249

avantgardnerio · 2022-09-19T16:26:24Z

Which issue does this PR close?

Closes #248 .

Rationale for this change

Described in the issue.

What changes are included in this PR?

Add dockerfiles
Add a docker-compose file

Are there any user-facing changes?

They should find dockerfiles and be happy.

avantgardnerio · 2022-09-19T16:36:25Z

@andygrove for your review queue

avantgardnerio · 2022-09-19T16:37:01Z

.dockerignore

@@ -5,7 +5,9 @@ dev/dist
 dev/release
 python
 **/docs
-**/target
+target/


The proper way to declare a folder

avantgardnerio · 2022-09-19T16:37:23Z

.dockerignore

 **/CHANGELOG.md
 **/tests
 **/data
+!target/release/ballista-scheduler
+!target/release/ballista-executor


Can't exclude the executables if we want them to go into the docker image.

Is this change still needed now that the executables are build in Docker?

Never mind ... after reviewing the rest of the PR I understand why we have this

avantgardnerio · 2022-09-19T16:37:56Z

.gitignore

@@ -96,3 +96,6 @@ venv/*

 # apache release artifacts
 dev/dist
+
+# logs
+logs/


Running Ballista in any way seems to put logs here that aren't gitignored, which is a pain for developers to not accidentally get them into PRs.

andygrove · 2022-09-19T17:12:43Z

scheduler.Dockerfile

+
+EXPOSE 50050
+
+CMD ./ballista-scheduler


What is the goal of these dockerfiles versus the existing ones in dev/docker? These are for faster dev testing as opposed to building releases?

We need to start the web ui as well, which we have with the existing dockerfiles under dev/docker

What is the goal of these dockerfiles

Oh, as stated in the issue, it wasn't discoverable due to

find . -name *.Dockerfile

Returning no results. I see now that it returns ./dev/docker/ballista-arm64.Dockerfile because it is capitalised according to the standard.

I'll amend this PR to use those instead, and capitalize them as most devops engineers would expect.

avantgardnerio · 2022-09-19T18:58:37Z

For future reference, the command was

find . -name "*.dockerfile" | sed -rn 's/(.*)\.dockerfile/\1.dockerfile \1.Dockerfile/p' | xargs -r -I {} bash -c "git mv {}"

avantgardnerio · 2022-09-19T19:31:47Z

dev/build-set-env.sh

@@ -17,4 +17,4 @@
 # specific language governing permissions and limitations
 # under the License.

-export BALLISTA_VERSION=$(awk -F'[ ="]+' '$1 == "version" { print $2 }' ballista/rust/core/Cargo.toml)


@andygrove When I changed the format of the toml file before to not redundantly specify the version, and you said you thought it broke the build somehow, is this why?

I think if we let cargo metadata do the toml parsing, then any correct toml syntax will work. awk was just pattern matching.

avantgardnerio · 2022-09-20T04:12:15Z

ballista/rust/client/src/context.rs

+            .read_csv(path, options)
+            .await
+            .map_err(|e| {
+                DataFusionError::Context(format!("Can't read CSV: {}", path), Box::new(e))


Say file location when we can't load it

avantgardnerio · 2022-09-20T04:12:44Z

ballista/ui/yarn.lock

@@ -0,0 +1,4 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Best practice to check this in, just like cargo.lock

avantgardnerio · 2022-09-20T04:13:40Z

benchmarks/tpch-gen.sh


 # Generate data into the ./data directory if it does not already exist
 FILE=./data/supplier.tbl
 if test -f "$FILE"; then
    echo "$FILE exists."
 else
  mkdir data 2>/dev/null
-  docker run -v `pwd`/data:/data -it --rm ballista-tpchgen:$BALLISTA_VERSION
+  docker run -v `pwd`/data:/data -it --rm ghcr.io/databloom-ai/tpch-docker:main -vf -s 1


Use off-the-shelf data generator to go faster. No need to re-invent this wheel.

avantgardnerio · 2022-09-20T04:14:07Z

dev/build-ballista-docker-arm64.sh

@@ -19,7 +19,7 @@

 if [ -z "${DOCKER_REPO}" ]; then
  echo "DOCKER_REPO env var must be set"
-  exit -1
+  exit 1


Negative error codes aren't actually valid?

avantgardnerio · 2022-09-20T04:14:38Z

dev/build-ballista-docker.sh

@@ -19,9 +19,13 @@

 set -e

+docker build -t ballista-builder --build-arg EXT_UID="$(id -u)" -f dev/docker/ballista-builder.Dockerfile .


Make a build container so that WSL & mac users are happy

avantgardnerio · 2022-09-20T04:15:18Z

dev/build-ballista-docker.sh

@@ -19,9 +19,13 @@

 set -e

+docker build -t ballista-builder --build-arg EXT_UID="$(id -u)" -f dev/docker/ballista-builder.Dockerfile .
+
+docker run -v $(pwd):/home/builder/workspace ballista-builder


Build in the build container, but don't use ephemeral storage and wipe everything so we have faster build & test cycles, but CI is still pure.

avantgardnerio · 2022-09-20T04:15:44Z

dev/build-ballista-docker.sh

+
+docker run -v $(pwd):/home/builder/workspace ballista-builder
+
+docker-compose build


docker-compose is our new build script

avantgardnerio · 2022-09-20T04:16:06Z

dev/build-ballista-docker.sh

We only need the BALLISTA_VERSION when tagging, not buiding.

avantgardnerio · 2022-09-20T04:16:30Z

dev/build-set-env.sh

@@ -17,4 +17,6 @@
 # specific language governing permissions and limitations
 # under the License.

-export BALLISTA_VERSION=$(awk -F'[ ="]+' '$1 == "version" { print $2 }' ballista/rust/core/Cargo.toml)
+cd ballista/rust/core/
+export BALLISTA_VERSION=$(cargo pkgid | cut '-d@' -f2)


The simplest version of this (that is correct) I could find.

avantgardnerio · 2022-09-20T04:17:03Z

dev/docker/ballista-benchmarks.Dockerfile

@@ -15,14 +15,21 @@
 # specific language governing permissions and limitations
 # under the License.

-ARG VERSION
-FROM apache/arrow-ballista:$VERSION
+FROM ubuntu:22.04


Use an OS that developers can install themselves for dev/prod parity.

avantgardnerio · 2022-09-20T04:17:27Z

dev/docker/ballista-benchmarks.Dockerfile

Copy in binaries to separate code build from docker build.

avantgardnerio · 2022-09-20T04:18:03Z

dev/docker/ballista-builder.Dockerfile


 RUN apt-get update && \
-    apt-get install -y git build-essential
+    apt-get -y install libssl-dev openssl zlib1g zlib1g-dev libpq-dev cmake protobuf-compiler netcat curl unzip \
+    nodejs npm && \


We probably want a newer node at some point, but I ran into a lot of trouble with every non-apt option.

andygrove · 2022-09-20T13:02:52Z

You can exclude yarn.lock from the RAT checks by adding it to dev/release/rat_exclude_files.txt

andygrove · 2022-09-20T13:50:25Z

I just tried this out on Ubuntu 20.04 after running docker system prune -a to remove any previous images and I ran into a couple issues that I was able to fix with the following patch:

diff --git a/dev/build-ballista-docker.sh b/dev/build-ballista-docker.sh
index d8af64b5..5efbb152 100755
--- a/dev/build-ballista-docker.sh
+++ b/dev/build-ballista-docker.sh
@@ -23,9 +23,9 @@ docker build -t ballista-builder --build-arg EXT_UID="$(id -u)" -f dev/docker/ba
 
 docker run -v $(pwd):/home/builder/workspace ballista-builder
 
-docker-compose build
+docker compose build
 
 . ./dev/build-set-env.sh
-docker tag apache/arrow-ballista-executor "apache/arrow-ballista-executor:$BALLISTA_VERSION"
-docker tag apache/arrow-ballista-scheduler "apache/arrow-ballista-scheduler:$BALLISTA_VERSION"
-docker tag apache/arrow-ballista-benchmarks "apache/arrow-ballista-benchmarks:$BALLISTA_VERSION"
+docker tag ballista-executor "apache/arrow-ballista-executor:$BALLISTA_VERSION"
+docker tag ballista-scheduler "apache/arrow-ballista-scheduler:$BALLISTA_VERSION"
+docker tag ballista-benchmarks "apache/arrow-ballista-benchmarks:$BALLISTA_VERSION"
diff --git a/docker-compose.yml b/docker-compose.yml
index cbfb400c..1865db69 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -48,7 +48,7 @@ services:
     depends_on:
       - ballista-scheduler
   ballista-client:
-    image: ballista-client
+    image: ballista-benchmarks
     build:
       dockerfile: dev/docker/ballista-benchmarks.Dockerfile
       context: .

This got me further - the images were built and tagged - but the integration test failed with:

Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "/run.sh": stat /run.sh: no such file or directory: unknown

avantgardnerio · 2022-09-20T15:18:16Z

+docker compose build

I don't understand how this works for you. I tested in docker in Ubuntu 20.04:

root@6b1248d5c2bd:/# docker --help | grep compose

root@6b1248d5c2bd:/# cat /etc/*-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=20.04
DISTRIB_CODENAME=focal
...

root@6b1248d5c2bd:/# docker compose

docker: 'compose' is not a docker command.
See 'docker --help'

root@6b1248d5c2bd:/# docker-compose
Define and run multi-container applications with Docker.

Usage:

The command is docker-compose unless I'm going crazy.

avantgardnerio · 2022-09-20T15:26:17Z

bgardner@bg-tensorbook:~$ docker --version
Docker version 20.10.12, build 20.10.12-0ubuntu4

and in docker ubuntu 20.04:

root@46a85dc6e8a1:/# docker --version
Docker version 20.10.12, build 20.10.12-0ubuntu2~20.04.1

avantgardnerio · 2022-09-20T17:25:51Z

@andygrove I take it back. I think this would be good to merge as soon as you feel the issues you listed have been addressed. Unfortunately I do not know how to reproduce, therefor fix, the one you are seeing with docker-compose not working.

avantgardnerio · 2022-09-20T17:36:49Z

Everything works now on a system with docker-compose including the integration-tests.sh

andygrove

Thanks @avantgardnerio! This looks great. I tested locally after removing all Ballista images again and it worked perfectly.

It would be good if @yahoNanJing and @thinkharderdev could also test this out in their environments.

andygrove · 2022-09-21T14:52:23Z

@yahoNanJing @thinkharderdev ok if I merge this, or do you want to test first?

thinkharderdev

This looks awesome! Thanks @avantgardnerio

andygrove · 2022-09-21T19:42:46Z

Thanks for the review @thinkharderdev. I will go ahead and merge this now.

avantgardnerio mentioned this pull request Sep 19, 2022

Add scheduler UI to ballista-scheduler Docker image #238

Merged

avantgardnerio commented Sep 19, 2022

View reviewed changes

andygrove reviewed Sep 19, 2022

View reviewed changes

github-actions bot added the development-process label Sep 19, 2022

avantgardnerio commented Sep 19, 2022

View reviewed changes

avantgardnerio commented Sep 20, 2022

View reviewed changes

Brent Gardner added 2 commits September 20, 2022 11:24

Fix docker

677c4d2

fmt

8eb30be

avantgardnerio force-pushed the bg_fix_docker branch from e45183e to 8eb30be Compare September 20, 2022 17:25

Fix integration tests

3dec1b5

Fix PR feedback

8c8b367

andygrove approved these changes Sep 20, 2022

View reviewed changes

thinkharderdev approved these changes Sep 21, 2022

View reviewed changes

andygrove merged commit d76a8eb into apache:master Sep 21, 2022

avantgardnerio deleted the bg_fix_docker branch September 22, 2022 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a docker-compose file and dockerfiles #249

Add a docker-compose file and dockerfiles #249

avantgardnerio commented Sep 19, 2022

avantgardnerio commented Sep 19, 2022

avantgardnerio Sep 19, 2022

avantgardnerio Sep 19, 2022

andygrove Sep 20, 2022

andygrove Sep 20, 2022

avantgardnerio Sep 19, 2022

andygrove Sep 19, 2022

avantgardnerio Sep 19, 2022

avantgardnerio commented Sep 19, 2022

avantgardnerio Sep 19, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

avantgardnerio Sep 20, 2022

andygrove commented Sep 20, 2022

andygrove commented Sep 20, 2022 •

edited

Loading

avantgardnerio commented Sep 20, 2022

avantgardnerio commented Sep 20, 2022

avantgardnerio commented Sep 20, 2022

avantgardnerio commented Sep 20, 2022

andygrove left a comment

andygrove commented Sep 21, 2022

thinkharderdev left a comment

andygrove commented Sep 21, 2022

		@@ -0,0 +1,4 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

		@@ -19,9 +19,13 @@

		set -e

		docker build -t ballista-builder --build-arg EXT_UID="$(id -u)" -f dev/docker/ballista-builder.Dockerfile .


		docker run -v $(pwd):/home/builder/workspace ballista-builder

		docker-compose build


		EXPOSE 50050

		CMD ./ballista-scheduler

Add a docker-compose file and dockerfiles #249

Add a docker-compose file and dockerfiles #249

Conversation

avantgardnerio commented Sep 19, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

avantgardnerio commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avantgardnerio commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Sep 20, 2022

andygrove commented Sep 20, 2022 • edited Loading

avantgardnerio commented Sep 20, 2022

avantgardnerio commented Sep 20, 2022

avantgardnerio commented Sep 20, 2022

avantgardnerio commented Sep 20, 2022

andygrove left a comment

Choose a reason for hiding this comment

andygrove commented Sep 21, 2022

thinkharderdev left a comment

Choose a reason for hiding this comment

andygrove commented Sep 21, 2022

andygrove commented Sep 20, 2022 •

edited

Loading