Skip to content

Commit

Permalink
Review Remarks
Browse files Browse the repository at this point in the history
  • Loading branch information
@trasc
trasc committed Oct 25, 2024
1 parent 307167b commit 97fb529
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 18 deletions.
2 changes: 1 addition & 1 deletion pkg/controller/jobframework/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func MaximumExecutionTimeSeconds(job GenericJob) *int32 {
return nil
}

return ptr.To[int32](int32(v))
return ptr.To(int32(v))
}

func workloadPriorityClassName(job GenericJob) string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/jobframework/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func validateCreateForMaxExecTime(job GenericJob) field.ErrorList {
}

if v <= 0 {
return field.ErrorList{field.Invalid(maxExecTimeLabelPath, v, "should be grater then 0")}
return field.ErrorList{field.Invalid(maxExecTimeLabelPath, v, "should be greater than 0")}
}
}
return nil
Expand Down
20 changes: 14 additions & 6 deletions pkg/controller/jobs/job/job_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func TestValidateCreate(t *testing.T) {
Indexed(true).
Obj(),
wantErr: field.ErrorList{
field.Invalid(maxExecTimeLabelPath, 0, "should be grater then 0"),
field.Invalid(maxExecTimeLabelPath, 0, "should be greater than 0"),
},
serverVersion: "1.31.0",
},
Expand All @@ -257,7 +257,7 @@ func TestValidateCreate(t *testing.T) {
Indexed(true).
Obj(),
wantErr: field.ErrorList{
field.Invalid(maxExecTimeLabelPath, -10, "should be grater then 0"),
field.Invalid(maxExecTimeLabelPath, -10, "should be greater than 0"),
},
serverVersion: "1.31.0",
},
Expand Down Expand Up @@ -488,8 +488,18 @@ func TestValidateUpdate(t *testing.T) {
Obj(),
newJob: testingutil.MakeJob("job", "default").
Suspend(false).
Parallelism(5).
Completions(6).
Label(constants.MaxExecTimeSecondsLabel, "20").
Obj(),
wantErr: apivalidation.ValidateImmutableField("20", "10", maxExecTimeLabelPath),
},
{
name: "immutable max exec time while transitioning to unsuspended",
oldJob: testingutil.MakeJob("job", "default").
Suspend(true).
Label(constants.MaxExecTimeSecondsLabel, "10").
Obj(),
newJob: testingutil.MakeJob("job", "default").
Suspend(false).
Label(constants.MaxExecTimeSecondsLabel, "20").
Obj(),
wantErr: apivalidation.ValidateImmutableField("20", "10", maxExecTimeLabelPath),
Expand All @@ -502,8 +512,6 @@ func TestValidateUpdate(t *testing.T) {
Obj(),
newJob: testingutil.MakeJob("job", "default").
Suspend(true).
Parallelism(5).
Completions(6).
Label(constants.MaxExecTimeSecondsLabel, "20").
Obj(),
},
Expand Down
97 changes: 87 additions & 10 deletions pkg/controller/jobs/pod/pod_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -691,8 +691,6 @@ func TestReconciler(t *testing.T) {
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Annotation(controllerconsts.ProvReqAnnotationPrefix+"test-annotation", "test-val").
Annotation("invalid-provreq-prefix/test-annotation-2", "test-val-2").
Group("test-group").
GroupTotalCount("2").
Obj(),
Expand All @@ -703,8 +701,6 @@ func TestReconciler(t *testing.T) {
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Annotation(controllerconsts.ProvReqAnnotationPrefix+"test-annotation", "test-val").
Annotation("invalid-provreq-prefix/test-annotation-2", "test-val-2").
Group("test-group").
GroupTotalCount("2").
Obj(),
Expand All @@ -716,8 +712,6 @@ func TestReconciler(t *testing.T) {
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Annotation(controllerconsts.ProvReqAnnotationPrefix+"test-annotation", "test-val").
Annotation("invalid-provreq-prefix/test-annotation-2", "test-val-2").
Group("test-group").
GroupTotalCount("2").
Obj(),
Expand All @@ -728,8 +722,6 @@ func TestReconciler(t *testing.T) {
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Annotation(controllerconsts.ProvReqAnnotationPrefix+"test-annotation", "test-val").
Annotation("invalid-provreq-prefix/test-annotation-2", "test-val-2").
Group("test-group").
GroupTotalCount("2").
Obj(),
Expand All @@ -747,8 +739,8 @@ func TestReconciler(t *testing.T) {
OwnerReference(corev1.SchemeGroupVersion.WithKind("Pod"), "pod", "test-uid").
OwnerReference(corev1.SchemeGroupVersion.WithKind("Pod"), "pod2", "test-uid").
Annotations(map[string]string{
"kueue.x-k8s.io/is-group-workload": "true",
controllerconsts.ProvReqAnnotationPrefix + "test-annotation": "test-val"}).
"kueue.x-k8s.io/is-group-workload": "true",
}).
MaximumExecutionTimeSeconds(10).
Obj(),
},
Expand All @@ -762,6 +754,91 @@ func TestReconciler(t *testing.T) {
},
},
},
"workload is recreated when max exec time changes": {
pods: []corev1.Pod{
*basePodWrapper.
Clone().
Label(constants.ManagedByKueueLabel, "true").
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Group("test-group").
GroupTotalCount("2").
Obj(),
*basePodWrapper.
Clone().
Name("pod2").
Label(constants.ManagedByKueueLabel, "true").
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Group("test-group").
GroupTotalCount("2").
Obj(),
},
workloads: []kueue.Workload{
*utiltesting.MakeWorkload("test-group", "ns").Finalizers(kueue.ResourceInUseFinalizerName).
PodSets(
*utiltesting.MakePodSet("dc85db45", 2).
Request(corev1.ResourceCPU, "1").
SchedulingGates(corev1.PodSchedulingGate{Name: "kueue.x-k8s.io/admission"}).
Obj(),
).
Queue("user-queue").
Priority(0).
OwnerReference(corev1.SchemeGroupVersion.WithKind("Pod"), "pod", "test-uid").
OwnerReference(corev1.SchemeGroupVersion.WithKind("Pod"), "pod2", "test-uid").
Annotations(map[string]string{"kueue.x-k8s.io/is-group-workload": "true"}).
MaximumExecutionTimeSeconds(5).
Obj(),
},
wantPods: []corev1.Pod{
*basePodWrapper.
Clone().
Label(constants.ManagedByKueueLabel, "true").
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Group("test-group").
GroupTotalCount("2").
Obj(),
*basePodWrapper.
Clone().
Name("pod2").
Label(constants.ManagedByKueueLabel, "true").
Label(controllerconsts.MaxExecTimeSecondsLabel, "10").
KueueFinalizer().
KueueSchedulingGate().
Group("test-group").
GroupTotalCount("2").
Obj(),
},
wantWorkloads: []kueue.Workload{
*utiltesting.MakeWorkload("test-group", "ns").Finalizers(kueue.ResourceInUseFinalizerName).
PodSets(
*utiltesting.MakePodSet("dc85db45", 2).
Request(corev1.ResourceCPU, "1").
SchedulingGates(corev1.PodSchedulingGate{Name: "kueue.x-k8s.io/admission"}).
Obj(),
).
Queue("user-queue").
Priority(0).
OwnerReference(corev1.SchemeGroupVersion.WithKind("Pod"), "pod", "test-uid").
OwnerReference(corev1.SchemeGroupVersion.WithKind("Pod"), "pod2", "test-uid").
Annotations(map[string]string{"kueue.x-k8s.io/is-group-workload": "true"}).
MaximumExecutionTimeSeconds(10).
Obj(),
},
workloadCmpOpts: defaultWorkloadCmpOpts,
wantEvents: []utiltesting.EventRecord{
{
Key: types.NamespacedName{Name: "pod", Namespace: "ns"},
EventType: "Normal",
Reason: "UpdatedWorkload",
Message: "Updated not matching Workload for suspended job: ns/test-group",
},
},
},
"workload is found for the pod group": {
pods: []corev1.Pod{
*basePodWrapper.
Expand Down

0 comments on commit 97fb529

Please sign in to comment.