Add successfulJobsHistoryLimit to UpgradeConfig spec

Allows cleanup of older successful jobs.
I did not add the same field for failed jobs since they need to be cleaned up manually anyways.
It is not possible by design to delete the most recent successful job.

Author: bastjan

api/v1beta1/upgradeconfig_types.go

@@ -24,6 +24,10 @@ type UpgradeConfigSpec struct {
 	// +kubebuilder:validation:Format=duration
 	// +kubebuilder:default:="1h"
 	MaxUpgradeStartDelay metav1.Duration `json:"maxUpgradeStartDelay"`
+	// SuccessfulJobsHistoryLimit is the number of successful jobs to keep.
+	// A value smaller or equal to zero indicates no limit.
+	// +optional
+	SuccessfulJobsHistoryLimit int `json:"successfulJobsHistoryLimit,omitempty"`
 
 	// JobTemplate defines the template for the upgrade job
 	JobTemplate UpgradeConfigJobTemplate `json:"jobTemplate"`

config/crd/bases/managedupgrade.appuio.io_upgradeconfigs.yaml

@@ -244,6 +244,11 @@ spec:
                 - cron
                 - location
                 type: object
+              successfulJobsHistoryLimit:
+                description: |-
+                  SuccessfulJobsHistoryLimit is the number of successful jobs to keep.
+                  A value smaller or equal to zero indicates no limit.
+                type: integer
             required:
             - jobTemplate
             - maxSchedulingDelay

controllers/upgradeconfig_controller.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -55,6 +56,8 @@ type UpgradeConfigReconciler struct {
 //+kubebuilder:rbac:groups=managedupgrade.appuio.io,resources=upgradesuspensionwindows,verbs=get;list;watch
 //+kubebuilder:rbac:groups=managedupgrade.appuio.io,resources=upgradesuspensionwindows/status,verbs=get
 
+//+kubebuilder:rbac:groups=managedupgrade.appuio.io,resources=upgradejobs,verbs=get;list;watch;create;update;patch;delete
+
 // Reconcile implements the reconcile loop for UpgradeConfig.
 // It schedules UpgradeJobs based on the UpgradeConfig's schedule - if an update is available.
 func (r *UpgradeConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
@@ -86,7 +89,8 @@ func (r *UpgradeConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 
 	jobSchedRequeue, schedErr := r.scheduleJob(ctx, &uc, sched, location)
 	statusRequeue, stErr := r.updateNextPossibleSchedulesStatus(ctx, &uc, sched, location)
-	if err := multierr.Combine(schedErr, stErr); err != nil {
+	cleanupErr := r.cleanupSuccessfulJobs(ctx, &uc)
+	if err := multierr.Combine(schedErr, stErr, cleanupErr); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -195,6 +199,39 @@ findNextRun:
 	return 0, r.setLastScheduledUpgrade(ctx, uc, nextRun)
 }
 
+func (r *UpgradeConfigReconciler) cleanupSuccessfulJobs(ctx context.Context, uc *managedupgradev1beta1.UpgradeConfig) error {
+	l := log.FromContext(ctx).WithName("UpgradeConfigReconciler.cleanupSuccessfulJobs")
+
+	jobs, err := r.getControlledJobs(ctx, uc)
+	if err != nil {
+		return fmt.Errorf("could not get controlled jobs: %w", err)
+	}
+
+	successfulJobs := make([]managedupgradev1beta1.UpgradeJob, 0, len(jobs))
+	for _, job := range jobs {
+		if apimeta.IsStatusConditionTrue(job.Status.Conditions, managedupgradev1beta1.UpgradeJobConditionSucceeded) {
+			successfulJobs = append(successfulJobs, job)
+		}
+	}
+
+	slices.SortFunc(successfulJobs, func(a, b managedupgradev1beta1.UpgradeJob) int {
+		return b.Spec.StartAfter.Time.Compare(a.Spec.StartAfter.Time)
+	})
+
+	var delErrs []error
+	if uc.Spec.SuccessfulJobsHistoryLimit > 0 && len(successfulJobs) > uc.Spec.SuccessfulJobsHistoryLimit {
+		for _, job := range successfulJobs[uc.Spec.SuccessfulJobsHistoryLimit:] {
+			l.Info("deleting successful job", "job", job.Name)
+			delErrs = append(delErrs, r.Delete(ctx, &job))
+		}
+	}
+	if err := multierr.Combine(delErrs...); err != nil {
+		return fmt.Errorf("could not delete jobs: %w", err)
+	}
+
+	return nil
+}
+
 func (r *UpgradeConfigReconciler) setLastScheduledUpgrade(ctx context.Context, uc *managedupgradev1beta1.UpgradeConfig, t time.Time) error {
 	uc.Status.LastScheduledUpgrade = &metav1.Time{Time: t}
 	return r.Status().Update(ctx, uc)

controllers/upgradeconfig_controller_test.go

@@ -10,10 +10,12 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	managedupgradev1beta1 "github.com/appuio/openshift-upgrade-controller/api/v1beta1"
@@ -409,6 +411,115 @@ func requireEventMatches(t *testing.T, recorder *record.FakeRecorder, substrings
 	require.NotEmpty(t, matchingEvent, "no event matches %v, got %v", substrings, events)
 }
 
+func Test_UpgradeConfigReconciler_Reconcile_CleanupSuccessfulJobs(t *testing.T) {
+	ctx := context.Background()
+	clock := mockClock{now: time.Date(2022, time.April, 4, 8, 0, 0, 0, time.UTC)}
+	t.Log("Now: ", clock.Now())
+	require.Equal(t, 14, func() int { _, isoweek := clock.Now().ISOWeek(); return isoweek }())
+	require.Equal(t, time.Monday, clock.Now().Weekday())
+
+	ucv := &configv1.ClusterVersion{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "version",
+		},
+	}
+
+	upgradeConfig := &managedupgradev1beta1.UpgradeConfig{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "daily-maintenance",
+			Namespace:         "appuio-openshift-upgrade-controller",
+			CreationTimestamp: metav1.Time{Time: clock.Now().Add(-time.Hour)},
+		},
+		Spec: managedupgradev1beta1.UpgradeConfigSpec{
+			Schedule: managedupgradev1beta1.UpgradeConfigSchedule{
+				Cron:     "0 22 * * *", // At 22:00 every day
+				Location: "UTC",
+				Suspend:  true,
+			},
+			MaxSchedulingDelay:         metav1.Duration{Duration: time.Minute},
+			SuccessfulJobsHistoryLimit: 2,
+		},
+	}
+
+	client := controllerClient(t, ucv, upgradeConfig)
+
+	jobStartTs := []time.Time{
+		clock.Now().Add(-24 * time.Hour),
+		clock.Now().Add(-24 * time.Hour * 2),
+		clock.Now().Add(-24 * time.Hour * 3),
+	}
+
+	for i, ts := range jobStartTs {
+		job := &managedupgradev1beta1.UpgradeJob{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "job-" + strconv.Itoa(i),
+				Namespace: upgradeConfig.Namespace,
+			},
+			Spec: managedupgradev1beta1.UpgradeJobSpec{
+				StartAfter: metav1.NewTime(ts),
+			},
+		}
+		{
+			successfulOwned := job.DeepCopy()
+			successfulOwned.Name = "successful-owned-" + successfulOwned.Name
+			require.NoError(t, controllerutil.SetControllerReference(upgradeConfig, successfulOwned, client.Scheme()))
+			require.NoError(t, client.Create(ctx, successfulOwned))
+			apimeta.SetStatusCondition(&successfulOwned.Status.Conditions, metav1.Condition{
+				Type:   managedupgradev1beta1.UpgradeJobConditionSucceeded,
+				Status: metav1.ConditionTrue,
+			})
+			client.Status().Update(ctx, successfulOwned)
+		}
+		{
+			notSuccessfulOwned := job.DeepCopy()
+			notSuccessfulOwned.Name = "not-successful-owned-" + notSuccessfulOwned.Name
+			require.NoError(t, controllerutil.SetControllerReference(upgradeConfig, notSuccessfulOwned, client.Scheme()))
+			require.NoError(t, client.Create(ctx, notSuccessfulOwned))
+		}
+		{
+			successfulNotOwned := job.DeepCopy()
+			successfulNotOwned.Name = "successful-not-owned-" + successfulNotOwned.Name
+			require.NoError(t, client.Create(ctx, successfulNotOwned))
+			apimeta.SetStatusCondition(&successfulNotOwned.Status.Conditions, metav1.Condition{
+				Type:   managedupgradev1beta1.UpgradeJobConditionSucceeded,
+				Status: metav1.ConditionTrue,
+			})
+			client.Status().Update(ctx, successfulNotOwned)
+		}
+	}
+
+	recorder := record.NewFakeRecorder(5)
+	subject := &UpgradeConfigReconciler{
+		Client:   client,
+		Scheme:   client.Scheme(),
+		Recorder: recorder,
+
+		Clock: &clock,
+
+		ManagedUpstreamClusterVersionName: "version",
+	}
+
+	_, err := subject.Reconcile(ctx, requestForObject(upgradeConfig))
+	require.NoError(t, err)
+
+	var nj managedupgradev1beta1.UpgradeJobList
+	require.NoError(t, client.List(ctx, &nj))
+	names := []string{}
+	for _, j := range nj.Items {
+		names = append(names, j.Name)
+	}
+	require.ElementsMatch(t, names, []string{
+		"not-successful-owned-job-0",
+		"not-successful-owned-job-1",
+		"not-successful-owned-job-2",
+		"successful-not-owned-job-0",
+		"successful-not-owned-job-1",
+		"successful-not-owned-job-2",
+		"successful-owned-job-0",
+		"successful-owned-job-1",
+	}, "only owned and successful jobs should be cleaned up, higher numbers are older")
+}
+
 // requireTimeEqual asserts that two times are equal, ignoring their timezones.
 func requireTimeEqual(t *testing.T, expected, actual time.Time, msgAndArgs ...any) {
 	t.Helper()