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

fix: Wait for management IP after creating ext network. #2049

Merged
merged 2 commits into from
Oct 22, 2019

Conversation

pradipd
Copy link
Contributor

@pradipd pradipd commented Sep 27, 2019

Reason for Change:

Fixes a connectivity issue we see intermittently in AKS. After the external network is created, it may take "some time" for the network adapter to get an IP address. It will have a 169.254.0.0 address. It is best to wait for the network adapter to get a proper IP address before continuing.
In the repro we looked at, HNS used the 169.254 address for many VFP rules. This means we created containers before the network adapter got its proper ip address.

Issue Fixed:

Fixes ICM 147788786. I do not think we have a public GitHub issue on this.

Requirements:

Notes:

@welcome
Copy link

welcome bot commented Sep 27, 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/M label Sep 27, 2019
@pradipd
Copy link
Contributor Author

pradipd commented Sep 27, 2019

@PatrickLang @marosset FYI.

@pradipd
Copy link
Contributor Author

pradipd commented Sep 27, 2019

/assign @PatrickLang @marosset @madhanrm

@daschott
Copy link

@jackfrancis FYI

@daschott
Copy link

FYI This PR contains an important fix. Without this change, all users using AKS-Engine with Window nodes are at risk of networking being plumbed incorrectly. We have customers in production that rely on this fix getting in. We have validated this change as well.

@marosset
Copy link
Contributor

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

Merging #2049 into master will decrease coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2049      +/-   ##
==========================================
- Coverage   76.67%   76.55%   -0.13%     
==========================================
  Files         135      135              
  Lines       20564    20709     +145     
==========================================
+ Hits        15768    15854      +86     
- Misses       3877     3931      +54     
- Partials      919      924       +5

@daschott
Copy link

@marosset @PatrickLang friendly ping :) This seems to be passing all the tests and was validated by Pradip as well. Are there any concerns or is there any additional work that you would like to see so this can be merged? Thanks!

@marosset
Copy link
Contributor

@daschott I left a comment about adding some additional logging here.
Can that get added in please?

Copy link
Contributor

@marosset marosset left a comment

Choose a reason for hiding this comment

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

/lgtm

thanks

@acs-bot acs-bot added the lgtm label Oct 17, 2019
@acs-bot
Copy link

acs-bot commented Oct 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marosset, pradipd

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

@daschott
Copy link

@marosset Thanks for the LGTM! Do you know how long it takes for these checks to complete so this can be merged?

@marosset
Copy link
Contributor

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marosset
Copy link
Contributor

Sorry, i forgot to trigger the e2e tests again after the changes.
The PR should merge automatically after those complete.

@marosset marosset merged commit 0f7564a into Azure:master Oct 22, 2019
@welcome
Copy link

welcome bot commented Oct 22, 2019

Congrats on merging your first pull request! 🎉🎉🎉

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

Successfully merging this pull request may close these issues.

6 participants