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

fix: Do not move nodes that have already been attached to load balanc… #6965

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

nilo19
Copy link
Contributor

@nilo19 nilo19 commented Sep 5, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix: Do not move nodes that have already been attached to load balancers after restarting

When using multislb, the node distribution could be changed after restarting the ccm. This is because the node distribution cache would be lost after restarting. This PR restores the node distribution each time the ccm is restarted.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This only affects cluster services.

Does this PR introduce a user-facing change?

fix: Do not move nodes that have already been attached to load balancers after restarting

When using multislb, the node distribution could be changed after restarting the ccm. This is because the node distribution cache would be lost after restarting. This PR restores the node distribution each time the ccm is restarted.

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


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 5, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 5, 2024
@nilo19 nilo19 force-pushed the fix/multi-slb-node branch 4 times, most recently from 5a603a8 to 01f3d9f Compare September 6, 2024 04:52
@coveralls
Copy link

coveralls commented Sep 6, 2024

Coverage Status

coverage: 77.258% (+0.03%) from 77.233%
when pulling 940b753 on nilo19:fix/multi-slb-node
into 6505378 on kubernetes-sigs:master.

@nilo19 nilo19 force-pushed the fix/multi-slb-node branch from 01f3d9f to beab47c Compare September 7, 2024 04:10
) error {
if init {
if err := az.recordExistingNodesOnLoadBalancers(clusterName, lbs); err != nil {
fmt.Println(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if strings.EqualFold(trimSuffixIgnoreCase(
lbName, consts.InternalLoadBalancerNameSuffix,
), multiSLBConfig.Name) {
az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes =
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ActiveNodes the node distribution cache you mentioned in the PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks.

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.

lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, nilo19

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

…ers after restarting

When using multislb, the node distribution could be changed after restarting the ccm.
This is because the node distribution cache would be lost after restarting.
This PR restores the node distribution each time the ccm is restarted.
@MartinForReal
Copy link
Contributor

/retest

@MartinForReal
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2024
@nilo19
Copy link
Contributor Author

nilo19 commented Sep 11, 2024

/retest

1 similar comment
@nilo19
Copy link
Contributor Author

nilo19 commented Sep 11, 2024

/retest

@k8s-ci-robot k8s-ci-robot merged commit 877138b into kubernetes-sigs:master Sep 11, 2024
18 checks passed
@nilo19 nilo19 deleted the fix/multi-slb-node branch September 11, 2024 12:39
@nilo19
Copy link
Contributor Author

nilo19 commented Sep 11, 2024

/cherrypick release-1.31

@k8s-infra-cherrypick-robot

@nilo19: new pull request created: #7021

In response to this:

/cherrypick release-1.31

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-sigs/prow repository.

@nilo19
Copy link
Contributor Author

nilo19 commented Sep 11, 2024

/cherrypick release-1.30

@k8s-infra-cherrypick-robot

@nilo19: new pull request created: #7022

In response to this:

/cherrypick release-1.30

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-sigs/prow repository.

@nilo19
Copy link
Contributor Author

nilo19 commented Sep 11, 2024

/cherrypick release-1.29

@nilo19
Copy link
Contributor Author

nilo19 commented Sep 11, 2024

/cherrypick release-1.28

@k8s-infra-cherrypick-robot

@nilo19: new pull request created: #7023

In response to this:

/cherrypick release-1.29

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-sigs/prow repository.

@k8s-infra-cherrypick-robot

@nilo19: new pull request created: #7024

In response to this:

/cherrypick release-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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants