Merge lp:~renukaapte/nova/lp745340 into lp:~citrix-openstack/nova/xenapi

[Renuka Apte]

[lp:~renukaapte/nova/lp745340]

926. By Renuka Apte on 2011-04-12

Minor fixes

[termie (termie)]

You've got a few places with funky indents, looks like you just need to update the indentation of some of the lines around the lines you've changed.

Just a suggestion but my favorite way of dealing with these split up execute calls is:

44 + (r, _e) = utils.execute('sudo', 'iscsiadm', '-m', 'discovery',
45 + '-t', 'sendtargets', '-p', volume_ref['host'])

Is like this:

(r, _e) = utils.execute('sudo', 'iscsiadm',
                        '-m', 'discovery',
                        '-t', 'sendtargets',
                        '-p', volume_ref['host'])

I don't really understand your -vol- -volume-- changes, but I'll take your word for it that -vol- was incorrect.

review: Needs Fixing

[lp:~renukaapte/nova/lp745340]

927. By Renuka Apte on 2011-05-04

Fix indentation.

[Armando Migliaccio (armando-migliaccio)]

lgtm. Maybe add a unit test to cover this part of the code?

review: Approve

[Armando Migliaccio (armando-migliaccio)]

I have noticed that this patch does not work when dealing with remove volumes (e.g. nova-volume running on a separate host from compute). Have a look at

http://bazaar.launchpad.net/~citrix-openstack/nova/cactus-addons/revision/995

review: Needs Fixing

[lp:~renukaapte/nova/lp745340]

928. By Renuka Apte on 2011-05-12

Fix remote volume code

[termie (termie)]

the code looks fine, but I don't really understand the change, could you explain why the change from -vol- to -volume- is happening?

[termie (termie)]

(that's technically an approve, btu i'd still love more info)

review: Approve

[Renuka Apte (renukaapte)]

The function _get_volume_id expects a volume_id or path and returns an integer ID to that volume. The path to a local volume which has been created looks like /dev/mapper/nova--volumes-volume--00000001 for example. The path probably had -vol- in the past, which explains the original code. We simply want to get the numeric part of this path and convert it to an integer.

[Renuka Apte (renukaapte)]

Moving this merge proposal to nova trunk, instead of citrix-openstack.

[Vish Ishaya (vishvananda)]

lgtm. Armando can you verify the current changes?

review: Approve

[Armando Migliaccio (armando-migliaccio)]

I checked the changes and they look fine. We'd be better off by passing the volume id to the compute driver so that we would avoid the string manipulations. This is due to the fact that only dev_path and mountpoint are passed to driver->attach_volume (though dev_path contains the volume_id we are interested in). Changing the attach_volume signature would require a bit of work across the nova code (to make sure that other hypervisors are not affected), but a volume_id=None on the signature would be fine. I guess this could be carried out in another bug fix.

review: Approve

=== modified file 'Authors'
--- Authors	2011-04-07 23:08:15 +0000
+++ Authors	2011-05-13 02:36:31 +0000
@@ -58,6 +58,7 @@
 Naveed Massjouni <[e-mail]>
 Nirmal Ranganathan <[e-mail]>
 Paul Voccio <[e-mail]>
+Renuka Apte <[e-mail]>
 Ricardo Carrillo Cruz <[e-mail]>
 Rick Clark <[e-mail]>
 Rick Harris <[e-mail]>

=== modified file 'nova/virt/xenapi/volume_utils.py'
--- nova/virt/xenapi/volume_utils.py	2011-03-11 23:48:44 +0000
+++ nova/virt/xenapi/volume_utils.py	2011-05-13 02:36:31 +0000
@@ -204,14 +204,17 @@
     if isinstance(path_or_id, int):
         return path_or_id
     # n must contain at least the volume_id
-    # /vol- is for remote volumes
-    # -vol- is for local volumes
+    # :volume- is for remote volumes
+    # -volume- is for local volumes
     # see compute/manager->setup_compute_volume
-    volume_id = path_or_id[path_or_id.find('/vol-') + 1:]
+    volume_id = path_or_id[path_or_id.find(':volume-') + 1:]
     if volume_id == path_or_id:
-        volume_id = path_or_id[path_or_id.find('-vol-') + 1:]
-        volume_id = volume_id.replace('--', '-')
-    return volume_id
+        volume_id = path_or_id[path_or_id.find('-volume--') + 1:]
+        volume_id = volume_id.replace('volume--', '')
+    else:
+        volume_id = volume_id.replace('volume-', '')
+        volume_id = volume_id[0:volume_id.find('-')]
+    return int(volume_id)
 
 
 def _get_target_host(iscsi_string):
@@ -244,25 +247,23 @@
     Gets iscsi name and portal from volume name and host.
     For this method to work the following are needed:
     1) volume_ref['host'] must resolve to something rather than loopback
-    2) ietd must bind only to the address as resolved above
-    If any of the two conditions are not met, fall back on Flags.
     """
-    volume_ref = db.volume_get_by_ec2_id(context.get_admin_context(),
-                                         volume_id)
+    volume_ref = db.volume_get(context.get_admin_context(),
+                               volume_id)
     result = (None, None)
     try:
-        (r, _e) = utils.execute("sudo iscsiadm -m discovery -t "
-                                     "sendtargets -p %s" %
-                                     volume_ref['host'])
+        (r, _e) = utils.execute('sudo', 'iscsiadm',
+                                '-m', 'discovery',
+                                '-t', 'sendtargets',
+                                '-p', volume_ref['host'])
     except exception.ProcessExecutionError, exc:
         LOG.exception(exc)
     else:
-        targets = r.splitlines()
-        if len(_e) == 0 and len(targets) == 1:
-            for target in targets:
-                if volume_id in target:
-                    (location, _sep, iscsi_name) = target.partition(" ")
-                    break
-            iscsi_portal = location.split(",")[0]
-            result = (iscsi_name, iscsi_portal)
+        volume_name = "volume-%08x" % volume_id
+        for target in r.splitlines():
+            if FLAGS.iscsi_ip_prefix in target and volume_name in target:
+                (location, _sep, iscsi_name) = target.partition(" ")
+                break
+        iscsi_portal = location.split(",")[0]
+        result = (iscsi_name, iscsi_portal)
     return result