-
Notifications
You must be signed in to change notification settings - Fork 5k
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): route health check stuck in 'Progressing' (#10037) #12959
base: master
Are you sure you want to change the base?
Conversation
fixes argoproj#10037 Signed-off-by: Christoph Hille <38468371+hille721@users.noreply.github.com>
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #12959 +/- ##
=======================================
Coverage 47.98% 47.98%
=======================================
Files 246 246
Lines 42221 42221
=======================================
Hits 20259 20259
Misses 19939 19939
Partials 2023 2023 see 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hille721 would you mind adding a test for this health check to prevent regressions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hille721 I don't think that just changing ==
to >=
would fix the issue you are describing.
If there are multiple hosts included as part of the Ingress spec, then status for each host should have all the conditions to be True
.
Your check will fail if one host has both status conditions false
and one host has both conditions true
. In this case the status will still show as Healthy
wherein it needs to be Degraded
.
@hille721 WDYT?
@ishitasequeira Same happen with current implementation, as if I understand your scenario correctly, you will end up with But yeah maybe we should extend the condition to |
Hi @crenshaw-dev, |
Yes, the current implementation does not consider the scenario raised in #10037. Adding
For adding a test case, you would need to add a file in the testdata folder similar to https://github.com/argoproj/argo-cd/blob/10e8a79be3899c2cf0aee19d069f52260c33c364/resource_customizations/route.openshift.io/Route/testdata/degraded.yaml. The Yaml file would include the route definition which defines the scenario you have specified. |
There was already a PR which tries to fix that the Openshift route health check stuck in 'Progressing' (#8170). But there is still a case when this current implementation of the healthcheck does not work:
As you can see there are two conditions with
type: Admitted
andstatus: 'True'
which results that the counternumTrue
will be greater thannumIngressRules
. But the current health check wants an equals to (https://github.com/argoproj/argo-cd/blob/master/resource_customizations/route.openshift.io/Route/health.lua#L20).It can be simple fix if we make the
==
to>=
.Fixes #10037
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist:
Please see Contribution FAQs if you have questions about your pull-request.