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

Add readiness endpoint #1029

Merged
merged 16 commits into from
Jul 9, 2020
Merged

Add readiness endpoint #1029

merged 16 commits into from
Jul 9, 2020

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented Jul 2, 2020

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 return 200.

There might be more docs needed.

Copy link
Contributor

@Rulox Rulox left a 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!

deployments/deployment/nginx-ingress.yaml Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/values.yaml Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
@lucacome lucacome requested a review from Rulox July 2, 2020 17:54
Copy link
Contributor

@pleshakov pleshakov left a 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.

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Show resolved Hide resolved
deployments/daemon-set/nginx-ingress.yaml Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
deployments/helm-chart/values.yaml Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/task_queue.go Show resolved Hide resolved
lucacome and others added 5 commits July 8, 2020 09:32
Co-authored-by: Raúl <Rulox@users.noreply.github.com>
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
@lucacome lucacome requested a review from pleshakov July 8, 2020 18:13
Copy link
Contributor

@pleshakov pleshakov left a 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.

Copy link
Contributor

@pleshakov pleshakov left a 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.

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
@lucacome lucacome merged commit 2f512b7 into master Jul 9, 2020
@lucacome lucacome deleted the readiness-probe branch July 9, 2020 20:02
@pleshakov pleshakov added change Pull requests that introduce a change enhancement Pull requests for new features/feature enhancements labels Jul 10, 2020
@Dean-Coakley Dean-Coakley mentioned this pull request Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Pull requests that introduce a change enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants