idler: delete completed pods (codeready-toolchain#660)

Author: MatousJobanek

controllers/idler/idler_controller.go

@@ -244,18 +244,18 @@ func shorterDuration(first, second time.Duration) time.Duration {
 // Send notification if the deleted pod was managed by a controller, was a standalone pod that was not completed or was crashlooping
 func deletePodsAndCreateNotification(podCtx context.Context, pod corev1.Pod, r *Reconciler, idler *toolchainv1alpha1.Idler) error {
 	logger := log.FromContext(podCtx)
-	var podReason string
-	podCondition := pod.Status.Conditions
-	for _, podCond := range podCondition {
+	isCompleted := false
+	for _, podCond := range pod.Status.Conditions {
 		if podCond.Type == "Ready" {
-			podReason = podCond.Reason
+			isCompleted = podCond.Reason == "PodCompleted"
+			break
 		}
 	}
 	appType, appName, deletedByController, err := r.scaleControllerToZero(podCtx, pod.ObjectMeta)
 	if err != nil {
 		return err
 	}
-	if !deletedByController { // Pod not managed by a controller. We can just delete the pod.
+	if !deletedByController || isCompleted { // Pod not managed by a controller or completed pod. We can just delete the pod.
 		logger.Info("Deleting pod without controller")
 		if err := r.AllNamespacesClient.Delete(podCtx, &pod); err != nil {
 			return err
@@ -267,8 +267,9 @@ func deletePodsAndCreateNotification(podCtx context.Context, pod corev1.Pod, r *
 		appType = "Pod"
 	}
 
-	// If a build pod is in "PodCompleted" status then it was not running so there's no reason to send an idler notification
-	if podReason != "PodCompleted" || deletedByController {
+	// If the pod was in the completed state (it wasn't running) and there was no controller scaled down,
+	// then  there's no reason to send an idler notification
+	if !isCompleted || deletedByController {
 		// By now either a pod has been deleted or scaled to zero by controller, idler Triggered notification should be sent
 		r.notify(podCtx, idler, appName, appType)
 	}

controllers/idler/idler_controller_test.go

@@ -360,8 +360,8 @@ func TestEnsureIdling(t *testing.T) {
 				t.Run("without VM", func(t *testing.T) {
 					// given
 					// delete all VM pods
-					for _, pod := range payloads.controlledPods {
-						if len(pod.OwnerReferences) > 0 && pod.OwnerReferences[0].Name == payloads.virtualmachineinstance.GetName() {
+					for _, pod := range payloads.allPods {
+						if len(pod.OwnerReferences) > 0 && strings.HasPrefix(pod.OwnerReferences[0].Name, payloads.virtualmachineinstance.GetName()) {
 							require.NoError(t, reconciler.AllNamespacesClient.Delete(context.TODO(), pod))
 						}
 					}
@@ -1534,6 +1534,10 @@ func preparePayloadsWithCreateFunc(t *testing.T, clients clientSet, namespace, n
 	require.NoError(t, err)
 	controlledPods = createPods(t, clients.allNamespacesClient, vmi, &vmstartTime, controlledPods, noRestart()) // vmi controls pod
 
+	// create completed pods owned by the VM, they should be deleted if timeout is reached
+	standalonePods := createPodsWithSuffix(t, "-completed", clients.allNamespacesClient, vmi, &vmstartTime,
+		make([]*corev1.Pod, 0, 3), noRestart(), corev1.PodCondition{Type: "Ready", Reason: "PodCompleted"})
+
 	// Standalone ReplicationController
 	standaloneRC := &corev1.ReplicationController{
 		ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("%s%s-replicationcontroller", namePrefix, namespace), Namespace: namespace},
@@ -1553,13 +1557,20 @@ func preparePayloadsWithCreateFunc(t *testing.T, clients clientSet, namespace, n
 		},
 		Spec: toolchainv1alpha1.IdlerSpec{TimeoutSeconds: 30},
 	}
-	standalonePods := createPods(t, clients.allNamespacesClient, idler, sTime, make([]*corev1.Pod, 0, 3), noRestart())
+	standalonePods = slices.Concat(standalonePods, createPods(t, clients.allNamespacesClient, idler, sTime, make([]*corev1.Pod, 0, 3), noRestart()))
 
 	// Pods with no owner.
 	for i := 0; i < 3; i++ {
 		pod := &corev1.Pod{
 			ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("%s%s-pod-%d", namePrefix, namespace, i), Namespace: namespace},
-			Status:     corev1.PodStatus{StartTime: sTime},
+			Status: corev1.PodStatus{
+				StartTime:  sTime,
+				Conditions: []corev1.PodCondition{{Type: "Ready", Reason: "Running"}},
+			},
+		}
+		// one of them is completed
+		if i == 1 {
+			pod.Status.Conditions = []corev1.PodCondition{{Type: "Ready", Reason: "PodCompleted"}}
 		}
 		require.NoError(t, err)
 		standalonePods = append(standalonePods, pod)
@@ -1709,9 +1720,13 @@ func restartingUnderThreshold() []corev1.ContainerStatus {
 }
 
 func createPods(t *testing.T, allNamespacesClient client.Client, owner metav1.Object, startTime *metav1.Time, podsToTrack []*corev1.Pod, restartStatus []corev1.ContainerStatus, conditions ...corev1.PodCondition) []*corev1.Pod {
+	return createPodsWithSuffix(t, "", allNamespacesClient, owner, startTime, podsToTrack, restartStatus, conditions...)
+}
+
+func createPodsWithSuffix(t *testing.T, suffix string, allNamespacesClient client.Client, owner metav1.Object, startTime *metav1.Time, podsToTrack []*corev1.Pod, restartStatus []corev1.ContainerStatus, conditions ...corev1.PodCondition) []*corev1.Pod {
 	for i := 0; i < 3; i++ {
 		pod := &corev1.Pod{
-			ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("%s-pod-%d", owner.GetName(), i), Namespace: owner.GetNamespace()},
+			ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("%s-pod-%d%s", owner.GetName(), i, suffix), Namespace: owner.GetNamespace()},
 			Status:     corev1.PodStatus{StartTime: startTime, Conditions: conditions, ContainerStatuses: restartStatus},
 		}
 		err := controllerutil.SetControllerReference(owner, pod, scheme.Scheme)