Use VM name instead of Node name for disk operations with Flex VMSS #2635
Conversation
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
|
|
k8s-ci-robot
added
cncf-cla: no
There was a problem hiding this comment.
Thank you so much for finding this issue. You are right that we should use VM name rather than nodeName (OS hostName) when updating VM, so line 102,109, 182, 189 should be fixed as you proposed in the PR.
As for the rest lines, since CSI Driver is using nodename as the primary key to identify nodes/VMs, can we keep the way of using vmName? Only changing line 102,109, 182, 189 should be able to fix the issue.
There was a problem hiding this comment.
Can you also update line 222 as well? Suggested change:
// UpdateVM updates a vm
func (fs *FlexScaleSet) UpdateVM(ctx context.Context, nodeName types.NodeName) error {
vmName := mapNodeNameToVMName(nodeName)
vm, err := fs.getVmssFlexVM(vmName, azcache.CacheReadTypeDefault)
if err != nil {
// if host doesn't exist, no need to update
klog.Warningf("azureDisk - cannot find node %s, skip updating vm)", nodeName)
return nil
}
nodeResourceGroup, err := fs.GetNodeResourceGroup(vmName)
if err != nil {
return err
}
defer func() {
_ = fs.DeleteCacheForNode(vmName)
}()
klog.V(2).Infof("azureDisk - update(%s): vm(%s)", nodeResourceGroup, vmName)
rerr := fs.VirtualMachinesClient.Update(ctx, nodeResourceGroup, *vm.Name, compute.VirtualMachineUpdate{}, "update_vm")
klog.V(2).Infof("azureDisk - update(%s): vm(%s) - returned with %v", nodeResourceGroup, vmName, rerr)
if rerr != nil {
return rerr.Error()
}
return nil
}
✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.
|
k8s-ci-robot
added
the
size/M
|
@zmyzheng thanks for the quick review! I updated the PR with your suggestions and updated test cases for the |
|
@zmyzheng: changing LGTM is restricted to collaborators In response to this:
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. |
|
@zmyzheng I rebased and squashed the commits to satisfy the bots, feel free to modify this PR however you think is necessary to get it merged. |
|
@feiskyer @andyzhangx could you help approve this PR? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nilo19, okushchenko, zmyzheng 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 |
k8s-ci-robot
added
the
approved
|
/retest |
|
@zmyzheng: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
What type of PR is this?
/kind bug
What this PR does / why we need it:
When using https://github.com/kubernetes-sigs/azuredisk-csi-driver with Flex VMSS, I observed that it fails to attach and detach disks from VMs. This is due to it trying to use k8s Node name instead of VM name when interacting with Azure APIs. This PR updates this logic to use a correct name.
Does this PR introduce a user-facing change?