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

Critical pods should not be limited to kube-system namespace #60596

Closed
bsalamat opened this issue Feb 28, 2018 · 17 comments · Fixed by #60653
Closed

Critical pods should not be limited to kube-system namespace #60596

bsalamat opened this issue Feb 28, 2018 · 17 comments · Fixed by #60653
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.

Comments

@bsalamat
Copy link
Member

When we used annotations to mark critical pods, we considered pods with such an annotation critical only when they were created in "kube-system" namespace. Going to the new era of using priority to indicate critical pods, we would like to remove the restriction of "kube-system" namespace and consider pods with critical priority as critical no matter what namespace they are in.

ref/ #60519 (comment)

/kind bug
/sig scheduling
/assign @ravisantoshgudimetla
cc/ @liggitt

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 28, 2018
@ravisantoshgudimetla
Copy link
Contributor

While I am fine with the approach, it seems the only way to ensure that those critical pods are not coming from some random namespace is whitelisting them using priority admission controller or using quota. Atleast previously we have one more level of security that critical pod has to be created in kube-system namespace.

@liggitt
Copy link
Member

liggitt commented Mar 1, 2018

previously we have one more level of security that critical pod has to be created in kube-system namespace

The requirement to limit the critical pod annotation meaning to the kube-system namespace was a workaround for the lack of access control and quota around the annotation. We actively work to prevent special handling of the kube-system namespace wherever possible.

@liggitt
Copy link
Member

liggitt commented Mar 1, 2018

the only way to ensure that those critical pods are not coming from some random namespace is whitelisting them using priority admission controller or using quota

Yes, which is why those aspects are essential to a functional priority system that works for a variety of cluster configurations

@derekwaynecarr
Copy link
Member

Agree with Ravi and Jordan on what is needed for final solution.

@bsalamat
Copy link
Member Author

bsalamat commented Mar 1, 2018

There is no doubt that we need to have quota for priority. It is needed not only for critical priorities, but also for any other high priority classes that users may define. That effort is going forward and we will have it soon.

k8s-github-robot pushed a commit that referenced this issue Mar 28, 2018
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>.

Critical pods shouldn't be restricted to kube-system

**What this PR does / why we need it**:
To make sure that critical pods are not restricted to kube-system namespace.
**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 #60596

**Special notes for your reviewer**:
@bsalamat @liggitt @aveshagarwal - Can we hold this till we merge quota restriction PR #57963.
**Release note**:

```release-note
NONE
```
@k82cn
Copy link
Member

k82cn commented Sep 27, 2018

@ravisantoshgudimetla , I think we also need to update Priority admission controller for that.

root@wgl-ubuntu:/home/k82cn/go_ws/src/k8s.io/kubernetes# kc create -f pod.yaml
Error from server (Forbidden): error when creating "pod.yaml": pods "rss-site" is forbidden: pods with system-cluster-critical priorityClass is not permitted in default namespace

root@wgl-ubuntu:/home/k82cn/go_ws/src/k8s.io/kubernetes# kc version
Client Version: version.Info{Major:"1", Minor:"11+", GitVersion:"v1.11.0-alpha.1.439+535426bf736a31-dirty", GitCommit:"535426bf736a31edeb76cf64f3b6d90e6996e49a", GitTreeState:"dirty", BuildDate:"2018-05-01T07:14:55Z", GoVersion:"go1.10.1", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"13+", GitVersion:"v1.13.0-alpha.0.1200+323e1375b3b15a", GitCommit:"323e1375b3b15a4144d46d41155ca402a2fcef63", GitTreeState:"clean", BuildDate:"2018-09-27T06:52:20Z", GoVersion:"go1.10.2", Compiler:"gc", Platform:"linux/amd64"}

root@wgl-ubuntu:/home/k82cn/go_ws/src/k8s.io/kubernetes# cat pod.yaml
apiVersion: v1
kind: Pod
metadata:
  name: rss-site
  labels:
    app: web
spec:
  priorityClassName: system-cluster-critical
  containers:
    - name: front-end
      image: nginx
      ports:
        - containerPort: 80

@liggitt
Copy link
Member

liggitt commented Sep 27, 2018

see later PR re-restricting this until the quota controls graduate to at least beta state: #65593

@k82cn
Copy link
Member

k82cn commented Sep 27, 2018

Got that, thanks :) That makes sense to me.

@llarsson
Copy link

If this is closed and there even was a merged pull request for this type of functionality, what is the current status? And is the documentation in sync with current status?

@nevalla
Copy link

nevalla commented Jan 30, 2019

It seems that kube-system namespace is still hard coded in Priority plugin, so I guess the current status is that system critical pods are still limited to kube-system namespace. Based on that I think this issue should not be closed.

@tlvenn
Copy link

tlvenn commented May 1, 2019

@bsalamat can we reopen this issue please because as pointed out by @nevalla, the issue still exists. Thanks.

@zimmertr
Copy link

zimmertr commented Oct 7, 2019

Waiting for this to be available myself. No reason my Node Exporter dataset has to run in the kube-system namespace instead of my Grafana namespace.

@vllry
Copy link
Contributor

vllry commented Oct 8, 2019

Confirmed code + behavior.
/open

@vllry
Copy link
Contributor

vllry commented Oct 8, 2019

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Oct 8, 2019
@k8s-ci-robot
Copy link
Contributor

@vllry: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@nesl247
Copy link

nesl247 commented Nov 5, 2019

If this is closed and there even was a merged pull request for this type of functionality, what is the current status? And is the documentation in sync with current status?

I too am confused based on that documentation. Can we get an update here with an answer and fix the documentation if it's wrong?

@liggitt
Copy link
Member

liggitt commented Nov 28, 2019

This is resolved in 1.17

See #76310

@liggitt liggitt closed this as completed Nov 28, 2019
JoelColledge added a commit to LINBIT/piraeus that referenced this issue Dec 12, 2019
Due to kubernetes/kubernetes#60596, we cannot
use 'priorityClass: system-node-critical' with Kubernetes versions prior
to 1.17.
JoelColledge added a commit to LINBIT/piraeus that referenced this issue Dec 12, 2019
Due to kubernetes/kubernetes#60596, we cannot
use 'priorityClass: system-node-critical' with Kubernetes versions prior
to 1.17.
rck pushed a commit to piraeusdatastore/piraeus that referenced this issue Dec 12, 2019
Due to kubernetes/kubernetes#60596, we cannot
use 'priorityClass: system-node-critical' with Kubernetes versions prior
to 1.17.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Projects
None yet
Development

Successfully merging a pull request may close this issue.