-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Only retrieve relevant volumes #41785
Only retrieve relevant volumes #41785
Conversation
Hi @jamiehannaford. 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 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. |
@@ -115,7 +115,8 @@ func (os *OpenStack) getVolume(diskName string) (volumes.Volume, error) { | |||
return volume, err | |||
} | |||
|
|||
err = volumes.List(sClient, nil).EachPage(func(page pagination.Page) (bool, error) { | |||
opts := &volumes.ListOpts{Name: diskName} | |||
err = volumes.List(sClient, opts).EachPage(func(page pagination.Page) (bool, error) { |
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.
What is the difference of using opts vs. nil?
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.
The Cinder API allows you to provide query parameters in the URL when retrieving a list of volumes. Filtering by name for example allows you to restrict the amount of volumes down considerably. Otherwise, the K8s code will traverse the entire collection of volumes in an OpenStack project to find a match (some project could have 10s of thousands). Filtering by name reduces/eliminates the need for subsequent pagination calls.
Any status update for this? |
@k8s-sig-openstack-pr-reviews Would somebody mind reviewing this? |
@idvoretskyi can you add the sig/openstack label please? |
lgtm |
aa14ff0
to
343ce66
Compare
@hogepodge Would you mind re-running tests? There seems to be a flaky test case failing |
@jingxu97 @xsgordon @grodrigues3 @k8s-sig-openstack-api-reviews Would somebody mind commenting with |
/approve |
@k8s-bot ok to test |
343ce66
to
85be278
Compare
@jingxu97 @anguslees Would somebody mind commenting with |
@@ -111,7 +111,9 @@ func (volumes *VolumesV2) createVolume(opts VolumeCreateOpts) (string, error) { | |||
func (volumes *VolumesV1) getVolume(diskName string) (Volume, error) { | |||
var volume_v1 volumes_v1.Volume | |||
var volume Volume | |||
err := volumes_v1.List(volumes.blockstorage, nil).EachPage(func(page pagination.Page) (bool, error) { | |||
|
|||
opts := &volumes_v1.ListOpts{Name: diskName} |
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.
The logic below looks for either name == diskName or the ID contains diskName. That latter case sounds amazingly terrible, but are we confident we don't need it? (and if so, can we remove it in this PR to make it clear that that logic isn't going to trigger anymore?)
@@ -149,7 +151,9 @@ func (volumes *VolumesV1) getVolume(diskName string) (Volume, error) { | |||
func (volumes *VolumesV2) getVolume(diskName string) (Volume, error) { | |||
var volume_v2 volumes_v2.Volume | |||
var volume Volume | |||
err := volumes_v2.List(volumes.blockstorage, nil).EachPage(func(page pagination.Page) (bool, error) { | |||
|
|||
opts := &volumes_v2.ListOpts{Name: diskName} |
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.
Ditto - clarify the "ID contains diskName" case below
25d7dc3
to
ba33b16
Compare
@anguslees I've refactored a bunch of signatures to make the volume ID stuff clearer. I don't why it was implemented to handle both names and IDs, but I've switched it to the latter. It also seems that the current volume implementation uses IDs too, so I think this is backwards compatible. |
@mikedanese Would you mind weighing in here and if necessary approving this change? |
I am not against the PR, I am trying to find out what it tries to do, as it is quite a long one and I do not want to miss some important spot hidden between renames. So far it looks good, please rebase and I'll mark it as lgtm. |
ba33b16
to
5588515
Compare
5588515
to
ccb3bb1
Compare
@jsafrane Thanks for the feedback. I've made those changes - would you mind taking another quick look and adding |
/lgtm |
@mikedanese Can you |
/approve |
ccb3bb1
to
cfa8371
Compare
9520b26
to
8fe7860
Compare
@jsafrane Can you add |
Bump cc/ @jsafrane |
8fe7860
to
05373e9
Compare
05373e9
to
4bd71a3
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jamiehannaford, jingxu97, jsafrane, mikedanese
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 38505, 41785, 46315) |
@jamiehannaford: The following test(s) failed:
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. |
…mance Automatic merge from submit-queue (batch tested with PRs 38505, 41785, 46315) Only retrieve relevant volumes **What this PR does / why we need it**: Improves performance for Cinder volume attach/detach calls. Currently when Cinder volumes are attached or detached, functions try to retrieve details about the volume from the Nova API. Because some only have the volume name not its UUID, they use the list function in gophercloud to iterate over all volumes to find a match. This incurs severe performance problems on OpenStack projects with lots of volumes (sometimes thousands) since it needs to send a new request when the current page does not contain a match. A better way of doing this is use the `?name=XXX` query parameter to refine the results. **Which issue this PR fixes**: kubernetes#26404 **Special notes for your reviewer**: There were 2 ways of addressing this problem: 1. Use the `name` query parameter 2. Instead of using the list function, switch to using volume UUIDs and use the GET function instead. You'd need to change the signature of a few functions though, such as [`DeleteVolume`](https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/cinder/cinder.go#L49), so I'm not sure how backwards compatible that is. Since #1 does effectively the same as #2, I went with it because it ensures BC. One assumption that is made is that the `volumeName` being retrieved matches exactly the name of the volume in Cinder. I'm not sure how accurate that is, but I see no reason why cloud providers would want to append/prefix things arbitrarily. **Release note**: ```release-note Improves performance of Cinder volume attach/detach operations ```
What this PR does / why we need it:
Improves performance for Cinder volume attach/detach calls.
Currently when Cinder volumes are attached or detached, functions try to retrieve details about the volume from the Nova API. Because some only have the volume name not its UUID, they use the list function in gophercloud to iterate over all volumes to find a match. This incurs severe performance problems on OpenStack projects with lots of volumes (sometimes thousands) since it needs to send a new request when the current page does not contain a match. A better way of doing this is use the
?name=XXX
query parameter to refine the results.Which issue this PR fixes:
#26404
Special notes for your reviewer:
There were 2 ways of addressing this problem:
name
query parameterDeleteVolume
, so I'm not sure how backwards compatible that is.Since #1 does effectively the same as #2, I went with it because it ensures BC.
One assumption that is made is that the
volumeName
being retrieved matches exactly the name of the volume in Cinder. I'm not sure how accurate that is, but I see no reason why cloud providers would want to append/prefix things arbitrarily.Release note: