-
Notifications
You must be signed in to change notification settings - Fork 39k
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
fix azure disk attachment error on Linux #70002
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx 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 |
} | ||
glog.V(5).Infof("azureDisk - WaitForAttach: GetDiskLun succeeded, got lun(%v)", lun) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a V(2) log here with device and lun number? It could help for troubleshoooting issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already addressed in #70012
func getDiskLUN(deviceInfo string) (int32, error) { | ||
var diskLUN string | ||
if len(deviceInfo) <= 2 { | ||
diskLUN = deviceInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check whether it is a number or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any possibility of supporting lun number >= 100 in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's checking under below:
lun, err := strconv.Atoi(diskLUN)
Azure platform only support up to 64 disk number
use new getDiskLUN func in linux
32475e4
to
f690687
Compare
ping @feiskyer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/test pull-kubernetes-verify |
In spite of some warnings during Pod relocation, I can confirm all volumes eventually get re-attached successfully after 2-3 minutes (Kubernetes 1.10, custom build including this patch). Related Pod events:
|
…0002-upstream-release-1.11 Automated cherry pick of #70002: improve azure disk attachment perf on Linux
…0002-upstream-release-1.12 Automated cherry pick of #70002: improve azure disk attachment perf on Linux
…0002-upstream-release-1.10 Automated cherry pick of #70002: improve azure disk attachment perf on Linux
What type of PR is this?
What this PR does / why we need it:
There PR is going to fix drawback after PR: #62612
following
getDiskController
code will fetch vmCache may lead to below error and the vmCache refresh time period would be 1 min, andMountVolume.WaitForAttach
may cost 1 min which is too long timekubernetes/pkg/volume/azure_dd/attacher.go
Line 162 in ba66014
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 #69262
Special notes for your reviewer:
This PR add a new func
getDiskLUN
to get disk LUN num from a deviceInfo, deviceInfo could be a LUN number or a device path, e.g. /dev/disk/azure/scsi1/lun2.While on Windows, since the deviceInfo could be a LUN num or a disk num(on windows), cannot tell whether it's a LUN num since both conditions are a number, we just keep the original code logic.
Does this PR introduce a user-facing change?:
Release note:
/sig azure
/assign @feiskyer @khenidak
cc @brendandburns