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

Default Value PriorityClass/storageClasses/allowedClasses #610

Closed
oliverbaehler opened this issue Jul 21, 2022 · 6 comments
Closed

Default Value PriorityClass/storageClasses/allowedClasses #610

oliverbaehler opened this issue Jul 21, 2022 · 6 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@oliverbaehler
Copy link
Collaborator

Describe the feature

Generally it's a default value for priorityClasses, storageClasses and allowedClasses (Ingress). Adding a default value would simplify working in the tenant for the customer, since they wouldn't have to worry to much about these values, since a default value is applied. But then con overwrite it, by setting their value.

Would you be happy to accept this feature(s)? I will do the contribution.

What would the new user story look like?

Story for PriorityClasses.

Add the feature to define a default PriorityClass for all pods within the tenant. The given default PriorityClass would be assigned to all pods, which don't have a priorityClass set. If a PriorityClass is set on a pad, the default is ignored. The default value is also verified against the allowed/regex.

A new tenant is created. The customer can assign two tiers tier-gold and tier-silver. To make it more convenient to user only has to tag pods with higher priority with tier-gold. While all the other pods are assigned the tier-silver. Manifest could look like this:

apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - name: alice
    kind: User
  priorityClasses:
    default: "tier-silver"
    allowed:
    - default
    allowedRegex: "^tier-.*$"
@oliverbaehler oliverbaehler added the blocked-needs-validation Issue need triage and validation label Jul 21, 2022
@oliverbaehler
Copy link
Collaborator Author

Maybe it would even make sense adding a namespace selector.

apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - name: alice
    kind: User
  priorityClasses:
    defaults:
      - value: "tier-prod"
         namespaceSelector:
            matchExpressions:
              - {key: environment, operator: NotIn, values: [dev]}
      - value: "tier-silver"
         namespaceSelector:
            matchExpressions:
              - {key: environment, operator: In, values: [dev]}
    allowed:
    - default
    allowedRegex: "^tier-.*$"

@prometherion
Copy link
Member

This feature would require a mutating webhook in order to patch pods with the default value.

Along with that, I would start using a bare value to keep the homogeneity of the other keys such as allowed that are referring to bare names.

Regarding the selector, it's something we are already evaluating in #436, and that would be absolutely breaking since it means dropping the exact match and the regex one.

@prometherion prometherion added enhancement New feature or request and removed blocked-needs-validation Issue need triage and validation labels Jul 22, 2022
@prometherion prometherion added this to the v0.2.0 milestone Oct 21, 2022
@prometherion prometherion self-assigned this Oct 21, 2022
@prometherion
Copy link
Member

We can work on this, once #644 is merged.

@oliverbaehler
Copy link
Collaborator Author

@prometherion you can assign it to me, i have already some code for this feature but will wait for the new apiv

@prometherion
Copy link
Member

prometherion commented Dec 26, 2022

Hey Oliver, is the code still valid? v1beta2 has been released upstream, wondering if we can reuse your logic here, otherwise, I'll build from scratch.

@oliverbaehler
Copy link
Collaborator Author

oliverbaehler commented Jan 1, 2023

Here some Notes/Discoveries (Important regarding the Review, updated over time)

## Persistence

Multiple Defaults are not allowed by default by the API

Multiple StorageClasses may be marked as default:

global-default (default)   kubernetes.io/no-provisioner   Delete          WaitForFirstConsumer   false                  11m
standard (default)         rancher.io/local-path          Delete          WaitForFirstConsumer   false                  79m

When trying to allocate a PV/PVC you will get the following reject:

Error from server (Forbidden): error when creating "pvc.yaml": persistentvolumeclaims "default-pvc" is forbidden: Internal error occurred: 2 default StorageClasses were found

Therefor we always expect the default StorageClass to be exactly one element at most.

Skip Update Handling

Field storageClassName can not be patched for PersistentVolume/Claim:

# persistentvolumeclaims "default-pvc" was not valid:
# * spec: Forbidden: spec is immutable after creation except resources.requests for bound claims
#   core.PersistentVolumeClaimSpec{
#       ... // 2 identical fields
#       Resources:        {Requests: {s"storage": {i: {...}, s: "30Gi", Format: "BinarySI"}}},
#       VolumeName:       "",
# -     StorageClassName: &"standard",
# +     StorageClassName: &"new-class",
#       VolumeMode:       &"Filesystem",
#       DataSource:       nil,
#       DataSourceRef:    nil,
#   }

Therefor no Handling on UPDATE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants