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

Ensure only one LoadBalancer rule is created when HA mode is enabled #99825

Merged
merged 1 commit into from Mar 11, 2021

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented Mar 5, 2021

What type of PR is this?

/kind bug
/sig cloud-provider
/area provider/azure

What this PR does / why we need it:

When highAvailabilityPorts enabled, the LoadBalancer port is always using 0, hence only one rule should be configured. Orelse, a RulesUseSameBackendPortProtocolAndPoolAndFrontendIPWithFloatingIPEnabled error would be reported from LoadBalancer API.

Which issue(s) this PR fixes:

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Ensure only one LoadBalancer rule is created when HA mode is enabled

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


@feiskyer feiskyer requested review from andyzhangx and nilo19 Mar 5, 2021
@k8s-ci-robot k8s-ci-robot added release-note kind/bug size/M sig/cloud-provider area/provider/azure cncf-cla: yes needs-triage needs-priority area/cloudprovider labels Mar 5, 2021
@k8s-ci-robot k8s-ci-robot requested a review from aramase Mar 5, 2021
@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 5, 2021

/priority critical-urgent
/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted label Mar 5, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 5, 2021

@feiskyer: The label(s) priority/critial-urgent cannot be applied, because the repository doesn't have them.

In response to this:

/priority critial-urgent
/triage accepted

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 removed the needs-triage label Mar 5, 2021
@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 5, 2021

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent and removed needs-priority labels Mar 5, 2021
@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 5, 2021

/assign @andrewsykim

@nilo19
Copy link
Member

@nilo19 nilo19 commented Mar 5, 2021

/lgtm
/approve

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 5, 2021

/retest

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 8, 2021

weird gce package unit tests are failing: FAIL: vendor/k8s.io/legacy-cloud-providers/gce TestFirewallObject. Rebased now to try again.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 8, 2021
@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 8, 2021

/retest

1 similar comment
@nilo19
Copy link
Member

@nilo19 nilo19 commented Mar 9, 2021

/retest

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 9, 2021

@andrewsykim @cheftako could you help to get this approved before code freeze?

/milestone v1.21

@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 9, 2021
@nilo19
Copy link
Member

@nilo19 nilo19 commented Mar 9, 2021

/retest

@nilo19
Copy link
Member

@nilo19 nilo19 commented Mar 9, 2021

@andrewsykim @cheftako the test failures are expected, we could ignore them.

for _, port := range ports {
if highAvailabilityPortsEnabled {
// Since the port is always 0 when enabling HA, only one rule should be configured.
Copy link
Member

@cheftako cheftako Mar 9, 2021

Choose a reason for hiding this comment

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

Might be nice to add an info line that you are skipping additional rules.

Also it seems like its possible that HA rule might not be the first, in which case you will have more than one rule, if a non HA rule snuck in first.

Copy link
Member Author

@feiskyer feiskyer Mar 9, 2021

Choose a reason for hiding this comment

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

Also it seems like its possible that HA rule might not be the first, in which case you will have more than one rule, if a non HA rule snuck in first.

this shouldn't happen as the HA mode is determined by service annotation, instead of some configure on ports. And, when the first HA rule created, highAvailabilityPortsEnabled would be marked as true.

Copy link
Member Author

@feiskyer feiskyer Mar 9, 2021

Choose a reason for hiding this comment

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

Might be nice to add an info line that you are skipping additional rules.

And this is actually not skipping additional rules. It is one rule to allow all ports, that's why the comment said only one rule should be configured.

@cheftako
Copy link
Member

@cheftako cheftako commented Mar 9, 2021

/lgtm
@feiskyer happy to approve but wanted to give you a chance to look at my observation first.

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 9, 2021
@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 9, 2021

@cheftako thanks for reviewing. Added comments and could you help to approve?

@cheftako
Copy link
Member

@cheftako cheftako commented Mar 10, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, 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

@k8s-ci-robot k8s-ci-robot added the approved label Mar 10, 2021
@fejta-bot
Copy link

@fejta-bot fejta-bot commented Mar 10, 2021

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@nilo19
Copy link
Member

@nilo19 nilo19 commented Mar 11, 2021

@feiskyer we need to reopen the pr to disable the failed azure file ci test

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 11, 2021

@feiskyer: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-azure-file-windows-containerd e48f806 link /test pull-kubernetes-e2e-azure-file-windows-containerd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@nilo19
Copy link
Member

@nilo19 nilo19 commented Mar 11, 2021

Test_Run_Positive_VolumeAttachMountUnmountDetach
this unit test failed

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Mar 11, 2021

/test pull-kubernetes-unit

@k8s-ci-robot k8s-ci-robot merged commit 20bc36c into kubernetes:master Mar 11, 2021
19 of 21 checks passed
@feiskyer feiskyer deleted the fix-ha-rule branch Mar 11, 2021
k8s-ci-robot added a commit that referenced this issue Mar 12, 2021
…25-upstream-release-1.20

Automated cherry pick of #99825: Ensure only one LoadBalancer rule is created when HA mode is
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/cloudprovider area/provider/azure cncf-cla: yes kind/bug lgtm priority/critical-urgent release-note sig/cloud-provider size/M triage/accepted
Projects
None yet
6 participants