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

Add node affinity for Azure unzoned managed disks #67229

Merged
merged 3 commits into from
Aug 15, 2018
Merged

Add node affinity for Azure unzoned managed disks #67229

merged 3 commits into from
Aug 15, 2018

Conversation

feiskyer
Copy link
Member

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/azure kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 10, 2018
@feiskyer
Copy link
Member Author

/retest

2 similar comments
@feiskyer
Copy link
Member Author

/retest

@feiskyer
Copy link
Member Author

/retest

ddebroy
ddebroy reviewed Aug 13, 2018
View reviewed changes
pkg/volume/azure_dd/azure_provision.go
// 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++ {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@feiskyer feiskyer Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

ddebroy
ddebroy reviewed Aug 13, 2018
View reviewed changes
pkg/volume/azure_dd/azure_provision.go
MatchExpressions: requirements,
})
} else {
// Set node affinity labels based on fault domains.
Copy link
Member

@ddebroy ddebroy Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Added in the new commit.

@feiskyer
Copy link
Member Author

ping @andyzhangx for review

andyzhangx
andyzhangx reviewed Aug 13, 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 in general, I will wait for @ddebroy comments, thanks.

pkg/volume/azure_dd/azure_provision.go Outdated
nodeSelectorTerms := make([]v1.NodeSelectorTerm, 0)

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

Choose a reason for hiding this comment

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

based on

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@ddebroy
Copy link
Member

ddebroy commented Aug 13, 2018 •
edited
Loading

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
Copy link
Member Author

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
Copy link
Member Author

Added in the new commit. @andyzhangx @ddebroy PTAL

@ddebroy
Copy link
Member

ddebroy commented Aug 14, 2018

lgtm ... thank you!

@feiskyer
Copy link
Member Author

/retest

@feiskyer
Copy link
Member Author

/test pull-kubernetes-e2e-kops-aws

andyzhangx
andyzhangx approved these changes Aug 15, 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 the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2018
@k8s-ci-robot
Copy link
Contributor

[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
Copy link

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
@feiskyer feiskyer deleted the unzoned-disks branch August 15, 2018 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddebroy ddebroy ddebroy left review comments

@andyzhangx andyzhangx andyzhangx approved these changes

@brendandburns brendandburns Awaiting requested review from brendandburns

@khenidak khenidak Awaiting requested review from khenidak

@msau42 msau42 Awaiting requested review from msau42

Assignees

@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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@feiskyer @ddebroy @k8s-ci-robot @k8s-github-robot @andyzhangx @msau42