fix: Increase discovery config burst rate

[mgruener]

This change increases the api call burst rate for the discovery client config.

On clusters with lots of CRDs (which is way more common these days), the default discovery burst rate of 100 is not enough, leading to massive slowdowns during sync operations because the client side rate-limiter kicks in (indicated by lots of "Waited for X.XXs due to client-side throttling, not priority and fairness").

During my tests with our OpenShift clusters (which have a lot of CRDs by default; our current setup has ~250), the creation of 100 ConfigMaps took 4:03 minutes (tested multiple times with different sync settings) without this change. After implementing this change, the time went down to 5 seconds.

In its own "oc" Kubernetes client for OpenShift, Redhat uses a burst rate of 250 (see https://bugzilla.redhat.com/show_bug.cgi?id=1899575) but in our experience, even this is not enough. Hence the value of 350 in this PR.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[sreber84]

Hi @mgruener,

It should be possible to set this already using ARGOCD_K8S_CLIENT_BURST environment variable in the controller spec. So basically something like the below should help achieve what you are proposing via code change,

spec:
  controller:
     env:
     - name: ARGOCD_K8S_CLIENT_BURST
       value: 300

Also it seems that documentation for this feature is still lacking. But there is argoproj/argo-cd#8527 which aims to improve this and which also shows what other options are available to configure kubernetes API options.

[mgruener]

Hi @sreber84,

if nothing has fundamentally changed since I opened this PR, the env var you mentioned only affects the k8s client used by ArgoCD to access its host cluster to interact with its CRs (Applications, AppProjects ...). The value of ARGOCD_K8S_CLIENT_BURST is not passed to the gitops-engine code, which has its own k8s client factory to create k8s clients based on the cluster secrets. These k8s clients are then used to apply the actual managed k8s resources to the target clusters. This PR targets the k8s clients used by the gitops-engine (ok, this is probably obvious given the PR is in the gitops-engine repo :D).
An alternative approach to implement this would have been to actually ensure the value of the ARGOCD_K8S_CLIENT_* vars are passed down to the gitops-engine code, but this would have been way more complex (and maybe not even what the ARGOCD_K8S_CLIENT_* env vars were originally designed to do, but that is an assumption).

[jannfis]

if nothing has fundamentally changed since I opened this PR

I guess there has been something changed, because there was a bug with evaluating those specific environment variables, which has been fixed with argoproj/argo-cd#7779 on Nov 25th, and this fix was released with v2.1.8 and v2.2.0. So effectively, the environment variables had no effect prior to this.

The value of ARGOCD_K8S_CLIENT_BURST is not passed to the gitops-engine code

This is not completely correct. gitops-engine receives a REST client configuration from Argo CD for performing operations on the cluster.

Can you please test again with ARGOCD_K8S_CLIENT_BURST (and possibly, ARGOCD_K8S_CLIENT_QPS) set to an appropriate value?

[jannfis]

OK, so in another discussion with someone who had a similar issue, we found out that the burst and QPS settings are indeed not passed to the discovery client.

The reason for this is that we still pass the REST configuration that we used for writing out a kubeconfig to the sync context here

gitops-engine/pkg/sync/sync_context.go

Line 205 in 61c0cc7

resourceOps, cleanup, err := kubectl.ManageResources(rawConfig, openAPISchema)

@alexmt Can you please verify? Would it make more sense to pass restConfig instead of rawConfig here? AFAIU, rawConfig should be deprecated ever since we moved away from forking kubectl.

[alexmt]

@jannfis thanks for bringing this up. rawConfig was required because GitOps engine was used to generate kubeconfig file. So we had to use the original "raw" config to support AWS authentication. Now we should be able to just use the same config

[jannfis]

Since there was no more activity on this PR, I decided to submit #395, which supersedes this one and allows the QPS and burst-rate to be configurable.

Are you OK with closing this, @mgruener ?

[mgruener]

@jannfis absolutely. If the issue is fixed in the end I am happy 😄Having it configurable is the better solution anyway. Closing.