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

fix device name change issue for azure disk #60346

Merged
merged 1 commit into from
Feb 25, 2018
Merged

fix device name change issue for azure disk #60346

merged 1 commit into from
Feb 25, 2018

Conversation

andyzhangx
Copy link
Member

@andyzhangx andyzhangx commented Feb 24, 2018 •
edited
Loading

What this PR does / why we need it:
fix device name change issue for azure disk due to default host cache setting changed from None to ReadWrite from v1.7, and default host cache setting in azure portal is None

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 #60344, #57444
also fixes following issues:
Azure/acs-engine#1918
Azure/AKS#201

Special notes for your reviewer:
From v1.7, default host cache setting changed from None to ReadWrite, this would lead to device name change after attach multiple disks on azure vm, finally lead to disk unaccessiable from pod.
For an example:
statefulset with 8 replicas(each with an azure disk) on one node will always fail, according to my observation, add the 6th data disk will always make dev name change, some pod could not access data disk after that.

I have verified this fix on v1.8.4
Without this PR on one node(dev name changes):

azureuser@k8s-agentpool2-40588258-0:~$ tree /dev/disk/azure
...
└── scsi1
    ├── lun0 -> ../../../sdk
    ├── lun1 -> ../../../sdj
    ├── lun2 -> ../../../sde
    ├── lun3 -> ../../../sdf
    ├── lun4 -> ../../../sdg
    ├── lun5 -> ../../../sdh
    └── lun6 -> ../../../sdi

With this PR on one node(no dev name change):

azureuser@k8s-agentpool2-40588258-1:~$ tree /dev/disk/azure
...
└── scsi1
    ├── lun0 -> ../../../sdc
    ├── lun1 -> ../../../sdd
    ├── lun2 -> ../../../sde
    ├── lun3 -> ../../../sdf
    ├── lun5 -> ../../../sdh
    └── lun6 -> ../../../sdi

Following myvm-0, myvm-1 is crashing due to dev name change, after controller manager replacement, myvm2-x pods work well.

Every 2.0s: kubectl get po                                                                                                                                                   Sat Feb 24 04:16:26 2018

NAME      READY     STATUS             RESTARTS   AGE
myvm-0    0/1       CrashLoopBackOff   13         41m
myvm-1    0/1       CrashLoopBackOff   11         38m
myvm-2    1/1       Running            0          35m
myvm-3    1/1       Running            0          33m
myvm-4    1/1       Running            0          31m
myvm-5    1/1       Running            0          29m
myvm-6    1/1       Running            0          26m

myvm2-0   1/1       Running            0          17m
myvm2-1   1/1       Running            0          14m
myvm2-2   1/1       Running            0          12m
myvm2-3   1/1       Running            0          10m
myvm2-4   1/1       Running            0          8m
myvm2-5   1/1       Running            0          5m
myvm2-6   1/1       Running            0          3m

Release note:

fix disk unavailable issue when mounting multiple azure disks due to dev name change

/assign @karataliu
/sig azure
@feiskyer could you mark it as v1.10 milestone?
@brendandburns @khenidak @rootfs @jdumars FYI

Since it's a critical bug, I will cherry pick this fix to v1.7-v1.9, note that v1.6 does not have this issue since default cachingmode is None

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/azure size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. 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 Feb 24, 2018
@feiskyer feiskyer added this to the v1.10 milestone Feb 24, 2018
@feiskyer feiskyer added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 24, 2018
@feiskyer
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 24, 2018
@feiskyer
Copy link
Member

From v1.7, default host cache setting changed from None to ReadWrite, this would lead to device name change after attach multiple disks on azure vm, finally lead to disk unaccessiable from pod.

Does this mean if user specified cachingmode=ReadWrite, then there are still such problems?

@andyzhangx
Copy link
Member Author

@feiskyer Yes, if customer specify cachingmode=ReadWrite, there is still such issue, we could tell customer not to do that in current stage, linux kernel team are still investigating the root cause. While by default, we need to set cachingmode=None

@feiskyer
Copy link
Member

Is there any performance penalty by changing this? Is there any documentation about this? I think caching should achieve much more better performance than none.

@andyzhangx
Copy link
Member Author

@feiskyer Here is the Azure Disk Caching doc, host cache setting depends on the workload, we should let customers decide host cache settings if they really want to enable it according to their real workload. While by default, we should set as None, otherwise disk would be not accessiable.

Below are the doc about host cache settings:
When you attach data disks to a VM, with standard storage accounts, you have the option to enable caching at the Azure Host level, up to four disks per VM. If you have light read workload, you can enable it, but if you have big file contents (more than few tens of GB) and/or write intensive workload, it is recommended to disable this option.

With Premium storage accounts, all disks can have caching enabled, but only OS disks can have Read/Write, data disks can be set only to ReadOnly or None. For write intensive I/O, it’s recommended to set caching to None on the additional data disks.

@andyzhangx andyzhangx mentioned this pull request Feb 24, 2018
Closed
@feiskyer
Copy link
Member

@andyzhangx Thanks. Reasonable changing default to None.

@feiskyer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 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

@andyzhangx andyzhangx mentioned this pull request Feb 25, 2018
Closed
@k8s-github-robot
Copy link

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 25, 2018 •
edited
Loading

@andyzhangx: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-unit c3e8f68 link /test pull-kubernetes-unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 60346, 60135, 60289, 59643, 52640). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 71c2135 into kubernetes:master Feb 25, 2018
This was referenced Feb 26, 2018
Merged
Merged
@andyzhangx andyzhangx mentioned this pull request Feb 26, 2018
Merged
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

k8s-github-robot pushed a commit that referenced this pull request Mar 1, 2018
Merge pull request #60402 from andyzhangx/automated-cherry-pick-of-#6…
08afba0 
…0346-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #60346

Cherry pick of #60346 on release-1.7.

#60346: fix device name change issue for azure disk
**Release note**:

```
fix disk unavailable issue when mounting multiple azure disks due to dev name change
```
k8s-github-robot pushed a commit that referenced this pull request Mar 3, 2018
Merge pull request #60400 from andyzhangx/automated-cherry-pick-of-#6…
d6f4eab 
…0346-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #60346: fix device name change issue for azure disk

Cherry pick of #60346 on release-1.9.

#60346: fix device name change issue for azure disk
**Release note**:

```
fix disk unavailable issue when mounting multiple azure disks due to dev name change
```
@andyzhangx andyzhangx mentioned this pull request Mar 8, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Mar 23, 2018
Merge pull request #60401 from andyzhangx/automated-cherry-pick-of-#6…
dcf8304 
…0346-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #60346

Cherry pick of #60346 on release-1.8.

#60346: fix device name change issue for azure disk

**Release note**:

```
fix disk unavailable issue when mounting multiple azure disks due to dev name change
```
@andyzhangx andyzhangx mentioned this pull request Apr 11, 2018
Closed
@andyzhangx andyzhangx deleted the fix-devname-change branch May 8, 2018 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane Awaiting requested review from jsafrane

@msau42 msau42 Awaiting requested review from msau42

Assignees

@feiskyer feiskyer

@karataliu karataliu

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/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.

device name change due to azure disk host cache setting
6 participants
@andyzhangx @feiskyer @k8s-ci-robot @k8s-github-robot @k8s-cherrypick-bot @karataliu