Update pods with readinessGate as healthy for deleted TGB #2524

oliviassss · 2022-02-23T00:07:40Z

Issue

Description

To avoid race condition between pods readiness gate, this PR adds a step to mark podCondition with associated TargetGroupBinding's readinessGate as healthy when delete a TargetGroupBinding.

Test

Created ingress and deployment in the namespace with pod readinessGate enabled, triggered the ingress update and deployment rollout at the same time, verified that the pod's readinessGate status is 1/1, and the new target group is registered to the TargetGroupBinding.
Deleted the resources in the namespace with pod readinessGate enabled, verified these resources can be cleaned up successfully
Created and deleted ingress and deployment in the namespace with pod readinessGate diabled, verified these resources can be created and deleted successfully, no behavior change in this use case

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

k8s-ci-robot · 2022-02-23T00:07:48Z

Hi @oliviassss. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

codecov-commenter · 2022-02-23T00:15:59Z

Codecov Report

Merging #2524 (9b99679) into main (779508f) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2524      +/-   ##
==========================================
- Coverage   53.97%   53.85%   -0.12%     
==========================================
  Files         144      144              
  Lines        8147     8165      +18     
==========================================
  Hits         4397     4397              
- Misses       3433     3451      +18     
  Partials      317      317

Impacted Files	Coverage Δ
pkg/targetgroupbinding/resource_manager.go	`15.27% <0.00%> (-1.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 779508f...9b99679. Read the comment docs.

kishorj · 2022-03-01T18:30:23Z

/ok-to-test

kishorj · 2022-03-15T16:47:12Z

/lgtm

kishorj · 2022-03-15T17:45:13Z

/approve

k8s-ci-robot · 2022-03-15T17:45:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kishorj, oliviassss

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:

~~OWNERS~~ [kishorj]

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

kishorj · 2022-03-15T22:36:42Z

/lgtm

M00nF1sh · 2022-03-16T21:57:04Z

pkg/targetgroupbinding/resource_manager.go

+			return err
+		}
+		if !exists {
+			return errors.New("couldn't find podInfo for ready endpoint")


we should just continue if pod not found.
we can fix it in future versions as it's rare to trigger and won't cause much defect.

M00nF1sh · 2022-03-16T22:00:31Z

pkg/targetgroupbinding/resource_manager.go

+
+	allPodKeys := m.podInfoRepo.ListKeys(ctx)
+	for _, podKey := range allPodKeys {
+		pod, exists, err := m.podInfoRepo.Get(ctx, podKey)


we should check for pod's namespace here as well.
Since in theory you can have two targetGroupBinding in two namespace.

In practice, with our default Ingress/Service implementation, the TGB's name is same as TargetGroup's name and will be unique across namespaces.
but this will impact user created targetGroupBinding.

we can fix it in future versions as the impact is minimal

…-sigs#2524) * mark pod with readiness gate as healthy when deleting TGB * list pod keys Co-authored-by: Kishor Joshi <[email protected]>

oliviassss added 2 commits February 22, 2022 11:14

mark pod with readiness gate as healthy when deleting TGB

b0a1fd5

list pod keys

28a7d16

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 23, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh February 23, 2022 00:08

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2022

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 15, 2022

k8s-ci-robot assigned kishorj Mar 15, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2022

kishorj approved these changes Mar 15, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2022

Merge branch 'main' into TGB_readiness

9b99679

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 15, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2022

k8s-ci-robot merged commit a18ab63 into kubernetes-sigs:main Mar 15, 2022

M00nF1sh reviewed Mar 16, 2022

View reviewed changes

kishorj mentioned this pull request Mar 17, 2022

🥳 aws-load-balancer-controller v2.4.1 Automated Release! 🥑 aws/eks-charts#713

Merged

oliviassss mentioned this pull request Mar 25, 2022

check namespace when update pod condition for deleted TGB #2585

Merged

6 tasks

oliviassss mentioned this pull request Apr 27, 2023

REQUEST: New membership for @oliviassss kubernetes/org#4183

Closed

9 tasks

oliviassss deleted the TGB_readiness branch May 15, 2023 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update pods with readinessGate as healthy for deleted TGB #2524

Update pods with readinessGate as healthy for deleted TGB #2524

Uh oh!

oliviassss commented Feb 23, 2022 •

edited

Loading

Uh oh!

k8s-ci-robot commented Feb 23, 2022

Uh oh!

codecov-commenter commented Feb 23, 2022 •

edited

Loading

Uh oh!

kishorj commented Mar 1, 2022

Uh oh!

kishorj commented Mar 15, 2022

Uh oh!

kishorj commented Mar 15, 2022

Uh oh!

k8s-ci-robot commented Mar 15, 2022

Uh oh!

kishorj commented Mar 15, 2022

Uh oh!

M00nF1sh Mar 16, 2022

Uh oh!

M00nF1sh Mar 16, 2022

Uh oh!

Uh oh!

Update pods with readinessGate as healthy for deleted TGB #2524

Update pods with readinessGate as healthy for deleted TGB #2524

Uh oh!

Conversation

oliviassss commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Test

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Uh oh!

k8s-ci-robot commented Feb 23, 2022

Uh oh!

codecov-commenter commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kishorj commented Mar 1, 2022

Uh oh!

kishorj commented Mar 15, 2022

Uh oh!

kishorj commented Mar 15, 2022

Uh oh!

k8s-ci-robot commented Mar 15, 2022

Uh oh!

kishorj commented Mar 15, 2022

Uh oh!

M00nF1sh Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oliviassss commented Feb 23, 2022 •

edited

Loading

codecov-commenter commented Feb 23, 2022 •

edited

Loading