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.

Sign up for GitHub

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 labels for workload parent #1032

Merged
merged 4 commits into from
Aug 3, 2023
Merged

Add labels for workload parent #1032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb1a28d
Added labels for workload parent
achernevskii Aug 2, 2023
2478cbd
Replace parent name and group-kind labels with parent uid
achernevskii Aug 2, 2023
866a09a
Rename parent-uid label to job-uid, change label validation
achernevskii Aug 3, 2023
0350ad8
Update job-uid label validation
achernevskii Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/controller/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,8 @@ const (
// ignores this Job from admission, and takes control of its suspension
// status based on the admission status of the parent workload.
ParentWorkloadAnnotation = "kueue.x-k8s.io/parent-workload"

// JobUIDLabel is the label key in the workload resource, that holds the UID of
// the owner job.
JobUIDLabel = "kueue.x-k8s.io/job-uid"
)
15 changes: 15 additions & 0 deletions pkg/controller/jobframework/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/client-go/tools/record"
"k8s.io/utils/pointer"
ctrl "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -456,19 +457,33 @@ func (r *JobReconciler) stopJob(ctx context.Context, job GenericJob, object clie

// constructWorkload will derive a workload from the corresponding job.
func (r *JobReconciler) constructWorkload(ctx context.Context, job GenericJob, object client.Object) (*kueue.Workload, error) {
log := ctrl.LoggerFrom(ctx)

podSets := job.PodSets()

wl := &kueue.Workload{
ObjectMeta: metav1.ObjectMeta{
Name: GetWorkloadNameForOwnerWithGVK(object.GetName(), job.GetGVK()),
Namespace: object.GetNamespace(),
Labels: map[string]string{},
},
Spec: kueue.WorkloadSpec{
PodSets: resetMinCounts(podSets),
QueueName: QueueName(job),
},
}

jobUid := string(job.Object().GetUID())
if errs := validation.IsValidLabelValue(jobUid); len(errs) == 0 {
wl.Labels[controllerconsts.JobUIDLabel] = jobUid
} else {
log.V(2).Info(
"Validation of the owner job UID label has failed. Creating workload without the label.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Validation of the owner job UID label has failed. Creating workload without the label.",
"Validation of the owner job UID label has failed. Creating workload without the label",

"ValidationErrors", errs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"ValidationErrors", errs,
"err", errs.ToAggregate(),

Copy link
Contributor Author

@achernevskii achernevskii Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not a ErrorListtype here, IsValidLabelValue will return a slice of strings, so I can't call this method here.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhhh, that's why the value is not present either

"LabelValue", jobUid,
Copy link
Contributor

Choose a reason for hiding this comment

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

this will probably already be part of the error, so no need to include again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the log message, that I'm getting while unit testing:

"Validation of the owner job UID label has failed. Creating workload without the label" 
"job"="ns/job" 
"ValidationErrors"=["must be no more than 63 characters"] 
"LabelValue"="long-uidlong-uidlong-uidlong-uidlong-uidlong-uidlong-uidlong-uid"

Label is not included in the error here.

)
}

priorityClassName, p, err := r.extractPriority(ctx, podSets, job)
if err != nil {
return nil, err
Expand Down
33 changes: 32 additions & 1 deletion pkg/controller/jobs/job/job_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package job

import (
"strings"
"testing"
"time"

Expand All @@ -31,6 +32,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1"
controllerconsts "sigs.k8s.io/kueue/pkg/controller/constants"
"sigs.k8s.io/kueue/pkg/controller/jobframework"
"sigs.k8s.io/kueue/pkg/features"
"sigs.k8s.io/kueue/pkg/util/pointer"
Expand Down Expand Up @@ -307,7 +309,10 @@ var (
cmpopts.SortSlices(func(a, b kueue.Workload) bool {
return a.Name < b.Name
}),
cmpopts.IgnoreFields(kueue.Workload{}, "TypeMeta", "ObjectMeta"),
cmpopts.IgnoreFields(
kueue.Workload{}, "TypeMeta", "ObjectMeta.OwnerReferences",
"ObjectMeta.Name", "ObjectMeta.ResourceVersion",
),
cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"),
}
)
Expand Down Expand Up @@ -424,16 +429,42 @@ func TestReconciler(t *testing.T) {
Clone().
Suspend(false).
Queue("test-queue").
UID("test-uid").
Obj(),
wantJob: *baseJobWrapper.
Clone().
Queue("test-queue").
UID("test-uid").
Obj(),
wantWorkloads: []kueue.Workload{
*utiltesting.MakeWorkload("job", "ns").
PodSets(*utiltesting.MakePodSet(kueue.DefaultPodSetName, 10).Request(corev1.ResourceCPU, "1").Obj()).
Queue("test-queue").
Priority(0).
Labels(map[string]string{
controllerconsts.JobUIDLabel: "test-uid",
}).
Obj(),
},
},
"the workload without uid label is created when job's uid is longer than 63 characters": {
job: *baseJobWrapper.
Clone().
Suspend(false).
Queue("test-queue").
UID(strings.Repeat("long-uid", 8)).
Obj(),
wantJob: *baseJobWrapper.
Clone().
Queue("test-queue").
UID(strings.Repeat("long-uid", 8)).
Obj(),
wantWorkloads: []kueue.Workload{
*utiltesting.MakeWorkload("job", "ns").
PodSets(*utiltesting.MakePodSet(kueue.DefaultPodSetName, 10).Request(corev1.ResourceCPU, "1").Obj()).
Queue("test-queue").
Priority(0).
Labels(map[string]string{}).
Obj(),
},
},
Expand Down
5 changes: 5 additions & 0 deletions pkg/util/testing/wrappers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ func (w *WorkloadWrapper) ReclaimablePods(rps ...kueue.ReclaimablePod) *Workload
return w
}

func (w *WorkloadWrapper) Labels(l map[string]string) *WorkloadWrapper {
w.ObjectMeta.Labels = l
return w
}

type PodSetWrapper struct{ kueue.PodSet }

func MakePodSet(name string, count int) *PodSetWrapper {
Expand Down