chore: infer managed resources health from redis instead of storing it in CRD #10191

alexmt · 2022-08-04T00:22:08Z

Signed-off-by: Alexander Matyushentsev AMatyushentsev@gmail.com

Argo CD historically stores managed resources' health details in Application CRD.

At some point, Argo CD started collecting health status for the whole resource hierarchy so now health status is duplicated in CRD and decided to store new health status in Redis to reduce application CRD side and reduce the number of updates.

This PR is the continuation of the original effort and introduces the following changes:

controller no longer stores health status in app CRD
API server infers health status from Redis and populate health status fields while serving application API requests

The change slightly reduces Application CRD but, most importantly, significantly reduces the number of application CRD updates. E.g. before, an application that manages Deployment with 10 pods would need to be updated 10+ times during rollout. The only change would be the change in health messages such as Waiting for rollout to finish: 7 of 10 updated replicas are available.... With changes, the application CRD would be updated just once.

For backward compatibility controller, by default, keep storing health status in CRD. The new behavior can be enabled using ARGOCD_PERSIST_RESOURCE_STATUS_HEALTH=true env variable.

codecov · 2022-08-04T00:34:07Z

Codecov Report

Attention: Patch coverage is 74.46809% with 12 lines in your changes missing coverage. Please review.

Project coverage is 45.94%. Comparing base (42863a4) to head (46cdd7d).
Report is 2619 commits behind head on master.

Files	Patch %	Lines
server/application/application.go	45.45%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10191      +/-   ##
==========================================
+ Coverage   45.92%   45.94%   +0.01%     
==========================================
  Files         229      229              
  Lines       28258    28268      +10     
==========================================
+ Hits        12978    12987       +9     
  Misses      13510    13510              
- Partials     1770     1771       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crenshaw-dev

Should docs be added explaining the benefit of using the option? Is there any plan to deprecate the CRD-storage mechanism?

Also I'm not sure if this is really testable... seems like maybe at least inferResourcesStatusHealth should have a test.

common/common.go

controller/health.go

crenshaw-dev · 2022-08-08T19:21:39Z

server/application/application.go

@@ -1897,6 +1905,22 @@ func (s *Server) GetApplicationSyncWindows(ctx context.Context, q *application.A
 	return res, nil
 }

+func (s *Server) inferResourcesStatusHealth(app *v1alpha1.Application) {
+	if !argocommon.PersistResourceStatusHealth {


Should this boolean be part of the Application cache key?

The upside would be that, after someone toggles on the option, there's no moment when health statuses are not returned.

The downside would be that, after someone toggles on the option, the entire Application cache is now out of date.

I've got, hopefully, a good idea: introduced resourceHealthSource field in application status that indicates if resources health is in CRD (by default) or in application tree.

This eliminates the requirement to coordinate new flag value between application controller & API server. Another benefit is that health status will be gradually removed from CRDs by controller.

jannfis

I think we should not remove the health status from the CRD, as many folks building solutions around Argo CD may use this information to feed their own systems instead of going through the API (which imho, is a perfectly fine use case).

So from my point of view, this is a heavily breaking change.

Just putting my Request for Change on here to prevent early merge and to facilitate further discussion on this :)

crenshaw-dev · 2022-08-10T19:06:09Z

I think Alex intends this feature to be opt-in rather than opt-out. If we find it super valuable, we could consider enabling it by default in 3.0. But as the PR is written now, I don't think it's a breaking change.

jannfis · 2022-08-10T19:42:13Z

Yeah, I was a little quick with making my assumption. Sorry - I'm tired and stupid :)

You're right. It's even mentioned it in the PR's description.

crenshaw-dev · 2022-08-10T20:45:18Z

I'm tired and stupid

Nah, just tired. And I can relate. 😴

alexmt · 2022-08-10T22:01:03Z

Thank you for review @jannfis and @crenshaw-dev ! PTAL

crenshaw-dev · 2022-08-12T16:00:35Z

@alexmt conflict time. :-)

crenshaw-dev

One nitpick!

Should there be a note somewhere in documentation about when/why this might be helpful (and that we might make it the default in 3.0, if that's the plan)?

cmd/argocd-application-controller/commands/argocd_application_controller.go

alexmt · 2022-08-12T18:45:19Z

Should there be a note somewhere in documentation about when/why this might be helpful (and that we might make it the default in 3.0, if that's the plan)?

I could not find and dedicated file that would be appropriate. So documented the setting in argocd-cmd-params-cm.yaml

crenshaw-dev

lgtm!

crenshaw-dev · 2022-08-15T20:17:31Z

@jannfis PTAL

jannfis

LGTM!

…t in CRD Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

…t in CRD (argoproj#10191) * chore: infer managed resources health from redis instead of storing it in CRD Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * apply reviewer notes Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: Yi Cai <yicai@redhat.com>

* fix: missing actions (#10327) (#10359) Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: Yi Cai <yicai@redhat.com> * chore: infer managed resources health from redis instead of storing it in CRD (#10191) * chore: infer managed resources health from redis instead of storing it in CRD Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * apply reviewer notes Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: Yi Cai <yicai@redhat.com> * fix: Add logic to handle for fileHandle.Close() (#9963) (#10361) Signed-off-by: xin.li <xin.li@daocloud.io> Signed-off-by: xin.li <xin.li@daocloud.io> Signed-off-by: Yi Cai <yicai@redhat.com> * docs: fix typo in upgrade notes (#10377) Signed-off-by: Xijun Dai <daixijun1990@gmail.com> Signed-off-by: Xijun Dai <daixijun1990@gmail.com> Signed-off-by: Yi Cai <yicai@redhat.com> * fix: add space before prompt in CLI (#10362) Signed-off-by: xin.li <xin.li@daocloud.io> Signed-off-by: xin.li <xin.li@daocloud.io> Signed-off-by: Yi Cai <yicai@redhat.com> * docs: fix indentation of example AppProject in 'Sync Windows' documentation (#10388) Signed-off-by: Yi Cai <yicai@redhat.com> * fix: Correctly assume cluster-scoped resources to be self-referenced (#10390) Signed-off-by: jannfis <jann@mistrust.net> Signed-off-by: jannfis <jann@mistrust.net> Signed-off-by: Yi Cai <yicai@redhat.com> * ui-make-https-repo-credential-editable Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> * Minor format fix Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> * Minor fix for unclickable input field Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> * Updates for comments Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> * ui-make-https-repo-credential-editable Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> * Minor format fix Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> * Minor fix for unclickable input field Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> * Updates for comments Signed-off-by: ciiay <yicai@redhat.com> Signed-off-by: Yi Cai <yicai@redhat.com> Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: Yi Cai <yicai@redhat.com> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: xin.li <xin.li@daocloud.io> Signed-off-by: Xijun Dai <daixijun1990@gmail.com> Signed-off-by: jannfis <jann@mistrust.net> Signed-off-by: ciiay <yicai@redhat.com> Co-authored-by: Michael Crenshaw <michael@crenshaw.dev> Co-authored-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: my-git9 <xin.li@daocloud.io> Co-authored-by: Xijun Dai <daixijun1990@gmail.com> Co-authored-by: Jun Duan <jun.duan@ibm.com> Co-authored-by: jannfis <jann@mistrust.net>

sbose78 · 2022-09-22T01:07:48Z

I think we should not remove the health status from the CRD, as many folks building solutions around Argo CD may use this information to feed their own systems

+1 on this.

dlemfh · 2022-11-09T09:19:03Z

Hey @alexmt (and community)!

May I ask, will setting controller.resource.health.persist: 'false' change the outcome of argocd app get <application_name> commands?

I want to use the benefits of the flag, but my ci/cd pipeline depends on resource health outputs of argocd app get <application_name>.

I've confirmed that using the false setting does indeed remove resource health statuses from the Application CRD, but I've also checked (from eye-reading) that the output of argocd app get <application_name> stays the same and still provides resource health status info (on argocd v2.5.2).

Can I trust that moving forward (even in v3.0), argocd cli output will stay the same? (Or at least that change in cli behavior will be properly documented/announced?)

Thanks always! 🤩

alexmt · 2022-11-09T18:01:10Z

Hello @dlemfh ,

CLI/API behavior won't change after setting controller.resource.health.persist: 'false'. API server retrieves health information from Redis.

You can assume that v2.x won't have breaking API/CLI changes. v3.0 might introduce it but we will make sure to notify about such change ahead of time and describe migration path. Currently there are no plans to introduce v3.0

dlemfh · 2022-11-10T01:15:34Z

@alexmt Awesome, thanks!! 🎉

crenshaw-dev requested changes Aug 8, 2022

View reviewed changes

jannfis requested changes Aug 10, 2022

View reviewed changes

alexmt force-pushed the managed-resources-health branch 3 times, most recently from 2e08d42 to 1b1fdac Compare August 10, 2022 21:55

alexmt requested review from crenshaw-dev and jannfis August 10, 2022 22:01

crenshaw-dev requested changes Aug 12, 2022

View reviewed changes

cmd/argocd-application-controller/commands/argocd_application_controller.go Outdated Show resolved Hide resolved

alexmt force-pushed the managed-resources-health branch from 1b1fdac to 774915a Compare August 12, 2022 18:40

alexmt requested a review from crenshaw-dev August 12, 2022 18:45

crenshaw-dev mentioned this pull request Aug 12, 2022

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

Open

crenshaw-dev approved these changes Aug 15, 2022

View reviewed changes

alexmt enabled auto-merge (squash) August 16, 2022 23:10

jannfis approved these changes Aug 17, 2022

View reviewed changes

alexmt added 2 commits August 17, 2022 13:09

chore: infer managed resources health from redis instead of storing i…

68d8798

…t in CRD Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

apply reviewer notes

46cdd7d

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt force-pushed the managed-resources-health branch from 774915a to 46cdd7d Compare August 17, 2022 20:09

alexmt merged commit 893a867 into argoproj:master Aug 17, 2022

jenting mentioned this pull request Jul 28, 2024

enhance: store status.reconciledAt to Redis rather than to App CR #19280

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: infer managed resources health from redis instead of storing it in CRD #10191

chore: infer managed resources health from redis instead of storing it in CRD #10191

alexmt commented Aug 4, 2022

codecov bot commented Aug 4, 2022 •

edited

Loading

crenshaw-dev left a comment

crenshaw-dev Aug 8, 2022

alexmt Aug 10, 2022

jannfis left a comment

crenshaw-dev commented Aug 10, 2022

jannfis commented Aug 10, 2022

crenshaw-dev commented Aug 10, 2022

alexmt commented Aug 10, 2022

crenshaw-dev commented Aug 12, 2022

crenshaw-dev left a comment

alexmt commented Aug 12, 2022

crenshaw-dev left a comment

crenshaw-dev commented Aug 15, 2022

jannfis left a comment

sbose78 commented Sep 22, 2022

dlemfh commented Nov 9, 2022

alexmt commented Nov 9, 2022

dlemfh commented Nov 10, 2022

chore: infer managed resources health from redis instead of storing it in CRD #10191

chore: infer managed resources health from redis instead of storing it in CRD #10191

Conversation

alexmt commented Aug 4, 2022

codecov bot commented Aug 4, 2022 • edited Loading

Codecov Report

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev Aug 8, 2022

Choose a reason for hiding this comment

alexmt Aug 10, 2022

Choose a reason for hiding this comment

jannfis left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Aug 10, 2022

jannfis commented Aug 10, 2022

crenshaw-dev commented Aug 10, 2022

alexmt commented Aug 10, 2022

crenshaw-dev commented Aug 12, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

alexmt commented Aug 12, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Aug 15, 2022

jannfis left a comment

Choose a reason for hiding this comment

sbose78 commented Sep 22, 2022

dlemfh commented Nov 9, 2022

alexmt commented Nov 9, 2022

dlemfh commented Nov 10, 2022

codecov bot commented Aug 4, 2022 •

edited

Loading