refactor: reset metrics during startup (#1246)

count resources during operator start-up and not once is the ToolchainStatus controller

---------

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

Author: xcoulon

cmd/main.go

@@ -424,6 +424,13 @@ func main() { // nolint:gocyclo
 			os.Exit(1)
 		}
 
+		setupLog.Info("Updating the metrics")
+		if err := metrics.Synchronize(ctx, mgr.GetClient(), namespace); err != nil {
+			setupLog.Error(err, "unable to update the metrics")
+			os.Exit(1)
+		}
+		setupLog.Info("Updated the metrics")
+
 		// create or update Toolchain status during the operator deployment
 		setupLog.Info("Creating/updating the ToolchainStatus resource")
 		if err := toolchainstatus.CreateOrUpdateResources(ctx, mgr.GetClient(), namespace, toolchainconfig.ToolchainStatusName); err != nil {

controllers/toolchainstatus/toolchainstatus_controller.go

@@ -67,7 +67,6 @@ const (
 	hostRoutesTag          statusComponentTag = "hostRoutes"
 	hostOperatorTag        statusComponentTag = "hostOperator"
 	memberConnectionsTag   statusComponentTag = "members"
-	metricsTag             statusComponentTag = "metrics"
 	durationAfterUnready   time.Duration      = 10 * time.Minute
 )
 
@@ -174,7 +173,6 @@ func (r *Reconciler) aggregateAndUpdateStatus(ctx context.Context, toolchainStat
 		memberConnectionsTag:   r.membersHandleStatus,
 		registrationServiceTag: r.registrationServiceHandleStatus,
 		hostRoutesTag:          r.hostRoutesHandleStatus,
-		metricsTag:             r.updateMetrics,
 	}
 
 	// track components that are not ready
@@ -259,22 +257,6 @@ func (r *Reconciler) restoredCheck(ctx context.Context, toolchainStatus *toolcha
 	return nil
 }
 
-// synchronizeWithCounter synchronizes the ToolchainStatus with the cached counter
-func (r *Reconciler) updateMetrics(ctx context.Context, _ *toolchainv1alpha1.ToolchainStatus) bool {
-	logger := log.FromContext(ctx)
-	logger.Info("updating the metrics")
-	namespace, err := commonconfig.GetWatchNamespace()
-	if err != nil {
-		logger.Error(err, "unable to get watch namespace")
-		return false
-	}
-	if err := metrics.Synchronize(ctx, r.Client, namespace); err != nil {
-		logger.Error(err, "unable to update the metrics")
-		return false
-	}
-	return true
-}
-
 // hostOperatorHandleStatus retrieves the Deployment for the host operator and adds its status to ToolchainStatus. It returns false
 // if the deployment is not determined to be ready
 func (r *Reconciler) hostOperatorHandleStatus(ctx context.Context, toolchainStatus *toolchainv1alpha1.ToolchainStatus) bool {

pkg/metrics/metrics.go

@@ -134,7 +134,6 @@ func Reset() {
 	allHistograms = nil
 	initMetrics()
 	cachedSpaceCounts.Clear()
-	initialized = false // allow the counters to be initialized again
 	logger.Info("metrics reset")
 }
 
@@ -257,8 +256,6 @@ func GetSpaceCountPerClusterSnapshot() map[string]int {
 	return spaceCounts
 }
 
-var initialized = false
-
 // Synchronize synchronizes the content of the ToolchainStatus with the cached counter
 //
 // If the counter hasn't been initialized yet, then it adds the actual cached count
@@ -270,12 +267,6 @@ var initialized = false
 // If the cached counter is initialized and ToolchainStatus contains already some numbers
 // then it updates the ToolchainStatus numbers with the one taken from the cached counter
 func Synchronize(ctx context.Context, cl runtimeclient.Client, namespace string) error {
-	// skip if cached counters are already initialized
-	if initialized {
-		logger.Info("skipping counters initialization because they are already initialized")
-		return nil
-	}
-
 	logger.Info("initializing counters from resources")
 	usersignups := &toolchainv1alpha1.UserSignupList{}
 	if err := cl.List(ctx, usersignups, runtimeclient.InNamespace(namespace)); err != nil {
@@ -312,7 +303,6 @@ func Synchronize(ctx context.Context, cl runtimeclient.Client, namespace string)
 	for _, space := range spaces.Items {
 		IncrementSpaceCount(space.Spec.TargetCluster)
 	}
-	initialized = true
 	logger.Info("metrics initialized from UserSignups, MasterUserRecords and Spaces", "userSignups", len(usersignups.Items), "murs", len(murs.Items), "spaces", len(spaces.Items))
 	return nil
 }

pkg/metrics/metrics_test.go

@@ -16,7 +16,6 @@ import (
 	"github.com/codeready-toolchain/toolchain-common/pkg/test/space"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test/usersignup"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -146,50 +145,20 @@ func TestInitializeCountersFromExistingResources(t *testing.T) {
 		})
 }
 
-func TestShouldNotInitializeAgain(t *testing.T) {
-	// given
-	metrics.Reset()
-	defer metrics.Reset()
-
-	initObjs := []runtimeclient.Object{}
-	for index := range 10 {
-		initObjs = append(initObjs, usersignup.NewUserSignup(usersignup.WithName(fmt.Sprintf("user-%d", index)), usersignup.WithAnnotation(toolchainv1alpha1.UserSignupActivationCounterAnnotationKey, strconv.Itoa(index+1))))
-		initObjs = append(initObjs, masteruserrecord.NewMasterUserRecord(t, fmt.Sprintf("user-%d", index), masteruserrecord.TargetCluster("member-1")))
-		initObjs = append(initObjs, space.NewSpace(test.HostOperatorNs, fmt.Sprintf("user-%d", index), space.WithSpecTargetCluster("member-1")))
-	}
-	fakeClient := test.NewFakeClient(t, initObjs...)
-	err := metrics.Synchronize(context.TODO(), fakeClient, test.HostOperatorNs)
-	require.NoError(t, err)
-
-	// when
-	err = fakeClient.Create(context.TODO(), masteruserrecord.NewMasterUserRecord(t, "ignored", masteruserrecord.TargetCluster("member-1")))
-	require.NoError(t, err)
-	err = fakeClient.Create(context.TODO(), space.NewSpace(test.HostOperatorNs, "ignored", space.WithSpecTargetCluster("member-1")))
-	require.NoError(t, err)
-	err = metrics.Synchronize(context.TODO(), fakeClient, test.HostOperatorNs)
-
-	// then
-	require.NoError(t, err)
-	metricstest.AssertThatCountersAndMetrics(t).
-		HaveMasterUserRecordsPerDomain(map[string]int{
-			string(metrics.Internal): 10, // same value
-		}).
-		HaveSpacesForCluster("member-1", 10)
-}
-
 func TestMultipleExecutionsInParallel(t *testing.T) {
 	// given
 	initObjs := []runtimeclient.Object{}
 	for index := range 10 {
-		initObjs = append(initObjs, masteruserrecord.NewMasterUserRecord(t, fmt.Sprintf("user-%d", index), masteruserrecord.TargetCluster("member-1")))
+		initObjs = append(initObjs, usersignup.NewUserSignup(usersignup.WithName(fmt.Sprintf("user-%d", index)), usersignup.WithEmail(fmt.Sprintf("user-%d@redhat.com", index))))
+		initObjs = append(initObjs, masteruserrecord.NewMasterUserRecord(t, fmt.Sprintf("user-%d", index), masteruserrecord.Email(fmt.Sprintf("user-%d@redhat.com", index)), masteruserrecord.TargetCluster("member-1")))
 		initObjs = append(initObjs, space.NewSpace(test.HostOperatorNs, fmt.Sprintf("user-%d", index), space.WithSpecTargetCluster("member-1")))
 	}
 	fakeClient := test.NewFakeClient(t, initObjs...)
 	metricstest.ResetCounters(t, fakeClient)
 	latch := new(sync.WaitGroup)
 	latch.Add(1)
 	waitForFinished := new(sync.WaitGroup)
-
+	// run 1002 iterations to increment and decrement counters in parallel
 	for i := range 1002 {
 		waitForFinished.Add(4) // 4 routines to increment counters
 		if i < 1000 {
@@ -241,23 +210,11 @@ func TestMultipleExecutionsInParallel(t *testing.T) {
 		}(i)
 	}
 
-	for range 102 {
-		waitForFinished.Add(1)
-		go func() {
-			defer waitForFinished.Done()
-			latch.Wait()
-			err := metrics.Synchronize(context.TODO(), fakeClient, test.HostOperatorNs)
-			assert.NoError(t, err) // require must only be used in the goroutine running the test function (testifylint)
-		}()
-	}
-
 	// when
 	latch.Done()
 	waitForFinished.Wait()
-	err := metrics.Synchronize(context.TODO(), fakeClient, test.HostOperatorNs)
 
 	// then
-	require.NoError(t, err)
 	metricstest.AssertThatCountersAndMetrics(t).
 		HaveMasterUserRecordsPerDomain(map[string]int{
 			string(metrics.Internal): 12, // all MURs have `@redhat.com` email address