Skip to content

refactor: Replace status.observedAt with redis pub/sub channels for resource tree updates (#1340) #4208

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

Jump to bottom
Merged
merged 1 commit into from
Aug 31, 2020
Merged

refactor: Replace status.observedAt with redis pub/sub channels for resource tree updates (#1340) #4208

merged 1 commit into from
Aug 31, 2020

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented Aug 31, 2020 •
edited
Loading

Closes #1340

More information about why this PR is required now:

The Application CRD status has two fields observedAt and reconciledAt.

  • the reconciledAt is updated when application resources are compared with Git and sync/health status got refreshed.
  • the observedAt is updated every time when reconciledAt is updated and when any "child" application resource (e.g. Pod, ReplicaSet etc) has changed.

The observedAt is changed very frequently and change is required just to notify UI that it should reload resources tree. If Argo CD manages thousands of application then observedAt field updates cause thousands of K8S PATCH requests every minute. This slowdown K8S API server and affects all controllers installed in the cluster.

This PR deprecates observedAt - controller no longer updates it. The UI is using /v1/stream/applications/<app-name>/resource-tree stream API which is powered by redis pub-sub.

@alexmt alexmt requested review from jannfis and jessesuen August 31, 2020 01:58
@alexmt
Copy link
Collaborator Author

alexmt commented Aug 31, 2020

PR should resolve or at least improve #3924 and #3556 . Argo CD should make significantly less K8S requests and reduce etcd load.

@alexmt alexmt force-pushed the 1340-redis-pub-sub branch from 0364358 to 593ae01 Compare August 31, 2020 02:10
@alexmt alexmt force-pushed the 1340-redis-pub-sub branch from 593ae01 to 628da9d Compare August 31, 2020 02:23
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2020 •
edited by codecov bot
Loading

Codecov Report

Attention: Patch coverage is 34.61538% with 34 lines in your changes missing coverage. Please review.

Project coverage is 42.84%. Comparing base (d7a70bf) to head (628da9d).
Report is 5498 commits behind head on master.

Files with missing lines Patch % Lines
util/cache/redis.go 7.14% 13 Missing ⚠️
server/application/application.go 0.00% 7 Missing ⚠️
util/cache/appstate/cache.go 33.33% 3 Missing and 1 partial ⚠️
util/cache/cache.go 0.00% 4 Missing ⚠️
util/cache/inmemory.go 0.00% 4 Missing ⚠️
server/cache/cache.go 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4208      +/-   ##
==========================================
- Coverage   42.98%   42.84%   -0.14%     
==========================================
  Files         124      124              
  Lines       18332    18377      +45     
==========================================
- Hits         7880     7874       -6     
- Misses       9438     9492      +54     
+ Partials     1014     1011       -3

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

jannfis
jannfis approved these changes Aug 31, 2020
View reviewed changes
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM, quite awesome fix!

@darshanime
Copy link
Member

The cache is throwaway in the current implementation, we can delete it and nothing will break. Will that change by this patch?

@alexmt
Copy link
Collaborator Author

alexmt commented Aug 31, 2020

@darshanime , it is still safe to lose cache. Redis recreates pub-sub channel on the fly. Also redis-go automatically re-connect to channel, so UI won't notice if redis restart.

@alexmt alexmt merged commit fca0f69 into argoproj:master Aug 31, 2020
@alexmt alexmt deleted the 1340-redis-pub-sub branch August 31, 2020 17:18
alexmt pushed a commit that referenced this pull request Aug 31, 2020
@alexmt
refactor: Replace status.observedAt with redis pub/sub channels for…
fba91ae 
… resource tree updates (#1340) (#4208)
@alexmt
Copy link
Collaborator Author

alexmt commented Aug 31, 2020 •
edited
Loading

The change significantly improved the performance of the busy Argo CD. Number of K8S requests reduced by 5x ~ 7x times:

image (3)

Avg reconciliation duration changed from 1s~2s to < 0.25s:

image (4)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannfis jannfis jannfis approved these changes

@jessesuen jessesuen Awaiting requested review from jessesuen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace status.observedAt with redis pub/sub channels for resource tree updates
4 participants
@alexmt @codecov-commenter @darshanime @jannfis