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

implement InstanceShutdownByProviderID for azure #68033

Merged
merged 1 commit into from
Sep 8, 2018
Merged

implement InstanceShutdownByProviderID for azure #68033

merged 1 commit into from
Sep 8, 2018

Conversation

yastij
Copy link
Member

@yastij yastij commented Aug 29, 2018 •
edited
Loading

What this PR does / why we need it: implements #66265

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

Special notes for your reviewer:

Release note:

Support NodeShutdown taint for azure

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note-none Denotes a PR that doesn't merit a release note. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 29, 2018
@yastij
Copy link
Member Author

yastij commented Aug 29, 2018 •
edited
Loading

/assign @feiskyer @andyzhangx

can you add this to the 1.12 milestone ?

cc @saad-ali @jingxu97 @gnufied @zetaab

@yastij
Copy link
Member Author

yastij commented Aug 29, 2018

/sig azure

@yastij yastij mentioned this pull request Aug 29, 2018
Closed
andyzhangx
andyzhangx approved these changes Aug 30, 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 lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2018
feiskyer
feiskyer suggested changes Aug 30, 2018
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

This doesn't work for vmss nodes and doesn't use caches which helps to reduces ARM API calls.

Could you add VMSS nodes support and use caches to get provisioning state? e.g. use getVmssVM() for VMSS and getVirtualMachine() for VMAS.

@feiskyer
Copy link
Member

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2018
@zetaab zetaab mentioned this pull request Sep 3, 2018
Closed
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 3, 2018
@yastij yastij force-pushed the instanceShutdown-azure branch 2 times, most recently from 2ee943b to 1752278 Compare September 3, 2018 21:09
feiskyer
feiskyer reviewed Sep 4, 2018
View reviewed changes
pkg/cloudprovider/providers/azure/azure_instances.go Outdated
return false, cloudprovider.NotImplemented

nodeName, err := az.vmSet.GetNodeNameByProviderID(providerID)

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove empty lines before if err != nil.

same below

feiskyer
feiskyer reviewed Sep 4, 2018
View reviewed changes
pkg/cloudprovider/providers/azure/azure_fakes.go Outdated
func (f *fakeVMSet) GetProvisioningStateByNodeName(name string) (string, error) {
return "", fmt.Errorf("unimplemented")
}

Copy link
Member

Choose a reason for hiding this comment

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

please gofmt this file

andyzhangx
andyzhangx reviewed Sep 4, 2018
View reviewed changes
pkg/cloudprovider/providers/azure/azure_instances.go Outdated
return false, err
}

return provisioningState == "Stopped" || provisioningState == "Deallocated", nil
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest all using lower case to do string comparison, to prevent case change in the future.

pkg/cloudprovider/providers/azure/azure_standard.go Outdated
@@ -346,6 +346,17 @@ func (as *availabilitySet) GetInstanceIDByNodeName(name string) (string, error)
return *machine.ID, nil
}

func (as *availabilitySet) GetProvisioningStateByNodeName(name string) (provisioningState string, err error) {

Copy link
Member

Choose a reason for hiding this comment

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

pls remove unncessary empty lines

@yastij
Copy link
Member Author

yastij commented Sep 4, 2018

@feiskyer @andyzhangx - fixe PTAL so we can ship this for 1.12, Thanks !

@yastij
Copy link
Member Author

yastij commented Sep 4, 2018

/retest pull-kubernetes-e2e-kops-aws

andyzhangx
andyzhangx reviewed Sep 5, 2018
View reviewed changes
pkg/cloudprovider/providers/azure/azure_standard.go Outdated
@@ -346,6 +346,16 @@ func (as *availabilitySet) GetInstanceIDByNodeName(name string) (string, error)
return *machine.ID, nil
}

func (as *availabilitySet) GetProvisioningStateByNodeName(name string) (provisioningState string, err error) {
vm, err := as.getVirtualMachine(types.NodeName(name))

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

pkg/cloudprovider/providers/azure/azure_vmss.go Outdated
@@ -128,6 +128,18 @@ func (ss *scaleSet) getVmssVM(nodeName string) (ssName, instanceID string, vm co
return ssName, instanceID, *(cachedVM.(*compute.VirtualMachineScaleSetVM)), nil
}

func (ss *scaleSet) GetProvisioningStateByNodeName(name string) (provisioningState string, err error) {

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

pkg/cloudprovider/providers/azure/azure_vmss.go Outdated
func (ss *scaleSet) GetProvisioningStateByNodeName(name string) (provisioningState string, err error) {

_, _, vm, err := ss.getVmssVM(name)

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

pkg/cloudprovider/providers/azure/azure_vmss.go Outdated
}

return *vm.ProvisioningState, nil

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

feiskyer
feiskyer approved these changes Sep 5, 2018
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

LGTM now, please remove the unnecessary empty lines.

@feiskyer
Copy link
Member

feiskyer commented Sep 5, 2018

/milestone v1.12

@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Sep 5, 2018
@feiskyer
Copy link
Member

feiskyer commented Sep 7, 2018

/lgtm
/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 8, 2018
@yastij
Copy link
Member Author

yastij commented Sep 8, 2018

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 8, 2018
@k8s-github-robot
Copy link

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

@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: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit 10bde50 into kubernetes:master Sep 8, 2018
@yastij yastij deleted the instanceShutdown-azure branch September 8, 2018 18:38
@marpaia marpaia mentioned this pull request Sep 20, 2018
Merged
@feiskyer feiskyer mentioned this pull request Sep 21, 2018
Merged
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

@feiskyer feiskyer feiskyer approved these changes

@khenidak khenidak Awaiting requested review from khenidak

Assignees

@feiskyer feiskyer

@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. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

Support InstanceShutdownByProviderID for Azure
5 participants
@yastij @feiskyer @k8s-ci-robot @k8s-github-robot @andyzhangx