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 exec to replace shell inside entrypoint with the actual binary #76

Closed
wants to merge 2 commits into from

Conversation

voron
Copy link
Contributor

@voron voron commented Aug 14, 2023

We need to pass signals to the actual binary to handle it correctly. The easiest way to accomplish this is to replace entrypoint's shell with the binary instead of telling the shell to execute this binary as a child process.
Temporary workaround for k8s with exec works fine in my tests

command:
  - /bin/bash
args:
  - -c
  - sqlx database setup && exec zksync_external_node

During k8s pod termination I get following lines in logs

INFO zksync_external_node: Stop signal received, shutting down
...
INFO zksync_core::api_server::healthcheck: Healthcheck server shut down

Copy link
Member

@hatemosphere hatemosphere left a comment

Choose a reason for hiding this comment

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

we're doing the same thing internally, so LGTM i guess

@RomanBrodetski
Copy link
Collaborator

@voron please rebase your branch off main

@voron voron requested a review from a team as a code owner October 2, 2023 11:56
@voron voron requested a review from bxpana October 2, 2023 11:56
@voron
Copy link
Contributor Author

voron commented Oct 2, 2023

@voron please rebase your branch off main

@RomanBrodetski will merge from GitHub's sync fork suit your needs ?

@voron
Copy link
Contributor Author

voron commented Oct 2, 2023

I see it's been handled by #134

@voron voron closed this Oct 2, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 6, 2023
…ry (#134)

# What ❔

Replaces container shell with EN and local-env executables in
corresponding Docker images to eg. properly handle termination signals.

More details in original PR:
#76

Courtesy of https://github.com/voron
Thanks for the contribution, and sorry it took so long to review - we've
been busy with FOSS'ing our repos.

## Why ❔

#76 (comment)

## Checklist

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [ ] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [ ] Code has been formatted via `zk fmt` and `zk lint`.

Co-authored-by: Alex Vorona <[email protected]>
Co-authored-by: Roman Brodetski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants