Converts the delete gitjobs to one-time job (#2928) (#2936)

The job also deletes all completed gitjobs, not leaving the last one active, as #2903 was merged. Also fixes the ServiceAccount to be able to list and delete jobs. Signed-off-by: Xavi Garcia <[email protected]>
rancher · Oct 7, 2024 · eb6f3a0 · eb6f3a0
1 parent e75a058
commit eb6f3a0
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 81 deletions.
diff --git a/charts/fleet/templates/job_cleanup_gitrepojobs.yaml b/charts/fleet/templates/job_cleanup_gitrepojobs.yaml
@@ -1,47 +1,41 @@
 {{- if .Values.migrations.gitrepoJobsCleanup }}
 ---
 apiVersion: batch/v1
-kind: CronJob
+kind: Job
 metadata:
   name: fleet-cleanup-gitrepo-jobs
   annotations:
     "helm.sh/hook": post-install, post-upgrade
     "helm.sh/hook-delete-policy": hook-succeeded, before-hook-creation
 spec:
-  schedule: "@daily"
-  concurrencyPolicy: Forbid
-  successfulJobsHistoryLimit: 0
-  failedJobsHistoryLimit: 1
-  jobTemplate:
+  template:
+    metadata:
+      labels:
+        app: fleet-job
     spec:
-      template:
-        metadata:
-          labels:
-            app: fleet-job
-        spec:
-          serviceAccountName: fleet-controller
-          restartPolicy: Never
-          securityContext:
-            runAsNonRoot: true
-            runAsGroup: 1000
-            runAsUser: 1000
-          containers:
-          - name: cleanup
-            image: "{{ template "system_default_registry" . }}{{.Values.image.repository}}:{{.Values.image.tag}}"
-            imagePullPolicy: {{ .Values.global.imagePullPolicy }}
-            securityContext:
-              allowPrivilegeEscalation: false
-              capabilities:
-                drop:
-                - ALL
-              readOnlyRootFilesystem: false
-              privileged: false
-            command:
-            - fleet
-            args:
-            - cleanup
-            - gitjob
-          nodeSelector: {{ include "linux-node-selector" . | nindent 12 }}
-          tolerations: {{ include "linux-node-tolerations" . | nindent 12 }}
-      backoffLimit: 1
+      serviceAccountName: gitjob
+      restartPolicy: Never
+      securityContext:
+        runAsNonRoot: true
+        runAsGroup: 1000
+        runAsUser: 1000
+      containers:
+      - name: cleanup
+        image: "{{ template "system_default_registry" . }}{{.Values.image.repository}}:{{.Values.image.tag}}"
+        imagePullPolicy: {{ .Values.global.imagePullPolicy }}
+        securityContext:
+          allowPrivilegeEscalation: false
+          capabilities:
+            drop:
+            - ALL
+          readOnlyRootFilesystem: false
+          privileged: false
+        command:
+        - fleet
+        args:
+        - cleanup
+        - gitjob
+      nodeSelector: {{ include "linux-node-selector" . | nindent 8 }}
+      tolerations: {{ include "linux-node-tolerations" . | nindent 8 }}
+  backoffLimit: 1
 {{- end }}
diff --git a/charts/fleet/templates/rbac.yaml b/charts/fleet/templates/rbac.yaml
@@ -38,7 +38,6 @@ rules:
     - 'events'
   verbs:
     - '*'
-
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding

diff --git a/integrationtests/cli/cleanup/cleanup_jobs_test.go b/integrationtests/cli/cleanup/cleanup_jobs_test.go
@@ -68,6 +68,13 @@ var _ = Describe("Fleet CLI jobs cleanup", Ordered, func() {
 				UID:        "1",
 			}
 
+			owner4 := metav1.OwnerReference{
+				APIVersion: "something",
+				Kind:       "somekind",
+				Name:       "somename",
+				UID:        "1",
+			}
+
 			spec := batchv1.JobSpec{
 				Template: corev1.PodTemplateSpec{
 					Spec: corev1.PodSpec{
@@ -165,10 +172,19 @@ var _ = Describe("Fleet CLI jobs cleanup", Ordered, func() {
 					Spec:   spec,
 					Status: succeeded(time.Now().Add(-1 * time.Hour)),
 				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "some-other-job",
+						Namespace:       namespace,
+						OwnerReferences: []metav1.OwnerReference{owner4},
+					},
+					Spec:   spec,
+					Status: succeeded(time.Now()),
+				},
 			}
 		})
 
-		It("deletes all resources and leaves most recent ones", func() {
+		It("deletes all resources that have the right owner and succeeded", func() {
 			Expect(act()).NotTo(HaveOccurred())
 
 			Eventually(func(g Gomega) {
@@ -182,8 +198,7 @@ var _ = Describe("Fleet CLI jobs cleanup", Ordered, func() {
 				}
 				g.Expect(names).To(ConsistOf(
 					namespace+"/job-running",
-					namespace+"/another-job",
-					otherns+"/job-1",
+					namespace+"/some-other-job",
 				))
 			}, 20*time.Second, 1*time.Second).Should(Succeed())
 		})

diff --git a/internal/cmd/cli/cleanup/cleanup.go b/internal/cmd/cli/cleanup/cleanup.go
@@ -2,7 +2,6 @@ package cleanup
 
 import (
 	"context"
-	"sort"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -164,57 +163,24 @@ func GitJobs(ctx context.Context, cl client.Client, bs int) error {
 }
 
 func cleanupGitJobs(ctx context.Context, logger logr.Logger, cl client.Client, jobs []batchv1.Job) error {
-	// jobs by namespace, gitrepo
-	gitjobs := map[string]map[string][]batchv1.Job{}
-	// gitrepos with running jobs
-	running := map[string]map[string]struct{}{}
-
 	for _, job := range jobs {
 		if job.OwnerReferences == nil {
 			continue
 		}
 		for _, or := range job.OwnerReferences {
-			if or.Kind == "GitRepo" && or.APIVersion == "fleet.cattle.io/v1alpha1" {
-				if job.Status.Succeeded != 1 || job.Status.CompletionTime == nil {
-					if running[job.Namespace] == nil {
-						running[job.Namespace] = map[string]struct{}{}
-					}
-					running[job.Namespace][or.Name] = struct{}{}
-				} else {
-					if gitjobs[job.Namespace] == nil {
-						gitjobs[job.Namespace] = map[string][]batchv1.Job{}
-					}
-					gitjobs[job.Namespace][or.Name] = append(gitjobs[job.Namespace][or.Name], job)
-				}
-				break
+			if or.Kind != "GitRepo" || or.APIVersion != "fleet.cattle.io/v1alpha1" {
+				continue
 			}
-		}
-	}
-
-	for ns, gitrepos := range gitjobs {
-		for gitrepo, jobs := range gitrepos {
-			sort.Slice(jobs, func(i, j int) bool {
-				return jobs[j].Status.CompletionTime.Before(jobs[i].Status.CompletionTime)
-			})
-
-			// if there is a running job delete all the jobs in the
-			// list, otherwise all but the newest
-			start := 1
-			if _, ok := running[ns][gitrepo]; ok {
-				start = 0
-			}
-
-			logger.V(1).Info("Deleting jobs for gitrepo", "n", len(jobs)-start, "namespace", ns, "gitrepo", gitrepo)
-
-			for i := start; i < len(jobs); i++ {
-				job := jobs[i]
-				logger.V(1).Info("Deleting job", "namespace", ns, "name", job.Name, "gitrepo", gitrepo)
+			if job.Status.Succeeded == 1 && job.Status.CompletionTime != nil {
+				logger.V(1).Info("Deleting job", "namespace", job.Namespace, "name", job.Name, "gitrepo", or.Name)
 				err := cl.Delete(ctx, &job, client.PropagationPolicy(metav1.DeletePropagationBackground))
 				if err != nil && !apierrors.IsNotFound(err) {
-					logger.Error(err, "Failed to delete job", "namespace", ns, "name", job.Name)
+					logger.Error(err, "Failed to delete job", "namespace", job.Namespace, "name", job.Name)
 				}
 			}
+			break
 		}
 	}
+
 	return nil
 }