-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add readiness endpoint #1029
Add readiness endpoint #1029
Conversation
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, I think there are some places where you used liveness
and it should be readiness
and I think there was a change missing in daemonset
manifests for OSS and Plus. Let me know if these make sense!
docs-web/configuration/global-configuration/command-line-arguments.md
Outdated
Show resolved
Hide resolved
docs-web/configuration/global-configuration/command-line-arguments.md
Outdated
Show resolved
Hide resolved
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.
Thanks @lucacome
I left a few comments and suggestions.
Also, there is another helm doc that's needed to be updated -- https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-helm/
Also, shall we make it the readiness probe by default? imho, it is important feature as it prevents IC pods that are not fully exposed to be exposed to the load balancer.
Co-authored-by: Raúl <[email protected]>
Co-authored-by: Michael Pleshakov <[email protected]>
docs-web/configuration/global-configuration/command-line-arguments.md
Outdated
Show resolved
Hide resolved
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.
@lucacome the changes look good!
there is another helm doc that's needed to be updated -- https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-helm/
let's enable the feature by default. It prevents non-ready IC pods to be exposed to a fronting load balancer, which will prevent clients from seeing errors related to partially-configured configuration. For example, this can happen during scaling up or upgrades.
…ents.md Co-authored-by: Michael Pleshakov <[email protected]>
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.
@lucacome looks good.
Small comments:
(1)
In the docs, we put the default values for agruments. For example:
.. option:: -nginx-status
Enable the NGINX stub_status, or the NGINX Plus API. (default true)
(2)
does it make sense to remove - -ready-status
from the manifests, considering now that it is enabled by default? otherwise, it might be confusing - if somebody simplify removes it, it will have no effect.
Implement the readiness endpoint. The endpoint returns
503
while NGINX is starting up and loading all the configs. As soon as this is done the endpoint will return200
.There might be more docs needed.