Merge pull request kubernetes-sigs#549 from danielvegamyhre/comment

Add comment explaining why we don't unconditionally compute firstFailedJob

pkg/controllers/jobset_controller.go

@@ -606,6 +606,10 @@ func executeSuccessPolicy(ctx context.Context, js *jobset.JobSet, ownedJobs *chi
 func executeFailurePolicy(ctx context.Context, js *jobset.JobSet, ownedJobs *childJobs, updateStatusOpts *statusUpdateOpts) {
 	// If no failure policy is defined, mark the JobSet as failed.
 	if js.Spec.FailurePolicy == nil {
+		// firstFailedJob is only computed if necessary since it is expensive to compute
+		// for JobSets with many child jobs. This is why we don't unconditionally compute
+		// it once at the beginning of the function and share the results between the different
+		// possible code paths here.
 		firstFailedJob := findFirstFailedJob(ownedJobs.failed)
 		setJobSetFailedCondition(ctx, js, constants.FailedJobsReason, messageWithFirstFailedJob(constants.FailedJobsMessage, firstFailedJob.Name), updateStatusOpts)
 		return

pkg/controllers/jobset_controller_test.go

@@ -38,7 +38,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-
 	jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2"
 	"sigs.k8s.io/jobset/pkg/constants"
 	testutils "sigs.k8s.io/jobset/pkg/util/testing"

pkg/util/testing/wrappers.go

@@ -96,6 +96,12 @@ func (j *JobSetWrapper) SetLabels(labels map[string]string) *JobSetWrapper {
 	return j
 }
 
+// SetConditions sets the value of the jobSet.status conditions.
+func (j *JobSetWrapper) SetConditions(conditions []metav1.Condition) *JobSetWrapper {
+	j.Status.Conditions = conditions
+	return j
+}
+
 // SetGenerateName sets the JobSet name.
 func (j *JobSetWrapper) SetGenerateName(namePrefix string) *JobSetWrapper {
 	// Name and GenerateName are mutually exclusive, so we must unset the Name field.
@@ -353,6 +359,24 @@ func (j *JobWrapper) Ready(ready int32) *JobWrapper {
 	return j
 }
 
+// DeletionTimestamp sets the job.metadata.deletionTimestamp.
+func (j *JobWrapper) DeletionTimestamp(deletionTimestamp *metav1.Time) *JobWrapper {
+	j.ObjectMeta.DeletionTimestamp = deletionTimestamp
+	return j
+}
+
+// StartTimestamp sets the job.status.startTime.
+func (j *JobWrapper) StartTimestamp(startTime *metav1.Time) *JobWrapper {
+	j.Status.StartTime = startTime
+	return j
+}
+
+// ResourceVersion sets the job.metadata.resourceVersion.
+func (j *JobWrapper) ResourceVersion(resourceVersion string) *JobWrapper {
+	j.ObjectMeta.ResourceVersion = resourceVersion
+	return j
+}
+
 // Tolerations set the tolerations.
 func (j *JobWrapper) Tolerations(t []corev1.Toleration) *JobWrapper {
 	j.Spec.Template.Spec.Tolerations = t