openstack: Add support for using multiple data volumes #688

meteorfox · 2015-12-01T23:07:29Z

Before this PR, data disks where not working. This PR fixes that and adds the ability to use multiple volumes as well.

Fix Pickling error caused by use of generator in class attributes.
Improve error handling of disk creation
Add switch for enabling verbose http log

meteorfox · 2015-12-01T23:07:47Z

I've tested this branch on OpenStack Kilo.

meteorfox · 2015-12-01T23:10:27Z

perfkitbenchmarker/providers/openstack/os_disk.py

@@ -34,7 +34,7 @@ def __init__(self, disk_spec, name, zone, image=None):
        self.name = name
        self.zone = zone
        self.device = ""
-        self.virtual_disks = (c for c in "cdefghijklmnopqrstuvwxyz")


This was causing a PicklingError when the spec was being pickled. The error was caused by the generator.

cmccoy · 2015-12-02T00:55:58Z

Looks good to me.

meteorfox · 2015-12-02T18:51:57Z

@cmccoy Thanks for the CR.

I'll wait until end of day, or until tomorrow for @kivio 's CR, since he developed most of the Openstack bindings; I want to make sure it doesn't break any of his stuff.

voellm · 2015-12-02T21:34:22Z

FYI we added a "Provider" contact list here -
https://github.com/GoogleCloudPlatform/PerfKitBenchmarker/wiki/Provider-contacts

This should help is getting reviews from the original developers. Feel
free to update.

On Wed, Dec 2, 2015 at 10:52 AM, Carlos Torres notifications@github.com
wrote:

@cmccoy https://github.com/cmccoy Thanks for the CR.

I'll wait until end of day, or until tomorrow for @kivio
https://github.com/kivio 's CR, since he developed most of the
Openstack bindings; I want to make sure it doesn't break any of his stuff.

—
Reply to this email directly or view it on GitHub
#688 (comment)
.

Anthony F. Voellm (aka Tony)
Google Voice: (650) 516-7382
https://www.google.com/voice/b/0?pli=1#phones
Blog: http://perfguy.blogspot.com/
Benchmark like a pro... PerfKitBenchmarker
https://github.com/GoogleCloudPlatform/PerfKitBenchmarker

kivio · 2015-12-03T19:24:35Z

@meteorfox looks good for me :) I haven't currently environment to test it :(

meteorfox · 2015-12-04T02:31:13Z

@kivio Thanks for reviewing.

@cmccoy Can I merge?

ctlaltlaltc · 2015-12-04T02:42:47Z

@meteorfox , Run error in my env version liberty. I don't know how to fix it yeah.

default_config_constants.yaml:

OpenStack:
machine_type: m1.small
zone: nova
image: ubuntu-pkb
OpenStack:
disk_type: standard
disk_size: 10
mount_point: /scratch

./pkb.py --cloud=OpenStack --benchmarks=fio --noinstall_packages
--openstack_public_network=external_network --openstack_private_network=internal_network

2015-12-04 10:11:56,727 0c7659b8 Thread-1 fio(1/1) ERROR Got exception running FormatDisk: Got non-zero return code (1) executing sudo mke2fs -F -E lazy_itable_init=0 -O ^has_journal -t ext4 -b 4096 /dev/disk/by-id/virtio-ef21dff6-2e72-4a4e-9

mke2fs 1.42.9 (4-Feb-2014)
mke2fs: No such file or directory while trying to determine filesystem size

meteorfox · 2015-12-04T02:45:57Z

@9723 Thanks for testing!

Hmm.. what's your hypervisor backend, is it KVM?

Also, do you consistently hit the error every time you try to run it or is it occasionally?

ctlaltlaltc · 2015-12-04T02:51:38Z

@meteorfox , yes, I got the error every time when running fio benchmark and hypervisor backend use KVM.

meteorfox · 2015-12-04T02:58:58Z

@9723 ok, thanks for catching the bug.

I'm investigating the issue, most likely the bug is because it assumes all device paths for attachments start with /dev/disk/by-id/virtio- which might not always be the case. We should instead be using the device_path returned by the attachments property.

meteorfox · 2015-12-04T03:44:47Z

@9723 Can you pull again and try the latest commit and check if it fixed the problem?

Before this last commit, I was able to replicate the issue but not consistently. After this last commit, I ran several tests with fio using 1 and 4 volumes and it has been consistently working for me.

Let me know if it works for you. Thanks!

meteorfox · 2015-12-04T03:48:30Z

I caught another bug. It's not deleting all the volumes when using more than one.

- Fix Pickling error caused by use of generator in class attributes. - Improve error handling of disk creation - Add switch for enabling verbose http log

meteorfox · 2015-12-04T04:21:47Z

@kivio Ok, turned out that the exception classes in OpenStack/utils.py were not really catching the errors thrown by the novaclient and it was aborting the thread earlier than expected, making it skip the delete part of the volumes. I couldn't import the exceptions from the novaclient package since those will break lazy loading, so I instead opted for catching the generic Exception.

@9723 Ok, this time should work and also clean up the volumes.

ctlaltlaltc · 2015-12-04T06:50:06Z

@meteorfox 👍 LGTM, Thanks. tested in liberty.

kivio · 2015-12-04T08:09:41Z

@9723 @meteorfox i known this error on OpenStack Juno long since but this is little problematic. First thing, path returned by nova client is just suggestion not sure path. It's described in Nova Client documentation. Second thing when you start more than one volume in one time you can have race condition with /virtio- path. For me best try was getting next /dev/vd(bced..) on each machine. But it still is not that easy. This problem is based on KVM i don't known how other hypervisors on OpenStack works with it.

voellm · 2015-12-04T15:50:52Z

Should we cherry pick this into v1.0.0? About ready?

On Fri, Dec 4, 2015 at 12:09 AM, Marcin Karkocha notifications@github.com
wrote:

@9723 https://github.com/9723 @meteorfox https://github.com/meteorfox
i known this error on OpenStack Juno long since but this is little
problematic. First thing, path returned by nova client is just suggestion
not sure path. It's described in Nova Client documentation. Second thing
when you start more than one volume in one time you can have race condition
with /virtio- path. For me best try was getting next /dev/vd(bced..) on
each machine. But it still is not that easy. This problem is based on KVM i
don't known how other hypervisors on OpenStack works with it.

—
Reply to this email directly or view it on GitHub
#688 (comment)
.

Anthony F. Voellm (aka Tony)
Google Voice: (650) 516-7382
https://www.google.com/voice/b/0?pli=1#phones
Blog: http://perfguy.blogspot.com/
Benchmark like a pro... PerfKitBenchmarker
https://github.com/GoogleCloudPlatform/PerfKitBenchmarker

meteorfox · 2015-12-04T16:30:41Z

perfkitbenchmarker/providers/openstack/os_disk.py

@@ -95,6 +95,13 @@ def Attach(self, vm):
                is_unattached = not(volume.status == "in-use"
                                    and volume.attachments)

+        for attachment in volume.attachments:
+            if self.attach_id == attachment.get('volume_id'):
+                self.device = attachment.get('device')


@kivio I think using the 'device' property from the attachments list of dict objects we get back from volumes.get() should be OK.

If you noticed at line 85, instead of giving a device hint, I just let OpenStack return the device path it got, then I just retrieve the actual device path once the volume is attached. I believe this is reliable, and will work for other kind of hypervisors.

For later releases of OpenStack, we will need to introduce the python-cinderclient library for volume management since these operations have been deprecated out of the nova client.

meteorfox · 2015-12-04T16:41:00Z

@voellm I'd say yes, it's working for both @9723 and me, but I want to wait for @kivio response since he's the maintainer.

kivio · 2015-12-07T09:12:45Z

@meteorfox ok, sounds good to me. 👍

meteorfox · 2015-12-07T16:37:03Z

@kivio I think the code addresses those concerns, and it works for Kilo (tested by me) and Liberty (tested by @9723 ). I would like to merge this change since is very low risk, and we have confidence that it works. If that's OK with you

meteorfox · 2015-12-07T16:37:59Z

@kivio Sorry, I didn't see your comment, disregard the comment above.

@voellm @cmccoy OK to merge ?

cmccoy · 2015-12-07T16:41:25Z

perfkitbenchmarker/providers/openstack/os_disk.py

                time.sleep(sleep)
                sleep_count += 1
                if sleep_count == 10:
                    sleep = 5
-        except (os_utils.NotFound, os_utils.BadRequest):
+        except Exception:


Would it be worth including some information on this in the logging statement? This could swallow an exception unrelated to the volume not being found...

@cmccoy Good point. I can append the exception message to the log.

cmccoy · 2015-12-07T16:43:17Z

@meteorfox: I just added a few questions.

Reduce broadness of exception clause by catching specific exceptions instead of the broad Exception class.

meteorfox · 2015-12-08T01:34:58Z

@cmccoy: I've narrowed down the exception scope to just the errors it expects to handle. I brought back the imports, directly in os_disk.py and os_virtual_machine.py but I'm lazy loading them. Tested it again and actually found a case were the volume failed to create and was throwing a None Exception that was being 'swallowed'

cmccoy · 2015-12-08T14:34:55Z

@meterfox - Thanks. 👍

meteorfox · 2015-12-08T16:44:46Z

Thanks everyone for the CRs

openstack: Add support for using multiple data volumes

googlebot added the cla: yes label Dec 1, 2015

meteorfox added the ready for review label Dec 1, 2015

meteorfox reviewed Dec 1, 2015
View reviewed changes

meteorfox self-assigned this Dec 1, 2015

meteorfox force-pushed the openstack-multi-volumes branch 3 times, most recently from 0c23199 to 616bb08 Compare December 4, 2015 03:30

openstack: Add support for using multiple data volumes

874170d

- Fix Pickling error caused by use of generator in class attributes. - Improve error handling of disk creation - Add switch for enabling verbose http log

meteorfox force-pushed the openstack-multi-volumes branch from 616bb08 to 874170d Compare December 4, 2015 04:04

meteorfox added ready for review and removed ready for review labels Dec 4, 2015

meteorfox reviewed Dec 4, 2015
View reviewed changes

cmccoy reviewed Dec 7, 2015
View reviewed changes

meteorfox removed the ready for review label Dec 7, 2015

openstack: Capture specific novaclient exceptions

bfb02ae

Reduce broadness of exception clause by catching specific exceptions instead of the broad Exception class.

meteorfox force-pushed the openstack-multi-volumes branch from e259681 to bfb02ae Compare December 7, 2015 19:34

openstack: Skip volume delete if failed to create

46d53da

meteorfox added the ready for review label Dec 8, 2015

meteorfox added a commit that referenced this pull request Dec 8, 2015

Merge pull request #688 from meteorfox/openstack-multi-volumes

34daa1c

openstack: Add support for using multiple data volumes

meteorfox merged commit 34daa1c into GoogleCloudPlatform:master Dec 8, 2015

meteorfox deleted the openstack-multi-volumes branch December 8, 2015 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openstack: Add support for using multiple data volumes #688

openstack: Add support for using multiple data volumes #688

meteorfox commented Dec 1, 2015

meteorfox commented Dec 1, 2015

meteorfox Dec 1, 2015

cmccoy commented Dec 2, 2015

meteorfox commented Dec 2, 2015

voellm commented Dec 2, 2015

kivio commented Dec 3, 2015

meteorfox commented Dec 4, 2015

ctlaltlaltc commented Dec 4, 2015

meteorfox commented Dec 4, 2015

ctlaltlaltc commented Dec 4, 2015

meteorfox commented Dec 4, 2015

meteorfox commented Dec 4, 2015

meteorfox commented Dec 4, 2015

meteorfox commented Dec 4, 2015

ctlaltlaltc commented Dec 4, 2015

kivio commented Dec 4, 2015

voellm commented Dec 4, 2015

meteorfox Dec 4, 2015

meteorfox commented Dec 4, 2015

kivio commented Dec 7, 2015

meteorfox commented Dec 7, 2015

meteorfox commented Dec 7, 2015

cmccoy Dec 7, 2015

meteorfox Dec 7, 2015

cmccoy commented Dec 7, 2015

meteorfox commented Dec 8, 2015

cmccoy commented Dec 8, 2015

meteorfox commented Dec 8, 2015

openstack: Add support for using multiple data volumes #688

openstack: Add support for using multiple data volumes #688

Conversation

meteorfox commented Dec 1, 2015

meteorfox commented Dec 1, 2015

meteorfox Dec 1, 2015

Choose a reason for hiding this comment

cmccoy commented Dec 2, 2015

meteorfox commented Dec 2, 2015

voellm commented Dec 2, 2015

kivio commented Dec 3, 2015

meteorfox commented Dec 4, 2015

ctlaltlaltc commented Dec 4, 2015

meteorfox commented Dec 4, 2015

ctlaltlaltc commented Dec 4, 2015

meteorfox commented Dec 4, 2015

meteorfox commented Dec 4, 2015

meteorfox commented Dec 4, 2015

meteorfox commented Dec 4, 2015

ctlaltlaltc commented Dec 4, 2015

kivio commented Dec 4, 2015

voellm commented Dec 4, 2015

meteorfox Dec 4, 2015

Choose a reason for hiding this comment

meteorfox commented Dec 4, 2015

kivio commented Dec 7, 2015

meteorfox commented Dec 7, 2015

meteorfox commented Dec 7, 2015

cmccoy Dec 7, 2015

Choose a reason for hiding this comment

meteorfox Dec 7, 2015

Choose a reason for hiding this comment

cmccoy commented Dec 7, 2015

meteorfox commented Dec 8, 2015

cmccoy commented Dec 8, 2015

meteorfox commented Dec 8, 2015