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

[Azure] Issue 4230: remove readiness check for cache exclusion #119128

Merged
merged 1 commit into from
Aug 2, 2023
Merged

[Azure] Issue 4230: remove readiness check for cache exclusion #119128

merged 1 commit into from
Aug 2, 2023

Conversation

alexanderConstantinescu
Copy link
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes kubernetes-sigs/cloud-provider-azure#4230

Issue kubernetes-sigs/cloud-provider-azure#4230 explains things in greater depth, but here's the TL;DR

#109706 stopped syncing load balancers when changes are observed to the readiness state of the Node object. Load balancers are essentially almost only re-synced whenever a Node is: added, deleted, has the exclusion label added. The service controller expectation is that the cloud-providers just accept the list of nodes provided through the UpdateLoadBalancerHost call and try to configure the load balancers with the entire set of nodes provided. Given that Azure performs additional filtering of the nodes and excludes NotReady nodes, there will be situations where the node which just transitioned to Ready isn't added to the load balancer node set, because the service controller doesn't re-sync load balancers due to the readiness state change.

This patch removes this additional filtering of the readiness state so that all NotReady nodes get added to the load balancer set. The health check probes used by the load balancer will then determine which nodes should be used for traffic load balancing.

Please have a look at the referenced enhancement proposal which merged in 1.26: kubernetes/enhancements#3458

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixes issue https://github.com/kubernetes-sigs/cloud-provider-azure/issues/4230 and removes the additional filtering on `NotReady` nodes by the azure cloud provider code

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig cloud-provider
/assign @feiskyer

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 6, 2023
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/cloudprovider labels Jul 6, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 6, 2023
feiskyer
feiskyer approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

/triage accepted
/lgtm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 17, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2526a48418f0c4c1db5c81ca63e6c78d2ad90063

@feiskyer
Copy link
Member

@bridgetkromhout could you help to approve this fix?

@bridgetkromhout
Copy link
Member

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2023
@cheftako
Copy link
Member

cheftako commented Aug 2, 2023

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderConstantinescu, bridgetkromhout, cheftako, feiskyer

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

@cheftako
Copy link
Member

cheftako commented Aug 2, 2023

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 2, 2023
@elmiko
Copy link
Contributor

elmiko commented Aug 2, 2023

from SIG meeting today, please do not backport to 1.25

@cheftako
Copy link
Member

cheftako commented Aug 2, 2023

Please do not backport to 1.25 (1.26 is fine)

@gracenng
Copy link
Member

gracenng commented Aug 2, 2023

/milestone 1.28

@k8s-ci-robot
Copy link
Contributor

@gracenng: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.16, v1.17, v1.18, v1.19, v1.20, v1.21, v1.22, v1.23, v1.24, v1.25, v1.26, v1.27, v1.28]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 1.28

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.

@gracenng
Copy link
Member

gracenng commented Aug 2, 2023

woops
/milestone v1.28

@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Aug 2, 2023
@bridgetkromhout
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit 2c6c456 into kubernetes:master Aug 2, 2023
18 checks passed
@alexanderConstantinescu
Copy link
Member Author

alexanderConstantinescu commented Aug 3, 2023 •
edited

So do we need to back-port it here for this to get into AKS 1.26 / 1.27, @cheftako / @feiskyer ?

I asked on the original PR in cloud-provider-azure, here: kubernetes-sigs/cloud-provider-azure#4234 (comment) but didn't get an answer. It's been back-ported in that repo, but I have no clue what the dependency tree is for AKS.

This was referenced Aug 14, 2023
Merged
Merged
@lzhecheng
Copy link
Contributor

So do we need to back-port it here for this to get into AKS 1.26 / 1.27, @cheftako / @feiskyer ?

I asked on the original PR in cloud-provider-azure, here: kubernetes-sigs/cloud-provider-azure#4234 (comment) but didn't get an answer. It's been back-ported in that repo, but I have no clue what the dependency tree is for AKS.

Yes, backport is needed and I did it for you. Please check. #119931 #119932

k8s-ci-robot added a commit that referenced this pull request Sep 6, 2023
@k8s-ci-robot
Merge pull request #119931 from lzhecheng/automated-cherry-pick-of-#1…
0b17f90 
…19128-upstream-release-1.27

Automated cherry pick of #119128: Issue 4230: remove readiness check for cache exclusion
k8s-ci-robot added a commit that referenced this pull request Sep 6, 2023
@k8s-ci-robot
Merge pull request #119932 from lzhecheng/automated-cherry-pick-of-#1…
7891032 
…19128-upstream-release-1.26

Automated cherry pick of #119128: Issue 4230: remove readiness check for cache exclusion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer approved these changes

@andyzhangx andyzhangx Awaiting requested review from andyzhangx

@nilo19 nilo19 Awaiting requested review from nilo19

Assignees

@feiskyer feiskyer

@cheftako cheftako

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
[sig-release] Bug Triage
Archived in project
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

Load Balancer Node membership not being updated for externalTrafficPolicy Local
8 participants
@alexanderConstantinescu @k8s-ci-robot @feiskyer @bridgetkromhout @cheftako @elmiko @gracenng @lzhecheng