From 782e7f8220418a810616c986385bf8661ed18ef6 Mon Sep 17 00:00:00 2001 From: Ankita Thomas Date: Thu, 20 Jun 2024 10:00:07 -0400 Subject: [PATCH] [OCPBUGS-34173]: fix sorting unpack jobs (#3299) * fix sorting unpack jobs Signed-off-by: Ankita Thomas * Add test cases to reproduce panic Signed-off-by: Catherine Chan-Tse * handle nil jobs while sorting unpack jobs Signed-off-by: Ankita Thomas --------- Signed-off-by: Ankita Thomas Signed-off-by: Catherine Chan-Tse Co-authored-by: Catherine Chan-Tse Upstream-repository: operator-lifecycle-manager Upstream-commit: 20a3cbcde1f46ed7f4d56042ccc8b5d962e4d1b8 --- .../pkg/controller/bundle/bundle_unpacker.go | 14 +++++++++- .../controller/bundle/bundle_unpacker_test.go | 27 +++++++++++++++++++ .../pkg/controller/bundle/bundle_unpacker.go | 14 +++++++++- 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go b/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go index 3fee77fa1b..a0768d08d5 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go +++ b/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go @@ -850,6 +850,9 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J // sort jobs so that latest job is first // with preference for non-failed jobs sort.Slice(jobs, func(i, j int) bool { + if jobs[i] == nil || jobs[j] == nil { + return jobs[i] != nil + } condI, failedI := getCondition(jobs[i], batchv1.JobFailed) condJ, failedJ := getCondition(jobs[j], batchv1.JobFailed) if failedI != failedJ { @@ -857,7 +860,16 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J } return condI.LastTransitionTime.After(condJ.LastTransitionTime.Time) }) + if jobs[0] == nil { + // all nil jobs + return + } latest = jobs[0] + nilJobsIndex := len(jobs) - 1 + for ; nilJobsIndex >= 0 && jobs[nilJobsIndex] == nil; nilJobsIndex-- { + } + + jobs = jobs[:nilJobsIndex+1] // exclude nil jobs from list of jobs to delete if len(jobs) <= maxRetainedJobs { return } @@ -867,7 +879,7 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J } // cleanup old failed jobs, n-1 recent jobs and the oldest job - for i := 0; i < maxRetainedJobs && i+maxRetainedJobs < len(jobs); i++ { + for i := 0; i < maxRetainedJobs && i+maxRetainedJobs < len(jobs)-1; i++ { toDelete = append(toDelete, jobs[maxRetainedJobs+i]) } diff --git a/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker_test.go b/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker_test.go index cca9371c3a..8a036dd65c 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker_test.go @@ -1993,6 +1993,15 @@ func TestSortUnpackJobs(t *testing.T) { }, } } + nilConditionJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nc", + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue, bundleUnpackRefLabel: "test"}, + }, + Status: batchv1.JobStatus{ + Conditions: nil, + }, + } failedJobs := []*batchv1.Job{ testJob("f-1", true, 1), testJob("f-2", true, 2), @@ -2024,6 +2033,24 @@ func TestSortUnpackJobs(t *testing.T) { }, { name: "empty job list", maxRetained: 1, + }, { + name: "nil job in list", + maxRetained: 1, + jobs: []*batchv1.Job{ + failedJobs[2], + nil, + failedJobs[1], + }, + expectedLatest: failedJobs[2], + }, { + name: "nil condition", + maxRetained: 3, + jobs: []*batchv1.Job{ + failedJobs[2], + nilConditionJob, + failedJobs[1], + }, + expectedLatest: nilConditionJob, }, { name: "retain oldest", maxRetained: 1, diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go index 3fee77fa1b..a0768d08d5 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go @@ -850,6 +850,9 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J // sort jobs so that latest job is first // with preference for non-failed jobs sort.Slice(jobs, func(i, j int) bool { + if jobs[i] == nil || jobs[j] == nil { + return jobs[i] != nil + } condI, failedI := getCondition(jobs[i], batchv1.JobFailed) condJ, failedJ := getCondition(jobs[j], batchv1.JobFailed) if failedI != failedJ { @@ -857,7 +860,16 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J } return condI.LastTransitionTime.After(condJ.LastTransitionTime.Time) }) + if jobs[0] == nil { + // all nil jobs + return + } latest = jobs[0] + nilJobsIndex := len(jobs) - 1 + for ; nilJobsIndex >= 0 && jobs[nilJobsIndex] == nil; nilJobsIndex-- { + } + + jobs = jobs[:nilJobsIndex+1] // exclude nil jobs from list of jobs to delete if len(jobs) <= maxRetainedJobs { return } @@ -867,7 +879,7 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J } // cleanup old failed jobs, n-1 recent jobs and the oldest job - for i := 0; i < maxRetainedJobs && i+maxRetainedJobs < len(jobs); i++ { + for i := 0; i < maxRetainedJobs && i+maxRetainedJobs < len(jobs)-1; i++ { toDelete = append(toDelete, jobs[maxRetainedJobs+i]) }