idler second known owner if idling the first one didn't work (codeready-toolchain#689)

Author: MatousJobanek

controllers/idler/idler_controller.go

@@ -130,24 +130,30 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 	return result, r.setStatusReady(ctx, idler)
 }
 
+func getTimeout(idler *toolchainv1alpha1.Idler, pod corev1.Pod) int32 {
+	timeoutSeconds := idler.Spec.TimeoutSeconds
+	if isOwnedByVM(pod.ObjectMeta) {
+		// use 1/12th of the timeout for VMs to have more aggressive idling to decrease
+		// the infra costs because VMs consume much more resources
+		timeoutSeconds = timeoutSeconds / 12
+	}
+	return timeoutSeconds
+}
+
 func (r *Reconciler) ensureIdling(ctx context.Context, idler *toolchainv1alpha1.Idler) (time.Duration, error) {
 	// Get all pods running in the namespace
 	podList := &corev1.PodList{}
 	if err := r.AllNamespacesClient.List(ctx, podList, client.InNamespace(idler.Name)); err != nil {
 		return 0, err
 	}
-	ownerIdler := newOwnerIdler(r.DiscoveryClient, r.DynamicClient, r.ScalesClient, r.RestClient)
+	ownerIdler := newOwnerIdler(idler, r)
 	requeueAfter := time.Duration(idler.Spec.TimeoutSeconds) * time.Second
 	var idleErrors []error
 	for _, pod := range podList.Items {
 		podLogger := log.FromContext(ctx).WithValues("pod_name", pod.Name, "pod_phase", pod.Status.Phase)
 		podCtx := log.IntoContext(ctx, podLogger)
-		timeoutSeconds := idler.Spec.TimeoutSeconds
-		if isOwnedByVM(pod.ObjectMeta) {
-			// use 1/12th of the timeout for VMs to have more aggressive idling to decrease
-			// the infra costs because VMs consume much more resources
-			timeoutSeconds = timeoutSeconds / 12
-		}
+
+		timeoutSeconds := getTimeout(idler, pod)
 		if pod.Status.StartTime != nil {
 			// check the restart count for the pod
 			restartCount := getHighestRestartCount(pod.Status)
@@ -167,6 +173,7 @@ func (r *Reconciler) ensureIdling(ctx context.Context, idler *toolchainv1alpha1.
 				// Check if it belongs to a controller (Deployment, DeploymentConfig, etc) and scale it down to zero.
 				err := r.deletePodsAndCreateNotification(podCtx, pod, idler, ownerIdler)
 				if err == nil {
+					requeueAfter = shorterDuration(requeueAfter, time.Duration(float32(timeoutSeconds)*0.05)*time.Second)
 					continue
 				}
 				idleErrors = append(idleErrors, err)

controllers/idler/idler_controller_test.go

@@ -234,7 +234,8 @@ func TestEnsureIdling(t *testing.T) {
 				HasConditions(memberoperatortest.Running(), memberoperatortest.IdlerNotificationCreated())
 
 			assert.True(t, res.Requeue)
-			assertRequeueTimeInDelta(t, res.RequeueAfter, idler.Spec.TimeoutSeconds/24)
+			// something was idled, expect the next reconcile in 5% of the timeout
+			assertRequeueTimeInDelta(t, res.RequeueAfter, int32(float32(idler.Spec.TimeoutSeconds)*0.05/12))
 
 			t.Run("No pods. requeue after idler timeout.", func(t *testing.T) {
 				//given
@@ -362,7 +363,8 @@ func TestEnsureIdling(t *testing.T) {
 		//then
 		require.NoError(t, err)
 		assert.True(t, res.Requeue)
-		assert.Equal(t, time.Duration(idler.Spec.TimeoutSeconds)*time.Second, res.RequeueAfter)
+		// something was idled, expect the next reconcile in 5% of the timeout
+		assert.Equal(t, time.Duration(int32(float32(idler.Spec.TimeoutSeconds)*0.05/12))*time.Second, res.RequeueAfter)
 		memberoperatortest.AssertThatIdler(t, idler.Name, fakeClients).
 			HasConditions(memberoperatortest.Running(), memberoperatortest.IdlerNotificationCreated())
 		//check the notification is actually created
@@ -384,7 +386,8 @@ func TestEnsureIdling(t *testing.T) {
 			//then
 			require.NoError(t, err)
 			assert.True(t, res.Requeue)
-			assert.Equal(t, time.Duration(idler.Spec.TimeoutSeconds)*time.Second, res.RequeueAfter)
+			// pods (exceeding the timeout) are still running, expect the next reconcile in 5% of the timeout
+			assert.Equal(t, time.Duration(int32(float32(idler.Spec.TimeoutSeconds)*0.05/12))*time.Second, res.RequeueAfter)
 			memberoperatortest.AssertThatIdler(t, idler.Name, fakeClients).
 				HasConditions(memberoperatortest.Running(), memberoperatortest.IdlerNotificationCreated())
 
@@ -606,7 +609,7 @@ func TestNotificationAppNameTypeForPods(t *testing.T) {
 	for pt, tcs := range testpod {
 		t.Run(pt, func(t *testing.T) {
 			reconciler, _, fakeClients := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
-			ownerIdler := newOwnerIdler(reconciler.DiscoveryClient, reconciler.DynamicClient, reconciler.ScalesClient, reconciler.RestClient)
+			ownerIdler := newOwnerIdler(idler, reconciler)
 			pod, appName := tcs.preparePayload(fakeClients)
 
 			// when

controllers/idler/owners.go

@@ -2,8 +2,12 @@ package idler
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"time"
 
+	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -16,55 +20,79 @@ import (
 )
 
 type ownerIdler struct {
+	idler         *toolchainv1alpha1.Idler
 	ownerFetcher  *ownerFetcher
 	dynamicClient dynamic.Interface
 	scalesClient  scale.ScalesGetter
 	restClient    rest.Interface
 }
 
-func newOwnerIdler(discoveryClient discovery.ServerResourcesInterface, dynamicClient dynamic.Interface, scalesClient scale.ScalesGetter, restClient rest.Interface) *ownerIdler {
+func newOwnerIdler(idler *toolchainv1alpha1.Idler, reconciler *Reconciler) *ownerIdler {
 	return &ownerIdler{
-		ownerFetcher:  newOwnerFetcher(discoveryClient, dynamicClient),
-		dynamicClient: dynamicClient,
-		scalesClient:  scalesClient,
-		restClient:    restClient,
+		idler:         idler,
+		ownerFetcher:  newOwnerFetcher(reconciler.DiscoveryClient, reconciler.DynamicClient),
+		dynamicClient: reconciler.DynamicClient,
+		scalesClient:  reconciler.ScalesClient,
+		restClient:    reconciler.RestClient,
 	}
 }
 
-// scaleOwnerToZero fetches the whole tree of the controller owners from the provided object (Deployment, ReplicaSet, etc).
+// scaleOwnerToZero fetches the whole tree of the controller owners from the provided pod.
 // If any known controller owner is found, then it's scaled down (or deleted) and its kind and name is returned.
+// If the pod has been running for longer than 105% of the idler timeout, it will also idle the second known owner.
+// This is a workaround for cases when the top owner controller fails to idle the workload. For example the AAP controller sometimes fails to scale down StatefulSets for postgres pods owned by the top AAP CR. Scaling down the StatefulSet (AAP -> StatefulSet -> Pods) mitigates that AAP controller bug.
 // Otherwise, returns empty strings.
-func (i *ownerIdler) scaleOwnerToZero(ctx context.Context, meta metav1.Object) (kind string, name string, err error) {
+func (i *ownerIdler) scaleOwnerToZero(ctx context.Context, pod *corev1.Pod) (string, string, error) {
 	logger := log.FromContext(ctx)
-	logger.Info("Scaling owner to zero", "name", meta.GetName())
+	logger.Info("Scaling owner to zero")
 
-	owners, err := i.ownerFetcher.getOwners(ctx, meta)
+	owners, err := i.ownerFetcher.getOwners(ctx, pod)
 	if err != nil {
 		logger.Error(err, "failed to find all owners, try to idle the workload with information that is available")
 	}
+
+	var topOwnerKind, topOwnerName string
+	var errToReturn error
 	for _, ownerWithGVR := range owners {
 		owner := ownerWithGVR.object
-		kind = owner.GetObjectKind().GroupVersionKind().Kind
-		name = owner.GetName()
-		switch kind {
+		ownerKind := owner.GetObjectKind().GroupVersionKind().Kind
+
+		switch ownerKind {
 		case "Deployment", "ReplicaSet", "Integration", "KameletBinding", "StatefulSet", "ReplicationController":
 			err = i.scaleToZero(ctx, ownerWithGVR)
-			return
 		case "DaemonSet", "Job":
 			err = i.deleteResource(ctx, ownerWithGVR) // Nothing to scale down. Delete instead.
-			return
 		case "DeploymentConfig":
 			err = i.scaleDeploymentConfigToZero(ctx, ownerWithGVR)
-			return
 		case "VirtualMachine":
 			err = i.stopVirtualMachine(ctx, ownerWithGVR) // Nothing to scale down. Stop instead.
-			return
 		case "AnsibleAutomationPlatform":
 			err = i.idleAAP(ctx, ownerWithGVR) // Nothing to scale down. Stop instead.
-			return
+		default:
+			continue // Skip unknown owner types
+		}
+
+		// Store the first processed owner's info and preserve its error
+		if topOwnerKind == "" {
+			topOwnerKind = ownerKind
+			topOwnerName = owner.GetName()
+			errToReturn = err
+		} else {
+			// if it's the second known owner being processed, then stop the loop
+			errToReturn = errors.Join(errToReturn, err)
+			break
+		}
+
+		// If no error occurred and the pod doesn't run for longer than 105% of the idler timeout, return immediately after the first owner was idled
+		timeoutSeconds := getTimeout(i.idler, *pod)
+		if err == nil && !time.Now().After(pod.Status.StartTime.Add(time.Duration(float64(timeoutSeconds)*1.05)*time.Second)) {
+			return topOwnerKind, topOwnerName, nil
 		}
+		logger.Info("Scaling the first known owner down either failed or the pod has been running for longer than 105% of the idler timeout. Scaling the next known owner.")
 	}
-	return "", "", nil
+
+	// Return the first processed owner's info (or empty if none were processed), and the list of errors (if any happened)
+	return topOwnerKind, topOwnerName, errToReturn
 }
 
 var supportedScaleResources = map[schema.GroupVersionKind]schema.GroupVersionResource{

controllers/idler/owners_test.go

@@ -8,8 +8,9 @@ import (
 	"slices"
 	"strings"
 	"testing"
+	"time"
 
-	"github.com/codeready-toolchain/api/api/v1alpha1"
+	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
 	"github.com/codeready-toolchain/member-operator/pkg/apis"
 	"github.com/codeready-toolchain/member-operator/test"
 	testcommon "github.com/codeready-toolchain/toolchain-common/pkg/test"
@@ -191,7 +192,7 @@ var testConfigs = map[string]createTestConfigFunc{
 }
 
 func TestAppNameTypeForControllers(t *testing.T) {
-	setup := func(t *testing.T, createTestConfig createTestConfigFunc) (*ownerIdler, *test.FakeClientSet, payloadTestConfig, payloads, *corev1.Pod) {
+	setup := func(t *testing.T, createTestConfig createTestConfigFunc, extraExceedTimeout bool) (*ownerIdler, *test.FakeClientSet, payloadTestConfig, payloads, *corev1.Pod) {
 		dynamicClient := fakedynamic.NewSimpleDynamicClient(scheme.Scheme)
 		fakeDiscovery := newFakeDiscoveryClient(allResourcesList(t)...)
 		scalesClient := &fakescale.FakeScaleClient{}
@@ -202,9 +203,35 @@ func TestAppNameTypeForControllers(t *testing.T) {
 			gock.OffAll()
 		})
 
-		ownerIdler := newOwnerIdler(fakeDiscovery, dynamicClient, scalesClient, restClient)
+		timeoutSeconds := int32(3600) // 1 hour
+		idler := &toolchainv1alpha1.Idler{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "test-idler",
+			},
+			Spec: toolchainv1alpha1.IdlerSpec{
+				TimeoutSeconds: timeoutSeconds,
+			},
+		}
+		ownerIdler := &ownerIdler{
+			idler:         idler,
+			ownerFetcher:  newOwnerFetcher(fakeDiscovery, dynamicClient),
+			dynamicClient: dynamicClient,
+			scalesClient:  scalesClient,
+			restClient:    restClient,
+		}
 
-		plds := preparePayloads(t, &test.FakeClientSet{DynamicClient: dynamicClient, AllNamespacesClient: testcommon.NewFakeClient(t)}, "alex-stage", "", freshStartTimes(60))
+		// Calculate start time based on whether timeout should be exceeded
+		timeoutRatio := 1.01
+		if extraExceedTimeout {
+			timeoutRatio = 1.1
+		}
+
+		startTimes := payloadStartTimes{
+			defaultStartTime: time.Now().Add(-time.Duration(float64(timeoutSeconds)*timeoutRatio) * time.Second),
+			vmStartTime:      time.Now().Add(-time.Duration(float64(timeoutSeconds)/12*timeoutRatio) * time.Second),
+		}
+
+		plds := preparePayloads(t, &test.FakeClientSet{DynamicClient: dynamicClient, AllNamespacesClient: testcommon.NewFakeClient(t)}, "alex-stage", "", startTimes)
 		tc := createTestConfig(plds)
 
 		p := plds.getFirstControlledPod(tc.podOwnerName)
@@ -220,7 +247,7 @@ func TestAppNameTypeForControllers(t *testing.T) {
 		for kind, createTestConfig := range testConfigs {
 			t.Run(kind, func(t *testing.T) {
 				//given
-				ownerIdler, fakeClients, testConfig, plds, pod := setup(t, createTestConfig)
+				ownerIdler, fakeClients, testConfig, plds, pod := setup(t, createTestConfig, false)
 
 				//when
 				appType, appName, err := ownerIdler.scaleOwnerToZero(context.TODO(), pod)
@@ -236,6 +263,29 @@ func TestAppNameTypeForControllers(t *testing.T) {
 						othersTCFunc(plds).ownerScaledUp(assertion)
 					}
 				}
+				assertOtherOwners(t, ownerIdler, pod, false)
+			})
+		}
+	})
+
+	t.Run("timeout exceeded - multiple owners processed", func(t *testing.T) {
+		for kind, createTestConfig := range testConfigs {
+			t.Run(kind, func(t *testing.T) {
+				//given - pod running for more than 105% of timeout
+				ownerIdler, fakeClients, testConfig, _, pod := setup(t, createTestConfig, true)
+
+				//when
+				appType, appName, err := ownerIdler.scaleOwnerToZero(context.TODO(), pod)
+
+				//then
+				require.NoError(t, err)
+				require.Equal(t, kind, appType)
+				require.Equal(t, testConfig.expectedAppName, appName)
+				assertion := test.AssertThatInIdleableCluster(t, fakeClients)
+				testConfig.ownerScaledDown(assertion)
+
+				// when there are multiple owners, then verify that the second one is scaled down too
+				assertOtherOwners(t, ownerIdler, pod, true)
 			})
 		}
 	})
@@ -244,7 +294,7 @@ func TestAppNameTypeForControllers(t *testing.T) {
 		for kind, createTestConfig := range testConfigs {
 			t.Run(kind, func(t *testing.T) {
 				//given
-				ownerIdler, fakeClients, testConfig, plds, pod := setup(t, createTestConfig)
+				ownerIdler, fakeClients, testConfig, plds, pod := setup(t, createTestConfig, false)
 				gock.OffAll()
 				// mock stop call
 				mockStopVMCalls(".*", ".*", http.StatusInternalServerError)
@@ -280,13 +330,14 @@ func TestAppNameTypeForControllers(t *testing.T) {
 						othersTCFunc(plds).ownerScaledUp(assertion)
 					}
 				}
+				assertOtherOwners(t, ownerIdler, pod, true)
 			})
 		}
 	})
 
 	t.Run("error when getting owner deployment is ignored", func(t *testing.T) {
 		// given
-		ownerIdler, fakeClients, testConfig, plds, pod := setup(t, testConfigs["Deployment"])
+		ownerIdler, fakeClients, testConfig, plds, pod := setup(t, testConfigs["Deployment"], false)
 		reactionChain := fakeClients.DynamicClient.ReactionChain
 		fakeClients.DynamicClient.PrependReactor("get", "deployments", func(action clienttest.Action) (handled bool, ret runtime.Object, err error) {
 			return true, nil, errors.New("can't get deployment")
@@ -310,6 +361,34 @@ func TestAppNameTypeForControllers(t *testing.T) {
 	})
 }
 
+func assertOtherOwners(t *testing.T, ownerIdler *ownerIdler, pod *corev1.Pod, secondOwnerIdled bool) {
+	owners, err := ownerIdler.ownerFetcher.getOwners(context.TODO(), pod)
+	if !apierrors.IsNotFound(err) {
+		require.NoError(t, err)
+	}
+	// if there are more owners than one
+	if len(owners) > 1 {
+		// by default, all other owners shouldn't be idled
+		notIdledOwnersStartIndex := 1
+		if secondOwnerIdled {
+			// if the second owner is supposed to be idled
+			assertReplicas(t, owners[1].object, 0)
+			// then set the start index for all other owners not idled at 2
+			notIdledOwnersStartIndex = 2
+		}
+		// check that all other owners are not idled
+		for i := notIdledOwnersStartIndex; i < len(owners)-1; i++ {
+			assertReplicas(t, owners[i].object, 3)
+		}
+	}
+}
+
+func assertReplicas(t *testing.T, object *unstructured.Unstructured, expReplicas int64) {
+	replicas, _, err := unstructured.NestedInt64(object.UnstructuredContent(), "spec", "replicas")
+	require.NoError(t, err)
+	require.Equal(t, expReplicas, replicas)
+}
+
 func TestGetAPIResourceList(t *testing.T) {
 	// given
 	pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod", Namespace: "test-namespace"}}
@@ -518,7 +597,7 @@ func TestGetOwnersFailures(t *testing.T) {
 				t.Run("intermediate owner is returned", func(t *testing.T) {
 					// given
 					pod := givenPod.DeepCopy()
-					idler := &v1alpha1.Idler{ObjectMeta: metav1.ObjectMeta{Name: "test-rc", Namespace: "test-namespace"}}
+					idler := &toolchainv1alpha1.Idler{ObjectMeta: metav1.ObjectMeta{Name: "test-rc", Namespace: "test-namespace"}}
 					require.NoError(t, controllerruntime.SetControllerReference(idler, pod, scheme.Scheme))
 					require.NoError(t, controllerruntime.SetControllerReference(ownerObject, idler, scheme.Scheme))
 					// when it's supposed to be "not found" then do not include it in the client