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

Fix handling of os signals in tink server #528

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

krishnanarchana
Copy link
Contributor

@krishnanarchana krishnanarchana commented Aug 31, 2021

Related to #527

Description

Previously tink server failed to respect os signals like SIGTERM,SIGINT etc.

This PR fixes the tink server so that os signals sent to the tink server process
will actually kill the process.

Why is this needed

Sending os signals actually terminates the tink server.

Fixes: #527

How Has This Been Tested?

  • Tested using docker-compose:
make images
make run
docker exec -it tink_tinkerbell_1 sh
ps
kill -SIGINT 1

Corresponding logs:

tinkerbell_1             | {"level":"info","ts":1630442128.6128566,"caller":"tink-server/main.go:198","msg":"signal received, stopping servers","service":"github.com/tinkerbell/tink","signal":"interrupt"}
tinkerbell_1             | {"level":"info","ts":1630442129.4250214,"caller":"metrics/metrics.go:58","msg":"initializing label values","service":"github.com/tinkerbell/tink"}
tinkerbell_1             | {"level":"info","ts":1630442129.4286315,"caller":"http-server/http_server.go:88","msg":"serving http","service":"github.com/tinkerbell/tink"}

Now the docker container exists and we can see the log that signal is received.

Signed-off-by: Archana krishnan [email protected]

@jacobweinstock
Copy link
Member

jacobweinstock commented Sep 1, 2021

Hey @krishnanarchana! Welcome and thank you for this contribution!

Looks like you'll need the DCO sign-off before we can merge this. There are a couple of guides here or here to help with that if needed.

@jacobweinstock jacobweinstock added kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Sep 1, 2021
jacobweinstock
jacobweinstock previously approved these changes Sep 1, 2021
@jacobweinstock jacobweinstock added the ready-to-merge Signal to Mergify to merge the PR. label Sep 1, 2021
@krishnanarchana
Copy link
Contributor Author

Hi @jacobweinstock thank you for your review, will do the DCO signoff.

Previously tink server failed to respect os signals like SIGTERM,SIGINT etc.

This PR fixes the tink server so that os signals sent to the tink server process
will actually kill the process.

Confirmed by runnning in docker-cpompose env:

```bash
make images
make run
docker exec -it tink_tinkerbell_1 sh
ps
kill -SIGINT 1
```

Corresponding logs:

```
tinkerbell_1             | {"level":"info","ts":1630442128.6128566,"caller":"tink-server/main.go:198","msg":"signal received, stopping servers","service":"github.com/tinkerbell/tink","signal":"interrupt"}
tinkerbell_1             | {"level":"info","ts":1630442129.4250214,"caller":"metrics/metrics.go:58","msg":"initializing label values","service":"github.com/tinkerbell/tink"}
tinkerbell_1             | {"level":"info","ts":1630442129.4286315,"caller":"http-server/http_server.go:88","msg":"serving http","service":"github.com/tinkerbell/tink"}
```

Now the docker container exists and we can see the log that signal is received.

Signed-off-by: Archana krishnan <[email protected]>
@krishnanarchana
Copy link
Contributor Author

Hi @jacobweinstock thank you for your review, will do the DCO signoff.

@jacobweinstock completed the DCO signoff. Thanks!

@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #528 (8d300f8) into master (b547d22) will not change coverage.
The diff coverage is n/a.

❗ Current head 8d300f8 differs from pull request most recent head 81ac85c. Consider uploading reports for the commit 81ac85c to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #528   +/-   ##
=======================================
  Coverage   33.61%   33.61%           
=======================================
  Files          44       44           
  Lines        3385     3385           
=======================================
  Hits         1138     1138           
  Misses       2150     2150           
  Partials       97       97           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b547d22...81ac85c. Read the comment docs.

@mergify mergify bot merged commit 18bc57d into tinkerbell:master Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tink server doesn't handle os signals
2 participants