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

Pod Priority Class support #262

Merged
merged 6 commits into from
May 28, 2021
Merged

Pod Priority Class support #262

merged 6 commits into from
May 28, 2021

Conversation

prometherion
Copy link
Member

Partially addressing #257.

We have to plan a new version to handle this feature.

@prometherion prometherion requested a review from bsctl May 25, 2021 15:17
@prometherion prometherion self-assigned this May 25, 2021
@prometherion prometherion force-pushed the issues/257 branch 2 times, most recently from 216111c to 4bc7359 Compare May 25, 2021 15:39
@bsctl
Copy link
Member

bsctl commented May 27, 2021

@prometherion
when the feature is not enabled, i.e. the annotations priorityclass.capsule.clastix.io/allowed and priorityclass.capsule.clastix.io/allowed-regex are not specified into the tenant manifest, the admission webhook should admit pods with any priorityClassName, including no priorityClassName:

apiVersion: v1
kind: Pod
metadata:
  name: nginx
  labels:
    env: test
spec:
  containers:
  - name: nginx
    image: nginx
    imagePullPolicy: IfNotPresent
  priorityClassName: critical

when the feature is enabled, i.e. the annotations priorityclass.capsule.clastix.io/allowed and priorityclass.capsule.clastix.io/allowed-regex are specified into the tenant manifest, the admission webhook should admit pods without priorityClassName, i.e.

apiVersion: v1
kind: Pod
metadata:
  name: nginx
  labels:
    env: test
spec:
  containers:
  - name: nginx
    image: nginx
    imagePullPolicy: IfNotPresent
  priorityClassName: 

@prometherion
Copy link
Member Author

Totally agree @bsctl, just fixed with the expected behavior.

@bsctl
Copy link
Member

bsctl commented May 27, 2021

@prometherion

It's better but a case is still not correct, imo. Pls, see below

  1. As cluster admin, create oil tenant and priority class
apiVersion: capsule.clastix.io/v1alpha1
kind: Tenant
metadata:
  annotations:
    priorityclass.capsule.clastix.io/allowed: oil
  name: oil
spec:
  owner: # required
    name: engineering
    kind: Group

---
apiVersion: scheduling.k8s.io/v1
kind: PriorityClass
metadata:
  name: oil
value: 1000000
globalDefault: false
  1. As tenant owner, create namespace
$ kubectl create ns `oil-production`
  1. As tenant owner, create a pod with oil priority class
apiVersion: v1
kind: Pod
metadata:
  name: nginx
  labels:
    env: test
  namespace: oil-production
spec:
  containers:
  - name: nginx
    image: nginx
    imagePullPolicy: IfNotPresent
  priorityClassName: oil
  1. apply
kubectl apply -f pods.yaml -n oil-production
pod/nginx created

Ok!

  1. As tenant owner, create a pod with system-cluster-critical priority class
apiVersion: v1
kind: Pod
metadata:
  name: nginx
  labels:
    env: test
  namespace: oil-production
spec:
  containers:
  - name: nginx
    image: nginx
    imagePullPolicy: IfNotPresent
  priorityClassName: system-cluster-critical
  1. apply
kubectl apply -f pods.yaml -n oil-production
Error from server: error when creating "pods.yaml": admission webhook "podpriority.capsule.clastix.io" denied the request: Pod Priorioty Class nginx is forbidden for the current Tenant: use one from the following list (oil)

Ok!

  1. As tenant owner, create a pod without priority class
apiVersion: v1
kind: Pod
metadata:
  name: nginx
  labels:
    env: test
  namespace: oil-production
spec:
  containers:
  - name: nginx
    image: nginx
    imagePullPolicy: IfNotPresent
  1. apply
kubectl apply -f pods.yaml -n oil-production
Error from server: error when creating "pods.yaml": admission webhook "podpriority.capsule.clastix.io" denied the request: Pod Priorioty Class nginx is forbidden for the current Tenant: use one from the following list (oil)

Ko!

A null Priority Class should be allowed, i.e. null is a valid choice. If we do not consider null, then the tenant owner is forced to specify a Priority Class for all pods.

@prometherion
Copy link
Member Author

prometherion commented May 28, 2021

A null Priority Class should be allowed, i.e. null is a valid choice. If we do not consider null, then the tenant owner is forced to specify a Priority Class for all pods.

I'm feeling awkward missing this, but yeah, going to work to address the case!

@prometherion
Copy link
Member Author

prometherion commented May 28, 2021

Tested manually with latest force-pushed changes:

$ kubectl describe tnt oil
Name:         oil
Namespace:    
Labels:       <none>
Annotations:  priorityclass.capsule.clastix.io/allowed: oil
API Version:  capsule.clastix.io/v1alpha1
Kind:         Tenant
Metadata:
  Creation Timestamp:  2021-05-28T07:25:35Z
  Generation:          1
  Resource Version:  212798
  UID:               d5415961-ad68-4f4b-84a1-6f3bc342ddcf
Spec:
  Owner:
    Kind:  Group
    Name:  engineering
Status:
  Namespaces:
    oil-production
  Size:  1
Events:  <none>

$ cat <<EOF | kubectl apply -f -
pipe heredoc> apiVersion: v1
kind: Pod    
metadata:    
  name: nginx
  labels:    
    env: test
  namespace: oil-production 
spec:   
  containers:
  - name: nginx   
    image: nginx  
    imagePullPolicy: IfNotPresent
pipe heredoc> EOF

pod/nginx created

Copy link
Member

@bsctl bsctl left a comment

Choose a reason for hiding this comment

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

lgtm @prometherion many thanks!

@prometherion prometherion merged commit 52a73e0 into master May 28, 2021
@prometherion prometherion deleted the issues/257 branch May 28, 2021 22:31
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.

2 participants