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.

Sign up for GitHub

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

Jump to bottom

tink server doesn't handle os signals #527

Closed
jacobweinstock opened this issue Aug 30, 2021 · 0 comments · Fixed by #528
Closed

tink server doesn't handle os signals #527

jacobweinstock opened this issue Aug 30, 2021 · 0 comments · Fixed by #528
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@jacobweinstock
Copy link
Member

A running tink-server does not shut down when a Linux signal (SIGINT, SIGQUIT, SIGTERM) is sent.

Expected Behaviour

When a SIGINT, SIGQUIT, or SIGTERM is sent to tink-server, it should shut down the gRPC and HTTP servers and exit.

Current Behaviour

Sending SIGINT, SIGQUIT, or SIGTERM does nothing. Servers do not shut down.

Possible Solution

One possible solutions is to remove this <-ctx.Done line, and add closer() after a signal is received here.

Steps to Reproduce (for bugs)

  1. start tink-server. docker-compose up
  2. shell into the tink-server container. docker exec -it tink_tinkerbell_1 sh
  3. send SIGINT, SIGQUIT, or SIGTERM signals. kill -SIGINT 1 kill -SIGQUIT 1 kill -SIGTERM 1
  4. observe that the server does not shut down or show any change.

Context

Your Environment

  • Operating System and version (e.g. Linux, Windows, MacOS):

  • How are you running Tinkerbell? Using Vagrant & VirtualBox, Vagrant & Libvirt, on Packet using Terraform, or give details:

  • Link to your project or a code example to reproduce issue:
@jacobweinstock jacobweinstock added kind/bug Categorizes issue or PR as related to a bug. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Aug 30, 2021
@tstromberg tstromberg added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 30, 2021
krishnanarchana added a commit to krishnanarchana/tink that referenced this issue Aug 31, 2021
@krishnanarchana
Fix handling of os signals in tink server
db976db 
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 seethelog that signal is received.

Related to tinkerbell#527
@krishnanarchana krishnanarchana mentioned this issue Aug 31, 2021
Merged
@mergify mergify bot closed this as completed in #528 Sep 1, 2021
mergify bot added a commit that referenced this issue Sep 1, 2021
@mergify
Fix handling of os signals in tink server (#528)
18bc57d 
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:

```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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix handling of os signals in tink server krishnanarchana/tink
2 participants
@tstromberg @jacobweinstock