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.

Sign up for GitHub

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

Jump to bottom

Fix aggressive VM calls for Azure VMSS #83102

Merged
merged 1 commit into from
Sep 29, 2019
Merged

Fix aggressive VM calls for Azure VMSS #83102

merged 1 commit into from
Sep 29, 2019

Conversation

feiskyer
Copy link
Member

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Fix aggressive VM calls for Azure VMSS by

  • Replacing GET VM with LIST VM so as to reduce the number of API calls
  • Do not LIST VMs when VMSS capacity is 0

Which issue(s) this PR fixes:

Fixes #82948

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix aggressive VM calls for Azure VMSS

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

/kind bug
/area provider/azure
/priority critial-urgent
/assign @andyzhangx

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 25, 2019
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/provider/azure Issues or PRs related to azure provider labels Sep 25, 2019
@k8s-ci-robot
Copy link
Contributor

@feiskyer: The label(s) priority/critial-urgent cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Fix aggressive VM calls for Azure VMSS by

  • Replacing GET VM with LIST VM so as to reduce the number of API calls
  • Do not LIST VMs when VMSS capacity is 0

Which issue(s) this PR fixes:

Fixes #82948

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix aggressive VM calls for Azure VMSS

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

/kind bug
/area provider/azure
/priority critial-urgent
/assign @andyzhangx

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 25, 2019
@feiskyer
Copy link
Member Author

/priority critical-urgent
/sig cloud-provider

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 25, 2019
@feiskyer
Copy link
Member Author

/retest

@feiskyer
Copy link
Member Author

/test pull-kubernetes-e2e-aks-engine-azure
/test pull-kubernetes-node-e2e

@feiskyer
Copy link
Member Author

/test pull-kubernetes-e2e-aks-engine-azure

@feiskyer
Copy link
Member Author

/retest

andyzhangx
andyzhangx approved these changes Sep 29, 2019
View reviewed changes
Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 merged commit 77d0ebe into kubernetes:master Sep 29, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 29, 2019
@feiskyer feiskyer deleted the fix-too-many-calls branch September 29, 2019 03:15
nilo19 pushed a commit to nilo19/kubernetes that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request kubernetes#83102 from feiskyer/fix-too-many-calls
20fd43c 
Fix aggressive VM calls for Azure VMSS
@nilo19 nilo19 mentioned this pull request Oct 9, 2019
Merged
nilo19 pushed a commit to nilo19/kubernetes that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request kubernetes#83102 from feiskyer/fix-too-many-calls
f1b628a 
Fix aggressive VM calls for Azure VMSS
@nilo19 nilo19 mentioned this pull request Oct 9, 2019
Merged
nilo19 pushed a commit to nilo19/kubernetes that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request kubernetes#83102 from feiskyer/fix-too-many-calls
5237945 
Fix aggressive VM calls for Azure VMSS
@nilo19 nilo19 mentioned this pull request Oct 9, 2019
Merged
nilo19 pushed a commit to nilo19/kubernetes that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request kubernetes#83102 from feiskyer/fix-too-many-calls
d763cf1 
Fix aggressive VM calls for Azure VMSS
@nilo19 nilo19 mentioned this pull request Oct 9, 2019
Merged
k8s-ci-robot added a commit that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request #83654 from nilo19/qi-cherry-pick-83102-release1.13
a8b5220 
Cherry pick of #83102: Fix aggressive VM calls for Azure VMSS.
k8s-ci-robot added a commit that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request #83655 from nilo19/qi-cherry-pick-83102-release1.14
f89b0b2 
Cherry pick of #83102: Fix aggressive VM calls for Azure VMSS.
k8s-ci-robot added a commit that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request #83656 from nilo19/qi-cherry-pick-83102-release1.15
fdc55db 
Cherry pick of #83102: Fix aggressive VM calls for Azure VMSS.
k8s-ci-robot added a commit that referenced this pull request Oct 9, 2019
@k8s-ci-robot
Merge pull request #83657 from nilo19/qi-cherry-pick-83102-release1.16
eb7c4c5 
Cherry pick of #83102: Fix aggressive VM calls for Azure VMSS.
andyzhangx
andyzhangx reviewed Nov 12, 2019
View reviewed changes
staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go
@@ -86,8 +86,9 @@ func (ss *scaleSet) AttachDisk(isManagedDisk bool, diskName, diskURI string, nod
defer cancel()

// Invalidate the cache right after updating
key := buildVmssCacheKey(nodeResourceGroup, ss.makeVmssVMName(ssName, instanceID))
defer ss.vmssVMCache.Delete(key)
if err = ss.deleteCacheForNode(vmName); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

looks like I got the root cause why it's dirty cache only in vmss, we should use defer here @feiskyer

@andyzhangx andyzhangx mentioned this pull request Nov 12, 2019
Closed
@andyzhangx andyzhangx mentioned this pull request Dec 6, 2019
Closed
@andyzhangx
Copy link
Member

found another regression due to this PR: #69262 (comment), it's due to the getVMSS ttl changed from 1min to 10min, fortunately there is a refactor from 1.15.x, won't have that issue from 1.15.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyzhangx andyzhangx andyzhangx approved these changes

@brendandburns brendandburns Awaiting requested review from brendandburns

@khenidak khenidak Awaiting requested review from khenidak

Assignees

@andyzhangx andyzhangx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider area/provider/azure Issues or PRs related to azure provider 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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

Empty Azure VM scale sets consume vmss_list API needlessly
3 participants
@feiskyer @k8s-ci-robot @andyzhangx