Merge pull request #75772 from tomwans/cronjob-sorting

Avoid panic in cronjob sorting

pkg/controller/cronjob/utils.go

@@ -196,13 +196,14 @@ func (o byJobStartTime) Len() int      { return len(o) }
 func (o byJobStartTime) Swap(i, j int) { o[i], o[j] = o[j], o[i] }
 
 func (o byJobStartTime) Less(i, j int) bool {
-	if o[j].Status.StartTime == nil {
+	if o[i].Status.StartTime == nil && o[j].Status.StartTime != nil {
-		return o[i].Status.StartTime != nil
+		return false
+	}
+	if o[i].Status.StartTime != nil && o[j].Status.StartTime == nil {
+		return true
 	}
-
 	if o[i].Status.StartTime.Equal(o[j].Status.StartTime) {
 		return o[i].Name < o[j].Name
 	}
-
 	return o[i].Status.StartTime.Before(o[j].Status.StartTime)
 }

pkg/controller/cronjob/utils_test.go

@@ -17,6 +17,8 @@ limitations under the License.
 package cronjob
 
 import (
+	"reflect"
+	"sort"
 	"strings"
 	"testing"
 	"time"
@@ -376,3 +378,66 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 		}
 	}
 }
+
+func TestByJobStartTime(t *testing.T) {
+	now := metav1.NewTime(time.Date(2018, time.January, 1, 2, 3, 4, 5, time.UTC))
+	later := metav1.NewTime(time.Date(2019, time.January, 1, 2, 3, 4, 5, time.UTC))
+	aNil := batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{Name: "a"},
+		Status:     batchv1.JobStatus{},
+	}
+	bNil := batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{Name: "b"},
+		Status:     batchv1.JobStatus{},
+	}
+	aSet := batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{Name: "a"},
+		Status:     batchv1.JobStatus{StartTime: &now},
+	}
+	bSet := batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{Name: "b"},
+		Status:     batchv1.JobStatus{StartTime: &now},
+	}
+	aSetLater := batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{Name: "a"},
+		Status:     batchv1.JobStatus{StartTime: &later},
+	}
+
+	testCases := []struct {
+		name            string
+		input, expected []batchv1.Job
+	}{
+		{
+			name:     "both have nil start times",
+			input:    []batchv1.Job{bNil, aNil},
+			expected: []batchv1.Job{aNil, bNil},
+		},
+		{
+			name:     "only the first has a nil start time",
+			input:    []batchv1.Job{aNil, bSet},
+			expected: []batchv1.Job{bSet, aNil},
+		},
+		{
+			name:     "only the second has a nil start time",
+			input:    []batchv1.Job{aSet, bNil},
+			expected: []batchv1.Job{aSet, bNil},
+		},
+		{
+			name:     "both have non-nil, equal start time",
+			input:    []batchv1.Job{bSet, aSet},
+			expected: []batchv1.Job{aSet, bSet},
+		},
+		{
+			name:     "both have non-nil, different start time",
+			input:    []batchv1.Job{aSetLater, bSet},
+			expected: []batchv1.Job{bSet, aSetLater},
+		},
+	}
+
+	for _, testCase := range testCases {
+		sort.Sort(byJobStartTime(testCase.input))
+		if !reflect.DeepEqual(testCase.input, testCase.expected) {
+			t.Errorf("case: '%s', jobs not sorted as expected", testCase.name)
+		}
+	}
+}