[ISSUE-48][FEATURE] Add Uniffle Dockerfile #132

wangao1236 · 2022-08-05T12:41:19Z

What changes were proposed in this pull request?

To solve issue #48 ，I add the Dockerfile for Uniffle.

Why are the changes needed?

Support K8S Operator

Does this PR introduce any user-facing change?

Yes, we will add the document later

How was this patch tested?

Manual test

wangao1236 · 2022-08-05T12:45:14Z

/assign @jerqi

deploy/kubernetes/docker/Dockerfile

deploy/kubernetes/docker/build.sh

deploy/kubernetes/docker/hadoopconfig/.gitkeep

deploy/kubernetes/docker/start.sh

deploy/kubernetes/docker/build.sh

jerqi · 2022-08-05T13:57:27Z

cc @zuston @czy006 Could you help me review this patch?

deploy/kubernetes/docker/start.sh

.gitignore

jerqi · 2022-08-05T14:12:17Z

Could you remove the file https://github.com/apache/incubator-uniffle/blob/master/deploy/kubernetes/docker/.gitkeep? Because the directory docker has files, don't need a keep file.

frankliee · 2022-08-05T14:47:13Z

deploy/kubernetes/docker/Dockerfile

@@ -0,0 +1,29 @@
+FROM openjdk:8-jdk-slim


I suggest to use other general-purpose images, such as alpine or centos.
Many companies may have their own JDK choices, so we should treat JDK as 3rd party lib hadoop.

It's ok to use openjdk. You can see Spark's Dockerfile https://github.com/apache/spark/blob/b481ed36d129982bdd5aedd0d5864dd954dad006/resource-managers/kubernetes/docker/src/main/dockerfiles/spark/Dockerfile#L19

I think we can use dependency_cache to save our build time

for example:
FROM openjdk:8-jdk-slim as dependency_cache

Seems 8-jdk-slim image is based on Debian stable, which is fine.
Can we use 8-jre-slim here?

Seems 8-jdk-slim image is based on Debian stable, which is fine. Can we use 8-jre-slim here?

What's your reason?

Seems 8-jdk-slim image is based on Debian stable, which is fine. Can we use 8-jre-slim here?

What's your reason?

It's smaller.

I think we can use dependency_cache to save our build time

for example: FROM openjdk:8-jdk-slim as dependency_cache

It's great. Could you raise a follow-up pr which uses dependency_cache?

Seems 8-jdk-slim image is based on Debian stable, which is fine. Can we use 8-jre-slim here?

What's your reason?

It's smaller.

It may need more tests. If 8-jdk-slim is fine, we can choose it first. When our e2e test is ready, wen can use e2e test to verify the 8-jre-slim.

jerqi · 2022-08-06T10:12:29Z

.gitignore

@@ -21,4 +21,5 @@ metastore_db/
 derby.log
 dependency-reduced-pom.xml
 rss-*.tgz
-
+hadoop-*.tar.gz
+/deploy/kubernetes/hadoopconfig/*


Why do we use absolute path here? if hadoopconfig is useful, why do we add it to .gitignore file?

zuston

Overall LGTM. Left some minor comments

zuston · 2022-08-07T02:03:03Z

deploy/kubernetes/docker/build.sh

+IMAGE=$REGISTRY/rss-server:$RSS_VERSION
+
+echo "building image: $IMAGE"
+docker build -t "$IMAGE" \


Using LABEL to add more metadata, like build-time/author/branch/commit-id?

zuston · 2022-08-07T02:12:22Z

deploy/kubernetes/docker/start.sh

+
+if [ "$SERVICE_NAME" == "coordinator" ];then
+
+    bash ${basedir}/bin/start-coordinator.sh &


Directly exit when executing startup script failed.

bash ${basedir}/bin/start-coordinator.sh || exit 1

Could you give me your reason?

start.sh will monitor the process existence, if the coordinator startup failed, we should exit directly, instead of waiting the non-existence PID and then exit 0

We kill server or coordinator, the process will not exit immediately sometimes. The process will occupy the port, we should wait for the port for a while.

zuston · 2022-08-07T02:15:39Z

deploy/kubernetes/docker/Dockerfile

+RUN tar -zxvf /data/rssadmin/hadoop-${HADOOP_VERSION}.tar.gz -C /data/rssadmin
+RUN mv /data/rssadmin/hadoop-${HADOOP_VERSION} /data/rssadmin/hadoop
+RUN rm -rf /data/rssadmin/hadoop-${HADOOP_VERSION}.tar.gz
+COPY hadoopconfig/ /data/rssadmin/hadoop/etc/hadoop


What files in hadoopconfig folder? core-site/hdfs-site.xml?

zuston · 2022-08-07T02:17:55Z

deploy/kubernetes/docker/Dockerfile

+ARG HADOOP_VERSION
+ARG RSS_VERSION
+
+RUN apt-get update && apt-get install -y zlib1g zlib1g-dev lzop lsof netcat dnsutils less procps iputils-ping \


Do we need to consider kerberos? If yes, we need to setup kerberos client.

Maybe we shouldn't consider this in early version.

We shouldn't consider the kerberos dependency here. It's ok for us if there are no big conflicts, it will be convenient to modify Dockerfile after we merge #53

jerqi

LGTM, @zuston @frankliee @kaijchen @czy006 Do you have any other suggestion?

jerqi · 2022-08-10T02:02:25Z

@zuston @kaijchen @frankliee @czy006 Thanks for your review, if you have any other suggestion, you can raise a follow-up pr. Merged.

wangao1236 changed the title ~~Add dockerfile of rss~~ [ISSUE-48] [Feature] Add dockerfile of rss Aug 5, 2022

jerqi changed the title ~~[ISSUE-48] [Feature] Add dockerfile of rss~~ [ISSUE-48][FEATURE] Add docker file Aug 5, 2022

jerqi changed the title ~~[ISSUE-48][FEATURE] Add docker file~~ [ISSUE-48][FEATURE] Add Dockerfile Aug 5, 2022

jerqi changed the title ~~[ISSUE-48][FEATURE] Add Dockerfile~~ [ISSUE-48][FEATURE] Add Uniffle Dockerfile Aug 5, 2022

jerqi reviewed Aug 5, 2022

View reviewed changes

deploy/kubernetes/docker/start.sh Outdated Show resolved Hide resolved

jerqi reviewed Aug 5, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

frankliee previously approved these changes Aug 5, 2022

View reviewed changes

wangao1236 dismissed frankliee’s stale review via 1a0a973 August 5, 2022 14:58

wangao1236 force-pushed the build-add-dockerfile branch 2 times, most recently from 1a0a973 to f776ce7 Compare August 5, 2022 15:24

jerqi reviewed Aug 6, 2022

View reviewed changes

zuston reviewed Aug 7, 2022

View reviewed changes

wangao1236 force-pushed the build-add-dockerfile branch from f776ce7 to d85a10e Compare August 8, 2022 03:26

Add dockerfile of rss

7da0c3e

wangao1236 force-pushed the build-add-dockerfile branch from d85a10e to 7da0c3e Compare August 8, 2022 12:32

jerqi approved these changes Aug 8, 2022

View reviewed changes

jerqi merged commit 18663ed into apache:master Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE-48][FEATURE] Add Uniffle Dockerfile #132

[ISSUE-48][FEATURE] Add Uniffle Dockerfile #132

wangao1236 commented Aug 5, 2022 •

edited by jerqi

Loading

wangao1236 commented Aug 5, 2022

jerqi commented Aug 5, 2022

jerqi commented Aug 5, 2022

frankliee Aug 5, 2022

jerqi Aug 5, 2022

czy006 Aug 7, 2022

kaijchen Aug 8, 2022

jerqi Aug 8, 2022

kaijchen Aug 8, 2022

jerqi Aug 8, 2022 •

edited

Loading

jerqi Aug 8, 2022

jerqi Aug 6, 2022

zuston left a comment

zuston Aug 7, 2022

zuston Aug 7, 2022

jerqi Aug 7, 2022

zuston Aug 7, 2022

jerqi Aug 8, 2022

zuston Aug 7, 2022

jerqi Aug 7, 2022

zuston Aug 7, 2022

jerqi Aug 7, 2022 •

edited

Loading

jerqi left a comment

jerqi commented Aug 10, 2022


		if [ "$SERVICE_NAME" == "coordinator" ];then

		bash ${basedir}/bin/start-coordinator.sh &

[ISSUE-48][FEATURE] Add Uniffle Dockerfile #132

[ISSUE-48][FEATURE] Add Uniffle Dockerfile #132

Conversation

wangao1236 commented Aug 5, 2022 • edited by jerqi Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

wangao1236 commented Aug 5, 2022

jerqi commented Aug 5, 2022

jerqi commented Aug 5, 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

jerqi Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuston left a 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

jerqi Aug 7, 2022 • edited Loading

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

jerqi commented Aug 10, 2022

wangao1236 commented Aug 5, 2022 •

edited by jerqi

Loading

jerqi Aug 8, 2022 •

edited

Loading

jerqi Aug 7, 2022 •

edited

Loading