feat: Implement ListVolumes #618

nearora-msft · 2020-12-16T09:11:42Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #378 #617

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

feat: Implement ListVolumes

k8s-ci-robot · 2020-12-16T09:11:49Z

Welcome @nearora-msft!

It looks like this is your first PR to kubernetes-sigs/azuredisk-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/azuredisk-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-12-16T09:11:50Z

Hi @nearora-msft. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

andyzhangx · 2020-12-16T11:03:59Z

/ok-to-test

andyzhangx

could you also add list-volumes in integration test:
https://github.com/kubernetes-sigs/azuredisk-csi-driver/blob/master/test/integration/run-test.sh

also is there any way to add e2e tests for this feature? thanks.

# csc controller list-volumes -h
NAME
    list-volumes -- invokes the rpc "ListVolumes"

SYNOPSIS
    csc controller list-volumes [flags]

ALIASES
    list-volumes, ls, list, volumes

OPTIONS
        --format
        The Go template format used to emit the results

    -h, --help
        help for list-volumes

        --max-entries
        The maximum number of entries to return

        --paging
        Enables auto-paging

        --starting-token
        The starting token used to retrieve paged data

pkg/azuredisk/controllerserver.go

nearora-msft · 2020-12-17T10:14:41Z

/test pull-azuredisk-csi-driver-e2e-windows

andyzhangx

Could you also add integration tests and fix the sanity test failure?

pkg/azuredisk/controllerserver_test.go

andyzhangx

in the error logs, pls provide as much info as possible, that would help in troubleshooting

pkg/azuredisk/controllerserver.go

andyzhangx · 2020-12-17T12:20:43Z

pkg/azuredisk/controllerserver.go

+	}
+
+	if start != 0 && start >= len(disks) {
+		return nil, status.Errorf(codes.Aborted, "ListVolumes starting token(%d) is greater than total number of volumes", start)


ListVolumes on rg(%s) starting token(%d) is greater than total number of volumes(%d)

pkg/azuredisk/controllerserver.go

andyzhangx · 2020-12-17T12:52:22Z

pkg/azuredisk/controllerserver.go

+		}
+	}
+
+	disks, derr := d.cloud.DisksClient.ListByResourceGroup(ctx, d.cloud.ResourceGroup)


actually there are other scenarios that disks are in created other resource group, so ListVolumes result is only accurate for default condition. On the other hand, agent nodes are all in d.cloud.ResourceGroup, so ListVolumes of all agent nodes would be accurate. I think this could be right solution: list disks of all agent nodes under d.cloud.ResourceGroup

I am thinking use d.cloud.KubeClient.CoreV1().PersistentVolumes() to get all PV info and then get all diskURI from pv.csi.volumeid and then get disk.ManagedBy, that would provide complete disk list, could refer to:

d.cloud.KubeClient.CoreV1() interface
https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/bb2550070c810ffe94c35eb38aad09c6a3136ade/pkg/azurefile/azurefile.go#L556

this would require much more work, while it could list all disks this cluster is using, otherwise it could not cover all disks and may provide incomplete disk list, not sure what would happen if disk list is incomplete.

andyzhangx · 2020-12-17T13:19:46Z

about sanity test failure, need to fix the 5 failures before upgrade to csi sanity test v4.0.2: #622

nearora-msft · 2020-12-17T13:24:51Z

about sanity test failure, need to fix the 5 failures before upgrade to csi sanity test v4.0.2: #622

@andyzhangx I think then we can enable the LIST_VOLUMES_PUBLISHED_NODES only after issue #622 is resolved?

andyzhangx · 2020-12-17T13:27:12Z

about sanity test failure, need to fix the 5 failures before upgrade to csi sanity test v4.0.2: #622

@andyzhangx I think then we can enable the LIST_VOLUMES_PUBLISHED_NODES only after issue #622 is resolved?

another way is disable that failed test in sanity tests temporarily

nearora-msft · 2020-12-18T07:21:47Z

about sanity test failure, need to fix the 5 failures before upgrade to csi sanity test v4.0.2: #622

@andyzhangx I think then we can enable the LIST_VOLUMES_PUBLISHED_NODES only after issue #622 is resolved?

another way is disable that failed test in sanity tests temporarily

Disabled the failing sanity test temporarily

andyzhangx

pls add csc integration test and squash all commits by git rebase -i HEAD~7

pkg/azuredisk/controllerserver_test.go

test/sanity/run-test.sh

nearora-msft · 2020-12-18T08:56:17Z

/test pull-azuredisk-csi-driver-sanity

nearora-msft · 2020-12-18T13:03:31Z

csc

Added the integration test. Will squash commits once all the checks complete successfully.

andyzhangx · 2020-12-18T13:16:51Z

• Failure [17.089 seconds]
Controller Service [Controller Server]
/home/prow/go/src/github.com/kubernetes-csi/csi-test/pkg/sanity/tests.go:44
  ListVolumes
  /home/prow/go/src/github.com/kubernetes-csi/csi-test/pkg/sanity/controller.go:166
    pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted [It]
skipped 1156 lines unfold_more
[Fail] Controller Service [Controller Server] ListVolumes [It] pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted 
/home/prow/go/src/github.com/kubernetes-csi/csi-test/pkg/sanity/controller.go:336
Ran 63 of 72 Specs in 503.670 seconds
FAIL! -- 62 Passed | 1 Failed | 0 Pending | 9 Skipped

nearora-msft · 2020-12-21T07:51:19Z

• Failure [17.089 seconds]
Controller Service [Controller Server]
/home/prow/go/src/github.com/kubernetes-csi/csi-test/pkg/sanity/tests.go:44
  ListVolumes
  /home/prow/go/src/github.com/kubernetes-csi/csi-test/pkg/sanity/controller.go:166
    pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted [It]
skipped 1156 lines unfold_more
[Fail] Controller Service [Controller Server] ListVolumes [It] pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted 
/home/prow/go/src/github.com/kubernetes-csi/csi-test/pkg/sanity/controller.go:336
Ran 63 of 72 Specs in 503.670 seconds
FAIL! -- 62 Passed | 1 Failed | 0 Pending | 9 Skipped

Disabled this test because of existing issue kubernetes-csi/csi-test#223

andyzhangx · 2020-12-21T13:34:14Z

/retest

andyzhangx · 2020-12-21T13:34:34Z

would you squash all commits first? thanks.

fix: Add ListVolumes capabilities to the driver fix: Handle case where maxEntries is greater than the number of disks Handle case when both starting_token and number of disks are zero fix: Rectify the attached node name and add more details in logs test: Disable the 'should return appropriate capabilities' sanity test temporarily test: Add unit tests and integration test for ListVolumes test: Fix the ListVolumes unit test test: Disable flaky pagination test test: Add the missing apostrophe at the end of skipped tests string test: Add UTs, integration tests for ListVolumes and disable some sanity tests feat: Implement ListVolumes

nearora-msft · 2020-12-22T04:21:07Z

/retest

nearora-msft · 2020-12-22T05:18:09Z

/retest

andyzhangx

/lgtm
/approve

k8s-ci-robot · 2020-12-22T08:47:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, nearora-msft

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:

~~OWNERS~~ [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andyzhangx · 2020-12-22T09:03:24Z

looks like it's a flacky test, could you fix it? thanks:
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azuredisk-csi-driver/618/pull-azuredisk-csi-driver-sanity/1341304585670627328

/hold

nearora-msft · 2020-12-22T10:32:50Z

/retest

nearora-msft · 2020-12-22T12:57:07Z

looks like it's a flacky test, could you fix it? thanks:
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azuredisk-csi-driver/618/pull-azuredisk-csi-driver-sanity/1341304585670627328

/hold

The test that is failing performs these steps:

Call List Volumes
Create a volume
List Volumes again

It expects that the List Volumes count in step 3 should be List Volumes count in step 1 + 1. But the actual case is that List Volumes count in step 3 = List Volumes count in step1 +2. This is why the test fails.

I have observed that when a vm is created a disk is attached to it by default. While running the tests, we create an rg and a sanity-test-node in that rg . The creation of this node should create a disk and attach is to the newly created node. So when the ListVolumes call is made the 1st time, it usually lists the disk that was created with the creation of sanity-test-node. So, we get 1 entry in the 1st call and 2 entries in the 2nd call.

But in this failed case, the 1st ListVolumes call doesn't give any results(Could be because the default disk attach has not happened yet ) And when the ListVolumes call is made the second time, it returns 2 disks(One that was created by the vm creation and the other that was created by the test)

Not sure how we can fix it at our end(if that's possible)

andyzhangx · 2020-12-22T13:56:51Z

/hold cancel

nearora-msft · 2020-12-22T17:09:59Z

/retest

andyzhangx · 2020-12-23T02:13:28Z

/test pull-azuredisk-csi-driver-e2e-windows

…ttacher feat: disable attacher by default

k8s-ci-robot added 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. labels Dec 16, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 16, 2020

k8s-ci-robot requested review from andyzhangx and ZeroMagic December 16, 2020 09:12

nearora-msft changed the title ~~Implement list volumes~~ feat: Implement ListVolumes Dec 16, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 16, 2020

andyzhangx reviewed Dec 16, 2020

View reviewed changes

pkg/azuredisk/controllerserver.go Outdated Show resolved Hide resolved

pkg/azuredisk/controllerserver.go Outdated Show resolved Hide resolved

edreed reviewed Dec 16, 2020

View reviewed changes

pkg/azuredisk/controllerserver.go Outdated Show resolved Hide resolved

andyzhangx reviewed Dec 17, 2020

View reviewed changes

pkg/azuredisk/controllerserver_test.go Show resolved Hide resolved

andyzhangx reviewed Dec 17, 2020

View reviewed changes

andyzhangx reviewed Dec 18, 2020

View reviewed changes

pkg/azuredisk/controllerserver_test.go Outdated Show resolved Hide resolved

andyzhangx reviewed Dec 18, 2020

View reviewed changes

test/sanity/run-test.sh Outdated Show resolved Hide resolved

This was referenced Dec 18, 2020

ListVolumes API doesn't list the volumes that are created in an external resource group #632

Closed

Enable the "should return appropriate capabilities" sanity test #633

Closed

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 18, 2020

andyzhangx approved these changes Dec 22, 2020

View reviewed changes

k8s-ci-robot assigned andyzhangx Dec 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 22, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 22, 2020

k8s-ci-robot merged commit 651a03a into kubernetes-sigs:master Dec 23, 2020

nearora-msft deleted the implement-ListVolumes branch December 24, 2020 07:26

nearora-msft mentioned this pull request Jan 12, 2021

fix: resize filesystem if cloned volume capacity is larger than the source volume #657

Merged

4 tasks

sozercan pushed a commit to sozercan/azuredisk-csi-driver that referenced this pull request Aug 16, 2021

Merge pull request kubernetes-sigs#618 from kubernetes-sigs/disable-a…

50ff8a4

…ttacher feat: disable attacher by default

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement ListVolumes #618

feat: Implement ListVolumes #618

nearora-msft commented Dec 16, 2020 •

edited by andyzhangx

Loading

k8s-ci-robot commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

andyzhangx commented Dec 16, 2020

andyzhangx left a comment

nearora-msft commented Dec 17, 2020

andyzhangx left a comment

andyzhangx left a comment

andyzhangx Dec 17, 2020

andyzhangx Dec 17, 2020

andyzhangx Dec 17, 2020 •

edited

Loading

andyzhangx Dec 17, 2020 •

edited

Loading

andyzhangx commented Dec 17, 2020

nearora-msft commented Dec 17, 2020

andyzhangx commented Dec 17, 2020

nearora-msft commented Dec 18, 2020

andyzhangx left a comment

nearora-msft commented Dec 18, 2020

nearora-msft commented Dec 18, 2020

andyzhangx commented Dec 18, 2020

nearora-msft commented Dec 21, 2020

andyzhangx commented Dec 21, 2020

andyzhangx commented Dec 21, 2020

nearora-msft commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

andyzhangx left a comment

k8s-ci-robot commented Dec 22, 2020

andyzhangx commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

andyzhangx commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

andyzhangx commented Dec 23, 2020

feat: Implement ListVolumes #618

feat: Implement ListVolumes #618

Conversation

nearora-msft commented Dec 16, 2020 • edited by andyzhangx Loading

k8s-ci-robot commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

andyzhangx commented Dec 16, 2020

andyzhangx left a comment

Choose a reason for hiding this comment

nearora-msft commented Dec 17, 2020

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx Dec 17, 2020

Choose a reason for hiding this comment

andyzhangx Dec 17, 2020

Choose a reason for hiding this comment

andyzhangx Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

andyzhangx Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

andyzhangx commented Dec 17, 2020

nearora-msft commented Dec 17, 2020

andyzhangx commented Dec 17, 2020

nearora-msft commented Dec 18, 2020

andyzhangx left a comment

Choose a reason for hiding this comment

nearora-msft commented Dec 18, 2020

nearora-msft commented Dec 18, 2020

andyzhangx commented Dec 18, 2020

nearora-msft commented Dec 21, 2020

andyzhangx commented Dec 21, 2020

andyzhangx commented Dec 21, 2020

nearora-msft commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 22, 2020

andyzhangx commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

andyzhangx commented Dec 22, 2020

nearora-msft commented Dec 22, 2020

andyzhangx commented Dec 23, 2020

nearora-msft commented Dec 16, 2020 •

edited by andyzhangx

Loading

andyzhangx Dec 17, 2020 •

edited

Loading

andyzhangx Dec 17, 2020 •

edited

Loading