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 non-preempting option to PriorityClasses #74614
add non-preempting option to PriorityClasses #74614
Conversation
faccb20
to
23151be
Compare
@@ -42,5 +42,5 @@ func init() { | |||
// We only register manually written functions here. The registration of the | |||
// generated functions takes place in the generated files. The separation | |||
// makes the code compile even when the generated files are missing. | |||
localSchemeBuilder.Register(RegisterDefaults) | |||
localSchemeBuilder.Register(addDefaultingFuncs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, the regitration for defaults should be under v1 ?
23151be
to
cfc3970
Compare
cfc3970
to
dadb9fd
Compare
cf92746
to
912a422
Compare
/retest |
912a422
to
d7af3ef
Compare
@@ -41,6 +41,9 @@ func ValidatePriorityClass(pc *scheduling.PriorityClass) field.ErrorList { | |||
// Non-system critical priority classes are not allowed to have a value larger than HighestUserDefinablePriority. | |||
allErrs = append(allErrs, field.Forbidden(field.NewPath("value"), fmt.Sprintf("maximum allowed value of a user defined priority is %v", scheduling.HighestUserDefinablePriority))) | |||
} | |||
if pc.PreemptionPolicy != nil { | |||
allErrs = append(allErrs, apivalidation.ValidatePreemptionPolicy(pc.PreemptionPolicy, field.NewPath("PreemptionPolicy"))...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"preemptionPolicy"
, since this is shown to the user and should match the serialized field name
@@ -52,5 +55,7 @@ func ValidatePriorityClassUpdate(pc, oldPc *scheduling.PriorityClass) field.Erro | |||
if pc.Value != oldPc.Value { | |||
allErrs = append(allErrs, field.Forbidden(field.NewPath("Value"), "may not be changed in an update.")) | |||
} | |||
// PreemptionPolicy is immutable and is checked by the ObjectMeta validator. | |||
allErrs = append(allErrs, apivalidation.ValidateImmutableField(pc.PreemptionPolicy, oldPc.PreemptionPolicy, field.NewPath("PreemptionPolicy"))...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"preemptionPolicy"
, since this is shown to the user and should match the serialized field name
|
||
cmd.Flags().Int32("value", 0, i18n.T("the value of this priority class.")) | ||
cmd.Flags().Bool("global-default", false, i18n.T("global-default specifies whether this PriorityClass should be considered as the default priority.")) | ||
cmd.Flags().String("description", "", i18n.T("description is an arbitrary string that usually provides guidelines on when this priority class should be used.")) | ||
cmd.Flags().String("preemption-policy", "PreemptLowerPriority", i18n.T("preemption-policy is the policy for preempting pods with lower priority.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't specify a default for alpha fields
verify test requires single-line invocations of SetFeatureGateDuringTest:
|
a few last comments and the verify test fixup, then this LGTM. thanks for all the work |
/approve API changes look good. needs a squash then lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, denkensk, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
08393b8
to
5e3e7ba
Compare
/lgtm |
/retest |
/test pull-kubernetes-e2e-gce-100-performance |
1 similar comment
/test pull-kubernetes-e2e-gce-100-performance |
Co-authored-by: Vallery Lancey <vallery@zeitgeistlabs.io> Co-authored-by: Tan shanshan <tan.shanshan@zte.com.cn>
5e3e7ba
to
52f3380
Compare
/lgtm |
/retest |
What type of PR is this?
/kind feature
/priority important-soon
What this PR does / why we need it:
Adds a NonPrempting field to the PriorityClass. If set on a class, it will continue to be prioritized above queued pods of a lesser class, but will not preempt running pods.
Which issue(s) this PR fixes:
Fixes #67671
kubernetes/enhancements#902