Add tini to allow termination signal handling #3320

shikharbhardwaj · 2023-03-16T16:20:00Z

Description

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

Issues Resolved

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

Test plan

Build and push opensearch image to dockerhub

./build-image-multi-arch.sh -f dockerfiles/opensearch.al2.dockerfile -v '1.3.9'  -p opensearch -a 'x64,arm64' -r bluefog/opensearch-test

Test signal handling with new image vs old image (more details in this gist)

docker run --mount type=bind,source="$(pwd)"/.idea/opensearch.yml,target=/usr/share/opensearch/config/opensearch.yml,readonly --pull=always bluefog/opensearch-test:1.3.9

# Send SIGTERM/SIGINT to the container
docker kill --signal="SIGTERM" <container id>

Opensearch container exits with the following logs

[2023-04-29T15:06:05,691][INFO ][o.o.n.Node               ] [c7ab96ede963] stopping ...
[2023-04-29T15:06:05,731][INFO ][o.o.n.Node               ] [c7ab96ede963] stopped
[2023-04-29T15:06:05,732][INFO ][o.o.n.Node               ] [c7ab96ede963] closing ...
[2023-04-29T15:06:05,744][INFO ][o.o.n.Node               ] [c7ab96ede963] closed

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.

Signed-off-by: shikharbhardwaj <shikhar.bhardwaj@instabase.com>

peterzhuamazon

Hi @shikharbhardwaj ,

Thanks very much for this PR.
Would you mind send some test results/logs if possible?
Showcasing the before/after of the change?

Also, not sure if this can be considered as a major change.
Since this is the first time we are changing how the process is being started.
So probably a good timing to merge this fix is when 3.x launch?

cc: @bbarani

Thanks.

peterzhuamazon · 2023-04-03T22:13:14Z

docker/release/dockerfiles/opensearch-dashboards.al2.dockerfile


 # Update packages
 # Install the tools we need: tar and gzip to unpack the OpenSearch tarball, and shadow-utils to give us `groupadd` and `useradd`.
 # Install which to allow running of securityadmin.sh
 RUN yum update -y && yum install -y tar gzip shadow-utils which && yum clean all

+# Add tini to use as init (PID1) process.
+ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /bin/tini


This wont work as this download is only the x64 version, will crash on the arm64 here.

tini: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=a3f773dca5734fef07e2ea80c0450b3c60bbaf74, stripped

Need code to handle download for x64 vs arm64.

Updated the download source according to the TARGETARCH supplied by docker

TARGETARCH seems only support multi-arch building?
Tried on the single arch build script and it could not figure out arch.
https://docs.docker.com/desktop/extensions-sdk/extensions/multi-arch/#adding-multi-arch-binaries

Step 20/39 : ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini-${TARGETARCH} /bin/tini ADD failed: failed to GET https://github.com/krallin/tini/releases/download/v0.19.0/tini- with status 404 Not Found: <!DOCTYPE html> <html lang="en" data-color-mode="auto" data-light-theme="light" data-dark-theme="dark" data-a11y-animated-images="system">

Although we release for multi-arch we also want to support single-arch if people want to build in their local.
Probably add this in the build-image-single-arch.sh?

--build-arg TARGETARCH=${arch_uname}

peterzhuamazon · 2023-04-03T22:18:15Z

docker/release/dockerfiles/opensearch-dashboards.al2.dockerfile

@@ -93,5 +98,5 @@ LABEL org.label-schema.schema-version="1.0" \
  org.label-schema.build-date="$BUILD_DATE"

 # CMD to run
-ENTRYPOINT ["./opensearch-dashboards-docker-entrypoint.sh"]
+ENTRYPOINT ["tini", "--", "./opensearch-dashboards-docker-entrypoint.sh"]


Is it possible to use ENTRYPOINT + CMD together?

ENTRYPOINT ["/tini", "--"] # Run your program under Tini CMD ["/your/program", "-and", "-its", "arguments"]

Not sure about this since this may change behavior for existing invocations, but AFAICS non-opensearch command invocations will just exec the supplied command with a couple of env var exports

Seems like you add this now?

peterzhuamazon · 2023-04-03T22:20:33Z

docker/release/dockerfiles/opensearch-dashboards.al2.dockerfile


 # Update packages
 # Install the tools we need: tar and gzip to unpack the OpenSearch tarball, and shadow-utils to give us `groupadd` and `useradd`.
 # Install which to allow running of securityadmin.sh
 RUN yum update -y && yum install -y tar gzip shadow-utils which && yum clean all

+# Add tini to use as init (PID1) process.
+ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /bin/tini
+RUN chmod +x /bin/tini


I kinda prefer to not make it globally available, and add full path in the entrypoint.
Tho I am not strongly against it.

probably use 755 specifically instead of +x so it is precise?

Sounds good, updated the chmod to 755. Open to path suggestions where we can keep this binary

As of path, just use WORKDIR? It is the home dir of os or osd on docker.
So instead of using /bin/tini just use ./tini?

peterzhuamazon

Add a block to this PR now just in case it get accidentally merged.
Thanks.

Signed-off-by: shikharbhardwaj <shikharbhardwaj68@gmail.com>

Signed-off-by: Shikhar Bhardwaj <shikharbhardwaj@users.noreply.github.com>

shikharbhardwaj · 2023-04-29T16:24:44Z

@peterzhuamazon sorry for the delay on turning this around, made some updates here along with the steps I did to test this. Let me know if there's a better set of steps to test this PR.

Not exactly sure about the major version requirement, since this is more of a fix than a new feature, but it is possible something downstream depends on this behavior.

codecov-commenter · 2023-04-30T02:48:19Z

Codecov Report

Merging #3320 (e31832a) into main (68a2fe7) will increase coverage by 0.28%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3320      +/-   ##
==========================================
+ Coverage   91.44%   91.72%   +0.28%     
==========================================
  Files         172      172              
  Lines        4991     4991              
==========================================
+ Hits         4564     4578      +14     
+ Misses        427      413      -14

see 2 files with indirect coverage changes

peterzhuamazon · 2023-05-05T18:55:05Z

docker/release/config/opensearch/opensearch-docker-entrypoint.sh

@@ -58,7 +58,8 @@ function setupPerformanceAnalyzerPlugin {
            echo "Disabling execution of $OPENSEARCH_HOME/bin/$PERFORMANCE_ANALYZER_PLUGIN/performance-analyzer-agent-cli for OpenSearch Performance Analyzer Plugin"
        else
            echo "Enabling execution of OPENSEARCH_HOME/bin/$PERFORMANCE_ANALYZER_PLUGIN/performance-analyzer-agent-cli for OpenSearch Performance Analyzer Plugin"
-            $OPENSEARCH_HOME/bin/opensearch-performance-analyzer/performance-analyzer-agent-cli > $OPENSEARCH_HOME/logs/PerformanceAnalyzer.log 2>&1 &
+            # Launch performance plugin and disown to PID1 (tini).
+            $OPENSEARCH_HOME/bin/opensearch-performance-analyzer/performance-analyzer-agent-cli > $OPENSEARCH_HOME/logs/PerformanceAnalyzer.log 2>&1 & disown


Good catch, tho I never try to use disown after a command tho.
I think it does the same thing as nohup in front.

I will add this change into another PR so this PR of yours only focused on tini.
If we need to revert later on we dont have to revert the entire tini implementation.
Thanks.

Resolve PA proc not disown and ml_dashboards settings missing in OSD docker entrypoint #3479

peterzhuamazon · 2023-05-05T19:24:47Z

Thanks @shikharbhardwaj I have reviewed for another pass.
Please take a look at the comments.

Thanks!

peterzhuamazon · 2023-05-05T19:26:14Z

You might also want to rebase after #3479 merged to consume all the CVE fixes, so that security scan can pass.
Thanks.

jordarlu · 2023-05-10T22:03:23Z

Hello, @shikharbhardwaj , would you be able to rebase and resolve the conflct to have the WhiteSource Security Check to validate again? thanks !

Signed-off-by: Shikhar Bhardwaj <shikharbhardwaj@users.noreply.github.com>

peterzhuamazon · 2023-05-18T22:04:47Z

Hi @shikharbhardwaj welcome back and would you mind check the follow up comments I added in previous callouts?
Thanks.

jordarlu · 2023-07-12T21:07:16Z

Greetings, @shikharbhardwaj , since it has been a while, wondering if you got time to comment on the feedback from @peterzhuamazon yet? thanks ..

bbarani · 2023-10-27T19:47:48Z

Hi @shikharbhardwaj, can you please take a look at the comments and provide your feedback?

dblock · 2024-07-22T16:20:11Z

@peterzhuamazon Let's close if this is not going to be merged?

[Catch All Triage w/ 1, 2, 3]

shikharbhardwaj requested review from dblock, peterzhuamazon, bbarani, gaiksaya, rishabh6788, zelinh, jordarlu, prudhvigodithi, Divyaasm and tianleh as code owners March 16, 2023 16:20

github-actions bot added the first-time-contributor label Mar 16, 2023

Add tini to allow termination signal handling

86ae4f1

Signed-off-by: shikharbhardwaj <shikhar.bhardwaj@instabase.com>

shikharbhardwaj force-pushed the add-tini-pid1 branch from 7d31c6d to 86ae4f1 Compare March 16, 2023 16:21

shikharbhardwaj mentioned this pull request Mar 23, 2023

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

Closed

peterzhuamazon reviewed Apr 3, 2023

View reviewed changes

peterzhuamazon requested changes Apr 3, 2023

View reviewed changes

peterzhuamazon added the docker label Apr 3, 2023

Download arch-specific tini binary, update perms

6d36279

Signed-off-by: shikharbhardwaj <shikharbhardwaj68@gmail.com>

shikharbhardwaj force-pushed the add-tini-pid1 branch from 0ec6d04 to 6d36279 Compare April 29, 2023 14:07

shikharbhardwaj and others added 2 commits April 29, 2023 21:06

Update entrypoint script to remove shell from the process tree

0e28a5b

Signed-off-by: shikharbhardwaj <shikharbhardwaj68@gmail.com>

Merge branch 'main' into add-tini-pid1

e31832a

Signed-off-by: Shikhar Bhardwaj <shikharbhardwaj@users.noreply.github.com>

peterzhuamazon reviewed May 5, 2023

View reviewed changes

Merge branch 'main' into add-tini-pid1

5c6669c

Signed-off-by: Shikhar Bhardwaj <shikharbhardwaj@users.noreply.github.com>

peterzhuamazon marked this pull request as draft November 14, 2023 18:49

chawinphat mentioned this pull request Nov 16, 2023

Add tini to allow termination signal handling #4217

Draft

dblock assigned peterzhuamazon Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tini to allow termination signal handling #3320

Add tini to allow termination signal handling #3320

shikharbhardwaj commented Mar 16, 2023 •

edited

Loading

peterzhuamazon left a comment

peterzhuamazon Apr 3, 2023

peterzhuamazon Apr 3, 2023

shikharbhardwaj Apr 29, 2023

peterzhuamazon May 5, 2023

peterzhuamazon Apr 3, 2023

shikharbhardwaj Apr 29, 2023 •

edited

Loading

peterzhuamazon May 5, 2023

peterzhuamazon Apr 3, 2023

peterzhuamazon Apr 3, 2023

shikharbhardwaj Apr 29, 2023

peterzhuamazon May 5, 2023

peterzhuamazon May 5, 2023

peterzhuamazon left a comment

shikharbhardwaj commented Apr 29, 2023

codecov-commenter commented Apr 30, 2023 •

edited

Loading

peterzhuamazon May 5, 2023

peterzhuamazon May 5, 2023

peterzhuamazon May 5, 2023

peterzhuamazon commented May 5, 2023

peterzhuamazon commented May 5, 2023

jordarlu commented May 10, 2023

peterzhuamazon commented May 18, 2023

jordarlu commented Jul 12, 2023

bbarani commented Oct 27, 2023

dblock commented Jul 22, 2024

Add tini to allow termination signal handling #3320

Are you sure you want to change the base?

Add tini to allow termination signal handling #3320

Conversation

shikharbhardwaj commented Mar 16, 2023 • edited Loading

Description

Issues Resolved

Test plan

peterzhuamazon 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

shikharbhardwaj Apr 29, 2023 • edited Loading

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

peterzhuamazon left a comment

Choose a reason for hiding this comment

shikharbhardwaj commented Apr 29, 2023

codecov-commenter commented Apr 30, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterzhuamazon commented May 5, 2023

peterzhuamazon commented May 5, 2023

jordarlu commented May 10, 2023

peterzhuamazon commented May 18, 2023

jordarlu commented Jul 12, 2023

bbarani commented Oct 27, 2023

dblock commented Jul 22, 2024

shikharbhardwaj commented Mar 16, 2023 •

edited

Loading

shikharbhardwaj Apr 29, 2023 •

edited

Loading

codecov-commenter commented Apr 30, 2023 •

edited

Loading