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

Azure : filter disks with ToBeDetached flag #84958

Merged
merged 2 commits into from Nov 14, 2019

Conversation

@kkmsft
Copy link
Contributor

kkmsft commented Nov 8, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Filter disks with ToBeDetached flag

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2019

Hi @kkmsft. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@kkmsft kkmsft changed the title [WIP] Filter disks with ToBeDetached flag [WIP] Azure : filter disks with ToBeDetached flag Nov 8, 2019
@k8s-ci-robot k8s-ci-robot requested review from andyzhangx and feiskyer Nov 8, 2019
if vm.StorageProfile != nil && vm.StorageProfile.DataDisks != nil {
disks = make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks))
copy(disks, *vm.StorageProfile.DataDisks)
unfilteredDisks = make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks))

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 8, 2019

Member

what about write like following, then you don't need to copy out to a unfilteredDisks

filteredDisks = make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks))
filterDetatchingDisks(*vm.StorageProfile.DataDisks, filteredDisks)
func filterDetatchingDisks( ...) {
  for _, disk := range dataDisks {
      if !*disk.ToBeDetached {
         append disk to filteredDisks
   }
  }
}
@@ -28,6 +28,21 @@ import (
"k8s.io/klog"
)

func filterDetatchDisks(oldDisks []compute.DataDisk) (disks []compute.DataDisk) {

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 8, 2019

Member

filterDetatchingDisks and also write a UT

@andyzhangx

This comment has been minimized.

Copy link
Member

andyzhangx commented Nov 8, 2019

/ok-to-test

@kkmsft

This comment has been minimized.

Copy link
Contributor Author

kkmsft commented Nov 9, 2019

/retest

1 similar comment
@kkmsft

This comment has been minimized.

Copy link
Contributor Author

kkmsft commented Nov 9, 2019

/retest

Copy link
Member

andyzhangx left a comment

for UT, pls refer to TestStandardDetachDisk.

if disk.ToBeDetached != nil {
if *disk.ToBeDetached {
if disk.Name != nil {
klog.V(2).Infof("Filtering disk: %s with ToBeDetached flag set.", *disk.Name)

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 9, 2019

Member

klog.V(12).Infof


func TestFilteredDetatchingDisks(t *testing.T) {

disks := []compute.DataDisk{

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 9, 2019

Member

pls add more tests, e.g. empty data disk list

@kkmsft kkmsft changed the title [WIP] Azure : filter disks with ToBeDetached flag Azure : filter disks with ToBeDetached flag Nov 9, 2019
@kkmsft

This comment has been minimized.

Copy link
Contributor Author

kkmsft commented Nov 9, 2019

/retest

@kkmsft kkmsft force-pushed the kkmsft:disk_fixes branch from ab809ad to a10f80f Nov 11, 2019
@andyzhangx

This comment has been minimized.

Copy link
Member

andyzhangx commented Nov 11, 2019

hi @chewong could you take a look at the aks-engine-azure test error? Thanks.

error building hyperkube error during make -C /go/src/k8s.io/kubernetes WHAT=cmd/hyperkube: exit status 2
@chewong

This comment has been minimized.

Copy link
Member

chewong commented Nov 11, 2019

hi @chewong could you take a look at the aks-engine-azure test error? Thanks.

error building hyperkube error during make -C /go/src/k8s.io/kubernetes WHAT=cmd/hyperkube: exit status 2

Jobs that involve building hyperkube on k8s master branch / PRs won't work because of the incompatibility between the new hyperkube image and aks-engine. kubernetes/test-infra#15190 and Azure/aks-engine#2298 will fix the issue.

@andyzhangx

This comment has been minimized.

Copy link
Member

andyzhangx commented Nov 11, 2019

/priority important-soon
/sig cloud-provider
/area provider/azure

@@ -42,8 +42,7 @@ func (as *availabilitySet) AttachDisk(isManagedDisk bool, diskName, diskURI stri
return err
}

disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks))
copy(disks, *vm.StorageProfile.DataDisks)
disks := filterDetachingDisks(*vm.StorageProfile.DataDisks)

This comment has been minimized.

Copy link
@feiskyer

feiskyer Nov 12, 2019

Member

if the disk is under detaching, then attach would still error, right? could we add a check here and fail earlier?

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 13, 2019

Member

I think this PR is on the right way, when there is disk under detaching(actually that issue is already fixed by #85158, ), this RP would remove those detaching disks and go on attaching another disk, this works according to vmss team's spec.

@@ -117,8 +116,7 @@ func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.N
return nil, err
}

disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks))
copy(disks, *vm.StorageProfile.DataDisks)
disks := filterDetachingDisks(*vm.StorageProfile.DataDisks)

This comment has been minimized.

Copy link
@feiskyer

feiskyer Nov 12, 2019

Member

same here, check whether the disk is under detaching and fail earlier

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 13, 2019

Member

no need to check

@feiskyer

This comment has been minimized.

Copy link
Member

feiskyer commented Nov 12, 2019

/hold

@feiskyer

This comment has been minimized.

Copy link
Member

feiskyer commented Nov 13, 2019

/hold cancel

Copy link
Member

andyzhangx left a comment

/lgtm
@feiskyer any other comments?

@@ -42,8 +42,7 @@ func (as *availabilitySet) AttachDisk(isManagedDisk bool, diskName, diskURI stri
return err
}

disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks))
copy(disks, *vm.StorageProfile.DataDisks)
disks := filterDetachingDisks(*vm.StorageProfile.DataDisks)

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 13, 2019

Member

I think this PR is on the right way, when there is disk under detaching(actually that issue is already fixed by #85158, ), this RP would remove those detaching disks and go on attaching another disk, this works according to vmss team's spec.

@@ -117,8 +116,7 @@ func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.N
return nil, err
}

disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks))
copy(disks, *vm.StorageProfile.DataDisks)
disks := filterDetachingDisks(*vm.StorageProfile.DataDisks)

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Nov 13, 2019

Member

no need to check

Copy link
Member

andyzhangx left a comment

/approve
/hold cancel

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, kkmsft

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

This comment has been minimized.

Copy link
Member

andyzhangx commented Nov 14, 2019

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug and removed needs-kind labels Nov 14, 2019
@k8s-ci-robot k8s-ci-robot merged commit 84318d9 into kubernetes:master Nov 14, 2019
9 of 15 checks passed
9 of 15 checks passed
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-e2e-kind Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
tide Not mergeable. Retesting: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation kkmsft authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 14, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 14, 2019

@kkmsft: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-aks-engine-azure ab809ad link /test pull-kubernetes-e2e-aks-engine-azure
pull-kubernetes-e2e-kind a10f80f link /test pull-kubernetes-e2e-kind

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.

@feiskyer

This comment has been minimized.

Copy link
Member

feiskyer commented Nov 16, 2019

looks good, thanks for fix.

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.