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

salt manifests for kube components breaks graceful termination #45959

Closed
jsravn opened this Issue May 17, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@jsravn
Contributor

jsravn commented May 17, 2017

Is this a request for help? (If yes, you should use our troubleshooting guide and community support channels, see http://kubernetes.io/docs/troubleshooting/.):
No

What keywords did you search in Kubernetes issues before filing this one? (If you have found any duplicates, you should instead reply there.):


Is this a BUG REPORT or FEATURE REQUEST? (choose one):
BUG

Kubernetes version (use kubectl version):
1.6.3

Environment:

  • Cloud provider or hardware configuration:
  • OS (e.g. from /etc/os-release):
  • Kernel (e.g. uname -a):
  • Install tools:
  • Others:

What happened:
All of the salt scripts bundled in kubernetes (for example, https://github.com/kubernetes/kubernetes/blob/master/cluster/saltbase/salt/kube-apiserver/kube-apiserver.manifest) start up the component in a broken way. They use /bin/sh -c which means the shell will intercept all signals. So these daemons are not gracefully shutdown when the pod is stopped.

This causes serious problems since when apiserver is killed abruptly it doesn't seem to close watches correctly. This was observed in #41916 for example.

What you expected to happen:

Running /bin/sh -c is incorrect. It should run the binary directly. Otherwise, it needs to perform an exec so the binary correctly replaces the shell script process.

How to reproduce it (as minimally and precisely as possible):
Run kube-apiserver inside a container with /bin/sh -c and try to stop it. You can't - eventually docker will force kill it.

Anything else we need to know:

@kargakis

This comment has been minimized.

Show comment
Hide comment
@kargakis

kargakis May 21, 2017

Member

@kubernetes/sig-cluster-lifecycle-misc

Member

kargakis commented May 21, 2017

@kubernetes/sig-cluster-lifecycle-misc

@roberthbailey

This comment has been minimized.

Show comment
Hide comment
@roberthbailey

roberthbailey May 23, 2017

Member

It looks like we changed from running the binaries directly to using /bin/sh -c in #7316 (from @ArtfulCoder) a few years ago to enable us to capture logs.

@dchen1107 or @lavalamp may recall why we had to use a shell since they reviewed that PR.

Member

roberthbailey commented May 23, 2017

It looks like we changed from running the binaries directly to using /bin/sh -c in #7316 (from @ArtfulCoder) a few years ago to enable us to capture logs.

@dchen1107 or @lavalamp may recall why we had to use a shell since they reviewed that PR.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Dec 25, 2017

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

fejta-bot commented Dec 25, 2017

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@roberthbailey

This comment has been minimized.

Show comment
Hide comment
@roberthbailey

roberthbailey Jan 3, 2018

Member

/remove-lifecycle stale

Member

roberthbailey commented Jan 3, 2018

/remove-lifecycle stale

k8s-merge-robot added a commit that referenced this issue Jan 3, 2018

Merge pull request #57756 from mborsz/exec-manifest
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Add 'exec' in all saltbase manifests using '/bin/sh -c'.

Right now, if docker sends SIGTERM, /bin/sh doesn't pass it to
underlying process, which breaks graceful process shutdown.

Changing '/bin/sh -c CMD > /var/log/FILE.log' pattern to '/bin/sh -c
exec CMD > /var/log/FILE.log' still allows to redirect output to log
file, but also passes all signals to CMD process.



**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #57707, Fixes #45959

**Special notes for your reviewer**:

**Release note**:

```release-note
Fix to allow kubernetes components to react to SIGTERM signal and shutdown gracefully.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment