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 initial availability zones support for Azure nodes #66242

Merged
merged 2 commits into from Jul 20, 2018

Conversation

@feiskyer
Copy link
Member

feiskyer commented Jul 16, 2018

What this PR does / why we need it:

The first part of Azure Availability Zone feature.

This PR adds initial availability zone (AZ) support for Azure nodes. With this PR, Azure nodes with AZ will have label failure-domain.beta.kubernetes.io/zone=<region>-<zoneID>, e.g. southeastasia-1.

It also updates instance metadata api-version to 2017-12-01, which is required for AZ.

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 #

Special notes for your reviewer:

VirtualMachineScaleSetVM doesn't have AZ info yet. It will be supported later after new Azure Go SDK releases.

Release note:

Azure nodes with availability zone now will have label `failure-domain.beta.kubernetes.io/zone=<region>-<zoneID>`.

/kind feature
/sig azure

/assign @brendandburns @khenidak @andyzhangx

@k8s-ci-robot k8s-ci-robot requested review from brendandburns and jdumars Jul 16, 2018

@feiskyer feiskyer changed the title Instance az Add initial availability zones support for Azure nodes Jul 16, 2018

@@ -212,6 +212,8 @@ func (ss *scaleSet) GetInstanceTypeByNodeName(name string) (string, error) {
}

// GetZoneByNodeName gets cloudprovider.Zone by node name.
// TODO(feiskyer): Add availability zone support of VirtualMachineScaleSetVM
// after it is supported in Azure Go SDK.

This comment has been minimized.

@feiskyer

feiskyer Jul 16, 2018

Author Member

Note: this is used in cloud-controller-manager. It is waiting for new Azure Go SDK releases.

@andyzhangx
Copy link
Member

andyzhangx left a comment

leave some comments first

// Get availability zone for the node.
if vm.Zones != nil && len(*vm.Zones) > 0 {
zones := *vm.Zones
zoneID, err := strconv.Atoi(zones[0])

This comment has been minimized.

@andyzhangx

andyzhangx Jul 17, 2018

Member

Is it possible that a VM has multiple zones?

This comment has been minimized.

@feiskyer

feiskyer Jul 17, 2018

Author Member

No, a VM must be in one exact zone (while there could multiple zones in scaleset itself).

@@ -415,6 +415,21 @@ func (as *availabilitySet) GetZoneByNodeName(name string) (cloudprovider.Zone, e
return cloudprovider.Zone{}, err
}

// Get availability zone for the node.

This comment has been minimized.

@andyzhangx

andyzhangx Jul 17, 2018

Member

would you also update the GetZoneByNodeName func comments?

This comment has been minimized.

@feiskyer

feiskyer Jul 17, 2018

Author Member

ack, yep

@@ -212,6 +212,8 @@ func (ss *scaleSet) GetInstanceTypeByNodeName(name string) (string, error) {
}

// GetZoneByNodeName gets cloudprovider.Zone by node name.
// TODO(feiskyer): Add availability zone support of VirtualMachineScaleSetVM
// after it is supported in Azure Go SDK.

This comment has been minimized.

@andyzhangx

andyzhangx Jul 17, 2018

Member

would you also add the Azure Go SDK link as it's not ready yet?

This comment has been minimized.

@feiskyer

feiskyer Jul 17, 2018

Author Member

oops, the pr should be included

@feiskyer feiskyer force-pushed the feiskyer:instance-az branch from 8329f7c to 1ba2346 Jul 17, 2018

return cloudprovider.Zone{}, fmt.Errorf("failed to parse zone %q: %v", zones, err)
}

return cloudprovider.Zone{

This comment has been minimized.

@andyzhangx

andyzhangx Jul 17, 2018

Member

could you combine following code , only failureDomain value is different between availability zone and , logic is like following:

if (availability zone) {
  failureDomain = ...
}
else {
   failureDomain = ...
}

This comment has been minimized.

@feiskyer

feiskyer Jul 17, 2018

Author Member

ack, added.

feiskyer added some commits Jul 16, 2018

@feiskyer feiskyer force-pushed the feiskyer:instance-az branch from 1ba2346 to e593fb9 Jul 17, 2018

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jul 18, 2018

/retest

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jul 18, 2018

ping @brendandburns @khenidak @andyzhangx any more comments?

@khenidak
Copy link
Contributor

khenidak left a comment

One comment other than that it is looking good. Thanks @feiskyer

@@ -408,14 +408,29 @@ func (as *availabilitySet) GetInstanceTypeByNodeName(name string) (string, error
return string(machine.HardwareProfile.VMSize), nil
}

// GetZoneByNodeName gets zone from instance view.
// GetZoneByNodeName gets availability zone for the specified node. If the node is not running
// with availability zone, then it returns fault domain.
func (as *availabilitySet) GetZoneByNodeName(name string) (cloudprovider.Zone, error) {
vm, err := as.getVirtualMachine(types.NodeName(name))

This comment has been minimized.

@khenidak

khenidak Jul 18, 2018

Contributor

Why don't we use instance metadata for this as well similar to vmss?

This comment has been minimized.

@feiskyer

feiskyer Jul 18, 2018

Author Member

there're two ways actually should be supported:

  • if cloud-controller-manager is used, then GetZoneByProviderID is used, which calls GetZoneByNodeName internally. Metadata won't work for this way because cloud-controller-manager is a master component.
  • If kube-controller-manager is used, then GetZone() is used, which calls instance metadata to fetch zone.
@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jul 20, 2018

@khenidak

This comment has been minimized.

Copy link
Contributor

khenidak commented Jul 20, 2018

/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 20, 2018

@andyzhangx
Copy link
Member

andyzhangx left a comment

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 20, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 20, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 20, 2018

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 b68c944 into kubernetes:master Jul 20, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
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-100-performance 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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@feiskyer feiskyer deleted the feiskyer:instance-az branch Jul 20, 2018

k8s-github-robot pushed a commit that referenced this pull request Jul 27, 2018

Kubernetes Submit Queue
Merge pull request #66648 from feiskyer/azure-sdk-update
Automatic merge from submit-queue (batch tested with PRs 66225, 66648, 65799, 66630, 66619). 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>.

Update Azure Go SDK to v19.0.0 and get availability zone for VirtualMachineScaleSetVM

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

Continue of #66242. This PR updates Azure Go SDK to v19.0.0 (with compute API 2018-04-01) and gets availability zones for VirtualMachineScaleSetVM.

**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 #

**Special notes for your reviewer**:

**Release note**:

```release-note
Azure Go SDK has been upgraded to v19.0.0 and VirtualMachineScaleSetVM now supports availability zones.
```

/sig azure
/assign @brendandburns @khenidak @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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.