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 generic cache for Azure VMSS #59652

Merged
merged 3 commits into from Feb 12, 2018

Conversation

@feiskyer
Member

feiskyer commented Feb 9, 2018

What this PR does / why we need it:

This PR adds a generic cache for VMSS and removes old list-based cache.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Continue of ##58770.

Special notes for your reviewer:

Depends on #59520.

Release note:

Add generic cache for Azure VMSS

@k8s-ci-robot k8s-ci-robot requested review from colemickens and khenidak Feb 9, 2018

@feiskyer feiskyer requested a review from karataliu Feb 9, 2018

@feiskyer feiskyer added this to the v1.10 milestone Feb 9, 2018

@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 9, 2018

How is this different from #59520 ?

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 10, 2018

@khenidak This PR adds cache for vmss resources based generic cache (which is introduced in #59520)

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels Feb 11, 2018

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 11, 2018

Rebased, and only kept vmss related codes in the PR

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 11, 2018

/retest

@andyzhangx

about the func naming of azure_vmss_cache.go, I would prefer to make main functions as external, e.g. newVmssCache --> NewVmssCache since we may let other azure_** packages also use these cache code in the future.

return nil, nil
}
result, err := ss.VirtualMachineScaleSetVMsClient.Get(ss.ResourceGroup, ssName, instanceID)

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

no check err?

This comment has been minimized.

@feiskyer

feiskyer Feb 12, 2018

Member

See lines below

return fmt.Sprintf("%s_%s", scaleSetName, instanceID)
}
func (ss *scaleSet) extractVmssVMName(name string) (string, string, error) {

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

I think also needs to add test for extractVmssVMName

This comment has been minimized.

@feiskyer
}
func (ss *scaleSet) extractVmssVMName(name string) (string, string, error) {
ret := strings.Split(name, "_")

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

use a const for seperator

This comment has been minimized.

@feiskyer
for _, vm := range vms {
if vm.OsProfile == nil || vm.OsProfile.ComputerName == nil {
glog.Warningf("failed to get computerName for vmssVM (%q)", *vm.Name)

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

need to check vm.Name != null since vm.OsProfile is nil otherwise there is crash, and why continue here, not break?

This comment has been minimized.

@feiskyer

feiskyer Feb 12, 2018

Member

If the vm is deleting, then this field may be nil. Continue to fill other VMs. Note that this is for caching, we should cache as much as possible.

type nodeNameToScaleSetMapping map[string]string
func (ss *scaleSet) makeVmssVMName(scaleSetName, instanceID string) string {
return fmt.Sprintf("%s_%s", scaleSetName, instanceID)

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

use const separator for _

This comment has been minimized.

@feiskyer
@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 12, 2018

about the func naming of azure_vmss_cache.go, I would prefer to make main functions as external, e.g. newVmssCache --> NewVmssCache since we may let other azure_** packages also use these cache code in the future.

All other cache interfaces are also not exported. we can do this later when required.

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 12, 2018

@andyzhangx Addressed comments. PTAL

@andyzhangx

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 12, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 12, 2018

[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 OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 12, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 12, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 69324f9 into kubernetes:master Feb 12, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation feiskyer 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 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

@feiskyer feiskyer deleted the feiskyer:vmss-cache branch Feb 12, 2018

@khenidak khenidak referenced this pull request Feb 13, 2018

Open

Relevant PRs/Issues #1

k8s-merge-robot added a commit that referenced this pull request Feb 14, 2018

Merge pull request #59716 from feiskyer/vmss-disk
Automatic merge from submit-queue (batch tested with PRs 59489, 59716). 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>.

Add AzureDisk support for vmss nodes

**What this PR does / why we need it**:

This PR adds AzureDisk support for vmss nodes. Changes include

- Upgrade vmss API to 2017-12-01
- Upgrade vmss clients with new version API
- Abstract AzureDisk operations for vmss and vmas
- Added AzureDisk support for vmss
- Unit tests and fake clients fix
 
**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 #43287

**Special notes for your reviewer**:

~~Depending on #59652 (the first two commits are from #59652).~~

**Release note**:

```release-note
Add AzureDisk support for vmss nodes
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment