From ba684eb631b661edf63a0046cb1637dac3072274 Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Tue, 24 Oct 2023 05:54:10 +0530 Subject: [PATCH 1/6] feat: added grace period for repo errors to prevent aggressive sync state unknowns Signed-off-by: Soumya Ghosh Dastidar --- .../commands/argocd_application_controller.go | 3 ++ cmd/argocd/commands/admin/app.go | 7 ++- controller/appcontroller.go | 14 +++-- controller/appcontroller_test.go | 1 + controller/state.go | 35 ++++++++++--- controller/state_test.go | 52 +++++++++---------- controller/sync.go | 3 +- 7 files changed, 76 insertions(+), 39 deletions(-) diff --git a/cmd/argocd-application-controller/commands/argocd_application_controller.go b/cmd/argocd-application-controller/commands/argocd_application_controller.go index 39d8e017b7f26..80ed431379fcd 100644 --- a/cmd/argocd-application-controller/commands/argocd_application_controller.go +++ b/cmd/argocd-application-controller/commands/argocd_application_controller.go @@ -50,6 +50,7 @@ func NewCommand() *cobra.Command { clientConfig clientcmd.ClientConfig appResyncPeriod int64 appHardResyncPeriod int64 + repoErrorGracePeriod int64 repoServerAddress string repoServerTimeoutSeconds int selfHealTimeoutSeconds int @@ -155,6 +156,7 @@ func NewCommand() *cobra.Command { resyncDuration, hardResyncDuration, time.Duration(selfHealTimeoutSeconds)*time.Second, + time.Duration(repoErrorGracePeriod)*time.Second, metricsPort, metricsCacheExpiration, metricsAplicationLabels, @@ -189,6 +191,7 @@ func NewCommand() *cobra.Command { clientConfig = cli.AddKubectlFlagsToCmd(&command) command.Flags().Int64Var(&appResyncPeriod, "app-resync", int64(env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", defaultAppResyncPeriod*time.Second, 0, math.MaxInt64).Seconds()), "Time period in seconds for application resync.") command.Flags().Int64Var(&appHardResyncPeriod, "app-hard-resync", int64(env.ParseDurationFromEnv("ARGOCD_HARD_RECONCILIATION_TIMEOUT", defaultAppHardResyncPeriod*time.Second, 0, math.MaxInt64).Seconds()), "Time period in seconds for application hard resync.") + command.Flags().Int64Var(&repoErrorGracePeriod, "repo-error-grace", int64(env.ParseDurationFromEnv("ARGOCD_REPO_ERROR_GRACE_PERIOD", defaultAppResyncPeriod*time.Second, 0, math.MaxInt64).Seconds()), "Grace period in seconds for ignoring consecutive errors while communicating with repo server.") command.Flags().StringVar(&repoServerAddress, "repo-server", env.StringFromEnv("ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER", common.DefaultRepoServerAddr), "Repo server address.") command.Flags().IntVar(&repoServerTimeoutSeconds, "repo-server-timeout-seconds", env.ParseNumFromEnv("ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_TIMEOUT_SECONDS", 60, 0, math.MaxInt64), "Repo server RPC call timeout seconds.") command.Flags().IntVar(&statusProcessors, "status-processors", env.ParseNumFromEnv("ARGOCD_APPLICATION_CONTROLLER_STATUS_PROCESSORS", 20, 0, math.MaxInt32), "Number of application status processors") diff --git a/cmd/argocd/commands/admin/app.go b/cmd/argocd/commands/admin/app.go index fbceb436f8609..86a97bea51df3 100644 --- a/cmd/argocd/commands/admin/app.go +++ b/cmd/argocd/commands/admin/app.go @@ -374,7 +374,7 @@ func reconcileApplications( ) appStateManager := controller.NewAppStateManager( - argoDB, appClientset, repoServerClient, namespace, kubeutil.NewKubectl(), settingsMgr, stateCache, projInformer, server, cache, time.Second, argo.NewResourceTracking(), false) + argoDB, appClientset, repoServerClient, namespace, kubeutil.NewKubectl(), settingsMgr, stateCache, projInformer, server, cache, time.Second, argo.NewResourceTracking(), false, 0) appsList, err := appClientset.ArgoprojV1alpha1().Applications(namespace).List(ctx, v1.ListOptions{LabelSelector: selector}) if err != nil { @@ -409,7 +409,10 @@ func reconcileApplications( sources = append(sources, app.Spec.GetSource()) revisions = append(revisions, app.Spec.GetSource().TargetRevision) - res := appStateManager.CompareAppState(&app, proj, revisions, sources, false, false, nil, false) + res, err := appStateManager.CompareAppState(&app, proj, revisions, sources, false, false, nil, false) + if err != nil { + return nil, err + } items = append(items, appReconcileResult{ Name: app.Name, Conditions: app.Status.Conditions, diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 46e3ede0f3a3e..ccf1a5adc3c7a 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -3,6 +3,7 @@ package controller import ( "context" "encoding/json" + goerror "errors" "fmt" "math" "net/http" @@ -142,6 +143,7 @@ func NewApplicationController( appResyncPeriod time.Duration, appHardResyncPeriod time.Duration, selfHealTimeout time.Duration, + repoErrorGracePeriod time.Duration, metricsPort int, metricsCacheExpiration time.Duration, metricsApplicationLabels []string, @@ -258,7 +260,7 @@ func NewApplicationController( } } stateCache := statecache.NewLiveStateCache(db, appInformer, ctrl.settingsMgr, kubectl, ctrl.metricsServer, ctrl.handleObjectUpdated, clusterFilter, argo.NewResourceTracking()) - appStateManager := NewAppStateManager(db, applicationClientset, repoClientset, namespace, kubectl, ctrl.settingsMgr, stateCache, projInformer, ctrl.metricsServer, argoCache, ctrl.statusRefreshTimeout, argo.NewResourceTracking(), persistResourceHealth) + appStateManager := NewAppStateManager(db, applicationClientset, repoClientset, namespace, kubectl, ctrl.settingsMgr, stateCache, projInformer, ctrl.metricsServer, argoCache, ctrl.statusRefreshTimeout, argo.NewResourceTracking(), persistResourceHealth, repoErrorGracePeriod) ctrl.appInformer = appInformer ctrl.appLister = appLister ctrl.projInformer = projInformer @@ -1491,16 +1493,20 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo } now := metav1.Now() - compareResult := ctrl.appStateManager.CompareAppState(app, project, revisions, sources, + ctrl.normalizeApplication(origApp, app) + + compareResult, err := ctrl.appStateManager.CompareAppState(app, project, revisions, sources, refreshType == appv1.RefreshTypeHard, comparisonLevel == CompareWithLatestForceResolve, localManifests, hasMultipleSources) + if goerror.Is(err, CompareStateRepoError) { + return // short circuit if git error is encountered + } + for k, v := range compareResult.timings { logCtx = logCtx.WithField(k, v.Milliseconds()) } - ctrl.normalizeApplication(origApp, app) - tree, err := ctrl.setAppManagedResources(app, compareResult) if err != nil { logCtx.Errorf("Failed to cache app resources: %v", err) diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 2129c88885fff..b74c9d46b5f3c 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -116,6 +116,7 @@ func newFakeController(data *fakeData) *ApplicationController { time.Minute, time.Hour, time.Minute, + time.Minute, common.DefaultPortArgoCDMetrics, data.metricsCacheExpiration, []string{}, diff --git a/controller/state.go b/controller/state.go index 2704545add445..fbdcef2f369de 100644 --- a/controller/state.go +++ b/controller/state.go @@ -3,12 +3,15 @@ package controller import ( "context" "encoding/json" + "errors" "fmt" - v1 "k8s.io/api/core/v1" "reflect" "strings" + goSync "sync" "time" + v1 "k8s.io/api/core/v1" + "github.com/argoproj/gitops-engine/pkg/diff" "github.com/argoproj/gitops-engine/pkg/health" "github.com/argoproj/gitops-engine/pkg/sync" @@ -40,6 +43,10 @@ import ( "github.com/argoproj/argo-cd/v2/util/stats" ) +var ( + CompareStateRepoError = errors.New("failed to get repo objects") +) + type resourceInfoProviderStub struct { } @@ -62,7 +69,7 @@ type managedResource struct { // AppStateManager defines methods which allow to compare application spec and actual application state. type AppStateManager interface { - CompareAppState(app *v1alpha1.Application, project *v1alpha1.AppProject, revisions []string, sources []v1alpha1.ApplicationSource, noCache bool, noRevisionCache bool, localObjects []string, hasMultipleSources bool) *comparisonResult + CompareAppState(app *v1alpha1.Application, project *v1alpha1.AppProject, revisions []string, sources []v1alpha1.ApplicationSource, noCache bool, noRevisionCache bool, localObjects []string, hasMultipleSources bool) (*comparisonResult, error) SyncAppState(app *v1alpha1.Application, state *v1alpha1.OperationState) } @@ -105,6 +112,8 @@ type appStateManager struct { statusRefreshTimeout time.Duration resourceTracking argo.ResourceTracking persistResourceHealth bool + repoErrorCache goSync.Map + repoErrorGracePeriod time.Duration } func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alpha1.ApplicationSource, appLabelKey string, revisions []string, noCache, noRevisionCache, verifySignature bool, proj *v1alpha1.AppProject) ([]*unstructured.Unstructured, []*apiclient.ManifestResponse, error) { @@ -345,7 +354,7 @@ func isManagedNamespace(ns *unstructured.Unstructured, app *v1alpha1.Application // CompareAppState compares application git state to the live app state, using the specified // revision and supplied source. If revision or overrides are empty, then compares against // revision and overrides in the app spec. -func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1alpha1.AppProject, revisions []string, sources []v1alpha1.ApplicationSource, noCache bool, noRevisionCache bool, localManifests []string, hasMultipleSources bool) *comparisonResult { +func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1alpha1.AppProject, revisions []string, sources []v1alpha1.ApplicationSource, noCache bool, noRevisionCache bool, localManifests []string, hasMultipleSources bool) (*comparisonResult, error) { ts := stats.NewTimingStats() appLabelKey, resourceOverrides, resFilter, err := m.getComparisonSettings() @@ -361,7 +370,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 Revisions: revisions, }, healthStatus: &v1alpha1.HealthStatus{Status: health.HealthStatusUnknown}, - } + }, nil } else { return &comparisonResult{ syncStatus: &v1alpha1.SyncStatus{ @@ -370,7 +379,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 Revision: revisions[0], }, healthStatus: &v1alpha1.HealthStatus{Status: health.HealthStatusUnknown}, - } + }, nil } } @@ -408,7 +417,19 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 targetObjs = make([]*unstructured.Unstructured, 0) msg := fmt.Sprintf("Failed to load target state: %s", err.Error()) conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: msg, LastTransitionTime: &now}) + if firstSeen, ok := m.repoErrorCache.Load(app.Name); ok { + if time.Since(firstSeen.(time.Time)) <= m.repoErrorGracePeriod && !noRevisionCache { + // if first seen is less than grace period and it's not a Level 3 comparison, + // ignore error and short circuit + return nil, CompareStateRepoError + } + } else if !noRevisionCache { + m.repoErrorCache.Store(app.Name, time.Now()) + return nil, CompareStateRepoError + } failedToLoadObjs = true + } else { + m.repoErrorCache.Delete(app.Name) } } else { // Prevent applying local manifests for now when signature verification is enabled @@ -776,7 +797,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 }) ts.AddCheckpoint("health_ms") compRes.timings = ts.Timings() - return &compRes + return &compRes, nil } func (m *appStateManager) persistRevisionHistory(app *v1alpha1.Application, revision string, source v1alpha1.ApplicationSource, revisions []string, sources []v1alpha1.ApplicationSource, hasMultipleSources bool, startedAt metav1.Time) error { @@ -832,6 +853,7 @@ func NewAppStateManager( statusRefreshTimeout time.Duration, resourceTracking argo.ResourceTracking, persistResourceHealth bool, + repoErrorGracePeriod time.Duration, ) AppStateManager { return &appStateManager{ liveStateCache: liveStateCache, @@ -847,6 +869,7 @@ func NewAppStateManager( statusRefreshTimeout: statusRefreshTimeout, resourceTracking: resourceTracking, persistResourceHealth: persistResourceHealth, + repoErrorGracePeriod: repoErrorGracePeriod, } } diff --git a/controller/state_test.go b/controller/state_test.go index dcb48e87fce9b..1049184189421 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -42,7 +42,7 @@ func TestCompareAppStateEmpty(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -80,7 +80,7 @@ func TestCompareAppStateNamespaceMetadataDiffers(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) @@ -127,7 +127,7 @@ func TestCompareAppStateNamespaceMetadataIsTheSame(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -154,7 +154,7 @@ func TestCompareAppStateMissing(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) @@ -185,7 +185,7 @@ func TestCompareAppStateExtra(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) assert.Equal(t, 1, len(compRes.resources)) @@ -215,7 +215,7 @@ func TestCompareAppStateHook(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) assert.Equal(t, 0, len(compRes.resources)) @@ -246,7 +246,7 @@ func TestCompareAppStateSkipHook(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) assert.Equal(t, 1, len(compRes.resources)) @@ -276,7 +276,7 @@ func TestCompareAppStateCompareOptionIgnoreExtraneous(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -308,7 +308,7 @@ func TestCompareAppStateExtraHook(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -336,7 +336,7 @@ func TestAppRevisionsSingleSource(t *testing.T) { app := newFakeApp() revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.NotEmpty(t, compRes.syncStatus.Revision) @@ -375,7 +375,7 @@ func TestAppRevisionsMultiSource(t *testing.T) { app := newFakeMultiSourceApp() revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Empty(t, compRes.syncStatus.Revision) @@ -422,7 +422,7 @@ func TestCompareAppStateDuplicatedNamespacedResources(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, 1, len(app.Status.Conditions)) @@ -458,7 +458,7 @@ func TestCompareAppStateManagedNamespaceMetadataWithLiveNsDoesNotGetPruned(t *te }, } ctrl := newFakeController(&data) - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, []string{}, app.Spec.Sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, []string{}, app.Spec.Sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, 0, len(app.Status.Conditions)) @@ -518,7 +518,7 @@ func TestSetHealth(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.Equal(t, health.HealthStatusHealthy, compRes.healthStatus.Status) } @@ -554,7 +554,7 @@ func TestSetHealthSelfReferencedApp(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.Equal(t, health.HealthStatusHealthy, compRes.healthStatus.Status) } @@ -628,7 +628,7 @@ func TestReturnUnknownComparisonStateOnSettingLoadError(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.Equal(t, health.HealthStatusUnknown, compRes.healthStatus.Status) assert.Equal(t, argoappv1.SyncStatusCodeUnknown, compRes.syncStatus.Status) @@ -768,7 +768,7 @@ func TestSignedResponseNoSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -794,7 +794,7 @@ func TestSignedResponseNoSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -825,7 +825,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -851,7 +851,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -877,7 +877,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -903,7 +903,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -932,7 +932,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes := ctrl.appStateManager.CompareAppState(app, &testProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &testProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -961,7 +961,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeUnknown, compRes.syncStatus.Status) @@ -990,7 +990,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -1019,7 +1019,7 @@ func TestSignedResponseSignatureRequired(t *testing.T) { sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) + compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) diff --git a/controller/sync.go b/controller/sync.go index 783183c17fc7c..dc9353f29a946 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -152,7 +152,8 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha revisions = []string{revision} } - compareResult := m.CompareAppState(app, proj, revisions, sources, false, true, syncOp.Manifests, app.Spec.HasMultipleSources()) + // error does not matter as noRevisionCache is set to true + compareResult, _ := m.CompareAppState(app, proj, revisions, sources, false, true, syncOp.Manifests, app.Spec.HasMultipleSources()) // We now have a concrete commit SHA. Save this in the sync result revision so that we remember // what we should be syncing to when resuming operations. From 36c7a5720dcf46c49630092ba722c134dfde0171 Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Tue, 24 Oct 2023 06:25:45 +0530 Subject: [PATCH 2/6] feat: updated docs Signed-off-by: Soumya Ghosh Dastidar --- .../server-commands/argocd-application-controller.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/operator-manual/server-commands/argocd-application-controller.md b/docs/operator-manual/server-commands/argocd-application-controller.md index 99e4c42df28db..a270de5465f40 100644 --- a/docs/operator-manual/server-commands/argocd-application-controller.md +++ b/docs/operator-manual/server-commands/argocd-application-controller.md @@ -55,6 +55,7 @@ argocd-application-controller [flags] --redis-insecure-skip-tls-verify Skip Redis server certificate validation. --redis-use-tls Use TLS when connecting to Redis. --redisdb int Redis database. + --repo-error-grace int Grace period in seconds for ignoring consecutive errors while communicating with repo server. (default 180) --repo-server string Repo server address. (default "argocd-repo-server:8081") --repo-server-plaintext Disable TLS on connections to repo server --repo-server-strict-tls Whether to use strict validation of the TLS cert presented by the repo server From b81f8fb811bfe00650aa0ed441a63187bcd24c16 Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Tue, 24 Oct 2023 09:04:36 +0530 Subject: [PATCH 3/6] fix: e2e test Signed-off-by: Soumya Ghosh Dastidar --- controller/appcontroller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controller/appcontroller.go b/controller/appcontroller.go index e41ec18a51059..92d98ad2982a1 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1513,8 +1513,6 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo } now := metav1.Now() - ctrl.normalizeApplication(origApp, app) - compareResult, err := ctrl.appStateManager.CompareAppState(app, project, revisions, sources, refreshType == appv1.RefreshTypeHard, comparisonLevel == CompareWithLatestForceResolve, localManifests, hasMultipleSources) @@ -1527,6 +1525,8 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo logCtx = logCtx.WithField(k, v.Milliseconds()) } + ctrl.normalizeApplication(origApp, app) + tree, err := ctrl.setAppManagedResources(app, compareResult) if err != nil { logCtx.Errorf("Failed to cache app resources: %v", err) From 04782a92e2fdf00861a5e09075e1ac6a33707c35 Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Mon, 30 Oct 2023 18:13:15 +0530 Subject: [PATCH 4/6] feat: resolved review comments Signed-off-by: Soumya Ghosh Dastidar --- .../commands/argocd_application_controller.go | 2 +- controller/appcontroller.go | 1 + controller/appcontroller_test.go | 114 +++++++------ controller/state.go | 3 +- controller/state_test.go | 154 +++++++++++------- controller/sync.go | 10 +- controller/sync_test.go | 10 +- .../operator-manual/argocd-cmd-params-cm.yaml | 2 + .../argocd-application-controller.md | 2 +- ...cd-application-controller-statefulset.yaml | 6 + manifests/core-install.yaml | 6 + manifests/ha/install.yaml | 6 + manifests/ha/namespace-install.yaml | 6 + manifests/install.yaml | 6 + manifests/namespace-install.yaml | 6 + pkg/client/clientset/versioned/clientset.go | 3 +- .../informers/externalversions/factory.go | 79 +-------- 17 files changed, 220 insertions(+), 196 deletions(-) diff --git a/cmd/argocd-application-controller/commands/argocd_application_controller.go b/cmd/argocd-application-controller/commands/argocd_application_controller.go index 80ed431379fcd..86b7a8e8deab3 100644 --- a/cmd/argocd-application-controller/commands/argocd_application_controller.go +++ b/cmd/argocd-application-controller/commands/argocd_application_controller.go @@ -191,7 +191,7 @@ func NewCommand() *cobra.Command { clientConfig = cli.AddKubectlFlagsToCmd(&command) command.Flags().Int64Var(&appResyncPeriod, "app-resync", int64(env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", defaultAppResyncPeriod*time.Second, 0, math.MaxInt64).Seconds()), "Time period in seconds for application resync.") command.Flags().Int64Var(&appHardResyncPeriod, "app-hard-resync", int64(env.ParseDurationFromEnv("ARGOCD_HARD_RECONCILIATION_TIMEOUT", defaultAppHardResyncPeriod*time.Second, 0, math.MaxInt64).Seconds()), "Time period in seconds for application hard resync.") - command.Flags().Int64Var(&repoErrorGracePeriod, "repo-error-grace", int64(env.ParseDurationFromEnv("ARGOCD_REPO_ERROR_GRACE_PERIOD", defaultAppResyncPeriod*time.Second, 0, math.MaxInt64).Seconds()), "Grace period in seconds for ignoring consecutive errors while communicating with repo server.") + command.Flags().Int64Var(&repoErrorGracePeriod, "repo-error-grace-period-seconds", int64(env.ParseDurationFromEnv("ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS", defaultAppResyncPeriod*time.Second, 0, math.MaxInt64).Seconds()), "Grace period in seconds for ignoring consecutive errors while communicating with repo server.") command.Flags().StringVar(&repoServerAddress, "repo-server", env.StringFromEnv("ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER", common.DefaultRepoServerAddr), "Repo server address.") command.Flags().IntVar(&repoServerTimeoutSeconds, "repo-server-timeout-seconds", env.ParseNumFromEnv("ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_TIMEOUT_SECONDS", 60, 0, math.MaxInt64), "Repo server RPC call timeout seconds.") command.Flags().IntVar(&statusProcessors, "status-processors", env.ParseNumFromEnv("ARGOCD_APPLICATION_CONTROLLER_STATUS_PROCESSORS", 20, 0, math.MaxInt32), "Number of application status processors") diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 92d98ad2982a1..c4c6ad3271b45 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1518,6 +1518,7 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo comparisonLevel == CompareWithLatestForceResolve, localManifests, hasMultipleSources) if goerrors.Is(err, CompareStateRepoError) { + logCtx.Warnf("Ignoring temporary failed attempt to compare app state against repo: %v", err) return // short circuit if git error is encountered } diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 1cd61dd2e4aa8..bbfdafcd104f3 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -59,7 +59,7 @@ type fakeData struct { applicationNamespaces []string } -func newFakeController(data *fakeData) *ApplicationController { +func newFakeController(data *fakeData, repoErr error) *ApplicationController { var clust corev1.Secret err := yaml.Unmarshal([]byte(fakeCluster), &clust) if err != nil { @@ -71,10 +71,18 @@ func newFakeController(data *fakeData) *ApplicationController { if len(data.manifestResponses) > 0 { for _, response := range data.manifestResponses { - mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(response, nil).Once() + if repoErr != nil { + mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(response, repoErr).Once() + } else { + mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(response, nil).Once() + } } } else { - mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(data.manifestResponse, nil) + if repoErr != nil { + mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(data.manifestResponse, repoErr).Once() + } else { + mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(data.manifestResponse, nil).Once() + } } mockRepoClientset := mockrepoclient.Clientset{RepoServerServiceClient: &mockRepoClient} @@ -116,7 +124,7 @@ func newFakeController(data *fakeData) *ApplicationController { time.Minute, time.Hour, time.Minute, - time.Minute, + time.Second*30, common.DefaultPortArgoCDMetrics, data.metricsCacheExpiration, []string{}, @@ -365,7 +373,7 @@ func newFakeCM() map[string]interface{} { func TestAutoSync(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -382,7 +390,7 @@ func TestAutoSync(t *testing.T) { func TestAutoSyncNotAllowEmpty(t *testing.T) { app := newFakeApp() app.Spec.SyncPolicy.Automated.Prune = true - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -395,7 +403,7 @@ func TestAutoSyncAllowEmpty(t *testing.T) { app := newFakeApp() app.Spec.SyncPolicy.Automated.Prune = true app.Spec.SyncPolicy.Automated.AllowEmpty = true - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -409,7 +417,7 @@ func TestSkipAutoSync(t *testing.T) { // Set current to 'aaaaa', desired to 'aaaa' and mark system OutOfSync t.Run("PreviouslySyncedToRevision", func(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", @@ -424,7 +432,7 @@ func TestSkipAutoSync(t *testing.T) { // Verify we skip when we are already Synced (even if revision is different) t.Run("AlreadyInSyncedState", func(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeSynced, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -440,7 +448,7 @@ func TestSkipAutoSync(t *testing.T) { t.Run("AutoSyncIsDisabled", func(t *testing.T) { app := newFakeApp() app.Spec.SyncPolicy = nil - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -457,7 +465,7 @@ func TestSkipAutoSync(t *testing.T) { app := newFakeApp() now := metav1.Now() app.DeletionTimestamp = &now - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -483,7 +491,7 @@ func TestSkipAutoSync(t *testing.T) { Source: *app.Spec.Source.DeepCopy(), }, } - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -497,7 +505,7 @@ func TestSkipAutoSync(t *testing.T) { t.Run("NeedsToPruneResourcesOnlyButAutomatedPruneDisabled", func(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -523,7 +531,7 @@ func TestAutoSyncIndicateError(t *testing.T) { }, }, } - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", @@ -558,7 +566,7 @@ func TestAutoSyncParameterOverrides(t *testing.T) { }, }, } - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) syncStatus := v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeOutOfSync, Revision: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", @@ -615,7 +623,7 @@ func TestFinalizeAppDeletion(t *testing.T) { appObj := kube.MustToUnstructured(&app) ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}, managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{ kube.GetResourceKey(appObj): appObj, - }}) + }}, nil) patched := false fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) @@ -665,7 +673,7 @@ func TestFinalizeAppDeletion(t *testing.T) { kube.GetResourceKey(appObj): appObj, kube.GetResourceKey(strayObj): strayObj, }, - }) + }, nil) patched := false fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) @@ -700,7 +708,7 @@ func TestFinalizeAppDeletion(t *testing.T) { appObj := kube.MustToUnstructured(&app) ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}, managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{ kube.GetResourceKey(appObj): appObj, - }}) + }}, nil) patched := false fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) defaultReactor := fakeAppCs.ReactionChain[0] @@ -729,7 +737,7 @@ func TestFinalizeAppDeletion(t *testing.T) { appObj := kube.MustToUnstructured(&app) ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}, managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{ kube.GetResourceKey(appObj): appObj, - }}) + }}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) defaultReactor := fakeAppCs.ReactionChain[0] @@ -793,7 +801,7 @@ func TestNormalizeApplication(t *testing.T) { { // Verify we normalize the app because project is missing - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) key, _ := cache.MetaNamespaceKeyFunc(app) ctrl.appRefreshQueue.AddRateLimited(key) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) @@ -815,7 +823,7 @@ func TestNormalizeApplication(t *testing.T) { // Verify we don't unnecessarily normalize app when project is set app.Spec.Project = "default" data.apps[0] = app - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) key, _ := cache.MetaNamespaceKeyFunc(app) ctrl.appRefreshQueue.AddRateLimited(key) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) @@ -840,7 +848,7 @@ func TestHandleAppUpdated(t *testing.T) { app.Spec.Destination.Server = v1alpha1.KubernetesInternalAPIServerAddr proj := defaultProj.DeepCopy() proj.Spec.SourceNamespaces = []string{test.FakeArgoCDNamespace} - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, proj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, proj}}, nil) ctrl.handleObjectUpdated(map[string]bool{app.InstanceName(ctrl.namespace): true}, kube.GetObjectRef(kube.MustToUnstructured(app))) isRequested, level := ctrl.isRefreshRequested(app.QualifiedName()) @@ -867,7 +875,7 @@ func TestHandleOrphanedResourceUpdated(t *testing.T) { proj := defaultProj.DeepCopy() proj.Spec.OrphanedResources = &v1alpha1.OrphanedResourcesMonitorSettings{} - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app1, app2, proj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app1, app2, proj}}, nil) ctrl.handleObjectUpdated(map[string]bool{}, corev1.ObjectReference{UID: "test", Kind: kube.DeploymentKind, Name: "test", Namespace: test.FakeArgoCDNamespace}) @@ -902,7 +910,7 @@ func TestGetResourceTree_HasOrphanedResources(t *testing.T) { kube.NewResourceKey("apps", "Deployment", "default", "deploy1"): {ResourceNode: orphanedDeploy1}, kube.NewResourceKey("apps", "Deployment", "default", "deploy2"): {ResourceNode: orphanedDeploy2}, }, - }) + }, nil) tree, err := ctrl.getResourceTree(app, []*v1alpha1.ResourceDiff{{ Namespace: "default", Name: "nginx-deployment", @@ -918,7 +926,7 @@ func TestGetResourceTree_HasOrphanedResources(t *testing.T) { } func TestSetOperationStateOnDeletedApp(t *testing.T) { - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) fakeAppCs.ReactionChain = nil patched := false @@ -949,7 +957,7 @@ func TestSetOperationStateLogRetries(t *testing.T) { t.Cleanup(func() { logrus.StandardLogger().ReplaceHooks(logrus.LevelHooks{}) }) - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) fakeAppCs.ReactionChain = nil patched := false @@ -1000,7 +1008,7 @@ func TestNeedRefreshAppStatus(t *testing.T) { app.Status.Sync.ComparedTo.Source = app.Spec.GetSource() } - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) t.Run("no need to refresh just reconciled application", func(t *testing.T) { needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour) @@ -1012,7 +1020,7 @@ func TestNeedRefreshAppStatus(t *testing.T) { assert.False(t, needRefresh) // use a one-off controller so other tests don't have a manual refresh request - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) // refresh app using the 'deepest' requested comparison level ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil) @@ -1040,7 +1048,7 @@ func TestNeedRefreshAppStatus(t *testing.T) { app := app.DeepCopy() // use a one-off controller so other tests don't have a manual refresh request - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour) assert.False(t, needRefresh) @@ -1070,7 +1078,7 @@ func TestNeedRefreshAppStatus(t *testing.T) { } // use a one-off controller so other tests don't have a manual refresh request - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour) assert.False(t, needRefresh) @@ -1150,7 +1158,7 @@ func TestNeedRefreshAppStatus(t *testing.T) { } func TestUpdatedManagedNamespaceMetadata(t *testing.T) { - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) app := newFakeApp() app.Spec.SyncPolicy.ManagedNamespaceMetadata = &v1alpha1.ManagedNamespaceMetadata{ Labels: map[string]string{ @@ -1174,7 +1182,7 @@ func TestUpdatedManagedNamespaceMetadata(t *testing.T) { } func TestUnchangedManagedNamespaceMetadata(t *testing.T) { - ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{}}, nil) app := newFakeApp() app.Spec.SyncPolicy.ManagedNamespaceMetadata = &v1alpha1.ManagedNamespaceMetadata{ Labels: map[string]string{ @@ -1217,7 +1225,7 @@ func TestRefreshAppConditions(t *testing.T) { t.Run("NoErrorConditions", func(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}}, nil) _, hasErrors := ctrl.refreshAppConditions(app) assert.False(t, hasErrors) @@ -1228,7 +1236,7 @@ func TestRefreshAppConditions(t *testing.T) { app := newFakeApp() app.Status.SetConditions([]v1alpha1.ApplicationCondition{{Type: v1alpha1.ApplicationConditionExcludedResourceWarning}}, nil) - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}}, nil) _, hasErrors := ctrl.refreshAppConditions(app) assert.False(t, hasErrors) @@ -1241,7 +1249,7 @@ func TestRefreshAppConditions(t *testing.T) { app.Spec.Project = "wrong project" app.Status.SetConditions([]v1alpha1.ApplicationCondition{{Type: v1alpha1.ApplicationConditionInvalidSpecError, Message: "old message"}}, nil) - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &defaultProj}}, nil) _, hasErrors := ctrl.refreshAppConditions(app) assert.True(t, hasErrors) @@ -1265,7 +1273,7 @@ func TestUpdateReconciledAt(t *testing.T) { Revision: "abc123", }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), - }) + }, nil) key, _ := cache.MetaNamespaceKeyFunc(app) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) fakeAppCs.ReactionChain = nil @@ -1323,7 +1331,7 @@ func TestProjectErrorToCondition(t *testing.T) { Revision: "abc123", }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), - }) + }, nil) key, _ := cache.MetaNamespaceKeyFunc(app) ctrl.appRefreshQueue.AddRateLimited(key) ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil) @@ -1342,7 +1350,7 @@ func TestProjectErrorToCondition(t *testing.T) { func TestFinalizeProjectDeletion_HasApplications(t *testing.T) { app := newFakeApp() proj := &v1alpha1.AppProject{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: test.FakeArgoCDNamespace}} - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, proj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, proj}}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) patched := false @@ -1358,7 +1366,7 @@ func TestFinalizeProjectDeletion_HasApplications(t *testing.T) { func TestFinalizeProjectDeletion_DoesNotHaveApplications(t *testing.T) { proj := &v1alpha1.AppProject{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: test.FakeArgoCDNamespace}} - ctrl := newFakeController(&fakeData{apps: []runtime.Object{&defaultProj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{&defaultProj}}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) receivedPatch := map[string]interface{}{} @@ -1384,7 +1392,7 @@ func TestProcessRequestedAppOperation_FailedNoRetries(t *testing.T) { app.Operation = &v1alpha1.Operation{ Sync: &v1alpha1.SyncOperation{}, } - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) receivedPatch := map[string]interface{}{} fakeAppCs.PrependReactor("patch", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { @@ -1409,7 +1417,7 @@ func TestProcessRequestedAppOperation_InvalidDestination(t *testing.T) { proj := defaultProj proj.Name = "test-project" proj.Spec.SourceNamespaces = []string{test.FakeArgoCDNamespace} - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &proj}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app, &proj}}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) receivedPatch := map[string]interface{}{} func() { @@ -1438,7 +1446,7 @@ func TestProcessRequestedAppOperation_FailedHasRetries(t *testing.T) { Sync: &v1alpha1.SyncOperation{}, Retry: v1alpha1.RetryStrategy{Limit: 1}, } - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) receivedPatch := map[string]interface{}{} fakeAppCs.PrependReactor("patch", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { @@ -1481,7 +1489,7 @@ func TestProcessRequestedAppOperation_RunningPreviouslyFailed(t *testing.T) { Revision: "abc123", }, } - ctrl := newFakeController(data) + ctrl := newFakeController(data, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) receivedPatch := map[string]interface{}{} fakeAppCs.PrependReactor("patch", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { @@ -1514,7 +1522,7 @@ func TestProcessRequestedAppOperation_HasRetriesTerminated(t *testing.T) { Revision: "abc123", }, } - ctrl := newFakeController(data) + ctrl := newFakeController(data, nil) fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) receivedPatch := map[string]interface{}{} fakeAppCs.PrependReactor("patch", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { @@ -1541,7 +1549,7 @@ func TestGetAppHosts(t *testing.T) { Revision: "abc123", }, } - ctrl := newFakeController(data) + ctrl := newFakeController(data, nil) mockStateCache := &mockstatecache.LiveStateCache{} mockStateCache.On("IterateResources", mock.Anything, mock.MatchedBy(func(callback func(res *clustercache.Resource, info *statecache.ResourceInfo)) bool { // node resource @@ -1591,15 +1599,15 @@ func TestGetAppHosts(t *testing.T) { func TestMetricsExpiration(t *testing.T) { app := newFakeApp() // Check expiration is disabled by default - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) assert.False(t, ctrl.metricsServer.HasExpiration()) // Check expiration is enabled if set - ctrl = newFakeController(&fakeData{apps: []runtime.Object{app}, metricsCacheExpiration: 10 * time.Second}) + ctrl = newFakeController(&fakeData{apps: []runtime.Object{app}, metricsCacheExpiration: 10 * time.Second}, nil) assert.True(t, ctrl.metricsServer.HasExpiration()) } func TestToAppKey(t *testing.T) { - ctrl := newFakeController(&fakeData{}) + ctrl := newFakeController(&fakeData{}, nil) tests := []struct { name string input string @@ -1619,7 +1627,7 @@ func TestToAppKey(t *testing.T) { func Test_canProcessApp(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) ctrl.applicationNamespaces = []string{"good"} t.Run("without cluster filter, good namespace", func(t *testing.T) { app.Namespace = "good" @@ -1652,7 +1660,7 @@ func Test_canProcessAppSkipReconcileAnnotation(t *testing.T) { appSkipReconcileFalse.Annotations = map[string]string{common.AnnotationKeyAppSkipReconcile: "false"} appSkipReconcileTrue := newFakeApp() appSkipReconcileTrue.Annotations = map[string]string{common.AnnotationKeyAppSkipReconcile: "true"} - ctrl := newFakeController(&fakeData{}) + ctrl := newFakeController(&fakeData{}, nil) tests := []struct { name string input interface{} @@ -1673,7 +1681,7 @@ func Test_canProcessAppSkipReconcileAnnotation(t *testing.T) { func Test_syncDeleteOption(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}) + ctrl := newFakeController(&fakeData{apps: []runtime.Object{app}}, nil) cm := newFakeCM() t.Run("without delete option object is deleted", func(t *testing.T) { cmObj := kube.MustToUnstructured(&cm) @@ -1700,7 +1708,7 @@ func TestAddControllerNamespace(t *testing.T) { ctrl := newFakeController(&fakeData{ apps: []runtime.Object{app, &defaultProj}, manifestResponse: &apiclient.ManifestResponse{}, - }) + }, nil) ctrl.processAppRefreshQueueItem() @@ -1719,7 +1727,7 @@ func TestAddControllerNamespace(t *testing.T) { apps: []runtime.Object{app, &proj}, manifestResponse: &apiclient.ManifestResponse{}, applicationNamespaces: []string{appNamespace}, - }) + }, nil) ctrl.processAppRefreshQueueItem() diff --git a/controller/state.go b/controller/state.go index fbdcef2f369de..15da3d2e624ed 100644 --- a/controller/state.go +++ b/controller/state.go @@ -117,7 +117,6 @@ type appStateManager struct { } func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alpha1.ApplicationSource, appLabelKey string, revisions []string, noCache, noRevisionCache, verifySignature bool, proj *v1alpha1.AppProject) ([]*unstructured.Unstructured, []*apiclient.ManifestResponse, error) { - ts := stats.NewTimingStats() helmRepos, err := m.db.ListHelmRepositories(context.Background()) if err != nil { @@ -421,9 +420,11 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 if time.Since(firstSeen.(time.Time)) <= m.repoErrorGracePeriod && !noRevisionCache { // if first seen is less than grace period and it's not a Level 3 comparison, // ignore error and short circuit + logCtx.Debugf("Ignoring repo error %v, already encountered error in grace period", err.Error()) return nil, CompareStateRepoError } } else if !noRevisionCache { + logCtx.Debugf("Ignoring repo error %v, new occurrence", err.Error()) m.repoErrorCache.Store(app.Name, time.Now()) return nil, CompareStateRepoError } diff --git a/controller/state_test.go b/controller/state_test.go index 1049184189421..0ca3cfd166822 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -2,6 +2,7 @@ package controller import ( "encoding/json" + "fmt" "os" "testing" "time" @@ -37,12 +38,13 @@ func TestCompareAppStateEmpty(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -51,6 +53,19 @@ func TestCompareAppStateEmpty(t *testing.T) { assert.Len(t, app.Status.Conditions, 0) } +// TestCompareAppStateRepoError tests the case when CompareAppState notices a repo error +func TestCompareAppStateRepoError(t *testing.T) { + app := newFakeApp() + ctrl := newFakeController(&fakeData{}, fmt.Errorf("test repo error")) + sources := make([]argoappv1.ApplicationSource, 0) + sources = append(sources, app.Spec.GetSource()) + revisions := make([]string, 0) + revisions = append(revisions, "") + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, compRes) + assert.EqualError(t, err, CompareStateRepoError.Error()) +} + // TestCompareAppStateNamespaceMetadataDiffers tests comparison when managed namespace metadata differs func TestCompareAppStateNamespaceMetadataDiffers(t *testing.T) { app := newFakeApp() @@ -75,12 +90,13 @@ func TestCompareAppStateNamespaceMetadataDiffers(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) @@ -122,12 +138,13 @@ func TestCompareAppStateNamespaceMetadataIsTheSame(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -149,12 +166,13 @@ func TestCompareAppStateMissing(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) @@ -180,12 +198,13 @@ func TestCompareAppStateExtra(t *testing.T) { key: pod, }, } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) assert.Equal(t, 1, len(compRes.resources)) @@ -210,12 +229,13 @@ func TestCompareAppStateHook(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) assert.Equal(t, 0, len(compRes.resources)) @@ -241,12 +261,13 @@ func TestCompareAppStateSkipHook(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) assert.Equal(t, 1, len(compRes.resources)) @@ -270,13 +291,14 @@ func TestCompareAppStateCompareOptionIgnoreExtraneous(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -303,12 +325,13 @@ func TestCompareAppStateExtraHook(t *testing.T) { key: pod, }, } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -331,12 +354,13 @@ func TestAppRevisionsSingleSource(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) app := newFakeApp() revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.NotEmpty(t, compRes.syncStatus.Revision) @@ -370,12 +394,13 @@ func TestAppRevisionsMultiSource(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) app := newFakeMultiSourceApp() revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources()) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Empty(t, compRes.syncStatus.Revision) @@ -417,12 +442,13 @@ func TestCompareAppStateDuplicatedNamespacedResources(t *testing.T) { kube.GetResourceKey(obj3): obj3, }, } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.Equal(t, 1, len(app.Status.Conditions)) @@ -457,8 +483,9 @@ func TestCompareAppStateManagedNamespaceMetadataWithLiveNsDoesNotGetPruned(t *te kube.GetResourceKey(ns): ns, }, } - ctrl := newFakeController(&data) - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, []string{}, app.Spec.Sources, false, false, nil, false) + ctrl := newFakeController(&data, nil) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, []string{}, app.Spec.Sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.Equal(t, 0, len(app.Status.Conditions)) @@ -512,13 +539,14 @@ func TestSetHealth(t *testing.T) { managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{ kube.GetResourceKey(deployment): deployment, }, - }) + }, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.Equal(t, health.HealthStatusHealthy, compRes.healthStatus.Status) } @@ -548,13 +576,14 @@ func TestSetHealthSelfReferencedApp(t *testing.T) { kube.GetResourceKey(deployment): deployment, kube.GetResourceKey(unstructuredApp): unstructuredApp, }, - }) + }, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.Equal(t, health.HealthStatusHealthy, compRes.healthStatus.Status) } @@ -574,7 +603,7 @@ func TestSetManagedResourcesWithOrphanedResources(t *testing.T) { AppName: "", }, }, - }) + }, nil) tree, err := ctrl.setAppManagedResources(app, &comparisonResult{managedResources: make([]managedResource, 0)}) @@ -603,7 +632,7 @@ func TestSetManagedResourcesWithResourcesOfAnotherApp(t *testing.T) { AppName: "app2", }, }, - }) + }, nil) tree, err := ctrl.setAppManagedResources(app1, &comparisonResult{managedResources: make([]managedResource, 0)}) @@ -622,13 +651,14 @@ func TestReturnUnknownComparisonStateOnSettingLoadError(t *testing.T) { configMapData: map[string]string{ "resource.customizations": "invalid setting", }, - }) + }, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.Equal(t, health.HealthStatusUnknown, compRes.healthStatus.Status) assert.Equal(t, argoappv1.SyncStatusCodeUnknown, compRes.syncStatus.Status) @@ -655,7 +685,7 @@ func TestSetManagedResourcesKnownOrphanedResourceExceptions(t *testing.T) { ResourceNode: argoappv1.ResourceNode{ResourceRef: argoappv1.ResourceRef{Kind: kube.ServiceAccountKind, Name: "kubernetes", Namespace: app.Namespace}}, }, }, - }) + }, nil) tree, err := ctrl.setAppManagedResources(app, &comparisonResult{managedResources: make([]managedResource, 0)}) @@ -668,7 +698,7 @@ func Test_appStateManager_persistRevisionHistory(t *testing.T) { app := newFakeApp() ctrl := newFakeController(&fakeData{ apps: []runtime.Object{app}, - }) + }, nil) manager := ctrl.appStateManager.(*appStateManager) setRevisionHistoryLimit := func(value int) { i := int64(value) @@ -763,12 +793,13 @@ func TestSignedResponseNoSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -789,12 +820,13 @@ func TestSignedResponseNoSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -820,12 +852,13 @@ func TestSignedResponseSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -846,12 +879,13 @@ func TestSignedResponseSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -872,12 +906,13 @@ func TestSignedResponseSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -898,12 +933,13 @@ func TestSignedResponseSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -925,14 +961,15 @@ func TestSignedResponseSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) testProj := signedProj testProj.Spec.SignatureKeys[0].KeyID = "4AEE18F83AFDEB24" sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &testProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &testProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -956,12 +993,13 @@ func TestSignedResponseSignatureRequired(t *testing.T) { } // it doesn't matter for our test whether local manifests are valid localManifests := []string{"foobar"} - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeUnknown, compRes.syncStatus.Status) @@ -985,12 +1023,13 @@ func TestSignedResponseSignatureRequired(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -1014,12 +1053,13 @@ func TestSignedResponseSignatureRequired(t *testing.T) { } // it doesn't matter for our test whether local manifests are valid localManifests := []string{""} - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "abc123") - compRes, _ := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &signedProj, revisions, sources, false, false, localManifests, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) @@ -1154,7 +1194,7 @@ func TestIsLiveResourceManaged(t *testing.T) { kube.GetResourceKey(unmanagedObjWrongGroup): unmanagedObjWrongGroup, kube.GetResourceKey(unmanagedObjWrongNamespace): unmanagedObjWrongNamespace, }, - }) + }, nil) manager := ctrl.appStateManager.(*appStateManager) appName := "guestbook" diff --git a/controller/sync.go b/controller/sync.go index dc9353f29a946..2b925b7782b9e 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -3,6 +3,7 @@ package controller import ( "context" "encoding/json" + goerrors "errors" "fmt" "os" "strconv" @@ -152,8 +153,13 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha revisions = []string{revision} } - // error does not matter as noRevisionCache is set to true - compareResult, _ := m.CompareAppState(app, proj, revisions, sources, false, true, syncOp.Manifests, app.Spec.HasMultipleSources()) + // ignore error if CompareStateRepoError, this shouldn't happen as noRevisionCache is true + compareResult, err := m.CompareAppState(app, proj, revisions, sources, false, true, syncOp.Manifests, app.Spec.HasMultipleSources()) + if err != nil && !goerrors.Is(err, CompareStateRepoError) { + state.Phase = common.OperationError + state.Message = err.Error() + return + } // We now have a concrete commit SHA. Save this in the sync result revision so that we remember // what we should be syncing to when resuming operations. diff --git a/controller/sync_test.go b/controller/sync_test.go index da68e5d9a3dfe..309f846ca6460 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -41,7 +41,7 @@ func TestPersistRevisionHistory(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) // Sync with source unspecified opState := &v1alpha1.OperationState{Operation: v1alpha1.Operation{ @@ -87,7 +87,7 @@ func TestPersistManagedNamespaceMetadataState(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) // Sync with source unspecified opState := &v1alpha1.OperationState{Operation: v1alpha1.Operation{ @@ -118,7 +118,7 @@ func TestPersistRevisionHistoryRollback(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) // Sync with source specified source := v1alpha1.ApplicationSource{ @@ -172,7 +172,7 @@ func TestSyncComparisonError(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) // Sync with source unspecified opState := &v1alpha1.OperationState{Operation: v1alpha1.Operation{ @@ -217,7 +217,7 @@ func TestAppStateManager_SyncAppState(t *testing.T) { }, managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) return &fixture{ project: project, diff --git a/docs/operator-manual/argocd-cmd-params-cm.yaml b/docs/operator-manual/argocd-cmd-params-cm.yaml index f9f8b3fc14d52..11661232f4ab5 100644 --- a/docs/operator-manual/argocd-cmd-params-cm.yaml +++ b/docs/operator-manual/argocd-cmd-params-cm.yaml @@ -58,6 +58,8 @@ data: controller.sharding.algorithm: legacy # Number of allowed concurrent kubectl fork/execs. Any value less than 1 means no limit. controller.kubectl.parallelism.limit: "20" + # Grace period in seconds for ignoring consecutive errors while communicating with repo server. + controller.repo.error.grace.period.seconds: "180" ## Server properties # Listen on given address for incoming connections (default "0.0.0.0") diff --git a/docs/operator-manual/server-commands/argocd-application-controller.md b/docs/operator-manual/server-commands/argocd-application-controller.md index a270de5465f40..e03cf7fc51536 100644 --- a/docs/operator-manual/server-commands/argocd-application-controller.md +++ b/docs/operator-manual/server-commands/argocd-application-controller.md @@ -55,7 +55,7 @@ argocd-application-controller [flags] --redis-insecure-skip-tls-verify Skip Redis server certificate validation. --redis-use-tls Use TLS when connecting to Redis. --redisdb int Redis database. - --repo-error-grace int Grace period in seconds for ignoring consecutive errors while communicating with repo server. (default 180) + --repo-error-grace-period-seconds int Grace period in seconds for ignoring consecutive errors while communicating with repo server. (default 180) --repo-server string Repo server address. (default "argocd-repo-server:8081") --repo-server-plaintext Disable TLS on connections to repo server --repo-server-strict-tls Whether to use strict validation of the TLS cert presented by the repo server diff --git a/manifests/base/application-controller/argocd-application-controller-statefulset.yaml b/manifests/base/application-controller/argocd-application-controller-statefulset.yaml index 33f02100a947a..0b7a230881c8e 100644 --- a/manifests/base/application-controller/argocd-application-controller-statefulset.yaml +++ b/manifests/base/application-controller/argocd-application-controller-statefulset.yaml @@ -35,6 +35,12 @@ spec: name: argocd-cm key: timeout.hard.reconciliation optional: true + - name: ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS + valueFrom: + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.error.grace.period.seconds + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/core-install.yaml b/manifests/core-install.yaml index 86e636e12ef59..8b438e9fa9961 100644 --- a/manifests/core-install.yaml +++ b/manifests/core-install.yaml @@ -21211,6 +21211,12 @@ spec: key: timeout.hard.reconciliation name: argocd-cm optional: true + - name: ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS + valueFrom: + configMapKeyRef: + key: controller.repo.error.grace.period.seconds + name: argocd-cmd-params-cm + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/ha/install.yaml b/manifests/ha/install.yaml index 043083de6be84..aa6df53f2e1a7 100644 --- a/manifests/ha/install.yaml +++ b/manifests/ha/install.yaml @@ -23013,6 +23013,12 @@ spec: key: timeout.hard.reconciliation name: argocd-cm optional: true + - name: ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS + valueFrom: + configMapKeyRef: + key: controller.repo.error.grace.period.seconds + name: argocd-cmd-params-cm + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/ha/namespace-install.yaml b/manifests/ha/namespace-install.yaml index 524ec8ace3e6c..1a52096e0bb1b 100644 --- a/manifests/ha/namespace-install.yaml +++ b/manifests/ha/namespace-install.yaml @@ -2669,6 +2669,12 @@ spec: key: timeout.hard.reconciliation name: argocd-cm optional: true + - name: ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS + valueFrom: + configMapKeyRef: + key: controller.repo.error.grace.period.seconds + name: argocd-cmd-params-cm + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/install.yaml b/manifests/install.yaml index a08c3a5cd1302..c1dc47ef21eb9 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -22057,6 +22057,12 @@ spec: key: timeout.hard.reconciliation name: argocd-cm optional: true + - name: ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS + valueFrom: + configMapKeyRef: + key: controller.repo.error.grace.period.seconds + name: argocd-cmd-params-cm + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index cfdf7756ff134..f247d6c0cac8c 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -1713,6 +1713,12 @@ spec: key: timeout.hard.reconciliation name: argocd-cm optional: true + - name: ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS + valueFrom: + configMapKeyRef: + key: controller.repo.error.grace.period.seconds + name: argocd-cmd-params-cm + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: configMapKeyRef: diff --git a/pkg/client/clientset/versioned/clientset.go b/pkg/client/clientset/versioned/clientset.go index 869b10d0f82d6..0c0911e0387c5 100644 --- a/pkg/client/clientset/versioned/clientset.go +++ b/pkg/client/clientset/versioned/clientset.go @@ -17,7 +17,8 @@ type Interface interface { ArgoprojV1alpha1() argoprojv1alpha1.ArgoprojV1alpha1Interface } -// Clientset contains the clients for groups. +// Clientset contains the clients for groups. Each group has exactly one +// version included in a Clientset. type Clientset struct { *discovery.DiscoveryClient argoprojV1alpha1 *argoprojv1alpha1.ArgoprojV1alpha1Client diff --git a/pkg/client/informers/externalversions/factory.go b/pkg/client/informers/externalversions/factory.go index 7d04eeb35ed52..57bd66c672490 100644 --- a/pkg/client/informers/externalversions/factory.go +++ b/pkg/client/informers/externalversions/factory.go @@ -31,11 +31,6 @@ type sharedInformerFactory struct { // startedInformers is used for tracking which informers have been started. // This allows Start() to be called multiple times safely. startedInformers map[reflect.Type]bool - // wg tracks how many goroutines were started. - wg sync.WaitGroup - // shuttingDown is true when Shutdown has been called. It may still be running - // because it needs to wait for goroutines. - shuttingDown bool } // WithCustomResyncConfig sets a custom resync period for the specified informer types. @@ -96,39 +91,20 @@ func NewSharedInformerFactoryWithOptions(client versioned.Interface, defaultResy return factory } +// Start initializes all requested informers. func (f *sharedInformerFactory) Start(stopCh <-chan struct{}) { f.lock.Lock() defer f.lock.Unlock() - if f.shuttingDown { - return - } - for informerType, informer := range f.informers { if !f.startedInformers[informerType] { - f.wg.Add(1) - // We need a new variable in each loop iteration, - // otherwise the goroutine would use the loop variable - // and that keeps changing. - informer := informer - go func() { - defer f.wg.Done() - informer.Run(stopCh) - }() + go informer.Run(stopCh) f.startedInformers[informerType] = true } } } -func (f *sharedInformerFactory) Shutdown() { - f.lock.Lock() - f.shuttingDown = true - f.lock.Unlock() - - // Will return immediately if there is nothing to wait for. - f.wg.Wait() -} - +// WaitForCacheSync waits for all started informers' cache were synced. func (f *sharedInformerFactory) WaitForCacheSync(stopCh <-chan struct{}) map[reflect.Type]bool { informers := func() map[reflect.Type]cache.SharedIndexInformer { f.lock.Lock() @@ -175,57 +151,10 @@ func (f *sharedInformerFactory) InformerFor(obj runtime.Object, newFunc internal // SharedInformerFactory provides shared informers for resources in all known // API group versions. -// -// It is typically used like this: -// -// ctx, cancel := context.Background() -// defer cancel() -// factory := NewSharedInformerFactory(client, resyncPeriod) -// defer factory.WaitForStop() // Returns immediately if nothing was started. -// genericInformer := factory.ForResource(resource) -// typedInformer := factory.SomeAPIGroup().V1().SomeType() -// factory.Start(ctx.Done()) // Start processing these informers. -// synced := factory.WaitForCacheSync(ctx.Done()) -// for v, ok := range synced { -// if !ok { -// fmt.Fprintf(os.Stderr, "caches failed to sync: %v", v) -// return -// } -// } -// -// // Creating informers can also be created after Start, but then -// // Start must be called again: -// anotherGenericInformer := factory.ForResource(resource) -// factory.Start(ctx.Done()) type SharedInformerFactory interface { internalinterfaces.SharedInformerFactory - - // Start initializes all requested informers. They are handled in goroutines - // which run until the stop channel gets closed. - Start(stopCh <-chan struct{}) - - // Shutdown marks a factory as shutting down. At that point no new - // informers can be started anymore and Start will return without - // doing anything. - // - // In addition, Shutdown blocks until all goroutines have terminated. For that - // to happen, the close channel(s) that they were started with must be closed, - // either before Shutdown gets called or while it is waiting. - // - // Shutdown may be called multiple times, even concurrently. All such calls will - // block until all goroutines have terminated. - Shutdown() - - // WaitForCacheSync blocks until all started informers' caches were synced - // or the stop channel gets closed. - WaitForCacheSync(stopCh <-chan struct{}) map[reflect.Type]bool - - // ForResource gives generic access to a shared informer of the matching type. ForResource(resource schema.GroupVersionResource) (GenericInformer, error) - - // InternalInformerFor returns the SharedIndexInformer for obj using an internal - // client. - InformerFor(obj runtime.Object, newFunc internalinterfaces.NewInformerFunc) cache.SharedIndexInformer + WaitForCacheSync(stopCh <-chan struct{}) map[reflect.Type]bool Argoproj() application.Interface } From dc564187dd8cdab8f4fb39fff6dfbc4008cdf4be Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Mon, 30 Oct 2023 18:26:03 +0530 Subject: [PATCH 5/6] feat: update unit test Signed-off-by: Soumya Ghosh Dastidar --- controller/appcontroller_test.go | 2 +- controller/state_test.go | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index bbfdafcd104f3..9c2933feeb6df 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -124,7 +124,7 @@ func newFakeController(data *fakeData, repoErr error) *ApplicationController { time.Minute, time.Hour, time.Minute, - time.Second*30, + time.Second*10, common.DefaultPortArgoCDMetrics, data.metricsCacheExpiration, []string{}, diff --git a/controller/state_test.go b/controller/state_test.go index 0ca3cfd166822..9d2df1f239185 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -56,7 +56,7 @@ func TestCompareAppStateEmpty(t *testing.T) { // TestCompareAppStateRepoError tests the case when CompareAppState notices a repo error func TestCompareAppStateRepoError(t *testing.T) { app := newFakeApp() - ctrl := newFakeController(&fakeData{}, fmt.Errorf("test repo error")) + ctrl := newFakeController(&fakeData{manifestResponses: make([]*apiclient.ManifestResponse, 3)}, fmt.Errorf("test repo error")) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) @@ -64,6 +64,18 @@ func TestCompareAppStateRepoError(t *testing.T) { compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) assert.Nil(t, compRes) assert.EqualError(t, err, CompareStateRepoError.Error()) + + // expect to still get compare state error to as inside grace period + compRes, err = ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, compRes) + assert.EqualError(t, err, CompareStateRepoError.Error()) + + time.Sleep(10 * time.Second) + // expect to not get error as outside of grace period, but status should be unknown + compRes, err = ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.NotNil(t, compRes) + assert.Nil(t, err) + assert.Equal(t, compRes.syncStatus.Status, argoappv1.SyncStatusCodeUnknown) } // TestCompareAppStateNamespaceMetadataDiffers tests comparison when managed namespace metadata differs From fc8a56facf59647102ac25034130bd639ef5c7eb Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Mon, 30 Oct 2023 21:09:19 +0530 Subject: [PATCH 6/6] fix: codegen Signed-off-by: Soumya Ghosh Dastidar --- pkg/client/clientset/versioned/clientset.go | 5 +- .../informers/externalversions/factory.go | 81 +++++++++++++++++-- 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/pkg/client/clientset/versioned/clientset.go b/pkg/client/clientset/versioned/clientset.go index 0c0911e0387c5..8d98fafcf4d5a 100644 --- a/pkg/client/clientset/versioned/clientset.go +++ b/pkg/client/clientset/versioned/clientset.go @@ -17,8 +17,7 @@ type Interface interface { ArgoprojV1alpha1() argoprojv1alpha1.ArgoprojV1alpha1Interface } -// Clientset contains the clients for groups. Each group has exactly one -// version included in a Clientset. +// Clientset contains the clients for groups. type Clientset struct { *discovery.DiscoveryClient argoprojV1alpha1 *argoprojv1alpha1.ArgoprojV1alpha1Client @@ -102,4 +101,4 @@ func New(c rest.Interface) *Clientset { cs.DiscoveryClient = discovery.NewDiscoveryClient(c) return &cs -} +} \ No newline at end of file diff --git a/pkg/client/informers/externalversions/factory.go b/pkg/client/informers/externalversions/factory.go index 57bd66c672490..39596fa531460 100644 --- a/pkg/client/informers/externalversions/factory.go +++ b/pkg/client/informers/externalversions/factory.go @@ -31,6 +31,11 @@ type sharedInformerFactory struct { // startedInformers is used for tracking which informers have been started. // This allows Start() to be called multiple times safely. startedInformers map[reflect.Type]bool + // wg tracks how many goroutines were started. + wg sync.WaitGroup + // shuttingDown is true when Shutdown has been called. It may still be running + // because it needs to wait for goroutines. + shuttingDown bool } // WithCustomResyncConfig sets a custom resync period for the specified informer types. @@ -91,20 +96,39 @@ func NewSharedInformerFactoryWithOptions(client versioned.Interface, defaultResy return factory } -// Start initializes all requested informers. func (f *sharedInformerFactory) Start(stopCh <-chan struct{}) { f.lock.Lock() defer f.lock.Unlock() + if f.shuttingDown { + return + } + for informerType, informer := range f.informers { if !f.startedInformers[informerType] { - go informer.Run(stopCh) + f.wg.Add(1) + // We need a new variable in each loop iteration, + // otherwise the goroutine would use the loop variable + // and that keeps changing. + informer := informer + go func() { + defer f.wg.Done() + informer.Run(stopCh) + }() f.startedInformers[informerType] = true } } } -// WaitForCacheSync waits for all started informers' cache were synced. +func (f *sharedInformerFactory) Shutdown() { + f.lock.Lock() + f.shuttingDown = true + f.lock.Unlock() + + // Will return immediately if there is nothing to wait for. + f.wg.Wait() +} + func (f *sharedInformerFactory) WaitForCacheSync(stopCh <-chan struct{}) map[reflect.Type]bool { informers := func() map[reflect.Type]cache.SharedIndexInformer { f.lock.Lock() @@ -151,14 +175,61 @@ func (f *sharedInformerFactory) InformerFor(obj runtime.Object, newFunc internal // SharedInformerFactory provides shared informers for resources in all known // API group versions. +// +// It is typically used like this: +// +// ctx, cancel := context.Background() +// defer cancel() +// factory := NewSharedInformerFactory(client, resyncPeriod) +// defer factory.WaitForStop() // Returns immediately if nothing was started. +// genericInformer := factory.ForResource(resource) +// typedInformer := factory.SomeAPIGroup().V1().SomeType() +// factory.Start(ctx.Done()) // Start processing these informers. +// synced := factory.WaitForCacheSync(ctx.Done()) +// for v, ok := range synced { +// if !ok { +// fmt.Fprintf(os.Stderr, "caches failed to sync: %v", v) +// return +// } +// } +// +// // Creating informers can also be created after Start, but then +// // Start must be called again: +// anotherGenericInformer := factory.ForResource(resource) +// factory.Start(ctx.Done()) type SharedInformerFactory interface { internalinterfaces.SharedInformerFactory - ForResource(resource schema.GroupVersionResource) (GenericInformer, error) + + // Start initializes all requested informers. They are handled in goroutines + // which run until the stop channel gets closed. + Start(stopCh <-chan struct{}) + + // Shutdown marks a factory as shutting down. At that point no new + // informers can be started anymore and Start will return without + // doing anything. + // + // In addition, Shutdown blocks until all goroutines have terminated. For that + // to happen, the close channel(s) that they were started with must be closed, + // either before Shutdown gets called or while it is waiting. + // + // Shutdown may be called multiple times, even concurrently. All such calls will + // block until all goroutines have terminated. + Shutdown() + + // WaitForCacheSync blocks until all started informers' caches were synced + // or the stop channel gets closed. WaitForCacheSync(stopCh <-chan struct{}) map[reflect.Type]bool + // ForResource gives generic access to a shared informer of the matching type. + ForResource(resource schema.GroupVersionResource) (GenericInformer, error) + + // InternalInformerFor returns the SharedIndexInformer for obj using an internal + // client. + InformerFor(obj runtime.Object, newFunc internalinterfaces.NewInformerFunc) cache.SharedIndexInformer + Argoproj() application.Interface } func (f *sharedInformerFactory) Argoproj() application.Interface { return application.New(f, f.namespace, f.tweakListOptions) -} +} \ No newline at end of file