Skip to content

Conversation

@kfei
Copy link
Contributor

@kfei kfei commented Dec 24, 2014

The problem is docker stop only sends SIGTERM to the PID 1 inside the container, and the PID 1 (/bin/sh -c ...) does not take care of signals. Hence the services (e.g., postgresql, redis, sidekiq, etc) never have chances to graceful shutdown. Docker just kills the container after its 10 seconds timeout by default.

What this commit does:

  • Add a wrapper as the entrypoint of Docker container. Which starts
    services through runit, reconfigure Gitlab by gitlab-ctl and
    gracefully shutdown all services when a SIGTERM is received.
  • Create an assets directory for assets.
  • Add .dockerignore file.

Now you'll see the following log messages after docker stop:

SIGTERM signal received, try to gracefully shutdown all services...
ok: down: logrotate: 1s, normally up
ok: down: nginx: 0s, normally up
ok: down: postgresql: 1s, normally up
ok: down: redis: 0s, normally up
ok: down: sidekiq: 0s, normally up
ok: down: unicorn: 0s, normally up

Signed-off-by: kfei kfei@kfei.net

@TeatroIO
Copy link

I've prepared a stage. Click to open.

@dosire
Copy link
Member

dosire commented Dec 28, 2014

@kfei Thanks for this PR, it is awesome and I really like the graceful shutdown. Is it needed to make the other changes to the docker/Dockerfile? The shutdown works fine but if I boot according to the instructions in the docker/readme I get:

/opt/gitlab/embedded/bin/runsvdir-start: line 24: ulimit: pending signals: cannot modify limit: Operation not permitted
/opt/gitlab/embedded/bin/runsvdir-start: line 34: ulimit: max user processes: cannot modify limit: Operation not permitted
/opt/gitlab/embedded/bin/runsvdir-start: line 37: /proc/sys/fs/file-max: Read-only file system

@dosire
Copy link
Member

dosire commented Dec 28, 2014

If the changes are needed please ensure to update the docker/readme accordingly if needed.

@kfei
Copy link
Contributor Author

kfei commented Dec 29, 2014

@dosire
Well, the runsvdir-start init script from official Omnibus package introduced this issue. It tried to increase some ulimit settings which is not allowed in a default Docker run (without --privileged flag). A solution may be adding the --privileged flag to docker run. In my environment I always do this. But --privileged also means the container is running as root user, which is kind of trade-off from the aspect of security. I'd love to tune the readme. But do you think this is OK?

@dosire
Copy link
Member

dosire commented Dec 29, 2014

@kfei Thanks for your response. Please help me understand this. Does the runsvdir-start / ulimit issue also cause an error in the Docker container without your changes?

@kfei
Copy link
Contributor Author

kfei commented Dec 30, 2014

@dosire
Yes it does. The Docker container built from current master branch also throws this error when starting up. Thanks for your reminder.

@ayufan
Copy link
Member

ayufan commented Jan 5, 2015

It's generally a bad idea to use ENTRYPOINT instead of CMD. In that case it will make this command not working: docker run -ti -e TERM=linux --rm --volumes-from gitlab_data ubuntu.

@kfei
Copy link
Contributor Author

kfei commented Jan 8, 2015

@ayufan Thanks for your reminder. I didn't notice that it conflicts with README.

I agree with you that ENTRYPOINT can sometimes confuse new Docker users. But it is required for dealing with signals. So I improved the wrapper to support the data volume container use case (kinda old-fashioned IMO) from README. Please have a look at that.

@kfei kfei force-pushed the docker/graceful-shutdown branch from 95e1d3b to 0a066ba Compare January 8, 2015 08:12
@ayufan
Copy link
Member

ayufan commented Jan 8, 2015

It is not entirely true. There are two variants of CMD (one with brackets
and not).

Kamil

On Thursday, January 8, 2015, LIN, KE-FEI notifications@github.com wrote:

@ayufan https://github.com/ayufan Thanks for your reminder. I didn't
notice that it conflicts with README.

I agree with you that ENTRYPOINT can sometimes confuse new Docker users.
But it is required for dealing with signals. So I improved the wrapper to
support the data volume container use case (kinda old-fashioned IMO) from
README. Please have a look at that.


Reply to this email directly or view it on GitHub
#8486 (comment).

Kamil Trzciński

ayufan@ayufan.eu
www.ayufan.eu

@kfei kfei force-pushed the docker/graceful-shutdown branch from 0a066ba to a78e02d Compare January 8, 2015 08:23
@kfei
Copy link
Contributor Author

kfei commented Jan 8, 2015

@ayufan You're right. I did not notice that CMD can also act as PID1. I've update my PR to use CMD instead. Thanks!

@dosire
Copy link
Member

dosire commented Jan 8, 2015

Thanks everyone, please let me know when it's updated.

@kfei
Copy link
Contributor Author

kfei commented Jan 9, 2015

@dosire It's updated. The commits on my branch are squashed. Please have a look at that.

@dosire
Copy link
Member

dosire commented Jan 9, 2015

@kfei Can I run it based on the existing README documentation?

@kfei
Copy link
Contributor Author

kfei commented Jan 13, 2015

@dosire Yes it's compatible with README now.

@jvanbaarsen jvanbaarsen added this to the 7.8 milestone Feb 7, 2015
@Razer6
Copy link
Member

Razer6 commented Feb 23, 2015

@kfei Can you rebase?

The problem is `docker stop` only sends SIGTERM to the PID 1 inside the
container, and the PID 1 (`/bin/sh -c ...`) does not take care of
signals. Hence the services (e.g., postgresql, redis, sidekiq, etc)
never have chances to graceful shutdown. Docker just kills the container
after its 10 seconds timeout by default.

What this commit does:

1) Add a wrapper as the default executable of Docker container. Which
   starts services through `runit`, reconfigure Gitlab by `gitlab-ctl`
   and gracefully shutdown all services when a SIGTERM is received.
2) Create an `assets` directory for assets.
3) Add `.dockerignore` file.

Now you'll see the following log messages after `docker stop`:
```
SIGTERM signal received, try to gracefully shutdown all services...
ok: down: logrotate: 1s, normally up
ok: down: nginx: 0s, normally up
ok: down: postgresql: 1s, normally up
ok: down: redis: 0s, normally up
ok: down: sidekiq: 0s, normally up
ok: down: unicorn: 0s, normally up
```

Signed-off-by: kfei <kfei@kfei.net>
@kfei kfei force-pushed the docker/graceful-shutdown branch from a78e02d to 9338c63 Compare February 24, 2015 11:23
@kfei
Copy link
Contributor Author

kfei commented Feb 24, 2015

@Razer6 Sure. Rebased!

dosire added a commit that referenced this pull request Feb 25, 2015
Gracefully shutdown services in Docker container
@dosire dosire merged commit 4ac5281 into gitlabhq:master Feb 25, 2015
@dosire
Copy link
Member

dosire commented Feb 25, 2015

Thanks @kfei, I merged it, can you test once more to verify everything still works?

@kfei kfei deleted the docker/graceful-shutdown branch February 26, 2015 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants