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

Graceful shutdown ... isn't #1946

Open
frostmar opened this issue Jan 8, 2024 · 3 comments · May be fixed by #2015
Open

Graceful shutdown ... isn't #1946

frostmar opened this issue Jan 8, 2024 · 3 comments · May be fixed by #2015
Assignees

Comments

@frostmar
Copy link
Contributor

frostmar commented Jan 8, 2024

Description of Problem / Feature Request

cmd/clair/main.go installs a signal handler (SIGINT only) to perform a graceful shutdown.
I don't think this is working as desired

Expected Outcome

I would have expected HTTP servers to stop accepting new connections, then a short grace period for in-flight requests to complete (exiting as soon as no requests are in flight), as a last resort terminating connections then exiting if any are still running when the grace period expires.

Actual Outcome

HTTP servers are shutdown immediately, with any in-flight HTTP requests being severed.

Recreate:

  1. start a local Postgres
  2. run a clair indexer locally:
    go run ./cmd/clair/ -conf ./local-dev/clair/config-frostmar-local.yaml -mode indexer
  3. start a long-running HTTP request, eg an indexing operation
    curl -H "Content-Type: application/json" http://localhost:6060/indexer/api/v1/index_report -d ...<snipped>...
  4. send SIGINT - [CTRL+C] the go run command, or kill -INT <pid>
    => logs output gracefully shutting down component=main signal=interrupt
    => process immediately exits
    => curl request immediately gives error curl: (52) Empty reply from server

I'd hope to see:
=> logs to output gracefully shutting down component=main signal=interrupt
=> 10 sec grace-period timeout
=> process exits only after the grace period
=> logs to output unregistered signal handler
=> curl to fail with an error response

Environment

Running locally on a Mac

  • Clair version/image: git tag v4.7.2
  • Clair client name/version: curl
  • Host OS: MacOS
  • Kernel (e.g. uname -a):
  • Kubernetes version (use kubectl version): n/a
  • Network/Firewall setup: n/a
@frostmar
Copy link
Contributor Author

frostmar commented Jan 8, 2024

Reading up with the help of https://dev.to/mokiat/proper-http-shutdown-in-go-3fji
I think the current behaviour is due to srvs.Wait() returning as soon as the servers are Shutdown() (stop accepting requests), which runs off the end of main.go and the process exits, even though other goroutines are still running. For example unregistered signal handler never gets logged

@frostmar
Copy link
Contributor Author

frostmar commented Jan 9, 2024

draft PR #1951 improves this a bit, but I can't get in-flight requests to continue processing for a grace period. Details in the draft PR

@hdonnay hdonnay self-assigned this Jan 12, 2024
hdonnay added a commit that referenced this issue Jan 18, 2024
This changes the incorrect-but-at-least-stops implementation with a
little tree of goroutines.

The full fix requires `context.WithoutCancel`, which we can't quite use
yet.

Closes: #1946
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
Copy link
Member

hdonnay commented Jan 18, 2024

I took a whack at this in #1954, but I think it will require go1.21 API to work correctly.

hdonnay added a commit to hdonnay/clair that referenced this issue Mar 22, 2024
This changes the incorrect-but-at-least-stops implementation with a
little tree of goroutines.

The full fix requires `context.WithoutCancel`, which we can't quite use
yet.

Closes: quay#1946
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay hdonnay linked a pull request Mar 22, 2024 that will close this issue
hdonnay added a commit to hdonnay/clair that referenced this issue Apr 17, 2024
This changes the incorrect-but-at-least-stops implementation with a
little tree of goroutines.

The full fix requires `context.WithoutCancel`, which we can't quite use
yet.

Closes: quay#1946
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants