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 tini to allow termination signal handling #4217

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chawinphat
Copy link
Contributor

@chawinphat chawinphat commented Nov 16, 2023

Description

Adds tini as the PID1 process in opensearch and opensearch-dashboards container to allow for termination signal handling.

Tini couldn't be found when using './tini' as the install path, so I reverted to using 'bin/tini'.

Issues Resolved

closes [BUG] OpenSearch container does not allow for graceful termination #3229

Test Plan and Results:

  1. Build single/multi arch image and push image to dockerhub as described on opensearch-build/docker.
  2. Run image

docker run -it -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" chawinphat/opensearch:1.3.9

  1. Test signal handling with new image. See that container is closed after calling "SIGTERM"
# Check for Container
> docker ps
CONTAINER ID   IMAGE                         COMMAND                  CREATED          STATUS          PORTS                                                                NAMES
e7297b7fcbba   chawinphat/opensearch:1.3.9   "/bin/tini -- ./open…"   19 minutes ago   Up 19 minutes   0.0.0.0:9200->9200/tcp, 9300/tcp, 0.0.0.0:9600->9600/tcp, 9650/tcp   vigorous_dewdney

# Send signal
> docker kill --signal="SIGTERM" e7297b7fcbba
e7297b7fcbba

# Check that container is gone
> docker ps                                  
CONTAINER ID   IMAGE     COMMAND   CREATED   STATUS    PORTS     NAMES
  1. Opensearch container exits with the following logs
[2023-11-15T23:36:53,554][INFO ][o.o.n.Node               ] [e7297b7fcbba] stopped
[2023-11-15T23:36:53,555][INFO ][o.o.n.Node               ] [e7297b7fcbba] closing ...
[2023-11-15T23:36:53,561][INFO ][o.o.s.a.i.AuditLogImpl   ] [e7297b7fcbba] Closing AuditLogImpl
[2023-11-15T23:36:53,568][INFO ][o.o.n.Node               ] [e7297b7fcbba] closed

I was able to pass this test process on both single and multi arch builds.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@@ -123,6 +123,5 @@ else
fi

# Docker build
docker build --build-arg VERSION=$VERSION --build-arg BUILD_DATE=`date -u +%Y-%m-%dT%H:%M:%SZ` --build-arg NOTES=$NOTES -f $DOCKERFILE $DIR -t opensearchproject/$PRODUCT:$VERSION
docker tag opensearchproject/$PRODUCT:$VERSION opensearchproject/$PRODUCT:latest
docker build --build-arg VERSION=$VERSION --build-arg BUILD_DATE=`date -u +%Y-%m-%dT%H:%M:%SZ` --build-arg NOTES=$NOTES -f $DOCKERFILE $DIR -t opensearchproject/$PRODUCT:$VERSION --build-arg TARGETARCH=${arch_uname}
Copy link
Member

Choose a reason for hiding this comment

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

Are you using the multi-arch script to test?
The problem with adding a targetarch approach is that you can only do so for a single arch, not multi.
Consider similar to this approach here:

RUN tar -xzpf $TEMP_DIR/opensearch-dashboards-`uname -p`.tgz -C $OPENSEARCH_DASHBOARDS_HOME --strip-components=1 && \

Copy link
Member

Choose a reason for hiding this comment

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

And why is the tag step being removed?

Copy link
Member

Choose a reason for hiding this comment

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

uname -m might be better tho at the time I use -p with similar effect.
Tho historically -m should be more portable than -p.
And we have used -m for all other purposes already.

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 accidentally removed the tag step line. Will add it back.

I have been building using the multi-arch script and its been working.

./build-image-multi-arch.sh -v 1.0.0 -f ./dockerfiles/opensearch.al2.dockerfile -p opensearch -a "x64,arm64" -r "<Docker Hub RepoName>/<Docker Image Name>:<Tag Name>"

and then running using:
$ docker run -it -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" opensearchproject/opensearch:1.1.0

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 changed to using 'uname -m' now. Tested with single arch and multi arch and both are able to gracefully terminate.

Signed-off-by: chawinphat <[email protected]>
Signed-off-by: chawinphat <[email protected]>
@chawinphat chawinphat marked this pull request as ready for review November 19, 2023 22:37
@peterzhuamazon peterzhuamazon marked this pull request as draft January 23, 2024 21:37
@peterzhuamazon
Copy link
Member

Put it to pending now as it might be able to solve just by using exec to start process. See issue comments.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] OpenSearch container does not allow for graceful termination
2 participants