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 sysctl support #34

Open
sttts opened this Issue Jul 20, 2016 · 35 comments

Comments

@sttts
Copy link
Contributor

sttts commented Jul 20, 2016

Description

This feature aims at extending the current pod specification with support
for namespaced kernel parameters (sysctls) set for each pod.

Scope of work planned for v1.11

  • graduate to beta

Progress Tracker

  • Before Alpha
    • Design Approval
    • Write (code done + tests done + docs done, ready to be merged) then get them merged. kubernetes/kubernetes#27180
      • Code needs to be disabled by default. Verified by code OWNERS
      • Minimal testing
      • Minimal docs: kubernetes/website#1126
        • cc @kubernetes/docs on docs PR
        • cc @kubernetes/feature-reviewers on this issue to get approval before checking this off
        • New apis: Glossary Section Item in the docs repo: kubernetes/kubernetes.github.io
      • Update release notes
  • Before Beta
    • Testing is sufficient for beta
    • User docs with tutorials
      • Updated walkthrough / tutorial in the docs repo: kubernetes/kubernetes.github.io
      • cc @kubernetes/docs on docs: kubernetes/website#8804
      • cc @kubernetes/feature-reviewers on this issue to get approval before checking this off
    • Thorough API review
      • cc @kubernetes/api
  • Before Stable
    • docs/proposals/foo.md moved to docs/design/foo.md
      • cc @kubernetes/feature-reviewers on this issue to get approval before checking this off
    • Soak, load testing
    • detailed user docs and examples
      • cc @kubernetes/docs
      • cc @kubernetes/feature-reviewers on this issue to get approval before checking this off

FEATURE_STATUS is used for feature tracking and to be updated by @kubernetes/feature-reviewers.
FEATURE_STATUS: IN_DEVELOPMENT

More advice:

Design

  • Once you get LGTM from a @kubernetes/feature-reviewers member, you can check this checkbox, and the reviewer will apply the "design-complete" label.

Coding

  • Use as many PRs as you need. Write tests in the same or different PRs, as is convenient for you.
  • As each PR is merged, add a comment to this issue referencing the PRs. Code goes in the http://github.com/kubernetes/kubernetes repository,
    and sometimes http://github.com/kubernetes/contrib, or other repos.
  • When you are done with the code, apply the "code-complete" label.
  • When the feature has user docs, please add a comment mentioning @kubernetes/feature-reviewers and they will
    check that the code matches the proposed feature and design, and that everything is done, and that there is adequate
    testing. They won't do detailed code review: that already happened when your PRs were reviewed.
    When that is done, you can check this box and the reviewer will apply the "code-complete" label.

Docs

  • Write user docs and get them merged in.
  • User docs go into http://github.com/kubernetes/kubernetes.github.io.
  • When the feature has user docs, please add a comment mentioning @kubernetes/docs.
  • When you get LGTM, you can check this checkbox, and the reviewer will apply the "docs-complete" label.

@idvoretskyi idvoretskyi added this to the v1.4 milestone Jul 20, 2016

@idvoretskyi idvoretskyi added the sig/node label Aug 4, 2016

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Aug 25, 2016

Kubernetes Submit Queue
Merge pull request #27180 from sttts/sysctl-implementation
Automatic merge from submit-queue

Add sysctl support

Implementation of proposal #26057, feature  kubernetes/enhancements#34

TODO:
- [x] change types.go
- [x] implement docker and rkt support
- [x] add e2e tests
- [x] decide whether we want apiserver validation
- ~~[ ] add documentation~~: api docs exist. Existing PodSecurityContext docs is very light and links back to the api docs anyway: https://github.com/kubernetes/kubernetes.github.io/blob/6684555ed9e3121388d30e9c49ac6556ef0241e0/docs/user-guide/security-context.md
- [x] change PodSecurityPolicy in types.go
- [x] write admission controller support for PodSecurityPolicy
- [x] write e2e test for PodSecurityPolicy
- [x] make sure we are compatible in the sense of https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api_changes.md
- [x] test e2e with rkt: it only works with kubenet, not with no-op network plugin. The later has no sysctl support.
- ~~[ ] add RunC implementation~~ (~~if that is already in kube,~~ it isn't)
- [x] update whitelist
- [x] switch PSC fields to annotations
- [x] switch PSP fields to annotations
- [x] decide about `--experimental-whitelist-sysctl` flag to be additive or absolute
- [x] decide whether to add a sysctl node whitelist annotation

### Release notes:

```release-note
The pod annotation `security.alpha.kubernetes.io/sysctls` now allows customization of namespaced and well isolated kernel parameters (sysctls), starting with `kernel.shm_rmid_forced`, `net.ipv4.ip_local_port_range`, `net.ipv4.tcp_max_syn_backlog` and `net.ipv4.tcp_syncookies` for Kubernetes 1.4.

The pod annotation  `security.alpha.kubernetes.io/unsafeSysctls` allows customization of namespaced sysctls where isolation is unclear. Unsafe sysctls must be enabled at-your-own-risk on the kubelet with the `--experimental-allowed-unsafe-sysctls` flag. Future versions will improve on resource isolation and more sysctls will be considered safe.
```
@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Sep 1, 2016

@kubernetes/docs here are the sysctl docs: kubernetes/website#1126

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Sep 1, 2016

/cc @kubernetes/feature-reviewers

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 2, 2018

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

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Mar 9, 2018

There are a number of people using sysctls now. I have not heard any issues with them.

I suggest to promote the current API (transformed to native fields in the PSP and on pods) to beta for 1.11.

@jeremyeder @vishh @derekwaynecarr @php-coder

@kubernetes/sig-node-api-reviews

@sttts sttts modified the milestones: v1.4, v1.11 Mar 21, 2018

@jeremyeder

This comment has been minimized.

Copy link
Member

jeremyeder commented Mar 21, 2018

Thanks, @sttts!

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Mar 21, 2018

@sttts it needs a feature gate.

from node side, it would be @sjenning who could help push this in sig-node. will sync w/ @dchen1107 next week. we discussed this briefly in last weeks sig-node.

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Mar 21, 2018

@derekwaynecarr in the kubelet not much would change code-wise. But of course we need a "go" from the node team that they think using sysctls is safe enough for beta. Note, that graduation to beta does not say anything about extending the list of safe sysctls.

It's already feature gated. As beta we would switch the default to true. Doesn't look like we had a feature gate sjenning/kubernetes@f4f7220

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Apr 17, 2018

@sttts
Any plans for this in 1.11?

If so, can you please ensure the feature is up-to-date with the appropriate:

  • Description
  • Milestone
  • Assignee(s)
  • Labels:
    • stage/{alpha,beta,stable}
    • sig/*
    • kind/feature

cc @idvoretskyi

@php-coder

This comment has been minimized.

Copy link

php-coder commented Apr 17, 2018

@sttts Do we need to wait until pod annotations become fields or it doesn't block us from graduating it to beta?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 17, 2018

@sttts Do we need to wait until pod annotations become fields or it doesn't block us from graduating it to beta?

yes, they need to become fields

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Apr 17, 2018

@php-coder @liggitt so just to clarify, no work planned for 1.11?
Also, would you mind updating the description to fit the new feature description template?

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Apr 17, 2018

@justaugustus promotion to beta is discussed in sig-node /cc @derekwaynecarr

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Apr 23, 2018

/remove-lifecycle stale

@ingvagabund

This comment has been minimized.

Copy link
Contributor

ingvagabund commented Apr 30, 2018

Working on the KEP for the graduation here: kubernetes/community#2093

@twilfong

This comment has been minimized.

Copy link

twilfong commented May 1, 2018

Are there also plans to include more sysctls in the safe set as part of this? My company would definitely make use of the ability to set net.ipv4.tcp_keepalive_time, tcp_keepalive_intvl, and tcp_keepalive_probes on a per-pod basis.

Example use: Java applications that depend on TCP keepalive, but which rely on the standard Socket class, can turn keepalive on with that class, but can't set those three parameters.

@php-coder

This comment has been minimized.

Copy link

php-coder commented May 2, 2018

There are also 2 open PRs for adding more safe sysctls: kubernetes/kubernetes#54896 and kubernetes/kubernetes#55011

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented May 2, 2018

@twilfong compare my comment kubernetes/kubernetes#54896 (comment). We are open to adding more sysctls to the safe set, but we need a kernel source analysis why it is safe. Note that also unsafe sysctls can be used, but they must be whitelisted in the kubelet.

@twilfong

This comment has been minimized.

Copy link

twilfong commented May 2, 2018

Thanks @php-coder and @sttts.

@sttts: I've read your comment and read through https://github.com/kubernetes/community/pull/700/files#diff-0e864ea85fc8d72b3bd0b0f39c34d143R342 and understand the basic requirements for whitelisting.

I have verified that the three net.ipv4.tcp_keepalive_* parameters are namespaced in net ns, but have not done an analysis to find if the memory resources caused by the sysctl are accounted for by the associated cgroup.

My guess is that this should meet the bar of not causing harm to the node or other containers on the same node where the pod with changed kernel parameter is run, since the keepalive parameters only control the timing of keepalive probes and when the socket is closed. (e.g. there should be no difference in memory allocation for any given socket, regardless of how these parameters are set.)

What is the recommended way to move forward with this? Should my team do a more deep analysis and then submit a pull request for a commit touching pkg/kubelet/sysctl/whitelist.go and pkg/kubelet/sysctl/whitelist_test.go? Or is there a different (better) recommended way to go about this?

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented May 2, 2018

@twilfong I would suggest to add a convincing discussion to the proposal in the community repo for documentation and a counter part PR in k/k against the whitelist. @sjenning @derekwaynecarr @vishh are the ones who can review this.

@ingvagabund

This comment has been minimized.

Copy link
Contributor

ingvagabund commented May 11, 2018

Promotion of annotations to API fields PR: kubernetes/kubernetes#63717

@mistyhacks

This comment has been minimized.

Copy link
Member

mistyhacks commented May 24, 2018

@sttts please fill out the appropriate line item of the
1.11 feature tracking spreadsheet
and open a placeholder docs PR against the
release-1.11 branch
by 5/25/2018 (tomorrow as I write this) if new docs or docs changes are
needed and a relevant PR has not yet been opened.

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented May 29, 2018

@ingvagabund

This comment has been minimized.

Copy link
Contributor

ingvagabund commented May 29, 2018

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 11, 2018

Feature issues opened in kubernetes/features should never be marked as frozen.
Feature Owners can ensure that features stay fresh by consistently updating their states across release cycles.

/remove-lifecycle frozen

@kacole2

This comment has been minimized.

Copy link
Member

kacole2 commented Jul 23, 2018

@sttts This feature was worked on in the previous milestone, so we'd like to check in and see if there are any plans for this to graduate stages in Kubernetes 1.12 since there is nothing in the original post.

If there are any updates, please explicitly ping @justaugustus, @kacole2, @robertsandoval, @rajendar38 to note that it is ready to be included in the Features Tracking Spreadsheet for Kubernetes 1.12.


Please note that the Features Freeze is July 31st, after which any incomplete Feature issues will require an Exception request to be accepted into the milestone.

In addition, please be aware of the following relevant deadlines:

  • Docs deadline (open placeholder PRs): 8/21
  • Test case freeze: 8/28

Please make sure all PRs for features have relevant release notes included as well.

Happy shipping!

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Jul 24, 2018

@kacole2 there is nothing planned to my knowledge in 1.12 about this feature. /cc @derekwaynecarr @ingvagabund @sjenning

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Jul 25, 2018

Thanks for the update, @sttts!
Can you modify this issue description to match the issue template?

@justaugustus justaugustus removed this from the v1.11 milestone Jul 25, 2018

@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Jul 26, 2018

@justaugustus @derekwaynecarr we need an owner of this feature. Is it sig-node?

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Jul 26, 2018

@sttts -- Based on the comment history, looks like this belongs to SIG Node & @derekwaynecarr.
Happy to chase people down if that isn't sufficient though.

@kacole2

This comment has been minimized.

Copy link
Member

kacole2 commented Oct 8, 2018

Hi
This enhancement has been tracked before, so we'd like to check in and see if there are any plans for this to graduate stages in Kubernetes 1.13. This release is targeted to be more ‘stable’ and will have an aggressive timeline. Please only include this enhancement if there is a high level of confidence it will meet the following deadlines:

  • Docs (open placeholder PRs): 11/8
  • Code Slush: 11/9
  • Code Freeze Begins: 11/15
  • Docs Complete and Reviewed: 11/27

Please take a moment to update the milestones on your original post for future tracking and ping @kacole2 if it needs to be included in the 1.13 Enhancements Tracking Sheet

Thanks!

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 6, 2019

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.

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

This comment has been minimized.

Copy link

fejta-bot commented Feb 5, 2019

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten

@krmayankk

This comment has been minimized.

Copy link
Contributor

krmayankk commented Feb 6, 2019

/remove-lifecycle rotten

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.