Skip to content
This repository has been archived by the owner on Oct 5, 2022. It is now read-only.

add --init to docker run #196

Merged
merged 1 commit into from
Jul 3, 2019
Merged

add --init to docker run #196

merged 1 commit into from
Jul 3, 2019

Conversation

tom-shan
Copy link
Member

@tom-shan tom-shan commented Jul 3, 2019

fix #195

Signed-off-by: tom-shan swt0008411@163.com

@@ -30,5 +30,8 @@ COPY --from=0 --chown=theia:theia /home/theia /home/theia
EXPOSE 3000
ENV SHELL /bin/bash
ENV USE_LOCAL_GIT true
ENV TINI_VERSION v0.18.0
ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini
RUN chmod +x /tini
Copy link
Member

Choose a reason for hiding this comment

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

I got a permission issue at first. Got it working with a little tweak, so that the theia user has execute permission as well:

RUN chmod 555 /tini

@@ -30,5 +30,8 @@ COPY --from=0 --chown=theia:theia /home/theia /home/theia
EXPOSE 3000
ENV SHELL /bin/bash
ENV USE_LOCAL_GIT true
ENV TINI_VERSION v0.18.0
ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini
Copy link
Member

Choose a reason for hiding this comment

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

I got a runtime exception when running tini from the entrypoint. Maybe the basic Ubuntu image is missing some dynamic library that's required? It worked with the statically linked version:

ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini-static /tini

@marcdumais-work
Copy link
Member

Hi @tom-shan ,

Thanks for this PR.

Looking at tini'sdocumentation, it has been possible for a couple of years to use tiny without changing the Docker images, by using the --init command-line option to docker run. When doing that, docker injects tiny in the image and runs the entrypoint through it (it still works if one added tini explicitly in the image).

Here's an interesting discussion about whether to explicitly use tini in docker images or use the docker command-line option: krallin/tini#81 . For me the best argument was that, at the time, a lot of users were likely to run a docker version without this feature. Over two years later, this may not be a big problem anymore.

So have you considered a documentation update instead of adding tini in the image? As it is, we would need to do the same for other images in this repo. If we instead document to use the --init option in the README, it would cover all images.

P.S: Node.js suggests using either --init or to include tini in images, rather than having node as init process:
https://github.com/nodejs/docker-node/blob/master/docs/BestPractices.md#handling-kernel-signals

@tom-shan
Copy link
Member Author

tom-shan commented Jul 3, 2019

I agree with you. Adding --init is actually a simpler way.

@marcdumais-work
Copy link
Member

@tom-shan would you care to submit an updated PR that documents the use of the --init docker run option? e.g. in the repo's README.md?

@tom-shan tom-shan changed the title add a valid init use tini add --init to docker run Jul 3, 2019
@tom-shan
Copy link
Member Author

tom-shan commented Jul 3, 2019

I just search the entire repo and modify these commands of docker run.

@marcdumais-work
Copy link
Member

I just search the entire repo and modify these commands of docker run

Up-to you if you think a mention of the reason why --init is needed would also be good to add.

@tom-shan
Copy link
Member Author

tom-shan commented Jul 3, 2019

Aha!

fix #195

Signed-off-by: tom-shan <swt0008411@163.com>
Copy link
Member

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution @tom-shan

@marcdumais-work marcdumais-work merged commit 1d16b38 into theia-ide:master Jul 3, 2019
@tom-shan tom-shan deleted the init-process branch July 4, 2019 01:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the init process in docker cause a lot of defunct processes
2 participants