-
Notifications
You must be signed in to change notification settings - Fork 290
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
[FLOC-4489] Use detailed volume list and update to a version of mimic that supports that endpoint #2892
Conversation
I'm trying to understand some separate failures on devstack
|
…o cinder-volume-detail-FLOC-4489
…' into cinder-volume-detail-FLOC-4489
…pdate the cleanup
These seem to occur only on Ubuntu 14.04 and the problem is that the hotplug system doesn't always create a I can force the symlink by running The tests are much more reliable on Ubuntu 16.04 |
|
@@ -530,7 +530,7 @@ def list_volumes(self): | |||
http://docs.rackspace.com/cbs/api/v1.0/cbs-devguide/content/GET_getVolumesDetail_v1__tenant_id__volumes_detail_volumes.html | |||
""" | |||
flocker_volumes = [] | |||
for cinder_volume in self.cinder_volume_manager.list(detailed=False): | |||
for cinder_volume in self.cinder_volume_manager.list(detailed=True): |
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 a case where we do not always need detailed=True
? I seem to remember the discussion that Cinder v1 would pass with detailed=False
, thus the detailed information is not needed? maybe im making this up, but making sure we're not requested uneeded information that may slow down the process being that every volume may have more information.
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.
Yes that's right.
- Cinder v1 API volume list includes metadata: http://developer.openstack.org/api-ref-blockstorage-v1.html#listVolumes
- Cinder v2 doesn't: http://developer.openstack.org/api-ref-blockstorage-v2.html#listVolumes
That's what led to this bug. I tested against Rackspace and saw the tests pass.
On OpenStack the detailed list doesn't include the attachment info.
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.
Should it be the case where we use False
for v1 instead of always True
for detailed
?
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'd be possible but probably more trouble than it's worth.
At this point the cinder version detection has already taken place and the CinderBlockDeviceAPI has an object conforming to an interface that we've defined ICinderVolumeManager
.
Since we're aiming to drop support for CinderV1 (Rackspace) I'm going to leave it.
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.
ok. I wonder if we should drop v1 though, there may be private deployments that may want to use v1, but not sure.
Did a preliminary review, but will revisit when ready and change status in JIRA, i think because this was at the top of a stack of other changes. Also looks like 1 check failing, looks like the same failure happening on #2893 |
@wallrj
to get Also, stilling seeing the following error in the trusty cinder CI test
If these concerns can safely be ignored or are unrelated then the PR looks good. Please answer the above but otherwise LGTM |
Thanks @wallnerryan
Yeah, we do need to investigate that. It only affects virtio OpenStack environments...which is where we do disk serial number based device path detection. It shouldn't affect Rackspace, which uses Xen volume driver. So I'll merge this and create followup issues:
|
Fixes: https://clusterhq.atlassian.net/browse/FLOC-4489
Depends on: ClusterHQ/mimic#1
I re-instated the use of detailed volume list.
This ensures that volume attachment information is returned, even when interacting with Cinder v2 API.
One of our tests attempts to hit the volumes/detail endpoint with a mimic based fake REST server.
So I also had to fork andupdate mimic to support that endpoint.
I'll post devstack based test results shortly.