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 managed-by label support. #407

Merged
merged 4 commits into from
Feb 13, 2024
Merged

Add managed-by label support. #407

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
e89fc47
Add managed-by label support.
trasc Feb 12, 2024
7d93e33
Review Reamarks
trasc Feb 12, 2024
37ccd54
Review Remarks
trasc Feb 13, 2024
a0556f3
Fix UT after rebase
trasc Feb 13, 2024
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
7 changes: 7 additions & 0 deletions api/jobset/v1alpha2/jobset_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ const (
NodeSelectorStrategyKey string = "alpha.jobset.sigs.k8s.io/node-selector"
NamespacedJobKey string = "alpha.jobset.sigs.k8s.io/namespaced-job"
NoScheduleTaintKey string = "alpha.jobset.sigs.k8s.io/no-schedule"

// LabelManagedBy is used to indicate the controller or entity that manages an JobSet
LabelManagedBy = "alpha.jobset.sigs.k8s.io/managed-by"

// JobSetManager is used as the value for LabelManagedBy to identify the jobset controller manager
// as the manager of a specific JobSet.
JobSetManager = "jobset"
)

type JobSetConditionType string
Expand Down
17 changes: 13 additions & 4 deletions api/jobset/v1alpha2/jobset_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ func (js *JobSet) Default() {
js.Spec.ReplicatedJobs[i].Template.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyOnFailure
}
}

if _, found := js.Labels[LabelManagedBy]; !found {
if js.Labels == nil {
js.Labels = make(map[string]string, 1)
}
js.Labels[LabelManagedBy] = JobSetManager
}
}

//+kubebuilder:webhook:path=/validate-jobset-x-k8s-io-v1alpha2-jobset,mutating=false,failurePolicy=fail,sideEffects=None,groups=jobset.x-k8s.io,resources=jobsets,verbs=create;update,versions=v1alpha2,name=vjobset.kb.io,admissionReviewVersions=v1
Expand Down Expand Up @@ -136,14 +143,16 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (js *JobSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
mungedSpec := js.Spec.DeepCopy()
oldSpec := old.(*JobSet).Spec
if ptr.Deref(oldSpec.Suspend, false) {
oldJS := old.(*JobSet)
if ptr.Deref(oldJS.Spec.Suspend, false) {
for index := range js.Spec.ReplicatedJobs {
mungedSpec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector = oldSpec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector
mungedSpec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector = oldJS.Spec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector
}
}
// Note that SucccessPolicy and failurePolicy are made immutable via CEL.
return nil, apivalidation.ValidateImmutableField(mungedSpec.ReplicatedJobs, oldSpec.ReplicatedJobs, field.NewPath("spec").Child("replicatedJobs")).ToAggregate()
errs := apivalidation.ValidateImmutableField(mungedSpec.ReplicatedJobs, oldJS.Spec.ReplicatedJobs, field.NewPath("spec").Child("replicatedJobs"))
errs = append(errs, apivalidation.ValidateImmutableField(js.Labels[LabelManagedBy], oldJS.Labels[LabelManagedBy], field.NewPath("metadata").Child("labels").Key(LabelManagedBy))...)
return nil, errs.ToAggregate()
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
Expand Down
132 changes: 132 additions & 0 deletions api/jobset/v1alpha2/jobset_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "job completion mode is unset",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -55,6 +58,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -75,6 +81,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "job completion mode is set to non-indexed",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
Network: &Network{EnableDNSHostnames: ptr.To(true)},
Expand All @@ -91,6 +100,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -111,6 +123,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "enableDNSHostnames is unset",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -127,6 +142,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -147,6 +165,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "enableDNSHostnames is false",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -164,6 +185,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -184,6 +208,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "pod restart policy unset",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -203,6 +230,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -227,6 +257,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "pod restart policy set",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -248,6 +281,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -272,6 +308,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "success policy unset",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
StartupPolicy: defaultStartupPolicy,
Network: &Network{EnableDNSHostnames: ptr.To(true)},
Expand All @@ -292,6 +331,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
StartupPolicy: defaultStartupPolicy,
SuccessPolicy: defaultSuccessPolicy,
Expand All @@ -316,6 +358,9 @@ func TestJobSetDefaulting(t *testing.T) {
{
name: "success policy operator set, replicatedJobNames unset",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: &SuccessPolicy{
Operator: OperatorAny,
Expand All @@ -339,6 +384,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: &SuccessPolicy{
Operator: OperatorAny,
Expand Down Expand Up @@ -390,6 +438,9 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: &SuccessPolicy{
Operator: OperatorAny,
Expand All @@ -415,6 +466,87 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
{
name: "managed-by label is unset",
trasc marked this conversation as resolved.
Show resolved Hide resolved
js: &JobSet{
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
Network: &Network{EnableDNSHostnames: ptr.To(true)},
ReplicatedJobs: []ReplicatedJob{
{
Template: batchv1.JobTemplateSpec{
Spec: batchv1.JobSpec{
Template: TestPodTemplate,
CompletionMode: completionModePtr(batchv1.IndexedCompletion),
},
},
},
},
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Network: &Network{EnableDNSHostnames: ptr.To(true)},
ReplicatedJobs: []ReplicatedJob{
{
Template: batchv1.JobTemplateSpec{
Spec: batchv1.JobSpec{
Template: TestPodTemplate,
CompletionMode: completionModePtr(batchv1.IndexedCompletion),
},
},
},
},
},
},
},
{
name: "when provided, managed-by label is preserved",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: "other-controller"},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
Network: &Network{EnableDNSHostnames: ptr.To(true)},
ReplicatedJobs: []ReplicatedJob{
{
Template: batchv1.JobTemplateSpec{
Spec: batchv1.JobSpec{
Template: TestPodTemplate,
CompletionMode: completionModePtr(batchv1.IndexedCompletion),
},
},
},
},
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: "other-controller"},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Network: &Network{EnableDNSHostnames: ptr.To(true)},
ReplicatedJobs: []ReplicatedJob{
{
Template: batchv1.JobTemplateSpec{
Spec: batchv1.JobSpec{
Template: TestPodTemplate,
CompletionMode: completionModePtr(batchv1.IndexedCompletion),
},
},
},
},
},
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/controllers/jobset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ func (r *JobSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr

log := ctrl.LoggerFrom(ctx).WithValues("jobset", klog.KObj(&js))
ctx = ctrl.LoggerInto(ctx, log)

if manager, found := js.Labels[jobset.LabelManagedBy]; found && manager != jobset.JobSetManager {
log.V(5).Info("Skipping JobSet managed by a different controller", "managed-by", manager)
return ctrl.Result{}, nil
}

log.V(2).Info("Reconciling JobSet")

// Get Jobs owned by JobSet.
Expand Down
6 changes: 6 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 @@ -78,6 +78,12 @@ func (j *JobSetWrapper) SetAnnotations(annotations map[string]string) *JobSetWra
return j
}

// SetLabels sets the value of the jobSet.metadata.labels.
func (j *JobSetWrapper) SetLabels(labels map[string]string) *JobSetWrapper {
j.Labels = labels
return j
}

// GenerateName sets the JobSet name.
func (j *JobSetWrapper) SetGenerateName(namePrefix string) *JobSetWrapper {
// Name and GenerateName are mutually exclusive, so we must unset the Name field.
Expand Down
Loading