Persist resource health in Redis instead of the Application CR by default

[crenshaw-dev]

Summary

As of 2.5, we'll have the option to persist resource health statuses in Redis instead of in the Application CRD. In 3.0, we should default to persisting in Redis.

Motivation

  # Specifies if resource health should be persisted in app CRD (default true)
  # Changing this to `false` significantly reduce number of Application CRD updates and improves controller performance.
  # However, disabling resource health by default might affect applications that communicate with Applications CRD directly
  # so we have to defer switching this to `false` by default till v3.0 release.
  controller.resource.health.persist: "true"

Proposal

Flip the switch. Maybe even remove support for CRD persistence entirely.

[csantanapr]

Minor clarification is Application CR not Application CRD
I agree with the statement

Flip the switch. Maybe even remove support for ArgoCD CR persistence entirely.

Saving the status in the CR/etcd has a performance impact, end user like Adobe had to switch the default to redis to improve scalability in their recent talk at GitOpsCon NA 2024 https://youtu.be/MDXrc_cLVns?t=404

[rumstead]

Will this break the applicationset controller's progressive sync functionality?

[crenshaw-dev]

@rumstead good question. I think the answer is "yes." One option would be to add an option that only persists top-level health changes to the status instead of changes for individual resources. Another option would be to modify the AppSet controller to pull the current health status from Redis.

[rumstead]

took me forever to find when the resource health cache was created - #1120

[rumstead]

Interested in everyone's thoughts on this overall approach before I go test.

#21455

[MrFreezeex]

I am not entirely sure this would work out with notification/notification controllers too. AFAIU the notification controller rely on the Application CR to be updated to be able to send notification in "real" time about sync event 🤔. Sorry if that's already taken care of, just wondering how this could actually works 😅.

[rumstead]

I am not entirely sure this would work out with notification/notification controllers too. AFAIU the notification controller rely on the Application CR to be updated to be able to send notification in "real" time about sync event 🤔. Sorry if that's already taken care of, just wondering how this could actually works 😅.

That is a good shout but I don't believe the notification controller looks at the health status of the individual resources, just the overall application health / sync status.

E.g., the diff in the status would look something like this:

persis = true (the default)

status:
  health:
    status: Healthy
  resources:
  - group: apps
    health:
      status: Healthy
    kind: Deployment
    name: demo-app-kust
    namespace: cluster-generator
    status: OutOfSync
    version: v1
  sync:
    status: OutOfSync

persist = false

# snippet CR status
status:
  health:
    status: Healthy
  resourceHealthSource: appTree
  resources:
  - group: apps
    kind: Deployment
    name: demo-app-kust
    namespace: cluster-generator
    status: OutOfSync
    version: v1
  sync:
    status: OutOfSync

Where persist = false is missing status.resources[].health

[rumstead]

@rumstead good question. I think the answer is "yes." One option would be to add an option that only persists top-level health changes to the status instead of changes for individual resources. Another option would be to modify the AppSet controller to pull the current health status from Redis.

Ok after looking into this deeper, progressive syncs won't be impacted. Progressive syncs only care about the application status. The application status depends on the underlying resources but the application controller handles properly setting it.