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

Add docker image, devcontainer, and editorconfig support #151

Merged
merged 30 commits into from
Jun 4, 2022

Conversation

bpkroth
Copy link
Collaborator

@bpkroth bpkroth commented Apr 18, 2022

Here's a simple PR to add support for developing inside a container either with VSCode or Github Codespaces.
This makes it a little quicker/easier to get all the necessary dependencies for building the project without bloating the rest of the dev environment.

It also adds very basic EditorConfig support for setting very basic editor tweaks for the repo's coding style (I tried to make them match the existing styles).

Update: also adds support for building a benchbase docker image that can be used for easy deployment for distributed benchmarking and some pipeline test cases for building it.
This does not yet handle publishing the image to a known container image repository for docker pull support yet though.

@apavlo apavlo requested a review from timveil April 19, 2022 02:35
@apavlo
Copy link
Member

apavlo commented Apr 19, 2022

@bpkroth Is it common practice to put the Dockerfile in the root dir? I just don't know what people normally do.

@bpkroth
Copy link
Collaborator Author

bpkroth commented Apr 19, 2022

@bpkroth Is it common practice to put the Dockerfile in the root dir? I just don't know what people normally do.

It depends. It is possible to hide it inside the .devcontainer/ directory for instance, but keeping it at the root level is not uncommon and makes discovery easy for a number of tools/users that aren't vscode/codespaces.

Maintainers choice I would say. This is just a convenience feature that I added locally for myself while I was working on #147. Figured it might be nice for others as well.

@bpkroth
Copy link
Collaborator Author

bpkroth commented Apr 19, 2022

One other comment: since this was just meant for local dev purposes, it doesn't copy the source files into the container image (instead relies on the -v volume mount option), so it isn't directly deployable as an image as is. That is easy/possible to add if desired as well.
If you're interested in having that support as well, I can refactor to place it in the docker/ directory and add some usage instructions to the README.

UPDATE: added this too now. May help for distributing basebench as a docker image for automated testing.
The resulting image is a little large atm since each profile contains a lot of duplicate content. The only real optimization I did there was to have them all share a single data/ directory.

bpkroth added 2 commits April 19, 2022 15:06
to also support deploying and running a built copy, not just for local
devcontainer purposes

most things are controllable via environment variables

- BENCHBASE_PROFILES (full image build)
- BENCHBASE_PROFILE (which profile to run)
@samialabed
Copy link

Very interested in this PR (especially with the deployable image, which will simplify using benchbase to run experiments on the cloud), thanks!

It might be outside the scope of this PR but would be good to take in options to override the config file URLs, port, and passwords. I suspect if the image is used in deployment these need to be configurable too.
Happy to extend this PR with that.
Would you suggest a python script that overrides these as an entry point in the image? or is there a cleaner way to change the configs inside docker.

@bpkroth
Copy link
Collaborator Author

bpkroth commented Apr 20, 2022

Very interested in this PR (especially with the deployable image, which will simplify using benchbase to run experiments on the cloud), thanks!

It might be outside the scope of this PR but would be good to take in options to override the config file URLs, port, and passwords. I suspect if the image is used in deployment these need to be configurable too. Happy to extend this PR with that. Would you suggest a python script that overrides these as an entry point in the image? or is there a cleaner way to change the configs inside docker.

I agree CLI options to adjust the configs seems somewhat nice, but wasn't something I wanted to tackle here.

As a workaround for now, you can do something like this to map the configfile from the host into the container:

docker run -v $PWD/configs/myconfig.xml:/benchbase/configs/myconfig.xml benchbase -c /benchbase/configs/myconfig.xml ...

Alternatively, what you suggest about using environment variables to pass in basic connection info and then rewrite a config template before executing the jar could work, but I suspect becomes complex as well. For instance:

docker run --env BENCHBASE_PROFILE=postgres --env CONNECT_URL=jdbc:postgres://.... benchbase -b tpcc # -c omitted intentionally

Could then use the BENCHBASE_PROFILE and CONNECT_URL environment variables and -b argument to select an appropriate config template and then adjust the dburl, username, and password properties using the CONNECT_URL, CONNECT_USER, and CONNECT_PASS environment variables, or something along those lines.
Past that, I think it becomes cumbersome to override all of the other potential config file settings using just environment variables or cli args.

Anyways, happy to hear actual maintainer thoughts about this, but I do agree that having deployable docker support would be very useful for scaling automated benchmarks on various platforms (e.g. k8s, Azure Batch, etc.).

@bpkroth bpkroth marked this pull request as draft April 22, 2022 21:40
bpkroth added 3 commits April 22, 2022 16:57
this avoids having to publish the docker image somewhere for a
downstream job to use

however, it requires use to manually lookup the docker network that the
test database service was setup with to connect the container to

we also have to dynamically rewrite the sample config to connect with a
name instead of localhost since the benchmark is running inside a
separate container
@bpkroth bpkroth changed the title Add devcontainer and editorconfig support Add docker image, devcontainer, and editorconfig support Apr 25, 2022
@bpkroth bpkroth marked this pull request as ready for review April 25, 2022 19:01
@bpkroth
Copy link
Collaborator Author

bpkroth commented Apr 25, 2022

Spent some time to get the docker image testing into the CI pipelines.
I think this is ready for review now (the current build failure looks transient and unrelated to me - I saw it during the SqlServer stuff as well).

@apavlo
Copy link
Member

apavlo commented Apr 27, 2022

@timveil Can you review this?

@timveil
Copy link
Collaborator

timveil commented Apr 27, 2022

Yes will do!

@bpkroth
Copy link
Collaborator Author

bpkroth commented May 24, 2022

Yes will do!

Any updates on this?

@timveil
Copy link
Collaborator

timveil commented May 24, 2022

Let me review this asap! Thank you for the reminder.

@apavlo
Copy link
Member

apavlo commented Jun 2, 2022

@bpkroth Can you resolve the conflicts? And then I will try to review this so we can merge.

@bpkroth
Copy link
Collaborator Author

bpkroth commented Jun 3, 2022

@bpkroth Can you resolve the conflicts? And then I will try to review this so we can merge.

Just did. Will see if it needs any CI adjustments after the other recent changes shortly. Thanks!

&& rm -rf /benchbase/.git /benchbase/target /benchbase/pom.xml

# Add an additional layer that also includes a built copy of the source.
FROM devcontainer AS fullimage
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, this is useful for deploying the image as benchmarking agent in k8s, Azure Batch, etc.
However, it probably doesn't need a full JDK at that point.
One can save a little bit of space by removing the /benchbase/src tree and the openssh-client (and apt autoremoveing to get rid of the dependencies), but the real wins are in having the fullimage use the JRE instead of the JDK.
I'm gonna call that future improvements for now though.

Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

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

LGTM

@apavlo apavlo merged commit 21265a6 into cmu-db:main Jun 4, 2022
@bpkroth bpkroth deleted the bpkroth/devcontainer branch December 11, 2023 16:54
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.

4 participants