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.

Sign up for GitHub

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

Jump to bottom

Pod readiness gates #955

Merged
merged 14 commits into from
Mar 9, 2020
Merged

Pod readiness gates #955

merged 14 commits into from
Mar 9, 2020

Conversation

alfredkrohmer
Copy link
Contributor

@alfredkrohmer alfredkrohmer commented Jun 18, 2019 •
edited
Loading

This adds a feature to set the status of pod readiness gates on pods that are registered with an ALB. See added documentation for details of implementation.

Todo:

  • fix up unit tests, add test cases for pod readiness gates
  • test manually

This closes #905.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 18, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @devkid. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 18, 2019
@alfredkrohmer alfredkrohmer mentioned this pull request Jun 18, 2019
Closed
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 16, 2019
@alfredkrohmer
Copy link
Contributor Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 17, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 16, 2019
This was referenced Jan 12, 2020
Closed
Closed
@lelikg
Copy link

lelikg commented Jan 13, 2020

+1 on this feature, we'd love to get it

@a6kme a6kme mentioned this pull request Jan 17, 2020
Closed
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 12, 2020
@runningman84
Copy link

/remove-lifecycle

@runningman84
Copy link

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 12, 2020
@alfredkrohmer
Set pod condition for pod readiness gate in order to only mark pod `R…
b134750 
…eady` when it's healthy and taking traffic from ALB
@alfredkrohmer alfredkrohmer force-pushed the feature/pod-readiness-gate branch from d8befc9 to b134750 Compare February 19, 2020 14:22
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 19, 2020
@alfredkrohmer
Copy link
Contributor Author 

1. should we simplify to use a single readiness gate instead of per targetGroup?  (personally I favor your current approach)

I think we should start with the per-target group approach. We can add the per-ingress functionality later if required.

2. should we have a timeout setting like 5 minutes. If pod didn't become healthy within 5 minute, unblock the readiness gate. (to prevent deployment lock for wrong settings, e.g. security groups).

I don't think we should have a timeout for this. The whole idea of readiness gates is that they should block deployments if the pods don't get registered in the load balancer (for whatever reason). Progressing with deployment when the readiness gate is not ok yet will most likely bring the service down eventually (e.g. when – for any reason – the ALB ingress controller is down).

@M00nF1sh
Copy link
Collaborator

overall looks good to me 👍 just we should remove the target-health-reconciliation-strategy.

@alfredkrohmer
Address review comments
355dde7 
* remove reconciliation strategy (always use `initial`)
* remove reconciliation interval (use healthcheckIntervalSeconds * healthyThresholdCount initially, then only healthcheckIntervalSeconds)

Also:
* include ALB target health description in the `Reason` field of pod condition
* initialize and pass the TargetHealthController in TargetGroupController instead of TargetsController (one level above)
@alfredkrohmer
Put target health description into pod condition message
1af529c
@alfredkrohmer
Copy link
Contributor Author

alfredkrohmer commented Feb 28, 2020 •
edited
Loading

@M00nF1sh removed the strategy and interval. It uses now healthCheckInterval * healthCheckThreshold for the first time.After and afterwards only healthCheckInterval.

@alfredkrohmer
Copy link
Contributor Author

@M00nF1sh is there anything else that prevents from merging? Or do you need to coordinate this with the v2 work?

@M00nF1sh
Copy link
Collaborator

M00nF1sh commented Mar 3, 2020

@devkid
There isn't, and we should get this into v1.1.6 🤣 . Let me do a test for the code logic and ship this by EOD.

@yhware yhware mentioned this pull request Mar 5, 2020
Closed
M00nF1sh
M00nF1sh approved these changes Mar 9, 2020
View reviewed changes
Copy link
Collaborator

@M00nF1sh M00nF1sh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devkid, M00nF1sh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2020
@M00nF1sh
Copy link
Collaborator

M00nF1sh commented Mar 9, 2020

/woof

@k8s-ci-robot
Copy link
Contributor

@M00nF1sh: dog image

In response to this:

/woof

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot merged commit 1aba9e5 into kubernetes-sigs:master Mar 9, 2020
nirnanaaa
nirnanaaa reviewed Mar 10, 2020
View reviewed changes
internal/ingress/backend/endpoint.go
if err != nil {
continue
}

Copy link

@nirnanaaa nirnanaaa Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@M00nF1sh @devkid shouldn't we also check if DeletionTimestamp is != nil? because pods, that are in state terminating can still be found in the store, but should already be set into state "draining" on the TG, right? I can still see quite a substantial amount of 5xx errors (502, 504) in our testcase, when the pods are actually receiving their sigterm signal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pods in Terminating state appear neither in Addresses nor in NotReadyAddresses.

Copy link

@nirnanaaa nirnanaaa Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

🤷‍♂️ for us they do, that's why I mentioned it here. Using EKS 1.14 and publishNotReadyAddresses on the service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's weird. I actually thought about implementing the check for DeletionTimestamp != nil but I verified on our end that terminating pods do not appear in NotReadyAddresses. Feel free to add the check. I'll have another look at this on our end as well next week.

@M00nF1sh M00nF1sh mentioned this pull request Mar 22, 2020
Merged
alebedev87 pushed a commit to alebedev87/aws-load-balancer-controller that referenced this pull request Oct 26, 2023
@k8s-ci-robot
Merge pull request kubernetes-sigs#955 from devkid/feature/pod-readin…
78dcccf 
…ess-gate

Pod readiness gates
alebedev87 pushed a commit to alebedev87/aws-load-balancer-controller that referenced this pull request Oct 26, 2023
@M00nF1sh
bug fix for edge cases not covered by pod readness gate PR kubernetes…
0dbd870 
…-sigs#955
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nirnanaaa nirnanaaa nirnanaaa left review comments

@M00nF1sh M00nF1sh M00nF1sh approved these changes

@bigkraig bigkraig Awaiting requested review from bigkraig

Assignees

@M00nF1sh M00nF1sh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Custom Pod Status for PodReadinessGate to Block Premature Pod Termination
7 participants
@alfredkrohmer @k8s-ci-robot @fejta-bot @lelikg @runningman84 @M00nF1sh @nirnanaaa