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.

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

Add cache for VirtualMachinesClient.Get in azure cloud provider #57432

Merged
merged 1 commit into from Jan 2, 2018

Conversation

@karataliu
Copy link
Contributor

commented Dec 20, 2017

What this PR does / why we need it:
Add a timed cache for 'VirtualMachinesClient.Get'

Currently cloud provider will send several get calls to same URL in short period, which is not necessary.

Which issue(s) this PR fixes:
Fixes #57031

Special notes for your reviewer:

Release note:

Add cache for VM get operation in azure cloud provider
@karataliu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

@jhorwit2

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

/sig azure
/area cloudprovider
/ok-to-test

@feiskyer

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

/release-note-none

@@ -54,25 +56,72 @@ func ignoreStatusNotFoundFromError(err error) error {
return err
}

// cache used by getVirtualMachine
var vmCache = cache.New(15*time.Second, 10*time.Minute)

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Dec 20, 2017

Member

what's 10*time.Minute for? add comments for expiration time explaination

This comment has been minimized.

Copy link
@karataliu

karataliu Dec 20, 2017

Author Contributor

Updated

@karataliu karataliu force-pushed the karataliu:azure_vmget_cache branch from 0fd2cbf to 453a166 Dec 20, 2017

@@ -54,25 +56,74 @@ func ignoreStatusNotFoundFromError(err error) error {
return err
}

// cache used by getVirtualMachine
// 15s for expiration duration, 10m for proactive cleanup interval
var vmCache = cache.New(15*time.Second, 10*time.Minute)

This comment has been minimized.

Copy link
@jhorwit2

This comment has been minimized.

Copy link
@brendandburns

brendandburns Dec 21, 2017

Contributor

yeah, seems better to use a library we're already using... thanks!

This comment has been minimized.

Copy link
@karataliu

karataliu Dec 22, 2017

Author Contributor

Thanks for pointing out. Updated.

@brendandburns

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

This LGTM to me in general, but I would try to re-use the existing cache.

Additionally, we should get a unit test of the caching behavior, if possible.

@karataliu karataliu force-pushed the karataliu:azure_vmget_cache branch from 453a166 to a8cdeb4 Dec 22, 2017

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Dec 22, 2017

@karataliu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

Thanks all for reviewing.
I've add a new cache implementation based on client-go's cache, plus some unit tests. PTAL.

@jdumars

This comment has been minimized.

Copy link
Member

commented Dec 26, 2017

@feiskyer
Copy link
Member

left a comment

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: #57031

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented Jan 2, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

3 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Jan 2, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot

This comment has been minimized.

Copy link

commented Jan 2, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot

This comment has been minimized.

Copy link

commented Jan 2, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@khenidak

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

Hey @karataliu - Thanks for this. few points to discuss:

  1. Can we convert this cache to a general purpose not just vms, While the provider is heavy on VM calls, its does alot more calls to network provider. and I want to use the same for network.

  2. can we shift VM.Request to a general request locking (i.e. fine grained locking item locking)?

  3. few code comments.

@fejta-bot

This comment has been minimized.

Copy link

commented Jan 2, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

Automatic merge from submit-queue (batch tested with PRs 49856, 56257, 57027, 57695, 57432). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f918d18 into kubernetes:master Jan 2, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation karataliu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@karataliu

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

@khenidak That's doable. Now that we have timedcache, the work is to add a resourceRequest on top of it. It'll first depend on identifying which API calls have such requirements, then we can set up cache upon them.

@karataliu karataliu deleted the karataliu:azure_vmget_cache branch Jan 3, 2018

k8s-github-robot pushed a commit that referenced this pull request Jan 5, 2018

Kubernetes Submit Queue
Merge pull request #57777 from karataliu/azure_vm_get
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Uniform Azure VM api calls

**What this PR does / why we need it**:
There is still a call to 'VirtualMachinesClient.Get' directly in azure_backoff, which does not go through the cache approach.

This PR uniforms all calls for getting azure vm to use 'getVirtualMachine'. Also refine unused 'exists' return value.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Related #57031
Follow-up #57432

**Special notes for your reviewer**:

**Release note**:
```release-note
NONE
```

k8s-github-robot pushed a commit that referenced this pull request Jan 17, 2018

Kubernetes Submit Queue
Merge pull request #58313 from karataliu/automated-cherry-pick-of-#57…
…432-#57777-#57994-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #57432 #57777 #57994 upstream release 1.9

**What this PR does / why we need it**:
Cherry pick of #57432 #57777 #57994 onto release 1.9

Add VM get cache to avoid api throttling, this helps: allow cloud-controller-manager initialization and also saves other VM get api calls.

**Which issue(s) this PR fixes**
Related #57031

**Release note**:
```release-note
Add cache for VM get operation in azure cloud provider
```


cc @andyzhangx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.