From c0281a15192636660a3b1cd7f61f8877d81751e7 Mon Sep 17 00:00:00 2001 From: Amritanshu Sikdar Date: Thu, 16 Jan 2025 10:15:50 +0100 Subject: [PATCH 1/5] remove module-metrics usages from codebase --- cmd/main.go | 14 ++++----- internal/controller/kyma/controller.go | 4 +-- internal/controller/manifest/controller.go | 4 +-- internal/controller/manifest/setup.go | 5 ++-- internal/declarative/v2/reconciler.go | 33 +--------------------- pkg/module/sync/runner.go | 9 ++---- 6 files changed, 14 insertions(+), 55 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index d33f49df17..89f56799e6 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -191,7 +191,6 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma descriptorProvider := provider.NewCachedDescriptorProvider() kymaMetrics := metrics.NewKymaMetrics(sharedMetrics) mandatoryModulesMetrics := metrics.NewMandatoryModulesMetrics() - moduleMetrics := metrics.NewModuleMetrics() // The maintenance windows policy should be passed to the reconciler to be resolved: https://github.com/kyma-project/lifecycle-manager/issues/2101 _, err = maintenancewindows.InitializeMaintenanceWindowsPolicy(setupLog, maintenanceWindowPoliciesDirectory, @@ -200,9 +199,8 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma setupLog.Error(err, "unable to set maintenance windows policy") } setupKymaReconciler(mgr, descriptorProvider, skrContextProvider, eventRecorder, flagVar, options, skrWebhookManager, - kymaMetrics, moduleMetrics, setupLog) - setupManifestReconciler(mgr, flagVar, options, sharedMetrics, mandatoryModulesMetrics, moduleMetrics, setupLog, - eventRecorder) + kymaMetrics, setupLog) + setupManifestReconciler(mgr, flagVar, options, sharedMetrics, mandatoryModulesMetrics, setupLog, eventRecorder) setupMandatoryModuleReconciler(mgr, descriptorProvider, flagVar, options, mandatoryModulesMetrics, setupLog) setupMandatoryModuleDeletionReconciler(mgr, descriptorProvider, eventRecorder, flagVar, options, setupLog) if flagVar.EnablePurgeFinalizer { @@ -279,8 +277,7 @@ func scheduleMetricsCleanup(kymaMetrics *metrics.KymaMetrics, cleanupIntervalInM func setupKymaReconciler(mgr ctrl.Manager, descriptorProvider *provider.CachedDescriptorProvider, skrContextFactory remote.SkrContextProvider, event event.Event, flagVar *flags.FlagVar, options ctrlruntime.Options, - skrWebhookManager *watcher.SKRWebhookManifestManager, kymaMetrics *metrics.KymaMetrics, - moduleMetrics *metrics.ModuleMetrics, setupLog logr.Logger, + skrWebhookManager *watcher.SKRWebhookManifestManager, kymaMetrics *metrics.KymaMetrics, setupLog logr.Logger, ) { options.RateLimiter = internal.RateLimiter(flagVar.FailureBaseDelay, flagVar.FailureMaxDelay, flagVar.RateLimiterFrequency, flagVar.RateLimiterBurst) @@ -304,7 +301,6 @@ func setupKymaReconciler(mgr ctrl.Manager, descriptorProvider *provider.CachedDe RemoteSyncNamespace: flagVar.RemoteSyncNamespace, IsManagedKyma: flagVar.IsKymaManaged, Metrics: kymaMetrics, - ModuleMetrics: moduleMetrics, RemoteCatalog: remote.NewRemoteCatalogFromKyma(mgr.GetClient(), skrContextFactory, flagVar.RemoteSyncNamespace), }).SetupWithManager( @@ -385,7 +381,7 @@ func setupPurgeReconciler(mgr ctrl.Manager, func setupManifestReconciler(mgr ctrl.Manager, flagVar *flags.FlagVar, options ctrlruntime.Options, sharedMetrics *metrics.SharedMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, - moduleMetrics *metrics.ModuleMetrics, setupLog logr.Logger, event event.Event, + setupLog logr.Logger, event event.Event, ) { options.RateLimiter = internal.RateLimiter(flagVar.FailureBaseDelay, flagVar.FailureMaxDelay, flagVar.RateLimiterFrequency, flagVar.RateLimiterBurst) @@ -405,7 +401,7 @@ func setupManifestReconciler(mgr ctrl.Manager, flagVar *flags.FlagVar, options c }, manifest.SetupOptions{ ListenerAddr: flagVar.ManifestListenerAddr, EnableDomainNameVerification: flagVar.EnableDomainNameVerification, - }, metrics.NewManifestMetrics(sharedMetrics), mandatoryModulesMetrics, moduleMetrics, + }, metrics.NewManifestMetrics(sharedMetrics), mandatoryModulesMetrics, manifestClient, ); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Manifest") diff --git a/internal/controller/kyma/controller.go b/internal/controller/kyma/controller.go index 474993bc4c..c81ae9d7b9 100644 --- a/internal/controller/kyma/controller.go +++ b/internal/controller/kyma/controller.go @@ -71,7 +71,6 @@ type Reconciler struct { RemoteSyncNamespace string IsManagedKyma bool Metrics *metrics.KymaMetrics - ModuleMetrics *metrics.ModuleMetrics RemoteCatalog *remote.RemoteCatalog } @@ -446,7 +445,6 @@ func (r *Reconciler) handleDeletingState(ctx context.Context, kyma *v1beta2.Kyma func (r *Reconciler) cleanupMetrics(kymaName string) { r.Metrics.CleanupMetrics(kymaName) - r.ModuleMetrics.CleanupMetrics(kymaName) } func (r *Reconciler) cleanupManifestCRs(ctx context.Context, kyma *v1beta2.Kyma) error { @@ -514,7 +512,7 @@ func (r *Reconciler) reconcileManifests(ctx context.Context, kyma *v1beta2.Kyma) if err := runner.ReconcileManifests(ctx, kyma, modules); err != nil { return fmt.Errorf("sync failed: %w", err) } - runner.SyncModuleStatus(ctx, kyma, modules, r.Metrics, r.ModuleMetrics) + runner.SyncModuleStatus(ctx, kyma, modules, r.Metrics) // If module get removed from kyma, the module deletion happens here. if err := r.DeleteNoLongerExistingModules(ctx, kyma); err != nil { return fmt.Errorf("error while syncing conditions during deleting non exists modules: %w", err) diff --git a/internal/controller/manifest/controller.go b/internal/controller/manifest/controller.go index 2773cd9063..afa47768ed 100644 --- a/internal/controller/manifest/controller.go +++ b/internal/controller/manifest/controller.go @@ -17,7 +17,7 @@ import ( func NewReconciler(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, manifestMetrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, - moduleMetrics *metrics.ModuleMetrics, manifestClient declarativev2.ManifestAPIClient, + manifestClient declarativev2.ManifestAPIClient, ) *declarativev2.Reconciler { kcp := &declarativev2.ClusterInfo{ Client: mgr.GetClient(), @@ -29,7 +29,7 @@ func NewReconciler(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, statefulChecker := statecheck.NewStatefulSetStateCheck() deploymentChecker := statecheck.NewDeploymentStateCheck() return declarativev2.NewFromManager( - mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, moduleMetrics, manifestClient, + mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, manifestClient, manifest.NewSpecResolver(keyChainLookup, extractor), declarativev2.WithCustomStateCheck(statecheck.NewManagerStateCheck(statefulChecker, deploymentChecker)), declarativev2.WithRemoteTargetCluster(lookup.ConfigResolver), diff --git a/internal/controller/manifest/setup.go b/internal/controller/manifest/setup.go index 5bff6d6cd4..242c94c766 100644 --- a/internal/controller/manifest/setup.go +++ b/internal/controller/manifest/setup.go @@ -37,8 +37,7 @@ type SetupOptions struct { func SetupWithManager(mgr manager.Manager, opts ctrlruntime.Options, requeueIntervals queue.RequeueIntervals, settings SetupOptions, manifestMetrics *metrics.ManifestMetrics, - mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, moduleMetrics *metrics.ModuleMetrics, - manifestClient declarativev2.ManifestAPIClient, + mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, manifestClient declarativev2.ManifestAPIClient, ) error { var verifyFunc watcherevent.Verify if settings.EnableDomainNameVerification { @@ -83,7 +82,7 @@ func SetupWithManager(mgr manager.Manager, opts ctrlruntime.Options, requeueInte predicate.LabelChangedPredicate{}))). WatchesRawSource(skrEventChannel). WithOptions(opts). - Complete(NewReconciler(mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, moduleMetrics, + Complete(NewReconciler(mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, manifestClient)); err != nil { return fmt.Errorf("failed to setup manager for manifest controller: %w", err) } diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index aa771d8149..943dc647fc 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -43,13 +43,12 @@ const ( ) func NewFromManager(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, metrics *metrics.ManifestMetrics, - mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, moduleMetrics *metrics.ModuleMetrics, + mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, manifestAPIClient ManifestAPIClient, specResolver SpecResolver, options ...Option, ) *Reconciler { reconciler := &Reconciler{} reconciler.ManifestMetrics = metrics reconciler.MandatoryModuleMetrics = mandatoryModulesMetrics - reconciler.ModuleMetrics = moduleMetrics reconciler.RequeueIntervals = requeueIntervals reconciler.specResolver = specResolver reconciler.manifestClient = manifestAPIClient @@ -76,7 +75,6 @@ type Reconciler struct { *Options ManifestMetrics *metrics.ManifestMetrics MandatoryModuleMetrics *metrics.MandatoryModulesMetrics - ModuleMetrics *metrics.ModuleMetrics specResolver SpecResolver manifestClient ManifestAPIClient managedLabelRemovalService ManagedLabelRemoval @@ -330,9 +328,6 @@ func (r *Reconciler) syncManifestState(ctx context.Context, skrClient Client, ma } if !manifest.GetDeletionTimestamp().IsZero() { if moduleCRState == shared.StateWarning { - if err := r.RecordModuleCRWarningCondition(manifest); err != nil { - return err - } status.ConfirmModuleCRCondition(manifest) } if status.RequireManifestStateUpdateAfterSyncResource(manifest, shared.StateDeleting) { @@ -352,32 +347,6 @@ func (r *Reconciler) syncManifestState(ctx context.Context, skrClient Client, ma return nil } -func (r *Reconciler) RecordModuleCRWarningCondition(manifest *v1beta2.Manifest) error { - kymaName, err := manifest.GetKymaName() - if err != nil { - return fmt.Errorf("failed to get kyma name: %w", err) - } - moduleName, err := manifest.GetModuleName() - if err != nil { - return fmt.Errorf("failed to get module name: %w", err) - } - r.ModuleMetrics.SetModuleCRWarningCondition(kymaName, moduleName) - return nil -} - -func (r *Reconciler) RemoveModuleCRWarningCondition(manifest *v1beta2.Manifest) error { - kymaName, err := manifest.GetKymaName() - if err != nil { - return fmt.Errorf("failed to get kyma name: %w", err) - } - moduleName, err := manifest.GetModuleName() - if err != nil { - return fmt.Errorf("failed to get module name: %w", err) - } - r.ModuleMetrics.RemoveModuleCRWarningCondition(kymaName, moduleName) - return nil -} - func (r *Reconciler) checkManagerState(ctx context.Context, clnt Client, target []*resource.Info) (shared.State, error, ) { diff --git a/pkg/module/sync/runner.go b/pkg/module/sync/runner.go index d21829152a..db2106f900 100644 --- a/pkg/module/sync/runner.go +++ b/pkg/module/sync/runner.go @@ -244,11 +244,10 @@ func (r *Runner) setupModule(module *common.Module, kyma *v1beta2.Kyma) error { } func (r *Runner) SyncModuleStatus(ctx context.Context, kyma *v1beta2.Kyma, modules common.Modules, - kymaMetrics *metrics.KymaMetrics, moduleMetrics *metrics.ModuleMetrics, + kymaMetrics *metrics.KymaMetrics, ) { updateModuleStatusFromExistingModules(kyma, modules) - DeleteNoLongerExistingModuleStatus(ctx, kyma, r.getModule, kymaMetrics.RemoveModuleStateMetrics, - moduleMetrics.RemoveModuleCRWarningCondition) + DeleteNoLongerExistingModuleStatus(ctx, kyma, r.getModule, kymaMetrics.RemoveModuleStateMetrics) } func updateModuleStatusFromExistingModules( @@ -363,7 +362,7 @@ func stateFromManifest(obj client.Object) shared.State { } func DeleteNoLongerExistingModuleStatus(ctx context.Context, kyma *v1beta2.Kyma, moduleFunc GetModuleFunc, - kymaMetricsRemoveMetrics, moduleMetricsRemoveMetrics RemoveMetricsFunc, + kymaMetricsRemoveMetrics RemoveMetricsFunc, ) { moduleStatusMap := kyma.GetModuleStatusMap() moduleStatusesToBeDeletedFromKymaStatus := kyma.GetNoLongerExistingModuleStatus() @@ -371,7 +370,6 @@ func DeleteNoLongerExistingModuleStatus(ctx context.Context, kyma *v1beta2.Kyma, moduleStatus := moduleStatusesToBeDeletedFromKymaStatus[idx] if moduleStatus.Manifest == nil { kymaMetricsRemoveMetrics(kyma.Name, moduleStatus.Name) - moduleMetricsRemoveMetrics(kyma.Name, moduleStatus.Name) delete(moduleStatusMap, moduleStatus.Name) continue } @@ -379,7 +377,6 @@ func DeleteNoLongerExistingModuleStatus(ctx context.Context, kyma *v1beta2.Kyma, err := moduleFunc(ctx, manifestCR) if util.IsNotFound(err) { kymaMetricsRemoveMetrics(kyma.Name, moduleStatus.Name) - moduleMetricsRemoveMetrics(kyma.Name, moduleStatus.Name) delete(moduleStatusMap, moduleStatus.Name) } else { moduleStatus.State = stateFromManifest(manifestCR) From 6f2440732352ab9721463ab3fdc2279251a86390 Mon Sep 17 00:00:00 2001 From: Amritanshu Sikdar Date: Thu, 16 Jan 2025 10:35:53 +0100 Subject: [PATCH 2/5] remove condition and related code --- internal/declarative/v2/reconciler.go | 6 +----- internal/manifest/modulecr/client.go | 12 +++++------- internal/manifest/status/condition.go | 21 --------------------- internal/manifest/status/init.go | 1 - tests/e2e/module_status_decoupling_test.go | 10 ---------- 5 files changed, 6 insertions(+), 44 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 943dc647fc..9646d3103b 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -317,8 +317,7 @@ func (r *Reconciler) syncManifestState(ctx context.Context, skrClient Client, ma ) error { manifestStatus := manifest.GetStatus() - moduleCRState, err := modulecr.NewClient(skrClient).SyncModuleCR(ctx, manifest) - if err != nil { + if err := modulecr.NewClient(skrClient).SyncModuleCR(ctx, manifest); err != nil { manifest.SetStatus(manifestStatus.WithState(shared.StateError).WithErr(err)) return err } @@ -327,9 +326,6 @@ func (r *Reconciler) syncManifestState(ctx context.Context, skrClient Client, ma return err } if !manifest.GetDeletionTimestamp().IsZero() { - if moduleCRState == shared.StateWarning { - status.ConfirmModuleCRCondition(manifest) - } if status.RequireManifestStateUpdateAfterSyncResource(manifest, shared.StateDeleting) { return errStateRequireUpdate } diff --git a/internal/manifest/modulecr/client.go b/internal/manifest/modulecr/client.go index 7e2a8b5b54..2d80a8a6a0 100644 --- a/internal/manifest/modulecr/client.go +++ b/internal/manifest/modulecr/client.go @@ -105,9 +105,9 @@ func (c *Client) deleteCR(ctx context.Context, manifest *v1beta2.Manifest) (bool // SyncModuleCR sync the manifest default custom resource status in the cluster, if not available it created the resource. // It is used to provide the controller with default data in the Runtime. -func (c *Client) SyncModuleCR(ctx context.Context, manifest *v1beta2.Manifest) (shared.State, error) { +func (c *Client) SyncModuleCR(ctx context.Context, manifest *v1beta2.Manifest) error { if manifest.Spec.Resource == nil { - return "", nil + return nil } resource := manifest.Spec.Resource.DeepCopy() @@ -117,14 +117,12 @@ func (c *Client) SyncModuleCR(ctx context.Context, manifest *v1beta2.Manifest) ( if err := c.Get(ctx, client.ObjectKeyFromObject(resource), resource); err != nil && util.IsNotFound(err) { if !manifest.GetDeletionTimestamp().IsZero() { - return "", nil + return nil } if err := c.Create(ctx, resource, client.FieldOwner(finalizer.CustomResourceManagerFinalizer)); err != nil && !apierrors.IsAlreadyExists(err) { - return "", fmt.Errorf("failed to create resource: %w", err) + return fmt.Errorf("failed to create resource: %w", err) } } - - stateFromCR, _, err := unstructured.NestedString(resource.Object, "status", "state") - return shared.State(stateFromCR), err + return nil } diff --git a/internal/manifest/status/condition.go b/internal/manifest/status/condition.go index 06a20172df..c8d99f163a 100644 --- a/internal/manifest/status/condition.go +++ b/internal/manifest/status/condition.go @@ -43,27 +43,6 @@ func initResourcesCondition(manifest *v1beta2.Manifest) apimetav1.Condition { } } -func initModuleCRCondition(manifest *v1beta2.Manifest) apimetav1.Condition { - return apimetav1.Condition{ - Type: string(ConditionTypeModuleCR), - Reason: string(ConditionReasonModuleCRWarning), - Status: apimetav1.ConditionFalse, - Message: "Module CR is in Warning state", - ObservedGeneration: manifest.GetGeneration(), - } -} - -func ConfirmModuleCRCondition(manifest *v1beta2.Manifest) { - status := manifest.GetStatus() - moduleCRCondition := initModuleCRCondition(manifest) - - if !meta.IsStatusConditionTrue(status.Conditions, moduleCRCondition.Type) { - moduleCRCondition.Status = apimetav1.ConditionTrue - meta.SetStatusCondition(&status.Conditions, moduleCRCondition) - manifest.SetStatus(status.WithOperation(moduleCRCondition.Message)) - } -} - func ConfirmResourcesCondition(manifest *v1beta2.Manifest) { status := manifest.GetStatus() resourceCondition := initResourcesCondition(manifest) diff --git a/internal/manifest/status/init.go b/internal/manifest/status/init.go index 54756b94e6..999d67ec62 100644 --- a/internal/manifest/status/init.go +++ b/internal/manifest/status/init.go @@ -18,7 +18,6 @@ func Initialize(manifest *v1beta2.Manifest) error { for _, condition := range []apimetav1.Condition{ initResourcesCondition(manifest), initInstallationCondition(manifest), - initModuleCRCondition(manifest), } { if meta.FindStatusCondition(status.Conditions, condition.Type) == nil { meta.SetStatusCondition(&status.Conditions, condition) diff --git a/tests/e2e/module_status_decoupling_test.go b/tests/e2e/module_status_decoupling_test.go index ca43643628..b0514ff5ec 100644 --- a/tests/e2e/module_status_decoupling_test.go +++ b/tests/e2e/module_status_decoupling_test.go @@ -3,13 +3,10 @@ package e2e_test import ( "context" - apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/manifest/status" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -96,13 +93,6 @@ func RunModuleStatusDecouplingTest(resourceKind ResourceKind) { WithContext(ctx). WithArguments(skrClient, moduleCR, shared.StateWarning). Should(BeTrue()) - By("And Manifest contains Module CR is in Warning state") - Eventually(ConditionExists). - WithContext(ctx). - WithArguments(kcpClient, kyma.GetName(), kyma.GetNamespace(), module.Name, - string(status.ConditionTypeModuleCR), string(status.ConditionReasonModuleCRWarning), - apimetav1.ConditionTrue). - Should(Succeed()) By("And count of metrics lifecycle_mgr_module_condition is 1") Eventually(GetModuleCRWarningConditionMetric). WithContext(ctx). From e3fed5fa47dcb4067ad957fa3d90e7d2a713ec60 Mon Sep 17 00:00:00 2001 From: Amritanshu Sikdar Date: Fri, 17 Jan 2025 10:41:22 +0100 Subject: [PATCH 3/5] adjust tests --- internal/declarative/v2/reconciler.go | 4 ++-- internal/manifest/modulecr/client_test.go | 4 ++-- pkg/module/sync/runner_test.go | 16 ++-------------- .../controller/eventfilters/suite_test.go | 1 - tests/integration/controller/kcp/suite_test.go | 1 - tests/integration/controller/kyma/suite_test.go | 1 - .../manifest/custom_resource_check/suite_test.go | 2 +- .../controller/manifest/suite_test.go | 2 +- .../controller/withwatcher/suite_test.go | 1 - 9 files changed, 8 insertions(+), 24 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 9646d3103b..eb08a33845 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -43,8 +43,8 @@ const ( ) func NewFromManager(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, metrics *metrics.ManifestMetrics, - mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, - manifestAPIClient ManifestAPIClient, specResolver SpecResolver, options ...Option, + mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, manifestAPIClient ManifestAPIClient, + specResolver SpecResolver, options ...Option, ) *Reconciler { reconciler := &Reconciler{} reconciler.ManifestMetrics = metrics diff --git a/internal/manifest/modulecr/client_test.go b/internal/manifest/modulecr/client_test.go index c0ec9db62f..ca125a809c 100644 --- a/internal/manifest/modulecr/client_test.go +++ b/internal/manifest/modulecr/client_test.go @@ -100,7 +100,7 @@ func TestClient_SyncModuleCR(t *testing.T) { manifest.Spec.Resource = &moduleCR // When syncing the module CR - _, err = skrClient.SyncModuleCR(ctx, manifest) + err = skrClient.SyncModuleCR(ctx, manifest) require.NoError(t, err) // Then the resource CR should be created @@ -118,7 +118,7 @@ func TestClient_SyncModuleCR(t *testing.T) { require.NoError(t, err) // And syncing again, it should recreate the resource - _, err = skrClient.SyncModuleCR(ctx, manifest) + err = skrClient.SyncModuleCR(ctx, manifest) require.NoError(t, err) err = skrClient.Get(ctx, client.ObjectKey{Name: moduleName, Namespace: shared.DefaultRemoteNamespace}, resource) diff --git a/pkg/module/sync/runner_test.go b/pkg/module/sync/runner_test.go index e4ff878893..21345c086a 100644 --- a/pkg/module/sync/runner_test.go +++ b/pkg/module/sync/runner_test.go @@ -42,14 +42,6 @@ func (m *KymaMockMetrics) RemoveModuleStateMetrics(kymaName, moduleName string) m.callCount++ } -type ModuleMockMetrics struct { - callCount int -} - -func (m *ModuleMockMetrics) RemoveModuleStateMetrics(kymaName, moduleName string) { - m.callCount++ -} - func TestMetricsOnDeleteNoLongerExistingModuleStatus(t *testing.T) { t.Parallel() tests := []struct { @@ -90,15 +82,12 @@ func TestMetricsOnDeleteNoLongerExistingModuleStatus(t *testing.T) { kyma := testutils.NewTestKyma("test-kyma") configureModuleInKyma(kyma, []string{ModuleShouldKeep}, []string{testCase.ModuleInStatus}) kymaMetrics := &KymaMockMetrics{} - moduleMetrics := &ModuleMockMetrics{} sync.DeleteNoLongerExistingModuleStatus(context.TODO(), kyma, testCase.getModule, - kymaMetrics.RemoveModuleStateMetrics, moduleMetrics.RemoveModuleStateMetrics) + kymaMetrics.RemoveModuleStateMetrics) if testCase.expectModuleMetricsGetCalled { assert.Equal(t, 1, kymaMetrics.callCount) - assert.Equal(t, 1, moduleMetrics.callCount) } else { assert.Equal(t, 0, kymaMetrics.callCount) - assert.Equal(t, 0, moduleMetrics.callCount) } }) } @@ -162,11 +151,10 @@ func TestDeleteNoLongerExistingModuleStatus(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { t.Parallel() kymaMetrics := &KymaMockMetrics{} - moduleMetrics := &ModuleMockMetrics{} kyma := testutils.NewTestKyma("test-kyma") configureModuleInKyma(kyma, testCase.ModulesInKymaSpec, testCase.ModulesInKymaStatus) sync.DeleteNoLongerExistingModuleStatus(context.TODO(), kyma, testCase.getModule, - kymaMetrics.RemoveModuleStateMetrics, moduleMetrics.RemoveModuleStateMetrics) + kymaMetrics.RemoveModuleStateMetrics) var modulesInFinalModuleStatus []string for _, moduleStatus := range kyma.Status.Modules { modulesInFinalModuleStatus = append(modulesInFinalModuleStatus, moduleStatus.Name) diff --git a/tests/integration/controller/eventfilters/suite_test.go b/tests/integration/controller/eventfilters/suite_test.go index af168dc8b4..73d8354129 100644 --- a/tests/integration/controller/eventfilters/suite_test.go +++ b/tests/integration/controller/eventfilters/suite_test.go @@ -148,7 +148,6 @@ var _ = BeforeSuite(func() { InKCPMode: false, RemoteSyncNamespace: flags.DefaultRemoteSyncNamespace, Metrics: metrics.NewKymaMetrics(metrics.NewSharedMetrics()), - ModuleMetrics: metrics.NewModuleMetrics(), }).SetupWithManager(mgr, ctrlruntime.Options{}, kyma.SetupOptions{ListenerAddr: randomPort}) Expect(err).ToNot(HaveOccurred()) diff --git a/tests/integration/controller/kcp/suite_test.go b/tests/integration/controller/kcp/suite_test.go index d4f1b6166e..a29f9c504e 100644 --- a/tests/integration/controller/kcp/suite_test.go +++ b/tests/integration/controller/kcp/suite_test.go @@ -151,7 +151,6 @@ var _ = BeforeSuite(func() { RemoteSyncNamespace: flags.DefaultRemoteSyncNamespace, IsManagedKyma: true, Metrics: metrics.NewKymaMetrics(metrics.NewSharedMetrics()), - ModuleMetrics: metrics.NewModuleMetrics(), RemoteCatalog: remote.NewRemoteCatalogFromKyma(kcpClient, testSkrContextFactory, flags.DefaultRemoteSyncNamespace), }).SetupWithManager(mgr, ctrlruntime.Options{}, kyma.SetupOptions{ListenerAddr: UseRandomPort}) diff --git a/tests/integration/controller/kyma/suite_test.go b/tests/integration/controller/kyma/suite_test.go index 58e4b50ee4..22f39c1b08 100644 --- a/tests/integration/controller/kyma/suite_test.go +++ b/tests/integration/controller/kyma/suite_test.go @@ -144,7 +144,6 @@ var _ = BeforeSuite(func() { InKCPMode: false, RemoteSyncNamespace: flags.DefaultRemoteSyncNamespace, Metrics: metrics.NewKymaMetrics(metrics.NewSharedMetrics()), - ModuleMetrics: metrics.NewModuleMetrics(), }).SetupWithManager(mgr, ctrlruntime.Options{ RateLimiter: internal.RateLimiter( 1*time.Second, 5*time.Second, diff --git a/tests/integration/controller/manifest/custom_resource_check/suite_test.go b/tests/integration/controller/manifest/custom_resource_check/suite_test.go index e4acc0cc3d..4d4059fb30 100644 --- a/tests/integration/controller/manifest/custom_resource_check/suite_test.go +++ b/tests/integration/controller/manifest/custom_resource_check/suite_test.go @@ -153,7 +153,7 @@ var _ = BeforeSuite(func() { Busy: 1 * time.Second, Error: 1 * time.Second, Warning: 1 * time.Second, - }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), nil, + }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), manifestClient, manifest.NewSpecResolver(keyChainLookup, extractor), declarativev2.WithRemoteTargetCluster( func(_ context.Context, _ declarativev2.Object) (*declarativev2.ClusterInfo, error) { return &declarativev2.ClusterInfo{Config: authUser.Config()}, nil diff --git a/tests/integration/controller/manifest/suite_test.go b/tests/integration/controller/manifest/suite_test.go index 6cbae1104e..c79bbc1cba 100644 --- a/tests/integration/controller/manifest/suite_test.go +++ b/tests/integration/controller/manifest/suite_test.go @@ -150,7 +150,7 @@ var _ = BeforeSuite(func() { Busy: 1 * time.Second, Error: 1 * time.Second, Warning: 1 * time.Second, - }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), nil, + }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), manifestClient, manifest.NewSpecResolver(keyChainLookup, extractor), declarativev2.WithRemoteTargetCluster( func(_ context.Context, _ declarativev2.Object) (*declarativev2.ClusterInfo, error) { return &declarativev2.ClusterInfo{Config: authUser.Config()}, nil diff --git a/tests/integration/controller/withwatcher/suite_test.go b/tests/integration/controller/withwatcher/suite_test.go index 106ba54779..d481c9d137 100644 --- a/tests/integration/controller/withwatcher/suite_test.go +++ b/tests/integration/controller/withwatcher/suite_test.go @@ -215,7 +215,6 @@ var _ = BeforeSuite(func() { RemoteSyncNamespace: flags.DefaultRemoteSyncNamespace, InKCPMode: true, Metrics: metrics.NewKymaMetrics(metrics.NewSharedMetrics()), - ModuleMetrics: metrics.NewModuleMetrics(), RemoteCatalog: remote.NewRemoteCatalogFromKyma(kcpClient, testSkrContextFactory, flags.DefaultRemoteSyncNamespace), }).SetupWithManager(mgr, ctrlruntime.Options{}, kyma.SetupOptions{ListenerAddr: listenerAddr}) Expect(err).ToNot(HaveOccurred()) From 2f74c75336700277771ace3d64d2d5e507ea636d Mon Sep 17 00:00:00 2001 From: Amritanshu Sikdar Date: Fri, 17 Jan 2025 12:01:21 +0100 Subject: [PATCH 4/5] adjust tests with hardcoded names --- internal/pkg/metrics/module.go | 51 ---------------------- tests/e2e/commontestutils/metrics.go | 30 ------------- tests/e2e/module_status_decoupling_test.go | 11 ----- 3 files changed, 92 deletions(-) delete mode 100644 internal/pkg/metrics/module.go diff --git a/internal/pkg/metrics/module.go b/internal/pkg/metrics/module.go deleted file mode 100644 index a743ccbb51..0000000000 --- a/internal/pkg/metrics/module.go +++ /dev/null @@ -1,51 +0,0 @@ -package metrics - -import ( - "github.com/prometheus/client_golang/prometheus" - ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" -) - -type ModuleCondition string - -const ( - MetricModuleCondition = "lifecycle_mgr_module_condition" - conditionLabel = "condition" - moduleCrWarning ModuleCondition = "ModuleCRWarning" -) - -type ModuleMetrics struct { - moduleCRConditionGauge *prometheus.GaugeVec -} - -func NewModuleMetrics() *ModuleMetrics { - metrics := &ModuleMetrics{ - moduleCRConditionGauge: prometheus.NewGaugeVec(prometheus.GaugeOpts{ - Name: MetricModuleCondition, - Help: "Manifest is in Deleting state and related ModuleCR is in Warning state, indicating that Deletion is blocked and requires user interaction.", - }, []string{KymaNameLabel, moduleNameLabel, conditionLabel}), - } - ctrlmetrics.Registry.MustRegister(metrics.moduleCRConditionGauge) - return metrics -} - -func (m *ModuleMetrics) SetModuleCRWarningCondition(kymaName, moduleName string) { - m.moduleCRConditionGauge.With(prometheus.Labels{ - KymaNameLabel: kymaName, - moduleNameLabel: moduleName, - conditionLabel: string(moduleCrWarning), - }).Set(1) -} - -func (m *ModuleMetrics) RemoveModuleCRWarningCondition(kymaName, moduleName string) { - m.moduleCRConditionGauge.DeletePartialMatch(prometheus.Labels{ - KymaNameLabel: kymaName, - moduleNameLabel: moduleName, - conditionLabel: string(moduleCrWarning), - }) -} - -func (m *ModuleMetrics) CleanupMetrics(kymaName string) { - m.moduleCRConditionGauge.DeletePartialMatch(prometheus.Labels{ - KymaNameLabel: kymaName, - }) -} diff --git a/tests/e2e/commontestutils/metrics.go b/tests/e2e/commontestutils/metrics.go index 3e32a1afc4..bab02dd0d9 100644 --- a/tests/e2e/commontestutils/metrics.go +++ b/tests/e2e/commontestutils/metrics.go @@ -153,36 +153,6 @@ func GetMandatoryModuleStateMetric(ctx context.Context, kymaName, moduleName, st return parseCount(re, bodyString) } -func GetModuleCRWarningConditionMetric(ctx context.Context, kymaName, moduleName string) (int, error) { - bodyString, err := getMetricsBody(ctx) - if err != nil { - return 0, err - } - - re := getModuleCRWarningConditionMetric(kymaName, moduleName) - return parseCount(re, bodyString) -} - -func getModuleCRWarningConditionMetric(kymaName, moduleName string) *regexp.Regexp { - return regexp.MustCompile(fmt.Sprintf(`%s{condition="ModuleCRWarning",kyma_name="%s",module_name="%s"} (\d+)`, - metrics.MetricModuleCondition, kymaName, moduleName)) -} - -func ModuleCRWarningConditionMetricNotFound(ctx context.Context, kymaName, moduleName string) error { - bodyString, err := getMetricsBody(ctx) - if err != nil { - return err - } - - re := getModuleCRWarningConditionMetric(kymaName, moduleName) - match := re.FindStringSubmatch(bodyString) - if len(match) < 1 { - return ErrMetricNotFound - } - - return nil -} - func getMetricsBody(ctx context.Context) (string, error) { clnt := &http.Client{} request, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://localhost:9081/metrics", nil) diff --git a/tests/e2e/module_status_decoupling_test.go b/tests/e2e/module_status_decoupling_test.go index b0514ff5ec..bdb4958f5c 100644 --- a/tests/e2e/module_status_decoupling_test.go +++ b/tests/e2e/module_status_decoupling_test.go @@ -93,11 +93,6 @@ func RunModuleStatusDecouplingTest(resourceKind ResourceKind) { WithContext(ctx). WithArguments(skrClient, moduleCR, shared.StateWarning). Should(BeTrue()) - By("And count of metrics lifecycle_mgr_module_condition is 1") - Eventually(GetModuleCRWarningConditionMetric). - WithContext(ctx). - WithArguments(kyma.GetName(), TestModuleName). - Should(Equal(1)) }) It("When blocking finalizers from Module CR get removed", func() { @@ -139,12 +134,6 @@ func RunModuleStatusDecouplingTest(resourceKind ResourceKind) { WithContext(ctx). WithArguments(kyma.GetName(), kyma.GetNamespace(), kcpClient, shared.StateReady). Should(Succeed()) - - By("And count of metrics lifecycle_mgr_module_condition is removed") - Eventually(ModuleCRWarningConditionMetricNotFound). - WithContext(ctx). - WithArguments(kyma.GetName(), TestModuleName). - Should(Equal(ErrMetricNotFound)) }) }) From 775c2c1c7391651eeae1277403d9d4d54d8fe061 Mon Sep 17 00:00:00 2001 From: Amritanshu Sikdar Date: Mon, 20 Jan 2025 13:02:26 +0100 Subject: [PATCH 5/5] upadte unit test coverage --- unit-test-coverage.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index 12414b91c0..1e1b9a09f3 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -13,7 +13,7 @@ packages: internal/manifest/labelsremoval: 93 internal/manifest/img: 65.5 internal/manifest/manifestclient: 5.0 - internal/manifest/modulecr: 51.1 + internal/manifest/modulecr: 50.0 internal/maintenancewindows: 87.5 internal/istio: 63.3 internal/pkg/resources: 91.7