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

Reduce API calls for Azure instance metadata #67478

Merged
merged 2 commits into from
Aug 23, 2018
Merged

Reduce API calls for Azure instance metadata #67478

merged 2 commits into from
Aug 23, 2018

Conversation

feiskyer
Copy link
Member

What this PR does / why we need it:

Azure cloud provider gets a lot of "Too many requests" error when getting availability zones, instance types and node addresses. Hence kubelet won't be able to initialize itself sometimes.

This PR reduces such calls and alos switches to json API which is more stable.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes Azure/acs-engine#3681

Special notes for your reviewer:

Release note:

Reduce API calls for Azure instance metadata.

cc @ritazh @khenidak @andyzhangx

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 16, 2018
@feiskyer
Copy link
Member Author

/sig azure

@feiskyer
Copy link
Member Author

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 16, 2018
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2018
@feiskyer feiskyer mentioned this pull request Aug 16, 2018
Closed
@andyzhangx
Copy link
Member

Do we have any rate limit doc about instance metadata feature @khenidak , it's a little weird for this fix: az.metadata.Object call has more rate limit than az.metadata.Text call, so we use az.metadata.Object call for this fix, I doubt in the future, we may hit such issue again...

Related calls are under here:

kubernetes/pkg/cloudprovider/providers/azure/azure_instance_metadata.go

Lines 75 to 76 in 59fdc02

func (i *InstanceMetadata) Object(path string, obj interface{}) error {
data, err := i.queryMetadataBytes(path, "json")

kubernetes/pkg/cloudprovider/providers/azure/azure_instance_metadata.go

Lines 84 to 85 in 59fdc02

func (i *InstanceMetadata) Text(path string) (string, error) {
data, err := i.queryMetadataBytes(path, "text")

@ritazh
Copy link
Member

ritazh commented Aug 16, 2018

@feiskyer why is this causing kube-scheduler to not even start?

@khenidak
Copy link
Contributor

I honestly think we should cache this call. Zone, ip, vm name etc will not change that frequently. I do second @andyzhangx comments wrt not understanding the difference between Text and Object - the number of calls is the same either way, right?

@feiskyer
Copy link
Member Author

@khenidak This PR reduces the original two calls to one: isCurrentInstance() will also make a API call while it's removed in this PR. And from my testing, Object way did reduce "Too many requests" error in kubelet logs. While with Text() way, there're a few such errors on kubelet start.

@ritazh Kubelet won't initialize itself successfully, hence some operations are not working as expected.

@feiskyer feiskyer added this to the v1.12 milestone Aug 22, 2018
@andyzhangx
Copy link
Member

@feiskyer I am ok if there is no better fix for this issue since v1.12 code freeze date is approaching.
@khenidak @ritazh any comments on this?

@ritazh
Copy link
Member

ritazh commented Aug 22, 2018

will test this first thing tomorrow morning

@andyzhangx
Copy link
Member

@ritazh that's great, then we will wait for your comment.

@feiskyer
Copy link
Member Author

@andyzhangx We may introduce caches for them in the future (which needs more work because we should cache all metadata requests), but now changing to json API does work to fix current issues.

@ritazh Thanks.

@ritazh
Copy link
Member

ritazh commented Aug 22, 2018

confirming this fixes Azure/acs-engine#3681

andyzhangx
andyzhangx approved these changes Aug 23, 2018
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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2018
@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-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@feiskyer
Copy link
Member Author

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 78fc9bf into kubernetes:master Aug 23, 2018
@feiskyer feiskyer deleted the zone-fix branch August 23, 2018 09:06
@feiskyer feiskyer mentioned this pull request Sep 26, 2018
Closed
@jackfrancis jackfrancis mentioned this pull request Apr 16, 2019
Merged
4 tasks
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

Assignees

@andyzhangx andyzhangx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

kube-scheduler is never created using k8s 1.12 and vmss
6 participants
@feiskyer @andyzhangx @ritazh @khenidak @k8s-ci-robot @k8s-github-robot