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

fix: check network to k8s api for vmss #2938

Merged
merged 7 commits into from Mar 25, 2020

Conversation

bowang-666
Copy link
Contributor

Reason for Change:

We need to also check network connection to k8s api for vmss. Preivous change: #2919

Issue Fixed:

Requirements:

Notes:

@bowang-666 bowang-666 changed the title Check network to k8s api for vmss fix: check network to k8s api for vmss Mar 19, 2020
xuto2
xuto2 previously approved these changes Mar 19, 2020
pkg/engine/virtualmachinescalesets.go Outdated Show resolved Hide resolved
@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 Mar 19, 2020

Codecov Report

Merging #2938 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2938      +/-   ##
==========================================
+ Coverage   72.55%   72.57%   +0.01%     
==========================================
  Files         141      141              
  Lines       25928    25934       +6     
==========================================
+ Hits        18813    18821       +8     
+ Misses       6016     6014       -2     
  Partials     1099     1099              
Impacted Files Coverage Δ
pkg/engine/templates_generated.go 34.82% <ø> (ø)
pkg/engine/virtualmachinescalesets.go 80.58% <100.00%> (ø)
pkg/engine/vmextensions.go 93.59% <100.00%> (ø)
cmd/root.go 59.89% <0.00%> (+2.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13e72f2...9e8e6b6. Read the comment docs.

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

@xuto2 @yangl900 strictly speaking, we could put this k8s apiserver check at the end of CSE instead of the beginning. In other words, there is nothing in the CSE code (let me know if I'm wrong) that won't work until the apiserver is responsive. And so by delaying the start of CSE until we get that response, we may end up delaying the overall cluster operation by 15-20 seconds or so (the time it takes to run CSE in a VHD-backed context).

@xuto2
Copy link
Contributor

xuto2 commented Mar 20, 2020

/lgtm

@xuto2
Copy link
Contributor

xuto2 commented Mar 21, 2020

@jackfrancis AKS does many things after creating ccp and before adding nodes, including setup dns for api server, deploy non-VMSS/non-VMAS resources in ARM, which take a while. So for most case, by the time of cse execution, our api server is already up and this check is a precondition check that we want to fail earlier if node cannot even talk to the api server.

I'm not sure about AKS-Engine scenarios though. Is it possible master node is not up when agent node is running cse when an AKS-Engine cluster is created?

@bowang-666
Copy link
Contributor Author

Actually we discussed and it makes more sense to put it in end, I will make the change.

@acs-bot acs-bot removed the lgtm label Mar 21, 2020
@acs-bot acs-bot added size/M and removed size/S labels Mar 24, 2020
@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jackfrancis
jackfrancis previously approved these changes Mar 24, 2020
Copy link
Member

@jackfrancis jackfrancis 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

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot acs-bot added the lgtm label Mar 25, 2020
@acs-bot
Copy link

acs-bot commented Mar 25, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowang-666, jackfrancis, xuto2

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 jackfrancis merged commit 06d438e into Azure:master Mar 25, 2020
bowang-666 added a commit to bowang-666/aks-engine that referenced this pull request Apr 1, 2020
xuto2 pushed a commit that referenced this pull request Apr 1, 2020
* fix: check network from node to k8s api server in CSE (#2919)

* fix: check network to k8s api for vmss (#2938)

* fix: check dns status before connection (#3002)

Co-authored-by: Bo Wang <bowa@microsoft.com>

Co-authored-by: Bo Wang <bowa@microsoft.com>
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.

None yet

5 participants