Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: Support existing load balancer backend address pool for agent nodes #1145

Merged
merged 3 commits into from Apr 25, 2019

Conversation

xuto2
Copy link
Contributor

@xuto2 xuto2 commented Apr 25, 2019

Reason for Change:
Azure Standard Load Balancer(SLB) introduces some fundamental difference to Azure Basic Load Balancer(BLB). Specifically, vmas/vmss behind Azure SLB don't automatically get internet access like BLB, and it's Azure's recommendation that we "always use outbound rules on a Standard public Load Balancer" (see https://docs.microsoft.com/en-us/azure/load-balancer/load-balancer-outbound-connections for details).

To support Azure SLB in AKS, AKS needs to maintain the outbound rules of the SLB (and the load balancer rules as well to workaround upstream issue when using SLB kubernetes/kubernetes#76691). So we need the ability to place AKS engine generated nodes (VMSS+VMAS) into an existing SLB's backend address pool.

Requirements:

Notes:

@welcome
Copy link

welcome bot commented Apr 25, 2019

💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - fix: change azure disk cachingMode to ReadOnly - feat: make maximumLoadBalancerRuleCount configurable - docs: add note on AKS Engine and AKS relationship
Make sure to check out the developer guide for guidance on testing your change.

@acs-bot acs-bot added the size/L label Apr 25, 2019
@acs-bot
Copy link

acs-bot commented Apr 25, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xuto2
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mboersma

If they are not already assigned, you can assign the PR to them by writing /assign @mboersma in a comment when ready.

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

@jackfrancis
Copy link
Member

@sylr Does this address any symptoms described in #1099?

@jackfrancis
Copy link
Member

/azp run pr-e2e

@CecileRobertMichon
Copy link
Contributor

/needs-rebase

@CecileRobertMichon CecileRobertMichon added the needs-rebase Changes in the target branch require a `git rebase` and `git push -f` label Apr 25, 2019
@jackfrancis jackfrancis force-pushed the AcceptExistingLBBackendPoolForVM branch from 217c4d1 to ad0dccf Compare April 25, 2019 17:25
@jackfrancis
Copy link
Member

@xuto2 FYI I rebased on top of master/force-pushed to this branch, so you'll need to pull down for future dev

@CecileRobertMichon
Copy link
Contributor

Squash commit message needs to be updated to use semantic message (or if a second commit is pushed the title is enough).

/hold

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@CecileRobertMichon CecileRobertMichon removed the needs-rebase Changes in the target branch require a `git rebase` and `git push -f` label Apr 25, 2019
@CecileRobertMichon
Copy link
Contributor

two commits will use PR title as squash message so we're good now.

/hold cancel

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Apr 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5f86d3d). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1145   +/-   ##
=========================================
  Coverage          ?   74.58%           
=========================================
  Files             ?      131           
  Lines             ?    18425           
  Branches          ?        0           
=========================================
  Hits              ?    13742           
  Misses            ?     3902           
  Partials          ?      781

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

lgtm

@CecileRobertMichon
Copy link
Contributor

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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

@jackfrancis
Copy link
Member

@sylr nevermind I understand what this does, I have another possible fix for the lack of UDP outbound incoming shortly...

@jackfrancis jackfrancis merged commit 5160a8f into Azure:master Apr 25, 2019
@welcome
Copy link

welcome bot commented Apr 25, 2019

Congrats on merging your first pull request! 🎉🎉🎉

@xuto2
Copy link
Contributor Author

xuto2 commented Apr 26, 2019

Thanks @jackfrancis @CecileRobertMichon @feiskyer !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants