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 node affinity for Azure unzoned managed disks #67229

Merged
merged 3 commits into from Aug 15, 2018

Conversation

@feiskyer
Copy link
Member

feiskyer commented Aug 10, 2018

What this PR does / why we need it:

Continue of Azure Availability Zone feature.

Add node affinity for Azure unzoned managed disks, so that unzoned disks only scheduled to unzoned nodes.

This is required because Azure doesn't allow attaching unzoned disks to zoned VMs.

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:

Unzoned nodes would label failure-domain.beta.kubernetes.io/zone=0 and the value is fault domain ( while availability zone is used for zoned nodes). So fault domain is used to populate unzoned disks.

Since there are at most 3 fault domains in each region, the PR adds 3 terms for them:

kubectl describe pv pvc-bdf93a67-9c45-11e8-ba6f-000d3a07de8c
Name:              pvc-bdf93a67-9c45-11e8-ba6f-000d3a07de8c
Labels:            <none>
Annotations:       pv.kubernetes.io/bound-by-controller=yes
                   pv.kubernetes.io/provisioned-by=kubernetes.io/azure-disk
                   volumehelper.VolumeDynamicallyCreatedByKey=azure-disk-dynamic-provisioner
Finalizers:        [kubernetes.io/pv-protection]
StorageClass:      azuredisk-unzoned
Status:            Bound
Claim:             default/unzoned-pvc
Reclaim Policy:    Delete
Access Modes:      RWO
Capacity:          5Gi
Node Affinity:     
  Required Terms:  
    Term 0:        failure-domain.beta.kubernetes.io/region in [southeastasia]
                   failure-domain.beta.kubernetes.io/zone in [0]
    Term 1:        failure-domain.beta.kubernetes.io/region in [southeastasia]
                   failure-domain.beta.kubernetes.io/zone in [1]
    Term 2:        failure-domain.beta.kubernetes.io/region in [southeastasia]
                   failure-domain.beta.kubernetes.io/zone in [2]
Message:           
Source:
    Type:         AzureDisk (an Azure Data Disk mount on the host and bind mount to the pod)
    DiskName:     k8s-5b3d7b8f-dynamic-pvc-bdf93a67-9c45-11e8-ba6f-000d3a07de8c
    DiskURI:      /subscriptions/<subscription>/resourceGroups/<rg-name>/providers/Microsoft.Compute/disks/k8s-5b3d7b8f-dynamic-pvc-bdf93a67-9c45-11e8-ba6f-000d3a07de8c
    Kind:         Managed
    FSType:       
    CachingMode:  None
    ReadOnly:     false
Events:           <none>

Release note:

Add node affinity for Azure unzoned managed disks

/sig azure
/kind feature

/cc @brendandburns @khenidak @andyzhangx @msau42

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 10, 2018

/retest

2 similar comments
@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 11, 2018

/retest

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 13, 2018

/retest

// This is required because unzoned AzureDisk can't be attached to zoned nodes.
// There are at most 3 fault domains available in each region.
// Refer https://docs.microsoft.com/en-us/azure/virtual-machines/windows/manage-availability.
for i := 0; i < 3; i++ {

This comment has been minimized.

@ddebroy

ddebroy Aug 13, 2018

Member

Is there a way to get the number of fault domains dynamically (e.g. from Azure metadata) instead of hardcoding it to 3 here? Some of the regions do not appear to have 3 fault domains like West Central US, Canada East according to the link above.

This comment has been minimized.

@feiskyer

feiskyer Aug 13, 2018

Author Member

There is no API for such use cases. 3 works for all regions, so it won't break any clusters, even with 2 FDs.

This comment has been minimized.

@msau42

msau42 Aug 13, 2018

Member

Is there any chance in the future that you would have more than 3 fault domains?

MatchExpressions: requirements,
})
} else {
// Set node affinity labels based on fault domains.

This comment has been minimized.

@ddebroy

ddebroy Aug 13, 2018

Member

Is it going to be legal to have AllowedTopology set for un-zoned AzureDisks (with zones set to fault domains)? It seems that should not be allowed based on the logic below. If so, should it be checked for and rejected?

This comment has been minimized.

@feiskyer

feiskyer Aug 13, 2018

Author Member

Good catch. Added in the new commit.

@feiskyer feiskyer force-pushed the feiskyer:unzoned-disks branch from d5ba5f4 to 9e6ad14 Aug 13, 2018

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 13, 2018

ping @andyzhangx for review

@andyzhangx
Copy link
Member

andyzhangx left a comment

lgtm in general, I will wait for @ddebroy comments, thanks.

nodeSelectorTerms := make([]v1.NodeSelectorTerm, 0)

if zoned {
// Set node affinity labels based availability zone labels.

This comment has been minimized.

@andyzhangx

andyzhangx Aug 13, 2018

Member

based on

This comment has been minimized.

@feiskyer

feiskyer Aug 13, 2018

Author Member

Fixed.

@feiskyer feiskyer force-pushed the feiskyer:unzoned-disks branch from 9e6ad14 to 145cc41 Aug 13, 2018

@ddebroy

This comment has been minimized.

Copy link
Member

ddebroy commented Aug 13, 2018

lgtm. Is there going to be a followup PR for unmanaged disks where nodeAffinity is populated in a similar fashion as for managed non-zoned disks?

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 14, 2018

lgtm. Is there going to be a followup PR for unmanaged disks where nodeAffinity is populated in a similar fashion as for managed non-zoned disks?

@ddebroy Thanks and good catch. Let add address this within this PR.

feiskyer added some commits Aug 9, 2018

@feiskyer feiskyer force-pushed the feiskyer:unzoned-disks branch from 145cc41 to dbdfd0f Aug 14, 2018

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 14, 2018

Added in the new commit. @andyzhangx @ddebroy PTAL

@ddebroy

This comment has been minimized.

Copy link
Member

ddebroy commented Aug 14, 2018

lgtm ... thank you!

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 14, 2018

/retest

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Aug 14, 2018

/test pull-kubernetes-e2e-kops-aws

@andyzhangx
Copy link
Member

andyzhangx left a comment

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 15, 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 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 Aug 15, 2018

Automatic merge from submit-queue (batch tested with PRs 66884, 67410, 67229, 67409). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 2a81c37 into kubernetes:master Aug 15, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
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:unzoned-disks branch Aug 15, 2018

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.