Add PriorityClass support to ComputeClasses #1815
Conversation
tylerslaton
force-pushed
the
priority-class-support
branch
from
837abf2
to
cb7f9bd
Compare
tylerslaton
force-pushed
the
priority-class-support
branch
3 times, most recently
from
dca4b09
to
a5a7bb7
Compare
tylerslaton
requested review from
thedadams,
StrongMonkey,
njhale and
g-linville
tylerslaton
force-pushed
the
priority-class-support
branch
from
a5a7bb7
to
28d0ab3
Compare
| // If a PriorityClass is specified, validate it exists | ||
| if cc.PriorityClassName != "" { | ||
| if err := s.client.Get(ctx, kclient.ObjectKey{Name: cc.PriorityClassName, Namespace: ""}, &schedulingv1.PriorityClass{}); err != nil { | ||
| return append(result, field.Invalid(field.NewPath("spec", "priorityClassName"), cc.PriorityClassName, err.Error())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we want to validate this on create? There are other instances where we don't validate such things. For example, we don't validate that a storage class exists when a volume class is created.
There was a problem hiding this comment.
Yeah so I had a similar thought. My thought process went slightly different so please let me know what you think.
If the PriorityClass isn't validated here, it is validated on App creation (like memory, cpu, etc is now). What felt different about that situation is that it felt off for an App to fail creation because a ComputeClass references a non-existent/misconfigured PriorityClass. What does feel weird about this is if a PriorityClass gets deleted after a ComputeClass gets created. In that event I added some controller logic on the App to verify that PriorityClass exists.
There was a problem hiding this comment.
Volume classes are similar. A volume class can reference any storage class it wants; we don't validate that on creation of the volume class.
If the storage class doesn't exist, then the app will fail when it is created because Kubernetes won't be able to find the storage class. It may not return an error, but it will hang forever because the volume won't be created.
There was a problem hiding this comment.
Okay, I agree with you here then. I've removed the API Server validation. The controller already had logic to check if the PriorityClass exists, do you think that should be removed as well in favor of just letting the Pods create?
tylerslaton
force-pushed
the
priority-class-support
branch
from
28d0ab3
to
e38e008
Compare
Signed-off-by: tylerslaton <mtslaton1@gmail.com>
tylerslaton
force-pushed
the
priority-class-support
branch
from
e38e008
to
0fffd51
Compare
With this PR,
ComputeClassesnow come with a new field,PriorityClassName. This allows users createComputeClassesto define aPriorityClassthat will get set on all Pods using theComputeClass.Checklist
This is a title (#1216). Here's an example