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

BOOKKEEPER-974: Add an official bookkeeper docker image #197

Closed
wants to merge 2 commits into from

Conversation

caliuf
Copy link
Contributor

@caliuf caliuf commented Jun 21, 2017

Docker directory now contains two sub dirs, "4.4.0" that will contain the centos docker build and "4.4.0-alpine" that contains the alpine build.


# -------------- #
# Wait for zookeeper server
#set +x
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these lines supposed to be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have un-commented them, it could be useful to wait for zk before start bookkeeper. Maybe can be enhanced with a timeout.

docker/Makefile Outdated

run-bk:
# Temporary gimmick: clear all data because of bookkeeper blocking check on host / data integrity
#-sudo rm -rf $(BK_LOCAL_CONTAINER_DATA_DIR)
Copy link
Contributor

@fpj fpj Jun 21, 2017

Choose a reason for hiding this comment

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

Should we get rid of this commented line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

-docker rm -f $(CONTAINER_NAME)
docker run -it\
--network $(DOCKER_NETWORK) \
--volume $(BK_LOCAL_CONTAINER_DATA_DIR)/journal:/data/journal \
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are multiple disk devices available, will the different directories be mapped to them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to manually mount them. E.g:
--volume /real/path/1:/data/journal/journal-1
--volume /real/path/2:/data/journal/journal-2

Copy link
Member

Choose a reason for hiding this comment

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

Can the script handle this case?

# Create and run a container running the bookkeeper tutorial application (a simple dice rolling application). It's possibile
# to run several dice applications in order to simulate a real life concurrent scenario.
# make run-dice
run-dice:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are to have this example in the makefile, then it might be a better idea to bring the example into bk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying to bring the tutorial code in bk repo and have a docker build for that image? I think it could be a good idea (so the code will be updated with releases), but it need a general consensus. @sijie @jvrao @jiazhai @eolivelli what do you think about it?

Copy link
Member

Choose a reason for hiding this comment

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

It is a good idea. Following @fpj's suggestion, the first step would be bring @ivankelly 's code into bookkeeper repo? We may need @ivankelly's idea and help on this. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 from me on making the tutorial easier to run. What is the purpose of this Makefile? A brief glance suggests it's for running multiple containers together. docker-compose may be better for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, docker-compose was my first-choice, but it lacks a way to start containers in a prefixed order, and this made it impossible to execute metaformat before running bk containers. With last additions to bk image, docker-compose could be used to run zk, the ensemble and tutorials instance.

BTW, I've done some changes in order to use the tutorial code in its own docker image. They can be found here: https://github.com/caiok/bookkeeper-tutorial

Maybe they could be merged before adding the code to bk, but some cleanup it's needed. I could do this if there is consensus about bringing this code in bk.

Even adding a docker-compose example could be much useful then the Makefile one.

Copy link
Member

Choose a reason for hiding this comment

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

@CaioK : I remembered that pulsar is using docker-compose to start zk, bk, and pulsar brokers. @merlimat can help confirm that.

@jiazhai @CaioK : can you guys create an issue to bring Ivan's tutorial to bookkeeper-turorials/dice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jiazhai
Copy link
Member

jiazhai commented Jun 21, 2017

There seems be an empty file: "docker/4.4.0/.keep", what is the reason to have it?



# -------------- #
# Trying to format bookkeeper dir on zookeeper. If the dir already exists, it does nothing.
Copy link
Member

Choose a reason for hiding this comment

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

How does it achieve this? Seems line 62 will always run?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should put the format in any entry script. this is dangerous. it should be run as a separate admin command.

Copy link
Member

Choose a reason for hiding this comment

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

Put it here will make the automatic deploy easy, or user (like in K8s/DCOS) will add some additional manual steps for the format things.
Usually only the first bookie should do the format, If the dirs, such as ../ledger/readonly, are there, the following bookies will check and not do format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've messed up someway this line before open the PR, the correct one is obviously "exec bookkeeper shell metaformat -nonInteractive || true"

@sijie This command should ignore format if it find zk dir structure already formatted, right? What kind of issues can it arise? (just my curiosity)
@jiazhai I will do metaformat only if it's passed a non empty env var BK_TRY_METAFORMAT, it should satisfy both requirements.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me, but It is better to check the related dirs in zookeeper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jiazhai If metaformat has not been performed the bk container could be stuck forever with the current operations order. If otherwise put metaformat before zk check, it could die because zk is not reachable in that moment. Maybe we could maybe do: zk port check, metaformat (if env var provided) and then zk dir check?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I got it. "nonInteractive" will abort the format, if it as been formatted before.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally I am hesitated to putting any format command in a script that will run every time. That is DANGEROUS to me.

@merlimat how do you handle this in pulsar docker image?

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

A few general comments:

  • It might be good to organize the Dockerfile in docker/// ?
  • for current version, it might be worth having a latest directory. when we cut a release, we copy the latest directory to the targeted release directory. for example, in 4.5.0, we can copy latest to a directory 4.5.0. Is that doable?

@@ -0,0 +1,59 @@
FROM java:openjdk-8-jre-alpine
MAINTAINER Francesco Caliumi <francesco.caliumi@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how does it work here.

shall it be "Apache BookKeeper" dev@bookkeeper.apache.org ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have usually seen physical persons in MAINTAINER command, but I'm fine with your proposal

@@ -0,0 +1,59 @@
FROM java:openjdk-8-jre-alpine
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to have a template and generate Docker files based on different JDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. I use to put placeholders and then have a makefile that generates the real dockerfile. It could be a good be a good idea, even for the bookkeeper version (I don't know how much bk changes between releases, but I suppose that the dockerfile, run.sh and healthcheck.sh will remain the same).

I'm not sure if to introduce this change now or in a second step. @jiazhai @fpj @merlimat Any preferences / ideas?


ENV ZK_SERVERS= \
BK_USER=bookkeeper \
BK_PORT= \
Copy link
Member

Choose a reason for hiding this comment

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

shall we provide a default BK_PORT? and what is the difference between BK_PORT and BK_BUILD_PORT?

Copy link
Member

Choose a reason for hiding this comment

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

Second this, Better to provide default values for these ENV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blank is the default so it let the run.sh script to know that the env variable has not been set by "docker run" and let the config file original value. I'm fine anyway in providing a default here with documentation purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sijie @jiazhai I remembered the reason behind this implementation. We provide to docker user the ability to mount configurations file at run time. These files will be rewritten with the env variables set at run. If we do provide a default for the port, in run.sh we have no way to determine if was passed by the user or it's the default, so it will be rewritten in each case, overwriting the port specified by the user in conf file. I will restore the BK_PORT=[blank] and BK_BUILD_PORT

Copy link
Member

Choose a reason for hiding this comment

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

ack

BK_PORT= \
BK_BUILD_PORT=3181 \
BOOKIE_OPTS="" \
BK_JOURNAL_DIR=/data/journal \
Copy link
Member

Choose a reason for hiding this comment

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

two comments for the disk layouts

  • it might be good to put directories under /data/bookkeeper. so it would be /data/bookkeeper/journal, /data/bookkeeper/ledger, /data/bookkeeper/index.
  • also it might be good to allow configuring multiple directories. for example, if user says two journal dirs, it should generate /data/bookkeeper/journal-0 and /data/bookkeeper/journal-1. if user only configures one journal dir, it can be /data/bookkeeper/journal.

Copy link
Member

Choose a reason for hiding this comment

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

FYI. there is also cmd line command to do the config: #75

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sijie

  1. IMO it's not particularly relevant because it's just a docker container internal dir and no other process are going to run in it out of bk
  2. The env variables BK_xxx_DIR are not in my opinion particularly useful to set up, being an internal of docker container. For achieving your configuration one has only to mount that journal-0, journal-1 dirs in docker container. If it's not clear I could explain it more in depth

Copy link
Member

Choose a reason for hiding this comment

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

I think my point is to allow user configuring multiple directories for journal / ledger / index. I am not sure how it is achieved, by overriding the env variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, env vars refer to internal container directories and I don't think it's very useful changing them.
Current build already creates three different dirs, so the end user just have to mount the desired host dirs to these dirs. Take a look at makefile example (target run-bk) for seeing this in action. It's just like the common mounting mechanism in unix, the target system will not notice the difference.

&& wget -q "https://archive.apache.org/dist/bookkeeper/bookkeeper-${BK_VERSION}/${DISTRO_NAME}.tar.gz" \
&& wget -q "https://archive.apache.org/dist/bookkeeper/bookkeeper-${BK_VERSION}/${DISTRO_NAME}.tar.gz.asc" \
&& export GNUPGHOME="$(mktemp -d)" \
# && gpg --keyserver ha.pool.sks-keyservers.net --recv-key "$GPG_KEY" \
Copy link
Member

Choose a reason for hiding this comment

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

why they are commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix and un-comment them

&& rm -r "$GNUPGHOME" "$DISTRO_NAME.tar.gz" "$DISTRO_NAME.tar.gz.asc" \
&& apk del .build-deps

ENV BK_DIR=/opt/bookkeeper-server-${BK_VERSION}
Copy link
Member

Choose a reason for hiding this comment

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

how about /opt/bookkeeper/${BK_VERSION} ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the version from the directory.. anyway, one container will have just one version of bookkeeper. Having the version means some tools and script will need to be updated each time.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for what @merlimat said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree

cp -vaf /conf/* ${BK_DIR}/conf || true
chown -R "$BK_USER" ${BK_DIR}/conf

# Bookkeeper setup
Copy link
Member

Choose a reason for hiding this comment

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

for the environment variables, can we have a script:

  • in the dockerfile, taking a bookkeeper template configuration and export them as the environment. for example, for a setting 'x', we can export it as 'BK_${X}'.
  • in the run.sh script, looping over the template configuration and for each parameters, check if its corresponding environment variable is set or not. if it is set, replace the settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the first step really needed or we could use the default bk_server.conf and then in run phase use found BK_xxx env variables for substitute confs in it? Furthermore, the first step will brake the possibility to mount an already set configuration file (it would be entirely rewritten).

Copy link
Member

Choose a reason for hiding this comment

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

I think Matteo's py script should already address this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matteo's py only handles not commented out properties present in original files. It needs to be enhanced with handling of commented ones IMHO, in order to not create confusion (if the doc says "you can set BK_xxxx for setting corresponding BK property" I expect this behavior, if it don't work it's not kind to demand the user dig in the code to figure out the reason). If we provide default conf files with all properties not commented out, then the problem arise if user mount it's own conf files.



# -------------- #
# Trying to format bookkeeper dir on zookeeper. If the dir already exists, it does nothing.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should put the format in any entry script. this is dangerous. it should be run as a separate admin command.

bash \
su-exec

ENV ZK_SERVERS= \
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a way to control all the variables that are there in bk_server.conf. For reference take a look at https://github.com/apache/incubator-pulsar/blob/master/docker/scripts/apply-config-from-env.py

The variables are being taken from the ENV and applied to the config files.

All the defaults are kept in the conf file.

Copy link
Member

Choose a reason for hiding this comment

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

+1. It is a great py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this script doesn't handle the commented out option lines, like the majority of options in https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/conf/bk_server.conf . It's a good idea anyway, and I'm fine to execute this script before the current substitutions done in run.sh. Maybe in a second step with a version that handles even commented out options?

Copy link
Member

Choose a reason for hiding this comment

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

is this approach applied to current pull request? or are you planning to do it in a separate pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it hasn't been applied, yet. I just created #260

wget \
&& mkdir -pv /opt \
&& cd /opt \
&& wget -q "https://archive.apache.org/dist/bookkeeper/bookkeeper-${BK_VERSION}/${DISTRO_NAME}.tar.gz" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of downloading the release, we should build the docker image from the newly build JARs. That way, as part of a release we can automatically push out the docker images

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docker build process will be executed from docker hub (it will retrieve dockerfile and than make the build every night or so). For creating an image for the the latest commit (the "on build" image, like it's usually named) we need to publish that jars anyway.

set -x -e -u

# Simple healtcheck on BK port
nc -z localhost ${BK_PORT}
Copy link
Contributor

Choose a reason for hiding this comment

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

This check only probes the port. We should have a more thorough probe here. For example with the coommand bookkeeper shell bookiesanity, that creates a ledger, writes a few entries, reads them and deletes the ledger.
In that way most critical paths are being exercised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting suggestion. Doing it every 3 seconds doesn't impact performances, right?

@sijie
Copy link
Member

sijie commented Jun 28, 2017

@CaioK any updates on this pull request?

ENV PATH=$PATH:${BK_DIR}/bin

WORKDIR ${BK_DIR}
VOLUME ["/conf", "${BK_JOURNAL_DIR}", "${BK_LEDGER_DIR}", "${BK_INDEX_DIR}"]
Copy link
Member

Choose a reason for hiding this comment

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

Hi @CaioK , Seems in this code, the configuration is depends on local dir mounted to "/conf", It is good to run docker locally, but for K8S and DC/OS, it is hard to have one "local dir" contains user specified data, because all disk resources are managed by and shared in K8S or DC/OS, when disk is arriving to satisfy user request, it usually contains nothing.
This problem also nags me for a while. I even thought to provide a way, which read the configurations from an internet place, but that seems not very user friendly. Until @merlimat provide the way to read env variables inside docker.
And this maybe the reason that @sijie, @merlimat and me are so care about the configurations and env variables.

@jiazhai
Copy link
Member

jiazhai commented Jul 19, 2017

Generally, looks good to me. We may merge it in as the first stage, and polish it, while using it.

@CaioK Still have one question of why we have 2 sub dirs -- "4.4.0", and "4.4.0-alpine", it seems not convient. This way contains a lot of duplicate code, and harder to maintain; and at last, when Create Automated Docker Build, only one Dockerfile is allowed provided to docker, we will have to make a decision to choose based on CentOS/Alpine sooner or later.

Here are 2 raw ideas from my view:
1, Let's only keep the alpine version at this time?
2, It maybe better to place the version info(4.4.0) into Makefile, and remove dir 4.4.0. It seems strange for dirs named by a version in master branch. Version could be added as the docker image version.

@caliuf
Copy link
Contributor Author

caliuf commented Jul 19, 2017

@jiazhai
It's ok to have such a structure. We need it in order to provide docker images of older versions. Having a CentOS image is ok too, when licensing concerns not allow to run alpine. Take a look at other "official" images, you will see that no images provide only one tag. The postgres example is quite complete.

It's normal that centos and alpine version shares a lot of code, unfortunately, but I don't see any solution to that. To reduce code replication between releases of the same os, I think in the end @sijie solution to generate versions from a template is the best.

@sijie
I missed your question: "for current version, it might be worth having a latest directory. when we cut a release, we copy the latest directory to the targeted release directory. for example, in 4.5.0, we can copy latest to a directory 4.5.0. Is that doable?"
It is possible and simple to create an "onbuild" docker build (not to be confused with "latest" that in docker means the latest stable version) if maybe in jenkins job we run the "package" target and then we publish the produced packet somewhere. Otherwise it's still possible to create a docker build that checkout the last commit (at risk to have something broken, with jenkins we can publish the package only if all tests pass), package it, unpack it in the usual dir and then clean everything it's not needed runtime, but it's require some effort, doubled by alpine + centos dichotomy, so it's better to create a dedicated issue.

@jiazhai
Copy link
Member

jiazhai commented Jul 19, 2017

Hi @CaioK,
Postgres example is not a "Automated Docker Build", While if bookkeeper image goes into apache, we could reference the apache examples.

@caliuf
Copy link
Contributor Author

caliuf commented Jul 19, 2017

@jiazhai Seems that Apache examples are a bit poor on this side. Here an example (the first I found) with an automated build with tags. You can refer to the official documentation in order to understand how it works. How you can see, you have a lot of flexibility about tags.

@jiazhai
Copy link
Member

jiazhai commented Jul 21, 2017

@CaioK, Thanks for the explain.
It is better to handle the code duplications in dirs 4.4.0 and 4.4.0-alpine. If it is hard to resolve, and CentOS version is preferred, It is Ok to keep CentOS version.
It is better to put version information into related version branch, like in branch-4.4 including docker files for docker-4.4.0, not in master. Master branch is better to always keep files for the latest docker file. We could do that in your example, but that seems a little mess.

@caliuf
Copy link
Contributor Author

caliuf commented Jul 21, 2017

@jiazhai
What are your concerns about this code duplication? I think providing a centos image for who is concerned about license and compatibility and an alpine one for who is concerned security and performances could be nice. There is no simple way to use only one copy of the files for both (you need to create a template to "compile" in respective files).

On image versions I fear the branch solution could rapidly became very uncomfortable (and I think this is the reason I never seen it used, in main images). If you released 3 versions and find a dockerfile bug, you have to commit this change on every branch? (you than have one or more commits "detached" from the release tag, that is maybe a little dirty).
I prefer @sijie solution with templates in order to reduce code duplication between different versions but I'm not closed to alternatives (even the branch one) if there is consensus.

@caliuf
Copy link
Contributor Author

caliuf commented Jul 25, 2017

@sijie @fpj @merlimat Any other doubts or requests for this PR? We need it merged before proceeding with the related issues (and any thoughts on these will be much appreciated too)

@sijie
Copy link
Member

sijie commented Jul 25, 2017

@CaioK : regarding the versioning here, I am not sure whether we need to keep the version here. Ideally the docker image should be built along with an apache release. For example when we cut an apache release 4.5.0, along with the release procedure, we will build the 4.5.0 image and push it to apache docker hub. The docker files for 4.5.0 will live in the branch 4.5.0. The docker files in master will evolve with the changes in next release (for example 4.6.0). That means the docker files for image 4.5.0 will live only in branch 4.5.0 and subsequent changes to 4.5.0 will happen only in branch 4.5.0 and new changes will live in master. How does that sound?

I see a few issues created for followup actions. I am fine with merging this pull request and iterate after that if we can achieve basic consensus here.

@fpj @merlimat : since you were reviewing this pull request, please try to take a look at this pull request when you have time. I'd like to move this forward with 4.5.0 release.

@caliuf
Copy link
Contributor Author

caliuf commented Jul 25, 2017

@sijie Thanks for the reply. We don't push images on dockerhub, dockerhub checkout our dockerfiles and builds them. Check this page in order to have an idea of the process.
Current dockerfile will be useless for master version until the package will be released somewhere (but we can create an onbuild dockerfile that checkout and build the code in the container).
Anyway, I'm fine to let version x.y.z of docker build to live on branch x.y.z, if this is the prevalent preference (there are some inconveniences that I explained to Jia, though)

@sijie
Copy link
Member

sijie commented Jul 25, 2017

@CaioK

Let me try to summarize here. There are two approaches:

  1. the approach suggested in this pull request. a summary for this approach.
  • use the docker hub autobuilds.
  • download released package in docker build
  • maintain different docker files for different versions

pro: you can use docker hub autobuilds the images
cons:

  • you have to manage the docker hub account in order to build the images, because you have to create the builds in docker hub side. in apache, we might not have access to apache docker account.
  1. the approach other people suggested in this pull request.
  • use the apache/travis ci build, build the image along as part of the CI
  • maintain only one copy of docker files for each release/branches.
  • push the ci built image to apache docker hub account (e.g. docker push)

pro: the image is built as part of CI and aligned with apache release. and we can simply use an API token from apache docker account to push built image and tag them.
cons: managing versions might be not as easy as the first approach, although I am not very sure.

Approach 2) is aligned with apache release and it is easy to configure if we want to push to apache docker account. 1) will be a bit tricky, not sure how we can create an autobuild in apache docker account.

Hope this summary is clear for the people involved in this pull request.

COPY run.sh healthcheck.sh /opt/

ENTRYPOINT [ "/bin/bash", "/opt/run.sh" ]
CMD ["bookkeeper", "bookie"]
Copy link
Contributor

Choose a reason for hiding this comment

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

@CaioK - here is the snippet you asked for (from my local Dockerfile)
CMD /bookkeeper/bookkeeper-server/bin/bookkeeper shell bookieformat -nonInteractive -force -deleteCookie; /bookkeeper/bookkeeper-server/bin/ bookkeeper bookie; /bookkeeper/bookkeeper-server/bin/bookkeeper autorecovery

@sijie
Copy link
Member

sijie commented Jul 27, 2017

notes about the docker image from today's meeting

  • follow what other apache projects are doing zeppelin
  • using docker autobuild
  • dockerfile is to download official bk release image
  • remove versions. docker autobuild will look for docker file location and release tags.
  • os versions. let's stick to centos-7 and jdk-8.
  • configuration: 1) allow providing a configuration from a mounted volume 2) if no configuration file is provided, it will use the template packaged in the binary and take the ENV variables that start with "bk_"
  • entry point: we need an entry point to start and stop bookie gracefully
  • documentation: docker-compose file to start a cluster for testing
  • optional: k8s and dcos instructions (can be separate pull requests)

@sijie
Copy link
Member

sijie commented Jul 28, 2017

@CaioK @jiazhai as we discussed in the community meeting, do you guys want to work together to move this forward?

@jiazhai
Copy link
Member

jiazhai commented Jul 29, 2017

@sijie @CaioK How about merge this first, and open another issue to improve it following the comments above?

@caliuf
Copy link
Contributor Author

caliuf commented Jul 31, 2017

Thanks @sijie for the recap of community meeting decisions.

@jiazhai I'm ok with merging this and move on. I will open new issues for what is still missing.

@caliuf caliuf mentioned this pull request Jul 31, 2017
10 tasks
@sijie
Copy link
Member

sijie commented Jul 31, 2017

@jiazhai @CaioK what is going to be reused here? as we discussed, we need to keep only one version. I am wondering why not just close this pull request and start a new one that incorporates the comments. that would be much clearer, right?

@jiazhai
Copy link
Member

jiazhai commented Aug 1, 2017

@sijie, I am OK with your idea to start a new one, since this will be changed a lot.
It depends on @CaioK's decision.

@caliuf
Copy link
Contributor Author

caliuf commented Aug 1, 2017

@sijie @jiazhai I agree, let's close this PR and start a new one. I will work on it as soon as possible.

@zhaijack
Copy link
Contributor

zhaijack commented Aug 1, 2017

@CaioK I am also starting do it. How could we collaborate together on it?

@jiazhai jiazhai mentioned this pull request Aug 1, 2017
10 tasks
@caliuf
Copy link
Contributor Author

caliuf commented Aug 1, 2017

@jiazhai As a first thing we should decide the issues we want included in this PR. I suggest #327 and the base step for #289 that, if we decide to implement this issue, should change the dir structure from a "all in docker in root dir" to a "general files in root dir and build files in two subdirs: release and onbuild". I prepend for the letter.

This should be quite easy and I'd like to create PR tonight. Then we can divide remaining issue between us and work in parallel. Sounds good to you?

I created #docker-image slack channel so we can decide quickly and report here the decisions.

jiazhai pushed a commit that referenced this pull request Aug 6, 2017
This is the first part of #335. And it is based on #197
Main changes:
 327: Docker image: Drop versions and Alpine support.
 260: Docker image: provide a way to pass any desired configuration property via ENV vars.

---
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

- [X] Make sure the PR title is formatted like:
    `<Issue # or BOOKKEEPER-#>: Description of pull request`
    `e.g. Issue 123: Description ...`
    `e.g. BOOKKEEPER-1234: Description ...`
- [ ] Make sure tests pass via `mvn clean apache-rat:check install findbugs:check`.
- [X] Replace `<Issue # or BOOKKEEPER-#>` in the title with the actual Issue/JIRA number.

---

Author: zhaijack <zhaijia03@gmail.com>

Reviewers: Matteo Merli <None>, Sijie Guo <None>

This closes #342 from zhaijack/issue_338, closes #338
dlg99 pushed a commit to dlg99/bookkeeper that referenced this pull request Aug 9, 2017
…ggestions

This is the first part of apache#335. And it is based on apache#197
Main changes:
 327: Docker image: Drop versions and Alpine support.
 260: Docker image: provide a way to pass any desired configuration property via ENV vars.

---
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

- [X] Make sure the PR title is formatted like:
    `<Issue # or BOOKKEEPER-#>: Description of pull request`
    `e.g. Issue 123: Description ...`
    `e.g. BOOKKEEPER-1234: Description ...`
- [ ] Make sure tests pass via `mvn clean apache-rat:check install findbugs:check`.
- [X] Replace `<Issue # or BOOKKEEPER-#>` in the title with the actual Issue/JIRA number.

---

Author: zhaijack <zhaijia03@gmail.com>

Reviewers: Matteo Merli <None>, Sijie Guo <None>

This closes apache#342 from zhaijack/issue_338, closes apache#338
sijie pushed a commit that referenced this pull request Aug 11, 2017
This is the first part of #335. And it is based on #197
Main changes:
 327: Docker image: Drop versions and Alpine support.
 260: Docker image: provide a way to pass any desired configuration property via ENV vars.

---
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

- [X] Make sure the PR title is formatted like:
    `<Issue # or BOOKKEEPER-#>: Description of pull request`
    `e.g. Issue 123: Description ...`
    `e.g. BOOKKEEPER-1234: Description ...`
- [ ] Make sure tests pass via `mvn clean apache-rat:check install findbugs:check`.
- [X] Replace `<Issue # or BOOKKEEPER-#>` in the title with the actual Issue/JIRA number.

---

Author: zhaijack <zhaijia03@gmail.com>

Reviewers: Matteo Merli <None>, Sijie Guo <None>

This closes #342 from zhaijack/issue_338, closes #338
sijie pushed a commit to sijie/bookkeeper that referenced this pull request Aug 14, 2017
…ggestions

This is the first part of apache#335. And it is based on apache#197
Main changes:
 327: Docker image: Drop versions and Alpine support.
 260: Docker image: provide a way to pass any desired configuration property via ENV vars.

---
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

- [X] Make sure the PR title is formatted like:
    `<Issue # or BOOKKEEPER-#>: Description of pull request`
    `e.g. Issue 123: Description ...`
    `e.g. BOOKKEEPER-1234: Description ...`
- [ ] Make sure tests pass via `mvn clean apache-rat:check install findbugs:check`.
- [X] Replace `<Issue # or BOOKKEEPER-#>` in the title with the actual Issue/JIRA number.

---

Author: zhaijack <zhaijia03@gmail.com>

Reviewers: Matteo Merli <None>, Sijie Guo <None>

This closes apache#342 from zhaijack/issue_338, closes apache#338
sijie added a commit to sijie/bookkeeper that referenced this pull request Jan 26, 2018
Descriptions of the changes in this PR:

add one page for how to deploy on k8s. it is based on http://bookkeeper.apache.org/docs/latest/deployment/kubernetes/ and use distributedlog image and add instructions on how to create distributedlog namespaces and run benchmark.

This change is based on apache#196

Author: Sijie Guo <sijie@apache.org>

Reviewers: Jia Zhai <None>

This closes apache#198 from sijie/add_docker, closes apache#197
rdhabalia added a commit to YahooArchive/bookkeeper that referenced this pull request Feb 3, 2018
…Provider #18 (apache#197)

* Issue apache#377: Make Prometheus stats logger registration idempotent

Make sure the metric are only registered once on Prometheus client lib.

Discusses in apache#377.

Author: Matteo Merli <mmerli@apache.org>

Reviewers: Enrico Olivelli <None>, Sijie Guo <None>

This closes apache#378 from merlimat/fix-metrics-double-registration, closes apache#377

* Fix cherry-picks
revans2 pushed a commit to revans2/bookkeeper that referenced this pull request Feb 7, 2018
…Provider apache#18 (apache#197)

* Issue apache#377: Make Prometheus stats logger registration idempotent

Make sure the metric are only registered once on Prometheus client lib.

Discusses in apache#377.

Author: Matteo Merli <mmerli@apache.org>

Reviewers: Enrico Olivelli <None>, Sijie Guo <None>

This closes apache#378 from merlimat/fix-metrics-double-registration, closes apache#377

* Fix cherry-picks
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.

8 participants