Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: grace period for repo errors to prevent aggressive unknown sync state #16085

Merged
merged 11 commits into from
Nov 2, 2023
Merged

feat: grace period for repo errors to prevent aggressive unknown sync state #16085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ba684eb
feat: added grace period for repo errors to prevent aggressive sync s…
gdsoumya Oct 24, 2023
b0208aa
Merge branch 'master' of github.com:argoproj/argo-cd into feat/retryG…
gdsoumya Oct 24, 2023
36c7a57
feat: updated docs
gdsoumya Oct 24, 2023
2a72af2
Merge branch 'master' into feat/retryGitErrors
gdsoumya Oct 24, 2023
b81f8fb
fix: e2e test
gdsoumya Oct 24, 2023
9992f2c
Merge branch 'master' into feat/retryGitErrors
gdsoumya Oct 26, 2023
04782a9
feat: resolved review comments
gdsoumya Oct 30, 2023
ded3e2c
Merge branch 'master' into feat/retryGitErrors
gdsoumya Oct 30, 2023
dc56418
feat: update unit test
gdsoumya Oct 30, 2023
fc8a56f
fix: codegen
gdsoumya Oct 30, 2023
c813525
Merge branch 'master' into feat/retryGitErrors
gdsoumya Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/argocd-application-controller/commands/argocd_application_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ func NewCommand() *cobra.Command {
clientConfig clientcmd.ClientConfig
appResyncPeriod int64
appHardResyncPeriod int64
repoErrorGracePeriod int64
repoServerAddress string
repoServerTimeoutSeconds int
selfHealTimeoutSeconds int
Expand Down Expand Up @@ -155,6 +156,7 @@ func NewCommand() *cobra.Command {
resyncDuration,
hardResyncDuration,
time.Duration(selfHealTimeoutSeconds)*time.Second,
time.Duration(repoErrorGracePeriod)*time.Second,
metricsPort,
metricsCacheExpiration,
metricsAplicationLabels,
Expand Down Expand Up @@ -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-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")
Expand Down
7 changes: 5 additions & 2 deletions cmd/argocd/commands/admin/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,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 {
Expand Down Expand Up @@ -419,7 +419,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,
Expand Down
10 changes: 8 additions & 2 deletions controller/appcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ func NewApplicationController(
appResyncPeriod time.Duration,
appHardResyncPeriod time.Duration,
selfHealTimeout time.Duration,
repoErrorGracePeriod time.Duration,
metricsPort int,
metricsCacheExpiration time.Duration,
metricsApplicationLabels []string,
Expand Down Expand Up @@ -259,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
Expand Down Expand Up @@ -1512,10 +1513,15 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
}
now := metav1.Now()

compareResult := ctrl.appStateManager.CompareAppState(app, project, revisions, sources,
compareResult, err := ctrl.appStateManager.CompareAppState(app, project, revisions, sources,
refreshType == appv1.RefreshTypeHard,
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
crenshaw-dev marked this conversation as resolved.
Show resolved Hide resolved
}

Comment on lines +1516 to +1524
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about introducing an error return value from CompareAppState method signature for the sole purpose of CompareStateRepoError feels unclean. I think it's pretty specialized, and the fact that we only return error in this circumstance is going to get lost over time.

Instead of introducing error for this one purpose, what if we return nil, (which is what you are already doing the PR). The caller should interpret a nil return value as indication comparison should be skipped/short-circuted. This way, it doesn't affect the method signature and the semantics seem cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially planned to do that, where we returned nil but then decided against it and made it more specific because in the future if the code changes and there's other cases where nil is returned we might forget to update this logic so I chose to go with a specific error for this case.

for k, v := range compareResult.timings {
logCtx = logCtx.WithField(k, v.Milliseconds())
}
Expand Down
Loading
Loading