Skip to content
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.

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

fix: Health check from lister not apiserver #11375

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

cwdsuzhou
Copy link
Contributor

@cwdsuzhou cwdsuzhou commented Jul 17, 2023

Fixes #11374

Motivation

Reduce the List check from controller. IMO, it is not reasonable to list from apiserver for health check. Informer is not so reliable and if informer is not healthy, the controller may exit or resync from apiserver.

Modifications

Change the health check logic from apiserver to the local cache

Verification

@cwdsuzhou cwdsuzhou changed the title Health check from lister but apiserver fix: Health check from lister but apiserver Jul 17, 2023
@cwdsuzhou cwdsuzhou changed the title fix: Health check from lister but apiserver fix: Health check from lister not apiserver Jul 17, 2023
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

See discussions in #11374

Signed-off-by: weidongcai <cwdsuzhou@gmail.com>
@stale
Copy link

stale bot commented Aug 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

@stale stale bot added the problem/stale This has not had a response in some time label Aug 12, 2023
@terrytangyuan
Copy link
Member

Bump

@stale stale bot removed the problem/stale This has not had a response in some time label Aug 17, 2023
@cwdsuzhou
Copy link
Contributor Author

Since we have disscussed #11374, can we work on this PR now?

@terrytangyuan

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

I am ok with this change. Let's watch for any issues once merged/released

@terrytangyuan terrytangyuan enabled auto-merge (squash) August 30, 2023 00:50
@terrytangyuan terrytangyuan merged commit e90d6bf into argoproj:master Aug 30, 2023
22 checks passed
@astraw99
Copy link
Contributor

astraw99 commented Sep 7, 2023

I am ok with this change. Let's watch for any issues once merged/released

Raised a PR #11770 to fix an issue #11769 introduced from this.
PTAL thanks.

terrytangyuan pushed a commit that referenced this pull request Oct 19, 2023
Signed-off-by: weidongcai <cwdsuzhou@gmail.com>
@juliev0
Copy link
Contributor

juliev0 commented Dec 6, 2023

@cwdsuzhou When you tested this, were you able to successfully execute line 37 while catching a Workflow that hadn't been reconciled yet? We noticed a panic occurring on this line, when the *Unstructured type in the Informer gets type asserted to a *Workflow type instead. It seems like the Informer holds *Unstructured types, so I'm not sure if this type conversion ever works. If you have any thoughts, let me know. Thanks!

@agilgur5 agilgur5 added the area/controller Controller issues, panics label Jan 22, 2024
@agilgur5
Copy link
Contributor

We noticed a panic occurring on this line, when the *Unstructured type in the Informer gets type asserted to a *Workflow type instead. It seems like the Informer holds *Unstructured types, so I'm not sure if this type conversion ever works.

For posterity, this was followed up in #12326 and #12353

dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
Signed-off-by: weidongcai <cwdsuzhou@gmail.com>
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Periodly healthz check may cause serious problems of apiserver/etcd
5 participants