feat(appset): add configurable max reconciliations to appset #13381

gdsoumya · 2023-05-01T00:31:49Z

This PR:

Adds the ability to configure max concurrent reconciles for appset controller (depends on feat(appset): applicationset controller use repo server (#10952) #12714)
Updated appset env vars to use appset config for repo server instead of reusing app controller config

Checklist:

Please see Contribution FAQs if you have questions about your pull-request.

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

gdsoumya · 2023-05-01T00:33:16Z

manifests/base/applicationset-controller/argocd-applicationset-controller-deployment.yaml

+            - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT
              valueFrom:
                configMapKeyRef:
                  name: argocd-cmd-params-cm
-                  key: controller.repo.server.plaintext
+                  key: applicationsetcontroller.repo.server.plaintext
                  optional: true
-            - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_STRICT_TLS
+            - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_STRICT_TLS
              valueFrom:
                configMapKeyRef:
                  name: argocd-cmd-params-cm
-                  key: controller.repo.server.strict.tls
+                  key: applicationsetcontroller.repo.server.strict.tls
+                  optional: true
+            - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_TIMEOUT_SECONDS
+              valueFrom:
+                configMapKeyRef:
+                  name: argocd-cmd-params-cm
+                  key: applicationsetcontroller.repo.server.timeout.seconds
+                  optional: true
+            - name: ARGOCD_APPLICATIONSET_CONTROLLER_CONCURRENT_RECONCILIATIONS
+              valueFrom:
+                configMapKeyRef:
+                  name: argocd-cmd-params-cm
+                  key: applicationsetcontroller.concurrent.reconciliations.max


updated the env var names from app controller to appset controller and also made them point to appset specific config instead of re-using app controller config.

gdsoumya · 2023-05-01T00:42:18Z

cmd/argocd-applicationset-controller/commands/applicationset_controller.go

+	command.Flags().BoolVar(&repoServerPlaintext, "repo-server-plaintext", env.ParseBoolFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT", false), "Disable TLS on connections to repo server")
+	command.Flags().BoolVar(&repoServerStrictTLS, "repo-server-strict-tls", env.ParseBoolFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_STRICT_TLS", false), "Whether to use strict validation of the TLS cert presented by the repo server")
+	command.Flags().IntVar(&repoServerTimeoutSeconds, "repo-server-timeout-seconds", env.ParseNumFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_TIMEOUT_SECONDS", 60, 0, math.MaxInt64), "Repo server RPC call timeout seconds.")
+	command.Flags().IntVar(&maxConcurrentReconciliations, "concurrent-reconciliations", env.ParseNumFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_CONCURRENT_RECONCILIATIONS", 10, 1, 100), "Max concurrent reconciliations limit for the controller")


set the default to 10 and max to 100 for now, let me know if that needs to be changed.

codecov · 2023-05-01T02:08:12Z

Codecov Report

Patch coverage: 63.63% and no project coverage change.

Comparison is base (064c8da) 49.14% compared to head (e6d046e) 49.15%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #13381   +/-   ##
=======================================
  Coverage   49.14%   49.15%           
=======================================
  Files         248      248           
  Lines       42891    42900    +9     
=======================================
+ Hits        21079    21087    +8     
- Misses      19693    19694    +1     
  Partials     2119     2119

Impacted Files	Coverage Δ
...cationset/controllers/applicationset_controller.go	`63.37% <20.00%> (-0.07%)`	⬇️
reposerver/repository/repository.go	`60.45% <100.00%> (+0.16%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

alexmt

LGTM!

rumstead · 2023-05-24T14:53:08Z

Chances this could be in 2.8 with the other repo server changes for caching?

…j#13381) * feat(appset): add configurable max reconciliations to appset Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com> * fix: lint errors Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com> --------- Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

vincentgna · 2023-08-23T03:54:35Z

manifests/base/applicationset-controller/argocd-applicationset-controller-deployment.yaml

              valueFrom:
                configMapKeyRef:
                  name: argocd-cmd-params-cm
-                  key: controller.repo.server.plaintext
+                  key: applicationsetcontroller.repo.server.plaintext


I think we should add docs for ppl upgrading from 2.7 to 2.8 running without in-cluster TLS (this seems to have broken applicationset controller with error "transport: authentication handshake failed: tls: first record does not look like a TLS handshake" )

didn't see a notice about this cmd param config change here
https://argo-cd.readthedocs.io/en/stable/operator-manual/upgrading/2.7-2.8/

and missing from the sample in docs
https://github.com/argoproj/argo-cd/blob/v2.8.1/docs/operator-manual/argocd-cmd-params-cm.yaml

feat(appset): add configurable max reconciliations to appset

f3cd7bd

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

gdsoumya commented May 1, 2023

View reviewed changes

fix: lint errors

51367b7

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

gdsoumya force-pushed the feat/appset_parallelization branch from eba9f2a to 51367b7 Compare May 1, 2023 02:15

gdsoumya marked this pull request as ready for review May 1, 2023 15:16

gdsoumya requested review from ishitasequeira, crenshaw-dev and alexmt and removed request for ishitasequeira May 2, 2023 02:46

Merge branch 'master' into feat/appset_parallelization

e6d046e

alexmt approved these changes May 5, 2023

View reviewed changes

alexmt merged commit 56d1733 into argoproj:master May 5, 2023
24 checks passed

blakepettersson mentioned this pull request May 29, 2023

Add reconciliation configuration for ApplicationSet #13047

Open

vincentgna reviewed Aug 23, 2023

View reviewed changes

so0k mentioned this pull request Aug 23, 2023

Document decoupling of ApplicationSet controller cmd parameters #15181

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appset): add configurable max reconciliations to appset #13381

feat(appset): add configurable max reconciliations to appset #13381

gdsoumya commented May 1, 2023 •

edited

gdsoumya May 1, 2023 •

edited

gdsoumya May 1, 2023

codecov bot commented May 1, 2023 •

edited

alexmt left a comment

rumstead commented May 24, 2023

vincentgna Aug 23, 2023 •

edited

vincentgna Aug 23, 2023

feat(appset): add configurable max reconciliations to appset #13381

feat(appset): add configurable max reconciliations to appset #13381

Conversation

gdsoumya commented May 1, 2023 • edited

gdsoumya May 1, 2023 • edited

Choose a reason for hiding this comment

gdsoumya May 1, 2023

Choose a reason for hiding this comment

codecov bot commented May 1, 2023 • edited

Codecov Report

alexmt left a comment

Choose a reason for hiding this comment

rumstead commented May 24, 2023

vincentgna Aug 23, 2023 • edited

Choose a reason for hiding this comment

vincentgna Aug 23, 2023

Choose a reason for hiding this comment

gdsoumya commented May 1, 2023 •

edited

gdsoumya May 1, 2023 •

edited

codecov bot commented May 1, 2023 •

edited

vincentgna Aug 23, 2023 •

edited