Merge lp:~justin-fathomdb/nova/justinsb-hpsan2 into lp:~hudson-openstack/nova/trunk

[justinsb]

Support HP/LeftHand SANs. We control the SAN by SSHing and issuing CLIQ commands. Also improved the way iSCSI volumes are mounted: try to store the iSCSI connection info in the volume entity, in preference to doing discovery. Also CHAP authentication support.

CHAP support is necessary to avoid the attach-volume command require an export on the SAN device. If we had to do that, the attach command would have to hit both the volume controller and the compute controller, and that would be complex.

[Jay Pipes (jaypipes)]

Hi! As I said on IRC, not much I can review as far as the volume stuff goes, so here is some general feedback :)

1 === added file 'nova/db/sqlalchemy/migrate_repo/versions/003_cactus.py'

As you mentioned on IRC, I think it would be best to name this something like 003_add_provider_columns.py instead of 003_cactus.py, to make the actual migration file more descriptive of its contents...

74 + # Create tables
75 + for table in (
76 + #certificates, consoles, console_pools, instance_actions
77 + ):
78 + try:
79 + table.create()
80 + except Exception:
81 + logging.info(repr(table))
82 + logging.exception('Exception while creating table')
83 + raise
84 +
85 + # Alter column types
86 + # auth_tokens.c.user_id.alter(type=String(length=255,
87 + # convert_unicode=False,
88 + # assert_unicode=None,
89 + # unicode_error=None,
90 + # _warn_on_bytestring=False))

I recognize you probably commented that stuff out thinking that we are only using a single 003_cactus.py migration file, but if that migration file is renamed to something more descriptive (and we can have >1 migration file per release), obviously I think you should remove all that commented out code :)

Missed i18n in these locations:

139 + LOG.warn("ISCSI provider_location not stored, using discovery")
175 + LOG.debug("ISCSI Discovery: Found %s" % (location))
552 + raise exception.Error("local_path not supported")

For 175 above, please use the following instead:

LOG.debug(_("ISCSI Discovery: Found %s"), location)

Not sure if you meant to i18n this or not :)

457 + message = ("Unexpected number of virtual ips for cluster %s. "
458 + "Result=%s" %
459 + (cluster_name, ElementTree.tostring(cluster_xml)))

But if so, should be:

message = _("Unexpected number of virtual ips for cluster %(cluster)s. "
            "Result=%(result)s" % {'cluster': cluster_name,
                           'result': ElementTree.tostring(cluster_xml)})

Other than those little nits above, the only other advice I could give would be to maybe sprinkle a few LOG.debug()s around HpSanISCSIDriver methods and maybe add a bit of docstring documentation to HpSanISCSIDriver that gives the rough flow of the commands executed against the volume controller?

Cheers, and thanks for a great contribution!
jay

review: Needs Fixing

[Todd Willey (xtoddx)]

If we're going to do something special with the return value of driver.create_volume and driver.update_volume, those should be documented as such, probably in nova/volume/driver.py#VolumeDriver (since all the others inherit from that).

I'm not well versed in iscsi, but I'd hope there would be a way to get more specific matches in _do_iscsi_discovery, maybe by making sure a field is surrounded by spaces, etc. The line I'm referencing is "if FLAGS.iscsi_ip_prefix in target and volume_name in target:". I'd hate for a target on 10.1.2.30 to match when we're looking for 10.1.2.3 just because one is a substring of the other.

It would also be great to document what fields should be in iscsi_properties, since this is a class that is inherited other places. A central point would be better than having to look it up in _run_iscsiadmin, discover_volume, etc, getting just a few of the fields each time.

I also see in HpSanISCSIDriver you're using CHAP, but in ISCSIDriver._do_iscsi_discovery you have a note that discovery doesn't work with CHAP. Can you let me know what I'm missing?

Also, I think it is strange for the volume layer to have knowledge of the database schema, and have to know column names and have a variable it returns called 'db_update'. Maybe just renaming it 'state_changes' or something would assuage my fear, and make me feel that if there was a schema change that would be okay because we don't actually have to pass the return value to database calls.

All in all a very nice contribution. ☺

[Jay Pipes (jaypipes)]

Thanks for fixing up the few nits I brought up Justin. Approved from me from a style/overview perspective. I trust Todd and some others will more thoroughly review the call sequences to the volume drivers.

Cheers!
jay

review: Approve

[justinsb (justin-fathomdb)]

I documented the return value in VolumeDriver create_volume & create_export.

I changed the name of the model state change Dictionary from db_update to model_update, based on your suggestion. The volume drivers are tightly coupled to the model definitions, so I don't think we're violating any separation of concerns. However, the naming "DB" was violating the separation, so I renamed it. Thanks for pointing this out.

The two new properties are 'provider_auth' and 'provider_location'. They're intended to be opaque properties for the driver's own use, but the natural assumption is that _location would be data as to the location and _auth info about authentication. I could use a dictionary instead, but I tried to pick two sensible fields that are hopefully fairly generic across drivers. Using a dictionary is problematic in terms of how it gets serialized efficiently. I documented the default iSCSI use of provider_location and provider_auth on ISCSIDriver.

The iSCSI discovery code is inherited, though the indentation level changed (I'm probably not blameless on the original code though!) I agree that it's fragile, so I'd like to move away from it. HP SANs and Solaris 'SANs' no longer use it; I can fix OpenISCSI next but that's a behavioural change on code that is potentially in active use so probably belongs in its own patch. There's a note that discovery is deprecated, but this code path is there until I can fix it for OpeniSCSI, but it is the same code as before. If you think I should clean it up in this patch I can do that, but I think it's best not to.

The CHAP authentication issue with discovery will go away when discovery is removed. HP SANs don't use discovery, and they're also the only ones using CHAP right now. If CHAP is supported with discovery (?), we'd definitely have to pass some extra parameters which aren't being passed right now, and I don't see the point of making this chaneg if I'm going to remove this code anyway :-)

I documented the current iscsi_properties (though I really don't know how to format this so it doesn't look terrible)

[justinsb (justin-fathomdb)]

Fixed up code. Can't merge (or even run unit tests) until metadata patch merges though, because of numbering issues on the DB migrations.

[Jay Pipes (jaypipes)]

> Fixed up code. Can't merge (or even run unit tests) until metadata patch
> merges though, because of numbering issues on the DB migrations.

Hi! There's a little-known feature of Launchpad's merge proposal system that allows you to set a "prerequisite branch". Edit the merge proposal and set the pre-requisite branch to your metadata branch, and Launchpad will:

* Not show any changes from the metadata branch in this merge proposal diff (this is so you can, say, merge your local metadata branch into this hpsan branch and not have the metadata changes show up in the merge diff here)
* Not allow someone to set this to Approved before the metadata branch is merged into trunk.

Cheers!
jay

[justinsb (justin-fathomdb)]

Added metadata as prerequisite branch. But can we get that merged please? I think it's good to go...

[Todd Willey (xtoddx)]

Looks pretty good. I'd suggest one more pass on the docstrings. Pep-257 outlines the format for multi-line docstrings, which is essentially that a short sentence goes on the first line, then you write the rest, and you end with a blank line.

You can look at nova/db/api.py to see how the flags in the database layer are documented in RST, which should be a good way to document the properties here as well.

[Jay Pipes (jaypipes)]

lgtm3

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[FUJITA Tomonori (fujita-tomonori-deactivatedaccount)]

I'm still learning how the volume stuff works but here are some comments about iSCSI stuff:

- Avoiding discovery session sounds better. You can directly insert the target info into open-iscsi db and let open-iscsi log in it.

- iSCSI supports the mutual authentication (the initiator can verify the target too) so it might be better to make 'provider_auth' capable to handle it.

- If you will not remove the discovery session, 'provider_auth' needs to handle discovery session CHAP auth too.

[justinsb (justin-fathomdb)]

Todd: Fixed docstrings so that they look better and comply with PEP 257. Is there an easy preview tool and PEP 257 checker?

FUJITA: The goal is to remove discovery. It's not done in this patch, but it's coming for OpeniSCSI. So I don't want to invest time in supporting CHAP at the moment for discovery, because that is hopefully going away, and CHAP isn't used with OpenISCSI at the moment anyway.

I do agree that mutual authentication would be nice, but I'd like to keep this patch as focused as is possible. It would be great if you want to add support!

[Todd Willey (xtoddx)]

Lets get this done!

review: Approve

[OpenStack Infra (hudson-openstack)]

No proposals found for merge of lp:~justin-fathomdb/nova/justinsb-metadata3 into lp:nova.

[justinsb (justin-fathomdb)]

Ah - I accidentally pushed trunk to the (already merged) justinsb-metadata3 branch, which I think is the cause of the above error from Hudson. Nonetheless, it looks like this branch did merge clean. I'm just double-checking that, but I put it back to 'merged' to stop mergebot getting really confused!

=== added file 'nova/db/sqlalchemy/migrate_repo/versions/006_add_provider_data_to_volumes.py'
--- nova/db/sqlalchemy/migrate_repo/versions/006_add_provider_data_to_volumes.py	1970-01-01 00:00:00 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/006_add_provider_data_to_volumes.py	2011-02-24 06:50:14 +0000
@@ -0,0 +1,72 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Justin Santa Barbara.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from sqlalchemy import *
+from migrate import *
+
+from nova import log as logging
+
+
+meta = MetaData()
+
+
+# Table stub-definitions
+# Just for the ForeignKey and column creation to succeed, these are not the
+# actual definitions of instances or services.
+#
+volumes = Table('volumes', meta,
+                Column('id', Integer(), primary_key=True, nullable=False),
+                )
+
+
+#
+# New Tables
+#
+# None
+
+#
+# Tables to alter
+#
+# None
+
+#
+# Columns to add to existing tables
+#
+
+volumes_provider_location = Column('provider_location',
+                                   String(length=256,
+                                          convert_unicode=False,
+                                          assert_unicode=None,
+                                          unicode_error=None,
+                                          _warn_on_bytestring=False))
+
+volumes_provider_auth = Column('provider_auth',
+                               String(length=256,
+                                      convert_unicode=False,
+                                      assert_unicode=None,
+                                      unicode_error=None,
+                                      _warn_on_bytestring=False))
+
+
+def upgrade(migrate_engine):
+    # Upgrade operations go here. Don't create your own engine;
+    # bind migrate_engine to your metadata
+    meta.bind = migrate_engine
+
+    # Add columns to existing tables
+    volumes.create_column(volumes_provider_location)
+    volumes.create_column(volumes_provider_auth)

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-02-17 23:00:18 +0000
+++ nova/db/sqlalchemy/models.py	2011-02-24 06:50:14 +0000
@@ -243,6 +243,9 @@
     display_name = Column(String(255))
     display_description = Column(String(255))
 
+    provider_location = Column(String(255))
+    provider_auth = Column(String(255))
+
 
 class Quota(BASE, NovaBase):
     """Represents quota overrides for a project."""

=== modified file 'nova/volume/driver.py'
--- nova/volume/driver.py	2011-02-04 17:04:55 +0000
+++ nova/volume/driver.py	2011-02-24 06:50:14 +0000
@@ -21,6 +21,7 @@
 """
 
 import time
+import os
 
 from nova import exception
 from nova import flags
@@ -36,6 +37,8 @@
                     'Which device to export the volumes on')
 flags.DEFINE_string('num_shell_tries', 3,
                     'number of times to attempt to run flakey shell commands')
+flags.DEFINE_string('num_iscsi_scan_tries', 3,
+                    'number of times to rescan iSCSI target to find volume')
 flags.DEFINE_integer('num_shelves',
                     100,
                     'Number of vblade shelves')
@@ -88,7 +91,8 @@
                                   % FLAGS.volume_group)
 
     def create_volume(self, volume):
-        """Creates a logical volume."""
+        """Creates a logical volume. Can optionally return a Dictionary of
+        changes to the volume object to be persisted."""
         if int(volume['size']) == 0:
             sizestr = '100M'
         else:
@@ -123,7 +127,8 @@
         raise NotImplementedError()
 
     def create_export(self, context, volume):
-        """Exports the volume."""
+        """Exports the volume. Can optionally return a Dictionary of changes
+        to the volume object to be persisted."""
         raise NotImplementedError()
 
     def remove_export(self, context, volume):
@@ -222,7 +227,18 @@
 
 
 class ISCSIDriver(VolumeDriver):
-    """Executes commands relating to ISCSI volumes."""
+    """Executes commands relating to ISCSI volumes.
+
+    We make use of model provider properties as follows:
+
+    :provider_location:    if present, contains the iSCSI target information
+                           in the same format as an ietadm discovery
+                           i.e. '<ip>:<port>,<portal> <target IQN>'
+
+    :provider_auth:    if present, contains a space-separated triple:
+                       '<auth method> <auth username> <auth password>'.
+                       `CHAP` is the only auth_method in use at the moment.
+    """
 
     def ensure_export(self, context, volume):
         """Synchronously recreates an export for a logical volume."""
@@ -294,40 +310,149 @@
         self._execute("sudo ietadm --op delete --tid=%s" %
                       iscsi_target)
 
-    def _get_name_and_portal(self, volume):
-        """Gets iscsi name and portal from volume name and host."""
+    def _do_iscsi_discovery(self, volume):
+        #TODO(justinsb): Deprecate discovery and use stored info
+        #NOTE(justinsb): Discovery won't work with CHAP-secured targets (?)
+        LOG.warn(_("ISCSI provider_location not stored, using discovery"))
+
         volume_name = volume['name']
-        host = volume['host']
+
         (out, _err) = self._execute("sudo iscsiadm -m discovery -t "
-                                    "sendtargets -p %s" % host)
+                                    "sendtargets -p %s" % (volume['host']))
         for target in out.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]
-        return (iscsi_name, iscsi_portal)
+                return target
+        return None
+
+    def _get_iscsi_properties(self, volume):
+        """Gets iscsi configuration
+
+        We ideally get saved information in the volume entity, but fall back
+        to discovery if need be. Discovery may be completely removed in future
+        The properties are:
+
+        :target_discovered:    boolean indicating whether discovery was used
+
+        :target_iqn:    the IQN of the iSCSI target
+
+        :target_portal:    the portal of the iSCSI target
+
+        :auth_method:, :auth_username:, :auth_password:
+
+            the authentication details. Right now, either auth_method is not
+            present meaning no authentication, or auth_method == `CHAP`
+            meaning use CHAP with the specified credentials.
+        """
+
+        properties = {}
+
+        location = volume['provider_location']
+
+        if location:
+            # provider_location is the same format as iSCSI discovery output
+            properties['target_discovered'] = False
+        else:
+            location = self._do_iscsi_discovery(volume)
+
+            if not location:
+                raise exception.Error(_("Could not find iSCSI export "
+                                        " for volume %s") %
+                                      (volume['name']))
+
+            LOG.debug(_("ISCSI Discovery: Found %s") % (location))
+            properties['target_discovered'] = True
+
+        (iscsi_target, _sep, iscsi_name) = location.partition(" ")
+
+        iscsi_portal = iscsi_target.split(",")[0]
+
+        properties['target_iqn'] = iscsi_name
+        properties['target_portal'] = iscsi_portal
+
+        auth = volume['provider_auth']
+
+        if auth:
+            (auth_method, auth_username, auth_secret) = auth.split()
+
+            properties['auth_method'] = auth_method
+            properties['auth_username'] = auth_username
+            properties['auth_password'] = auth_secret
+
+        return properties
+
+    def _run_iscsiadm(self, iscsi_properties, iscsi_command):
+        command = ("sudo iscsiadm -m node -T %s -p %s %s" %
+                   (iscsi_properties['target_iqn'],
+                    iscsi_properties['target_portal'],
+                    iscsi_command))
+        (out, err) = self._execute(command)
+        LOG.debug("iscsiadm %s: stdout=%s stderr=%s" %
+                  (iscsi_command, out, err))
+        return (out, err)
+
+    def _iscsiadm_update(self, iscsi_properties, property_key, property_value):
+        iscsi_command = ("--op update -n %s -v %s" %
+                         (property_key, property_value))
+        return self._run_iscsiadm(iscsi_properties, iscsi_command)
 
     def discover_volume(self, volume):
         """Discover volume on a remote host."""
-        iscsi_name, iscsi_portal = self._get_name_and_portal(volume)
-        self._execute("sudo iscsiadm -m node -T %s -p %s --login" %
-                      (iscsi_name, iscsi_portal))
-        self._execute("sudo iscsiadm -m node -T %s -p %s --op update "
-                      "-n node.startup -v automatic" %
-                      (iscsi_name, iscsi_portal))
-        return "/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" % (iscsi_portal,
-                                                           iscsi_name)
+        iscsi_properties = self._get_iscsi_properties(volume)
+
+        if not iscsi_properties['target_discovered']:
+            self._run_iscsiadm(iscsi_properties, "--op new")
+
+        if iscsi_properties.get('auth_method'):
+            self._iscsiadm_update(iscsi_properties,
+                                  "node.session.auth.authmethod",
+                                  iscsi_properties['auth_method'])
+            self._iscsiadm_update(iscsi_properties,
+                                  "node.session.auth.username",
+                                  iscsi_properties['auth_username'])
+            self._iscsiadm_update(iscsi_properties,
+                                  "node.session.auth.password",
+                                  iscsi_properties['auth_password'])
+
+        self._run_iscsiadm(iscsi_properties, "--login")
+
+        self._iscsiadm_update(iscsi_properties, "node.startup", "automatic")
+
+        mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
+                        (iscsi_properties['target_portal'],
+                         iscsi_properties['target_iqn']))
+
+        # The /dev/disk/by-path/... node is not always present immediately
+        # TODO(justinsb): This retry-with-delay is a pattern, move to utils?
+        tries = 0
+        while not os.path.exists(mount_device):
+            if tries >= FLAGS.num_iscsi_scan_tries:
+                raise exception.Error(_("iSCSI device not found at %s") %
+                                      (mount_device))
+
+            LOG.warn(_("ISCSI volume not yet found at: %(mount_device)s. "
+                       "Will rescan & retry.  Try number: %(tries)s") %
+                     locals())
+
+            # The rescan isn't documented as being necessary(?), but it helps
+            self._run_iscsiadm(iscsi_properties, "--rescan")
+
+            tries = tries + 1
+            if not os.path.exists(mount_device):
+                time.sleep(tries ** 2)
+
+        if tries != 0:
+            LOG.debug(_("Found iSCSI node %(mount_device)s "
+                        "(after %(tries)s rescans)") %
+                      locals())
+
+        return mount_device
 
     def undiscover_volume(self, volume):
         """Undiscover volume on a remote host."""
-        iscsi_name, iscsi_portal = self._get_name_and_portal(volume)
-        self._execute("sudo iscsiadm -m node -T %s -p %s --op update "
-                      "-n node.startup -v manual" %
-                      (iscsi_name, iscsi_portal))
-        self._execute("sudo iscsiadm -m node -T %s -p %s --logout " %
-                      (iscsi_name, iscsi_portal))
-        self._execute("sudo iscsiadm -m node --op delete "
-                      "--targetname %s" % iscsi_name)
+        iscsi_properties = self._get_iscsi_properties(volume)
+        self._iscsiadm_update(iscsi_properties, "node.startup", "manual")
+        self._run_iscsiadm(iscsi_properties, "--logout")
+        self._run_iscsiadm(iscsi_properties, "--op delete")
 
 
 class FakeISCSIDriver(ISCSIDriver):

=== modified file 'nova/volume/manager.py'
--- nova/volume/manager.py	2011-02-15 18:19:52 +0000
+++ nova/volume/manager.py	2011-02-24 06:50:14 +0000
@@ -107,10 +107,14 @@
             vol_size = volume_ref['size']
             LOG.debug(_("volume %(vol_name)s: creating lv of"
                     " size %(vol_size)sG") % locals())
-            self.driver.create_volume(volume_ref)
+            model_update = self.driver.create_volume(volume_ref)
+            if model_update:
+                self.db.volume_update(context, volume_ref['id'], model_update)
 
             LOG.debug(_("volume %s: creating export"), volume_ref['name'])
-            self.driver.create_export(context, volume_ref)
+            model_update = self.driver.create_export(context, volume_ref)
+            if model_update:
+                self.db.volume_update(context, volume_ref['id'], model_update)
         except Exception:
             self.db.volume_update(context,
                                   volume_ref['id'], {'status': 'error'})

=== modified file 'nova/volume/san.py'
--- nova/volume/san.py	2011-02-09 19:25:18 +0000
+++ nova/volume/san.py	2011-02-24 06:50:14 +0000
@@ -16,13 +16,16 @@
 #    under the License.
 """
 Drivers for san-stored volumes.
+
 The unique thing about a SAN is that we don't expect that we can run the volume
- controller on the SAN hardware.  We expect to access it over SSH or some API.
+controller on the SAN hardware.  We expect to access it over SSH or some API.
 """
 
 import os
 import paramiko
 
+from xml.etree import ElementTree
+
 from nova import exception
 from nova import flags
 from nova import log as logging
@@ -41,37 +44,19 @@
                     'Password for SAN controller')
 flags.DEFINE_string('san_privatekey', '',
                     'Filename of private key to use for SSH authentication')
+flags.DEFINE_string('san_clustername', '',
+                    'Cluster name to use for creating volumes')
+flags.DEFINE_integer('san_ssh_port', 22,
+                    'SSH port to use with SAN')
 
 
 class SanISCSIDriver(ISCSIDriver):
     """ Base class for SAN-style storage volumes
-        (storage providers we access over SSH)"""
-    #Override because SAN ip != host ip
-    def _get_name_and_portal(self, volume):
-        """Gets iscsi name and portal from volume name and host."""
-        volume_name = volume['name']
-
-        # TODO(justinsb): store in volume, remerge with generic iSCSI code
-        host = FLAGS.san_ip
-
-        (out, _err) = self._execute("sudo iscsiadm -m discovery -t "
-                                    "sendtargets -p %s" % host)
-
-        location = None
-        find_iscsi_name = self._build_iscsi_target_name(volume)
-        for target in out.splitlines():
-            if find_iscsi_name in target:
-                (location, _sep, iscsi_name) = target.partition(" ")
-                break
-        if not location:
-            raise exception.Error(_("Could not find iSCSI export "
-                                    " for volume %s") %
-                                  volume_name)
-
-        iscsi_portal = location.split(",")[0]
-        LOG.debug("iscsi_name=%s, iscsi_portal=%s" %
-                  (iscsi_name, iscsi_portal))
-        return (iscsi_name, iscsi_portal)
+
+    A SAN-style storage value is 'different' because the volume controller
+    probably won't run on it, so we need to access is over SSH or another
+    remote protocol.
+    """
 
     def _build_iscsi_target_name(self, volume):
         return "%s%s" % (FLAGS.iscsi_target_prefix, volume['name'])
@@ -85,6 +70,7 @@
         ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
         if FLAGS.san_password:
             ssh.connect(FLAGS.san_ip,
+                        port=FLAGS.san_ssh_port,
                         username=FLAGS.san_login,
                         password=FLAGS.san_password)
         elif FLAGS.san_privatekey:
@@ -92,10 +78,11 @@
             # It sucks that paramiko doesn't support DSA keys
             privatekey = paramiko.RSAKey.from_private_key_file(privatekeyfile)
             ssh.connect(FLAGS.san_ip,
+                        port=FLAGS.san_ssh_port,
                         username=FLAGS.san_login,
                         pkey=privatekey)
         else:
-            raise exception.Error("Specify san_password or san_privatekey")
+            raise exception.Error(_("Specify san_password or san_privatekey"))
         return ssh
 
     def _run_ssh(self, command, check_exit_code=True):
@@ -124,10 +111,10 @@
     def check_for_setup_error(self):
         """Returns an error if prerequisites aren't met"""
         if not (FLAGS.san_password or FLAGS.san_privatekey):
-            raise exception.Error("Specify san_password or san_privatekey")
+            raise exception.Error(_("Specify san_password or san_privatekey"))
 
         if not (FLAGS.san_ip):
-            raise exception.Error("san_ip must be set")
+            raise exception.Error(_("san_ip must be set"))
 
 
 def _collect_lines(data):
@@ -155,17 +142,27 @@
 
 class SolarisISCSIDriver(SanISCSIDriver):
     """Executes commands relating to Solaris-hosted ISCSI volumes.
+
     Basic setup for a Solaris iSCSI server:
+
     pkg install storage-server SUNWiscsit
+
     svcadm enable stmf
+
     svcadm enable -r svc:/network/iscsi/target:default
+
     pfexec itadm create-tpg e1000g0 ${MYIP}
+
     pfexec itadm create-target -t e1000g0
 
+
     Then grant the user that will be logging on lots of permissions.
     I'm not sure exactly which though:
+
     zfs allow justinsb create,mount,destroy rpool
+
     usermod -P'File System Management' justinsb
+
     usermod -P'Primary Administrator' justinsb
 
     Also make sure you can login using san_login & san_password/san_privatekey
@@ -306,6 +303,17 @@
             self._run_ssh("pfexec /usr/sbin/stmfadm add-view -t %s %s" %
                           (target_group_name, luid))
 
+        #TODO(justinsb): Is this always 1? Does it matter?
+        iscsi_portal_interface = '1'
+        iscsi_portal = FLAGS.san_ip + ":3260," + iscsi_portal_interface
+
+        db_update = {}
+        db_update['provider_location'] = ("%s %s" %
+                                          (iscsi_portal,
+                                           iscsi_name))
+
+        return db_update
+
     def remove_export(self, context, volume):
         """Removes an export for a logical volume."""
 
@@ -333,3 +341,245 @@
         if self._is_lu_created(volume):
             self._run_ssh("pfexec /usr/sbin/sbdadm delete-lu %s" %
                           (luid))
+
+
+class HpSanISCSIDriver(SanISCSIDriver):
+    """Executes commands relating to HP/Lefthand SAN ISCSI volumes.
+
+    We use the CLIQ interface, over SSH.
+
+    Rough overview of CLIQ commands used:
+
+    :createVolume:    (creates the volume)
+
+    :getVolumeInfo:    (to discover the IQN etc)
+
+    :getClusterInfo:    (to discover the iSCSI target IP address)
+
+    :assignVolumeChap:    (exports it with CHAP security)
+
+    The 'trick' here is that the HP SAN enforces security by default, so
+    normally a volume mount would need both to configure the SAN in the volume
+    layer and do the mount on the compute layer.  Multi-layer operations are
+    not catered for at the moment in the nova architecture, so instead we
+    share the volume using CHAP at volume creation time.  Then the mount need
+    only use those CHAP credentials, so can take place exclusively in the
+    compute layer.
+    """
+
+    def _cliq_run(self, verb, cliq_args):
+        """Runs a CLIQ command over SSH, without doing any result parsing"""
+        cliq_arg_strings = []
+        for k, v in cliq_args.items():
+            cliq_arg_strings.append(" %s=%s" % (k, v))
+        cmd = verb + ''.join(cliq_arg_strings)
+
+        return self._run_ssh(cmd)
+
+    def _cliq_run_xml(self, verb, cliq_args, check_cliq_result=True):
+        """Runs a CLIQ command over SSH, parsing and checking the output"""
+        cliq_args['output'] = 'XML'
+        (out, _err) = self._cliq_run(verb, cliq_args)
+
+        LOG.debug(_("CLIQ command returned %s"), out)
+
+        result_xml = ElementTree.fromstring(out)
+        if check_cliq_result:
+            response_node = result_xml.find("response")
+            if response_node is None:
+                msg = (_("Malformed response to CLIQ command "
+                         "%(verb)s %(cliq_args)s. Result=%(out)s") %
+                       locals())
+                raise exception.Error(msg)
+
+            result_code = response_node.attrib.get("result")
+
+            if result_code != "0":
+                msg = (_("Error running CLIQ command %(verb)s %(cliq_args)s. "
+                         " Result=%(out)s") %
+                       locals())
+                raise exception.Error(msg)
+
+        return result_xml
+
+    def _cliq_get_cluster_info(self, cluster_name):
+        """Queries for info about the cluster (including IP)"""
+        cliq_args = {}
+        cliq_args['clusterName'] = cluster_name
+        cliq_args['searchDepth'] = '1'
+        cliq_args['verbose'] = '0'
+
+        result_xml = self._cliq_run_xml("getClusterInfo", cliq_args)
+
+        return result_xml
+
+    def _cliq_get_cluster_vip(self, cluster_name):
+        """Gets the IP on which a cluster shares iSCSI volumes"""
+        cluster_xml = self._cliq_get_cluster_info(cluster_name)
+
+        vips = []
+        for vip in cluster_xml.findall("response/cluster/vip"):
+            vips.append(vip.attrib.get('ipAddress'))
+
+        if len(vips) == 1:
+            return vips[0]
+
+        _xml = ElementTree.tostring(cluster_xml)
+        msg = (_("Unexpected number of virtual ips for cluster "
+                 " %(cluster_name)s. Result=%(_xml)s") %
+               locals())
+        raise exception.Error(msg)
+
+    def _cliq_get_volume_info(self, volume_name):
+        """Gets the volume info, including IQN"""
+        cliq_args = {}
+        cliq_args['volumeName'] = volume_name
+        result_xml = self._cliq_run_xml("getVolumeInfo", cliq_args)
+
+        # Result looks like this:
+        #<gauche version="1.0">
+        #  <response description="Operation succeeded." name="CliqSuccess"
+        #            processingTime="87" result="0">
+        #    <volume autogrowPages="4" availability="online" blockSize="1024"
+        #       bytesWritten="0" checkSum="false" clusterName="Cluster01"
+        #       created="2011-02-08T19:56:53Z" deleting="false" description=""
+        #       groupName="Group01" initialQuota="536870912" isPrimary="true"
+        #       iscsiIqn="iqn.2003-10.com.lefthandnetworks:group01:25366:vol-b"
+        #       maxSize="6865387257856" md5="9fa5c8b2cca54b2948a63d833097e1ca"
+        #       minReplication="1" name="vol-b" parity="0" replication="2"
+        #       reserveQuota="536870912" scratchQuota="4194304"
+        #       serialNumber="9fa5c8b2cca54b2948a63d833097e1ca0000000000006316"
+        #       size="1073741824" stridePages="32" thinProvision="true">
+        #      <status description="OK" value="2"/>
+        #      <permission access="rw"
+        #            authGroup="api-34281B815713B78-(trimmed)51ADD4B7030853AA7"
+        #            chapName="chapusername" chapRequired="true" id="25369"
+        #            initiatorSecret="" iqn="" iscsiEnabled="true"
+        #            loadBalance="true" targetSecret="supersecret"/>
+        #    </volume>
+        #  </response>
+        #</gauche>
+
+        # Flatten the nodes into a dictionary; use prefixes to avoid collisions
+        volume_attributes = {}
+
+        volume_node = result_xml.find("response/volume")
+        for k, v in volume_node.attrib.items():
+            volume_attributes["volume." + k] = v
+
+        status_node = volume_node.find("status")
+        if not status_node is None:
+            for k, v in status_node.attrib.items():
+                volume_attributes["status." + k] = v
+
+        # We only consider the first permission node
+        permission_node = volume_node.find("permission")
+        if not permission_node is None:
+            for k, v in status_node.attrib.items():
+                volume_attributes["permission." + k] = v
+
+        LOG.debug(_("Volume info: %(volume_name)s => %(volume_attributes)s") %
+                  locals())
+        return volume_attributes
+
+    def create_volume(self, volume):
+        """Creates a volume."""
+        cliq_args = {}
+        cliq_args['clusterName'] = FLAGS.san_clustername
+        #TODO(justinsb): Should we default to inheriting thinProvision?
+        cliq_args['thinProvision'] = '1' if FLAGS.san_thin_provision else '0'
+        cliq_args['volumeName'] = volume['name']
+        if int(volume['size']) == 0:
+            cliq_args['size'] = '100MB'
+        else:
+            cliq_args['size'] = '%sGB' % volume['size']
+
+        self._cliq_run_xml("createVolume", cliq_args)
+
+        volume_info = self._cliq_get_volume_info(volume['name'])
+        cluster_name = volume_info['volume.clusterName']
+        iscsi_iqn = volume_info['volume.iscsiIqn']
+
+        #TODO(justinsb): Is this always 1? Does it matter?
+        cluster_interface = '1'
+
+        cluster_vip = self._cliq_get_cluster_vip(cluster_name)
+        iscsi_portal = cluster_vip + ":3260," + cluster_interface
+
+        model_update = {}
+        model_update['provider_location'] = ("%s %s" %
+                                             (iscsi_portal,
+                                              iscsi_iqn))
+
+        return model_update
+
+    def delete_volume(self, volume):
+        """Deletes a volume."""
+        cliq_args = {}
+        cliq_args['volumeName'] = volume['name']
+        cliq_args['prompt'] = 'false'  # Don't confirm
+
+        self._cliq_run_xml("deleteVolume", cliq_args)
+
+    def local_path(self, volume):
+        # TODO(justinsb): Is this needed here?
+        raise exception.Error(_("local_path not supported"))
+
+    def ensure_export(self, context, volume):
+        """Synchronously recreates an export for a logical volume."""
+        return self._do_export(context, volume, force_create=False)
+
+    def create_export(self, context, volume):
+        return self._do_export(context, volume, force_create=True)
+
+    def _do_export(self, context, volume, force_create):
+        """Supports ensure_export and create_export"""
+        volume_info = self._cliq_get_volume_info(volume['name'])
+
+        is_shared = 'permission.authGroup' in volume_info
+
+        model_update = {}
+
+        should_export = False
+
+        if force_create or not is_shared:
+            should_export = True
+            # Check that we have a project_id
+            project_id = volume['project_id']
+            if not project_id:
+                project_id = context.project_id
+
+            if project_id:
+                #TODO(justinsb): Use a real per-project password here
+                chap_username = 'proj_' + project_id
+                # HP/Lefthand requires that the password be >= 12 characters
+                chap_password = 'project_secret_' + project_id
+            else:
+                msg = (_("Could not determine project for volume %s, "
+                         "can't export") %
+                         (volume['name']))
+                if force_create:
+                    raise exception.Error(msg)
+                else:
+                    LOG.warn(msg)
+                    should_export = False
+
+        if should_export:
+            cliq_args = {}
+            cliq_args['volumeName'] = volume['name']
+            cliq_args['chapName'] = chap_username
+            cliq_args['targetSecret'] = chap_password
+
+            self._cliq_run_xml("assignVolumeChap", cliq_args)
+
+            model_update['provider_auth'] = ("CHAP %s %s" %
+                                             (chap_username, chap_password))
+
+        return model_update
+
+    def remove_export(self, context, volume):
+        """Removes an export for a logical volume."""
+        cliq_args = {}
+        cliq_args['volumeName'] = volume['name']
+
+        self._cliq_run_xml("unassignVolume", cliq_args)