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

Introduce managedBy field and Remove managed-by label #487

Merged
merged 22 commits into from
Mar 29, 2024
Merged

Introduce managedBy field and Remove managed-by label #487

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
cd66b3e
Remove in const declaration and add field to JobSetSpec.
Mar 27, 2024
8c45f05
Remove references to LabelManagedBy in jobset_webhook.go.
Mar 27, 2024
7820c47
Add struct tag for ManagedBy field of JobSetSpec.
Mar 27, 2024
83ee86f
Add missing ` character in ManagedBy struct tag.
Mar 27, 2024
0713346
Update jobset_webhook_test.go to remove references to LabelManagedBy.
Mar 27, 2024
e866ed4
Update Reconcile to use ManagedBy field of JobSetSpec.
Mar 27, 2024
34c2b26
Update jobset_controller_test.go to use ManagedBy field of JobSetSpec.
Mar 27, 2024
ef199e7
Update jobset_webhook_test.go to use ManagedBy field of JobSetSpec.
Mar 27, 2024
55fb065
Fix bug where uninitialized value for js.Spec.ManagedBy was treated a…
Mar 28, 2024
6d3f0a0
Add an integration test that only tests job creation.
Mar 28, 2024
80643db
Ran non-test related make commands.
Mar 28, 2024
c3f9aef
Correct grammatical error.
Mar 28, 2024
0db3446
Regenerated files.
Mar 28, 2024
a10faa1
Remove TODO for jobset controller.
Mar 28, 2024
a82e440
Update comment for JobSetManager to no longer reference LabelManagedBy.
Mar 28, 2024
66b55c4
Change implementation of JobSet controller manager to better align wi…
Mar 29, 2024
f8a2bf5
Add comment providing context for checking JobSet manager.
Mar 29, 2024
dae1c28
Add validation for managedBy field.
Mar 29, 2024
7559f34
Create tests for jobset controller name validation.
Mar 29, 2024
98238ac
Create variable for overly long controller name error.
Mar 29, 2024
76d5546
Add tests for valid values of managedBy.
Mar 29, 2024
80ac00c
Correct test for unset jobset controller name.
Mar 29, 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
12 changes: 6 additions & 6 deletions api/jobset/v1alpha2/jobset_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,9 @@ const (
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"
// JobSetControllerName is the reserved value for the managedBy field for the built-in
// JobSet controller.
JobSetControllerName = "jobset.sigs.k8s.io/jobset-controller"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we to introduce validation in the webhook with IsDomainPrefixedPath, analogously as for Job: https://github.com/kubernetes/kubernetes/blob/f4e246bc93ffb68b33ed67c7896c379efa4207e7/pkg/apis/batch/validation/validation.go#L220-L223. Potentially also length.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think so, we have been bitten before by not having strict enough validation, so I'd like to enforce the field is restricted to expected format and length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code. Please resolve if nothing else is needed.

Copy link
Contributor

@danielvegamyhre danielvegamyhre Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you add unit tests ensuring the validation works as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

)

type JobSetConditionType string
Expand Down Expand Up @@ -94,6 +91,9 @@ type JobSetSpec struct {

// Suspend suspends all running child Jobs when set to true.
Suspend *bool `json:"suspend,omitempty"`

// ManagedBy is used to indicate the controller or entity that manages a JobSet
ManagedBy *string `json:"managedBy,omitempty"`
jedwins1998 marked this conversation as resolved.
Show resolved Hide resolved
}

// JobSetStatus defines the observed state of JobSet
Expand Down
9 changes: 3 additions & 6 deletions api/jobset/v1alpha2/jobset_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,8 @@ func (js *JobSet) Default() {
}
}

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

Expand Down Expand Up @@ -179,7 +176,7 @@ func (js *JobSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
}
// Note that SucccessPolicy and failurePolicy are made immutable via CEL.
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))...)
errs = append(errs, apivalidation.ValidateImmutableField(mungedSpec.ManagedBy, oldJS.Spec.ManagedBy, field.NewPath("spec").Child("labels").Key("managedBy"))...)
return nil, errs.ToAggregate()
}

Expand Down
80 changes: 20 additions & 60 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,9 +39,6 @@ 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,12 +52,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -75,15 +70,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
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 @@ -97,12 +90,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -117,15 +108,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
name: "enableDNSHostnames is unset",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -139,12 +128,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -159,15 +146,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
name: "enableDNSHostnames is false",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -182,12 +167,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -202,15 +185,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
name: "pod restart policy unset",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -227,12 +208,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -251,15 +230,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
name: "pod restart policy set",
js: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -278,12 +255,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -302,15 +277,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
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 @@ -328,12 +301,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
StartupPolicy: defaultStartupPolicy,
SuccessPolicy: defaultSuccessPolicy,
Expand All @@ -352,15 +323,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
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 @@ -381,12 +350,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: &SuccessPolicy{
Operator: OperatorAny,
Expand All @@ -407,6 +374,7 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
Expand Down Expand Up @@ -438,9 +406,6 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: &SuccessPolicy{
Operator: OperatorAny,
Expand All @@ -463,6 +428,7 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
Expand All @@ -485,9 +451,6 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: JobSetManager},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -502,15 +465,13 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To(JobSetControllerName),
},
},
},
{
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)},
Expand All @@ -524,12 +485,10 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To("other-controller"),
},
},
want: &JobSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{LabelManagedBy: "other-controller"},
},
Spec: JobSetSpec{
SuccessPolicy: defaultSuccessPolicy,
StartupPolicy: defaultStartupPolicy,
Expand All @@ -544,6 +503,7 @@ func TestJobSetDefaulting(t *testing.T) {
},
},
},
ManagedBy: ptr.To("other-controller"),
},
},
},
Expand Down
7 changes: 7 additions & 0 deletions api/jobset/v1alpha2/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions api/jobset/v1alpha2/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading