-
Notifications
You must be signed in to change notification settings - Fork 497
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
Extension health checks support Progressing status #2289
Extension health checks support Progressing status #2289
Conversation
7336c4e
to
f0fadfd
Compare
/kind/enhancement |
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.
Code looks good to me - nice to have now error codes & progressing condition status allowing more sophisticated health checks in general.
Have you already build an image that I can re-use for testing it with the extension providers?
f0fadfd
to
ba32873
Compare
No, I haven't built an image, I was revendoring the AWS provider, started it locally on my machine and then performed my tests (easier/faster turn-around cycle). |
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.
Tested and works for me. Only nits regarding logging.
Maybe as a special remark for reviewers you could add next time that you have to add the
scheme.AddKnownTypes(machinev1alpha1.SchemeGroupVersion,
&machinev1alpha1.MachineDeploymentList{},
&machinev1alpha1.MachineSetList{},
&machinev1alpha1.MachineList{},
)
to the scheme in the extension provider- in case reviewers are not aware.
ba32873
to
84371ec
Compare
84371ec
to
2fb410f
Compare
Build is failing |
2fb410f
to
bd8f203
Compare
bd8f203
to
6ef60cd
Compare
/needs/review |
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.
Looks quite good, I have only minor comments for you :)
6ef60cd
to
5f63bca
Compare
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.
Looks good now, only the loop thingy left :)
5f63bca
to
692bb3d
Compare
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.
Nice, thanks for incorporating my suggestions.
/lgtm now
@danielfoehrKn any more feedback? |
What this PR does / why we need it:
With this PR the extension health check library is enhanced to allow individual health checks to return the
Progressing
status. This is helpful in order to provide more accurate status information and less false negative reports.Also, the worker extension health check particularly was improved. It can now better detect regular scale-down and scale-up situations. Consequently, the "too many nodes" check was removed from Gardener's generic health check and moved into the responsibility of the worker extension health check (as it can more accurately determine what is going on).
Release note: