Merge pull request #395 from danielvegamyhre/subdomain-fix

Default service name in JobSet controller

api/jobset/v1alpha2/jobset_webhook.go

@@ -63,10 +63,6 @@ func (js *JobSet) Default() {
 		if js.Spec.Network.EnableDNSHostnames == nil {
 			js.Spec.Network.EnableDNSHostnames = ptr.To(true)
 		}
-		// Subdomain defaults to the JobSet name
-		if js.Spec.Network.Subdomain == "" {
-			js.Spec.Network.Subdomain = js.Name
-		}
 
 		// Default pod restart policy to OnFailure.
 		if js.Spec.ReplicatedJobs[i].Template.Spec.Template.Spec.RestartPolicy == "" {

api/jobset/v1alpha2/jobset_webhook_test.go

@@ -138,49 +138,6 @@ func TestJobSetDefaulting(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "subdomain defaults to jobset name",
-			js: &JobSet{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "custom-jobset",
-				},
-				Spec: JobSetSpec{
-					SuccessPolicy: defaultSuccessPolicy,
-					ReplicatedJobs: []ReplicatedJob{
-						{
-							Template: batchv1.JobTemplateSpec{
-								Spec: batchv1.JobSpec{
-									Template:       TestPodTemplate,
-									CompletionMode: completionModePtr(batchv1.IndexedCompletion),
-								},
-							},
-						},
-					},
-				},
-			},
-			want: &JobSet{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "custom-jobset",
-				},
-				Spec: JobSetSpec{
-					SuccessPolicy: defaultSuccessPolicy,
-					Network: &Network{
-						EnableDNSHostnames: ptr.To(true),
-						Subdomain:          "custom-jobset",
-					},
-					ReplicatedJobs: []ReplicatedJob{
-						{
-							Template: batchv1.JobTemplateSpec{
-								Spec: batchv1.JobSpec{
-									Template:       TestPodTemplate,
-									CompletionMode: completionModePtr(batchv1.IndexedCompletion),
-								},
-							},
-						},
-					},
-				},
-			},
-		},
 		{
 			name: "enableDNSHostnames is false",
 			js: &JobSet{

pkg/controllers/jobset_controller.go

@@ -94,6 +94,14 @@ func (r *JobSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	ctx = ctrl.LoggerInto(ctx, log)
 	log.V(2).Info("Reconciling JobSet")
 
+	// If enableDNSHostnames is set, and subdomain is unset, default the subdomain to be the JobSet name.
+	// This must be done in the controller rather than in the request-time defaulting, since if a JobSet
+	// uses generateName rather than setting the name explicitly, the JobSet name will still be an empty
+	// string at that time.
+	if dnsHostnamesEnabled(&js) && js.Spec.Network.Subdomain == "" {
+		js.Spec.Network.Subdomain = js.Name
+	}
+
 	// Get Jobs owned by JobSet.
 	ownedJobs, err := r.getChildJobs(ctx, &js)
 	if err != nil {
@@ -368,11 +376,6 @@ func (r *JobSetReconciler) createJobs(ctx context.Context, js *jobset.JobSet, ow
 
 	// If pod DNS hostnames are enabled, create a headless service for the JobSet
 	if dnsHostnamesEnabled(js) {
-
-		// Don't try to create a headless service without a subdomain
-		if js.Spec.Network.Subdomain == "" {
-			return fmt.Errorf("a subdomain should be set if dns hostnames are enabled")
-		}
 		if err := r.createHeadlessSvcIfNotExist(ctx, js); err != nil {
 			return err
 		}

pkg/util/testing/wrappers.go

@@ -72,6 +72,14 @@ func (j *JobSetWrapper) SetAnnotations(annotations map[string]string) *JobSetWra
 	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.
+	j.Name = ""
+	j.GenerateName = namePrefix
+	return j
+}
+
 // Obj returns the inner JobSet.
 func (j *JobSetWrapper) Obj() *jobset.JobSet {
 	return &j.JobSet

test/integration/controller/jobset_controller_test.go

@@ -697,6 +697,20 @@ var _ = ginkgo.Describe("JobSet controller", func() {
 				},
 			},
 		}),
+		ginkgo.Entry("jobset using generateName with enableDNSHostnames should have headless service name set to the jobset name", &testCase{
+			makeJobSet: func(ns *corev1.Namespace) *testing.JobSetWrapper {
+				return testJobSet(ns).SetGenerateName("name-prefix").EnableDNSHostnames(true)
+			},
+			updates: []*update{
+				{
+					checkJobSetState: func(js *jobset.JobSet) {
+						gomega.Eventually(func() error {
+							return k8sClient.Get(ctx, types.NamespacedName{Name: js.Name, Namespace: js.Namespace}, &corev1.Service{})
+						}, timeout, interval).Should(gomega.Succeed())
+					},
+				},
+			},
+		}),
 	) // end of DescribeTable
 }) // end of Describe
 
@@ -970,7 +984,6 @@ func testJobSet(ns *corev1.Namespace) *testing.JobSetWrapper {
 	return testing.MakeJobSet(jobSetName, ns.Name).
 		SuccessPolicy(&jobset.SuccessPolicy{Operator: jobset.OperatorAll, TargetReplicatedJobs: []string{}}).
 		EnableDNSHostnames(true).
-		NetworkSubdomain(jobSetName).
 		ReplicatedJob(testing.MakeReplicatedJob("replicated-job-a").
 			Job(testing.MakeJobTemplate("test-job-A", ns.Name).PodSpec(testing.TestPodSpec).Obj()).
 			Replicas(1).