> Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>
> ---
>  doc/design-shared-storage.rst |  204 ++++++++++++++++++++++------------------
>  1 files changed, 112 insertions(+), 92 deletions(-)

> diff --git a/doc/design-shared-storage.rst b/doc/design-shared-storage.rst
> index c175476..7080182 100644
> --- a/doc/design-shared-storage.rst
> +++ b/doc/design-shared-storage.rst
> @@ -64,15 +64,11 @@ The design addresses the following procedures:
>    filesystems.
>  - Introduction of shared block device disk template with device
>    adoption.
> +- Introduction of an External Storage Interface.

>  Additionally, mid- to long-term goals include:

>  - Support for external “storage pools”.
> -- Introduction of an interface for communicating with external scripts,
> -  providing methods for the various stages of a block device's and
> -  instance's life-cycle. In order to provide storage provisioning
> -  capabilities for various SAN appliances, external helpers in the form
> -  of a “storage driver” will be possibly introduced as well.

>  Refactoring of all code referring to constants.DTS_NET_MIRROR
>  =============================================================
> @@ -159,6 +155,104 @@ The shared block device template will make the following assumptions:
>  - The device will be available with the same path under all nodes in the
>    node group.

> +Introduction of an External Storage Interface
> +==============================================
> +Overview
> +--------
> +
> +To extend the shared block storage template and give Ganeti the ability
> +to control and manipulate external storage (provisioning, removal,
> +growing, etc.) we need a more generic approach. The generic method for
> +supporting external shared storage in Ganeti will be to have an
> +ExtStorage provider for each external shared storage hardware type. The
> +ExtStorage provider will be a set of files (executable scripts and text
> +files), contained inside a directory which will be named after the
> +provider. This directory must be present across all nodes of a nodegroup
> +(Ganeti doesn't replicate it), in order for the provider to be usable by
> +Ganeti for this nodegroup (valid).

>> Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>
>> ---
>>   doc/design-shared-storage.rst |  204 ++++++++++++++++++++++------------------
>>   1 files changed, 112 insertions(+), 92 deletions(-)

>> diff --git a/doc/design-shared-storage.rst b/doc/design-shared-storage.rst
>> index c175476..7080182 100644
>> --- a/doc/design-shared-storage.rst
>> +++ b/doc/design-shared-storage.rst
>> @@ -64,15 +64,11 @@ The design addresses the following procedures:
>>     filesystems.
>>   - Introduction of shared block device disk template with device
>>     adoption.
>> +- Introduction of an External Storage Interface.

>>   Additionally, mid- to long-term goals include:

>>   - Support for external “storage pools”.
>> -- Introduction of an interface for communicating with external scripts,
>> -  providing methods for the various stages of a block device's and
>> -  instance's life-cycle. In order to provide storage provisioning
>> -  capabilities for various SAN appliances, external helpers in the form
>> -  of a “storage driver” will be possibly introduced as well.

>>   Refactoring of all code referring to constants.DTS_NET_MIRROR
>>   =============================================================
>> @@ -159,6 +155,104 @@ The shared block device template will make the following assumptions:
>>   - The device will be available with the same path under all nodes in the
>>     node group.

>> +Introduction of an External Storage Interface
>> +==============================================
>> +Overview
>> +--------
>> +
>> +To extend the shared block storage template and give Ganeti the ability
>> +to control and manipulate external storage (provisioning, removal,
>> +growing, etc.) we need a more generic approach. The generic method for
>> +supporting external shared storage in Ganeti will be to have an
>> +ExtStorage provider for each external shared storage hardware type. The
>> +ExtStorage provider will be a set of files (executable scripts and text
>> +files), contained inside a directory which will be named after the
>> +provider. This directory must be present across all nodes of a nodegroup
>> +(Ganeti doesn't replicate it), in order for the provider to be usable by
>> +Ganeti for this nodegroup (valid).
> How will Ganeti behave if they are not consistent? Report errors? (in
> cluster verify?) Ignore the provider? Etc.

>> The external shared storage hardware
>> +should also be accessible by all nodes of this nodegroup too.
>> +
>> +An “ExtStorage provider” will have to provide the following methods:
>> +
>> +- Create a disk
>> +- Remove a disk
>> +- Grow a disk
>> +- Attach a disk to a given node
>> +- Detach a disk from a given node
>> +- Verify its supported parameters
>> +
>> +The proposed ExtStorage interface borrows heavily from the OS
>> +interface and follows a one-script-per-function approach. An ExtStorage
>> +provider is expected to provide the following scripts:
>> +
>> +- `create`
>> +- `remove`
>> +- `grow`
>> +- `attach`
>> +- `detach`
>> +- `verify`
>> +
>> +All scripts will be called with no arguments and get their input via
>> +environment variables. A common set of variables will be exported for
>> +all commands, and some of them might have extra ones.
>> +
>> +- `VOL_NAME`: The name of the volume. This is unique for Ganeti and it
>> +  uses it to refer to a specific volume inside the external storage.
>> +- `VOL_SIZE`: The volume's size in mebibytes.
>> +- `VOL_NEW_SIZE`: Available only to the `grow` script. It declares the
>> +  new size of the volume after grow (in mebibytes).
>> +- `EXTP_name`: ExtStorage parameter, where `name` is the parameter in
>> +  upper-case (same as OS interface's `OSP_*` parameters).
>> +
>> +All scripts except `attach` should return 0 on success and non-zero on
>> +error, accompanied by an appropriate error message on stderr. The
>> +`attach` script should return a string on stdout on success, which is
>> +the block device's full path, after it has been successfully attached to
>> +the host node. On error it should return non-zero.
>> +
>> +Implementation
>> +--------------
>> +
>> +To support the ExtStorage interface, we will introduce a new disk
>> +template called `ext`. This template will implement the existing Ganeti
>> +disk interface in `lib/bdev.py` (create, remove, attach, assemble,
>> +shutdown, grow), and will simultaneously pass control to the external
>> +scripts to actually handle the above actions. The `ext` disk template
>> +will act as a translation layer between the current Ganeti disk
>> +interface and the ExtStorage providers.
>> +
>> +We will also introduce a new IDISK_PARAM called `IDISK_PROVIDER =
>> +provider`, which will be used at the command line to select the desired
>> +ExtStorage provider. This parameter will be valid only for template
>> +`ext` e.g.::
>> +
>> + gnt-instance add -t ext --disk=0:size=2G,provider=sample_provider1
>> +
>> +The Extstorage interface will support different disks to be created by
>> +different providers. e.g.::
>> +
>> + gnt-instance add -t ext --disk=0:size=2G,provider=sample_provider1
>> +                         --disk=1:size=1G,provider=sample_provider2
>> +                         --disk=2:size=3G,provider=sample_provider1
> This (also in the context of your other design changes) makes me a bit
> uneasy, with regards to coordinating changes across multiple providers
> in live migration and similar changes (even startup). Have you thought
> about this?

>> +Finally, the ExtStorage interface will support passing of parameters to
>> +the ExtStorage provider. This will also be done per disk, from the
>> +command line::
>> +
>> + gnt-instance add -t ext --disk=0:size=1G,provider=sample_provider1,
>> +                                  param1=value1,param2=value2
>> +
>> +The above parameters will be exported to the ExtStorage provider's
>> +scripts as the enviromental variables:
>> +
>> +- `EXTP_PARAM1 = str(value1)`
>> +- `EXTP_PARAM2 = str(value2)`
>> +
>> +We will also introduce a new Ganeti client called `gnt-storage` which
>> +will be used to diagnose ExtStorage providers and show information about
>> +them, similarly to the way  `gnt-os diagose` and `gnt-os info` handle OS
>> +definitions.
> Hmm… you got me here (I was hoping to avoid new python-based CLI
> front-ends, but it's too early for the switch ;-).

> Except for the above two small comments, LGTM, thanks.

> iustin

> >>Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>
> >>---
> >>  doc/design-shared-storage.rst |  204 ++++++++++++++++++++++------------------
> >>  1 files changed, 112 insertions(+), 92 deletions(-)

> >>diff --git a/doc/design-shared-storage.rst b/doc/design-shared-storage.rst
> >>index c175476..7080182 100644
> >>--- a/doc/design-shared-storage.rst
> >>+++ b/doc/design-shared-storage.rst
> >>@@ -64,15 +64,11 @@ The design addresses the following procedures:
> >>    filesystems.
> >>  - Introduction of shared block device disk template with device
> >>    adoption.
> >>+- Introduction of an External Storage Interface.
> >>  Additionally, mid- to long-term goals include:
> >>  - Support for external “storage pools”.
> >>-- Introduction of an interface for communicating with external scripts,
> >>-  providing methods for the various stages of a block device's and
> >>-  instance's life-cycle. In order to provide storage provisioning
> >>-  capabilities for various SAN appliances, external helpers in the form
> >>-  of a “storage driver” will be possibly introduced as well.
> >>  Refactoring of all code referring to constants.DTS_NET_MIRROR
> >>  =============================================================
> >>@@ -159,6 +155,104 @@ The shared block device template will make the following assumptions:
> >>  - The device will be available with the same path under all nodes in the
> >>    node group.
> >>+Introduction of an External Storage Interface
> >>+==============================================
> >>+Overview
> >>+--------
> >>+
> >>+To extend the shared block storage template and give Ganeti the ability
> >>+to control and manipulate external storage (provisioning, removal,
> >>+growing, etc.) we need a more generic approach. The generic method for
> >>+supporting external shared storage in Ganeti will be to have an
> >>+ExtStorage provider for each external shared storage hardware type. The
> >>+ExtStorage provider will be a set of files (executable scripts and text
> >>+files), contained inside a directory which will be named after the
> >>+provider. This directory must be present across all nodes of a nodegroup
> >>+(Ganeti doesn't replicate it), in order for the provider to be usable by
> >>+Ganeti for this nodegroup (valid).
> >How will Ganeti behave if they are not consistent? Report errors? (in
> >cluster verify?) Ignore the provider? Etc.

> The ExtStorage code follows exactly the behavior of the code
> handling OS definitions: It produces appropriate error messages
> and also comes with `gnt-storage {diagnose, info}' similarly to
> `gnt-os {diagnose, info}'.

> There is only one difference compared to the way OS defs are
> handled:

> The ExtStorage diagnose code calculates the validity of each provider
> for each nodegroup in the cmdlib logic rather than in the client.
> This was marked as 'TODO'' inside cmdlib for OS diagnose.

> This gives you the flexibility to do neat things easily, such as running
> the LU from inside cluster verify and producing validity statuses
> each provider-nodegroup combination. So, presumably this can also
> be used inside `gnt-cluster verify' in the future.

> I'm not sure I can understand your point completely. Given the
> diagnose functionality described above, are you concerned providers
> are going to be in inconsistent state among nodes? Is it a matter
> of how the allocator decides the target node given different providers?

> This makes me happy :-)
> I understand this may need some porting to Haskell later on.
> I hope my Haskell skills will have improved enough by then, for me to
> be able to contribute to the effort.

>>>> Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>
>>>> ---
>>>>   doc/design-shared-storage.rst |  204 ++++++++++++++++++++++------------------
>>>>   1 files changed, 112 insertions(+), 92 deletions(-)

>>>> diff --git a/doc/design-shared-storage.rst b/doc/design-shared-storage.rst
>>>> index c175476..7080182 100644
>>>> --- a/doc/design-shared-storage.rst
>>>> +++ b/doc/design-shared-storage.rst
>>>> @@ -64,15 +64,11 @@ The design addresses the following procedures:
>>>>     filesystems.
>>>>   - Introduction of shared block device disk template with device
>>>>     adoption.
>>>> +- Introduction of an External Storage Interface.
>>>>   Additionally, mid- to long-term goals include:
>>>>   - Support for external “storage pools”.
>>>> -- Introduction of an interface for communicating with external scripts,
>>>> -  providing methods for the various stages of a block device's and
>>>> -  instance's life-cycle. In order to provide storage provisioning
>>>> -  capabilities for various SAN appliances, external helpers in the form
>>>> -  of a “storage driver” will be possibly introduced as well.
>>>>   Refactoring of all code referring to constants.DTS_NET_MIRROR
>>>>   =============================================================
>>>> @@ -159,6 +155,104 @@ The shared block device template will make the following assumptions:
>>>>   - The device will be available with the same path under all nodes in the
>>>>     node group.
>>>> +Introduction of an External Storage Interface
>>>> +==============================================
>>>> +Overview
>>>> +--------
>>>> +
>>>> +To extend the shared block storage template and give Ganeti the ability
>>>> +to control and manipulate external storage (provisioning, removal,
>>>> +growing, etc.) we need a more generic approach. The generic method for
>>>> +supporting external shared storage in Ganeti will be to have an
>>>> +ExtStorage provider for each external shared storage hardware type. The
>>>> +ExtStorage provider will be a set of files (executable scripts and text
>>>> +files), contained inside a directory which will be named after the
>>>> +provider. This directory must be present across all nodes of a nodegroup
>>>> +(Ganeti doesn't replicate it), in order for the provider to be usable by
>>>> +Ganeti for this nodegroup (valid).
>>> How will Ganeti behave if they are not consistent? Report errors? (in
>>> cluster verify?) Ignore the provider? Etc.
>> The ExtStorage code follows exactly the behavior of the code
>> handling OS definitions: It produces appropriate error messages
>> and also comes with `gnt-storage {diagnose, info}' similarly to
>> `gnt-os {diagnose, info}'.

>> There is only one difference compared to the way OS defs are
>> handled:

>> The ExtStorage diagnose code calculates the validity of each provider
>> for each nodegroup in the cmdlib logic rather than in the client.
>> This was marked as 'TODO'' inside cmdlib for OS diagnose.

>> This gives you the flexibility to do neat things easily, such as running
>> the LU from inside cluster verify and producing validity statuses
>> each provider-nodegroup combination. So, presumably this can also
>> be used inside `gnt-cluster verify' in the future.
> Sounds very good, thanks!

>>>> The external shared storage hardware
>>>> +should also be accessible by all nodes of this nodegroup too.
>>>> +
>>>> +An “ExtStorage provider” will have to provide the following methods:
>>>> +
>>>> +- Create a disk
>>>> +- Remove a disk
>>>> +- Grow a disk
>>>> +- Attach a disk to a given node
>>>> +- Detach a disk from a given node
>>>> +- Verify its supported parameters
>>>> +
>>>> +The proposed ExtStorage interface borrows heavily from the OS
>>>> +interface and follows a one-script-per-function approach. An ExtStorage
>>>> +provider is expected to provide the following scripts:
>>>> +
>>>> +- `create`
>>>> +- `remove`
>>>> +- `grow`
>>>> +- `attach`
>>>> +- `detach`
>>>> +- `verify`
>>>> +
>>>> +All scripts will be called with no arguments and get their input via
>>>> +environment variables. A common set of variables will be exported for
>>>> +all commands, and some of them might have extra ones.
>>>> +
>>>> +- `VOL_NAME`: The name of the volume. This is unique for Ganeti and it
>>>> +  uses it to refer to a specific volume inside the external storage.
>>>> +- `VOL_SIZE`: The volume's size in mebibytes.
>>>> +- `VOL_NEW_SIZE`: Available only to the `grow` script. It declares the
>>>> +  new size of the volume after grow (in mebibytes).
>>>> +- `EXTP_name`: ExtStorage parameter, where `name` is the parameter in
>>>> +  upper-case (same as OS interface's `OSP_*` parameters).
>>>> +
>>>> +All scripts except `attach` should return 0 on success and non-zero on
>>>> +error, accompanied by an appropriate error message on stderr. The
>>>> +`attach` script should return a string on stdout on success, which is
>>>> +the block device's full path, after it has been successfully attached to
>>>> +the host node. On error it should return non-zero.
>>>> +
>>>> +Implementation
>>>> +--------------
>>>> +
>>>> +To support the ExtStorage interface, we will introduce a new disk
>>>> +template called `ext`. This template will implement the existing Ganeti
>>>> +disk interface in `lib/bdev.py` (create, remove, attach, assemble,
>>>> +shutdown, grow), and will simultaneously pass control to the external
>>>> +scripts to actually handle the above actions. The `ext` disk template
>>>> +will act as a translation layer between the current Ganeti disk
>>>> +interface and the ExtStorage providers.
>>>> +
>>>> +We will also introduce a new IDISK_PARAM called `IDISK_PROVIDER =
>>>> +provider`, which will be used at the command line to select the desired
>>>> +ExtStorage provider. This parameter will be valid only for template
>>>> +`ext` e.g.::
>>>> +
>>>> + gnt-instance add -t ext --disk=0:size=2G,provider=sample_provider1
>>>> +
>>>> +The Extstorage interface will support different disks to be created by
>>>> +different providers. e.g.::
>>>> +
>>>> + gnt-instance add -t ext --disk=0:size=2G,provider=sample_provider1
>>>> +                         --disk=1:size=1G,provider=sample_provider2
>>>> +                         --disk=2:size=3G,provider=sample_provider1
>>> This (also in the context of your other design changes) makes me a bit
>>> uneasy, with regards to coordinating changes across multiple providers
>>> in live migration and similar changes (even startup). Have you thought
>>> about this?
>> I'm not sure I can understand your point completely. Given the
>> diagnose functionality described above, are you concerned providers
>> are going to be in inconsistent state among nodes? Is it a matter
>> of how the allocator decides the target node given different providers?
> Ah no, see below.

>> Can you expand on the "coordinating changes across multiple providers
>> in live migration and similar changes (even startup)" part of your
>> question? Perhaps with some examples?
> I'll try :)

> For example, let's say we have an instance with first disk DRBD, second
> disk ext,provider=p1, third disk ext,provider=p2.

> We know we can live migrate an instance across node groups for DRBD, and
> we now we can migrate ext providers if they are available in both
> groups. But combining all these checks across multiple disks is just 2%
> more tricky: we need to move from "disk_template in
> constants.DTS_MIRRORED" to something like "does all instance disks allow
> migration/failover/move from (nodegroup A, nodes [a,b]) to (nodegroup B,
> nodes [c,d])" (where A could be equal to B)?

> This is doable, just means that a lot of decisions about the instance
> behaviour (can be moved, can be live migrated, etc.) will move away from
> the instance level (disk_template) and become an aggregate of the
> instance's disk capabilities.

> The ExtStorage Interface provides Ganeti with the ability to interact
> with externally connected shared storage pools, visible by all
> VM-capable nodes. This means that Ganeti is able to handle VM disks
> that reside inside a NAS/SAN or any distributed block storage provider.

> The ExtStorage Interface provides a clear API, heavily inspired by the
> gnt-os-interface API, that can be used by storage vendors or sysadmins
> to write simple ExtStorage Providers (correlated to gnt-os-interface's
> OS Definitions). Those Providers will glue externally attached shared
> storage with Ganeti, without the need of preprovisioned block devices
> on Ganeti VM-capable nodes as confined be the current `blockdev' disk
> template.

> To do so, we implement a new disk template called `ext' (of type
> DTS_EXT_MIRROR) that passes control to externally provided scripts
> (the ExtStorage Provider) for the template's basic functions:

>  create / attach / detach / remove / grow

> The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
> and only one ExtStorage Provider is supported called `ext'.

> The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
> is generated as in disk template `plain' and X is the disk's index.

> diff --git a/Makefile.am b/Makefile.am
> index 58daa7a..99c1bc2 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1175,6 +1175,7 @@ lib/_autoconf.py: Makefile | stamp-directories
>      echo "SSH_CONFIG_DIR = '$(SSH_CONFIG_DIR)'"; \
>      echo "EXPORT_DIR = '$(EXPORT_DIR)'"; \
>      echo "OS_SEARCH_PATH = [$(OS_SEARCH_PATH)]"; \
> +    echo "ES_SEARCH_PATH = [$(ES_SEARCH_PATH)]"; \
>      echo "XEN_BOOTLOADER = '$(XEN_BOOTLOADER)'"; \
>      echo "XEN_KERNEL = '$(XEN_KERNEL)'"; \
>      echo "XEN_INITRD = '$(XEN_INITRD)'"; \
> diff --git a/configure.ac b/configure.ac
> index 5bb14af..a697e31 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -60,6 +60,18 @@ AC_ARG_WITH([os-search-path],
>    [os_search_path="'/srv/ganeti/os'"])
>  AC_SUBST(OS_SEARCH_PATH, $os_search_path)

> +# --with-extstorage-search-path=...
> +# same black sed magic for quoting of the strings in the list
> +AC_ARG_WITH([extstorage-search-path],
> +  [AS_HELP_STRING([--with-extstorage-search-path=LIST],
> +    [comma separated list of directories to]
> +    [ search for External Storage Providers]
> +    [ (default is /srv/ganeti/extstorage)]
> +  )],
> +  [es_search_path=`echo -n "$withval" | sed -e "s/\([[^,]]*\)/'\1'/g"`],
> +  [es_search_path="'/srv/ganeti/extstorage'"])
> +AC_SUBST(ES_SEARCH_PATH, $es_search_path)
> +
>  # --with-iallocator-search-path=...
>  # do a bit of black sed magic to for quoting of the strings in the list
>  AC_ARG_WITH([iallocator-search-path],
> diff --git a/lib/bdev.py b/lib/bdev.py
> index d6c1a66..b0ed7c0 100644
> --- a/lib/bdev.py
> +++ b/lib/bdev.py
> @@ -33,6 +33,7 @@ import logging
>  from ganeti import utils
>  from ganeti import errors
>  from ganeti import constants
> +from ganeti import pathutils
>  from ganeti import objects
>  from ganeti import compat
>  from ganeti import netutils
> @@ -2648,11 +2649,328 @@ class RADOSBlockDevice(BlockDev):
>                    result.fail_reason, result.output)

> +class ExtStorageDevice(BlockDev):
> +  """A block device provided by an ExtStorage Provider.
> +
> +  This class implements the External Storage Interface, which means
> +  handling of the externally provided block devices.
> +
> +  """
> +  def __init__(self, unique_id, children, size, params):
> +    """Attaches to an extstorage block device.
> +
> +    """
> +    super(ExtStorageDevice, self).__init__(unique_id, children, size, params)
> +    if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
> +      raise ValueError("Invalid configuration data %s" % str(unique_id))
> +
> +    self.driver, self.vol_name = unique_id
> +
> +    self.major = self.minor = None
> +    self.Attach()
> +
> +  @classmethod
> +  def Create(cls, unique_id, children, size, params):
> +    """Create a new extstorage device.
> +
> +    Provision a new volume using an extstorage provider, which will
> +    then be mapped to a block device.
> +
> +    """
> +    if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
> +      raise errors.ProgrammerError("Invalid configuration data %s" %
> +                                   str(unique_id))
> +
> +    # Call the External Storage's create script,
> +    # to provision a new Volume inside the External Storage
> +    _ExtStorageAction(constants.ES_ACTION_CREATE, unique_id, str(size))
> +
> +    return ExtStorageDevice(unique_id, children, size, params)
> +
> +  def Remove(self):
> +    """Remove the extstorage device.
> +
> +    """
> +    if not self.minor and not self.Attach():
> +      # The extstorage device doesn't exist.
> +      return
> +
> +    # First shutdown the device (remove mappings).
> +    self.Shutdown()
> +
> +    # Call the External Storage's remove script,
> +    # to remove the Volume from the External Storage
> +    _ExtStorageAction(constants.ES_ACTION_REMOVE, self.unique_id)
> +
> +  def Rename(self, new_id):
> +    """Rename this device.
> +
> +    """
> +    pass
> +
> +  def Attach(self):
> +    """Attach to an existing extstorage device.
> +
> +    This method maps the extstorage volume that matches our name with
> +    a corresponding block device and then attaches to this device.
> +
> +    """
> +    self.attached = False
> +
> +    # Call the External Storage's attach script,
> +    # to attach an existing Volume to a block device under /dev
> +    self.dev_path = _ExtStorageAction(constants.ES_ACTION_ATTACH,
> +                                      self.unique_id)
> +
> +    try:
> +      st = os.stat(self.dev_path)
> +    except OSError, err:
> +      logging.error("Error stat()'ing %s: %s", self.dev_path, str(err))
> +      return False
> +
> +    if not stat.S_ISBLK(st.st_mode):
> +      logging.error("%s is not a block device", self.dev_path)
> +      return False
> +
> +    self.major = os.major(st.st_rdev)
> +    self.minor = os.minor(st.st_rdev)
> +    self.attached = True
> +
> +    return True
> +
> +  def Assemble(self):
> +    """Assemble the device.
> +
> +    """
> +    pass
> +
> +  def Shutdown(self):
> +    """Shutdown the device.
> +
> +    """
> +    if not self.minor and not self.Attach():
> +      # The extstorage device doesn't exist.
> +      return
> +
> +    # Call the External Storage's detach script,
> +    # to detach an existing Volume from it's block device under /dev
> +    _ExtStorageAction(constants.ES_ACTION_DETACH, self.unique_id)
> +
> +    self.minor = None
> +    self.dev_path = None
> +
> +  def Open(self, force=False):
> +    """Make the device ready for I/O.
> +
> +    """
> +    pass
> +
> +  def Close(self):
> +    """Notifies that the device will no longer be used for I/O.
> +
> +    """
> +    pass
> +
> +  def Grow(self, amount, dryrun, backingstore):
> +    """Grow the Volume.
> +
> +    @type amount: integer
> +    @param amount: the amount (in mebibytes) to grow with
> +    @type dryrun: boolean
> +    @param dryrun: whether to execute the operation in simulation mode
> +        only, without actually increasing the size
> +
> +    """
> +    if not backingstore:
> +      return
> +    if not self.Attach():
> +      _ThrowError("Can't attach to extstorage device during Grow()")
> +
> +    if dryrun:
> +      # we do not support dry runs of resize operations for now.
> +      return
> +
> +    new_size = self.size + amount
> +
> +    # Call the External Storage's grow script,
> +    # to grow an existing Volume inside the External Storage
> +    _ExtStorageAction(constants.ES_ACTION_GROW, self.unique_id,
> +                      str(self.size),

> The option is needed because during gnt-instance modify parameters'
> validation, we do not know the type of the disk template
> (whereas in gnt-instance add we find it from the -t option).

> The option is called 'arbit' and not 'ext' params because it may be
> useful in other contexts too, in the future.

> The corresponding prereq checks for the existence of the 'provider'
> parameter in case of an 'ext' template are also taken care off in
> this commit. It serves as a general introduction of the 'ext'
> template in gnt-instance modify.

> Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>

> Conflicts:

>    lib/cmdlib.py

>> The ExtStorage Interface provides Ganeti with the ability to interact
>> with externally connected shared storage pools, visible by all
>> VM-capable nodes. This means that Ganeti is able to handle VM disks
>> that reside inside a NAS/SAN or any distributed block storage provider.

>> The ExtStorage Interface provides a clear API, heavily inspired by the
>> gnt-os-interface API, that can be used by storage vendors or sysadmins
>> to write simple ExtStorage Providers (correlated to gnt-os-interface's
>> OS Definitions). Those Providers will glue externally attached shared
>> storage with Ganeti, without the need of preprovisioned block devices
>> on Ganeti VM-capable nodes as confined be the current `blockdev' disk
>> template.

>> To do so, we implement a new disk template called `ext' (of type
>> DTS_EXT_MIRROR) that passes control to externally provided scripts
>> (the ExtStorage Provider) for the template's basic functions:

>>   create / attach / detach / remove / grow

>> The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
>> and only one ExtStorage Provider is supported called `ext'.

>> The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
>> is generated as in disk template `plain' and X is the disk's index.
> A few general comments, or rather questions. Some of the are rather
> design level, but they become more apparent when reading the code only,
> sorry.

> Also, it seems by reading the code that Ganeti generates an UUID for the
> volume, and it asks the provider to create one such drive; basically,
> the provider will get "please create disk f0559a.0 of size 500G", right?

>> Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>
>> ---
>>   Makefile.am               |    1 +
>>   configure.ac              |   12 ++
>>   lib/bdev.py               |  318 +++++++++++++++++++++++++++++++++++++++++++++
>>   lib/client/gnt_cluster.py |    2 +
>>   lib/cmdlib.py             |   16 ++-
>>   lib/constants.py          |   40 +++++-
>>   lib/masterd/instance.py   |    1 +
>>   lib/objects.py            |   20 +++-
>>   lib/pathutils.py          |    2 +
>>   tools/burnin              |    1 +
>>   10 files changed, 402 insertions(+), 11 deletions(-)

>> diff --git a/Makefile.am b/Makefile.am
>> index 58daa7a..99c1bc2 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -1175,6 +1175,7 @@ lib/_autoconf.py: Makefile | stamp-directories
>>          echo "SSH_CONFIG_DIR = '$(SSH_CONFIG_DIR)'"; \
>>          echo "EXPORT_DIR = '$(EXPORT_DIR)'"; \
>>          echo "OS_SEARCH_PATH = [$(OS_SEARCH_PATH)]"; \
>> +        echo "ES_SEARCH_PATH = [$(ES_SEARCH_PATH)]"; \
>>          echo "XEN_BOOTLOADER = '$(XEN_BOOTLOADER)'"; \
>>          echo "XEN_KERNEL = '$(XEN_KERNEL)'"; \
>>          echo "XEN_INITRD = '$(XEN_INITRD)'"; \
>> diff --git a/configure.ac b/configure.ac
>> index 5bb14af..a697e31 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -60,6 +60,18 @@ AC_ARG_WITH([os-search-path],
>>     [os_search_path="'/srv/ganeti/os'"])
>>   AC_SUBST(OS_SEARCH_PATH, $os_search_path)

>> +# --with-extstorage-search-path=...
>> +# same black sed magic for quoting of the strings in the list
>> +AC_ARG_WITH([extstorage-search-path],
>> +  [AS_HELP_STRING([--with-extstorage-search-path=LIST],
>> +    [comma separated list of directories to]
>> +    [ search for External Storage Providers]
>> +    [ (default is /srv/ganeti/extstorage)]
>> +  )],
>> +  [es_search_path=`echo -n "$withval" | sed -e "s/\([[^,]]*\)/'\1'/g"`],
>> +  [es_search_path="'/srv/ganeti/extstorage'"])
>> +AC_SUBST(ES_SEARCH_PATH, $es_search_path)
>> +
>>   # --with-iallocator-search-path=...
>>   # do a bit of black sed magic to for quoting of the strings in the list
>>   AC_ARG_WITH([iallocator-search-path],
>> diff --git a/lib/bdev.py b/lib/bdev.py
>> index d6c1a66..b0ed7c0 100644
>> --- a/lib/bdev.py
>> +++ b/lib/bdev.py
>> @@ -33,6 +33,7 @@ import logging
>>   from ganeti import utils
>>   from ganeti import errors
>>   from ganeti import constants
>> +from ganeti import pathutils
>>   from ganeti import objects
>>   from ganeti import compat
>>   from ganeti import netutils
>> @@ -2648,11 +2649,328 @@ class RADOSBlockDevice(BlockDev):
>>                     result.fail_reason, result.output)

>> +class ExtStorageDevice(BlockDev):
>> +  """A block device provided by an ExtStorage Provider.
>> +
>> +  This class implements the External Storage Interface, which means
>> +  handling of the externally provided block devices.
>> +
>> +  """
>> +  def __init__(self, unique_id, children, size, params):
>> +    """Attaches to an extstorage block device.
>> +
>> +    """
>> +    super(ExtStorageDevice, self).__init__(unique_id, children, size, params)
>> +    if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
>> +      raise ValueError("Invalid configuration data %s" % str(unique_id))
>> +
>> +    self.driver, self.vol_name = unique_id
>> +
>> +    self.major = self.minor = None
>> +    self.Attach()
>> +
>> +  @classmethod
>> +  def Create(cls, unique_id, children, size, params):
>> +    """Create a new extstorage device.
>> +
>> +    Provision a new volume using an extstorage provider, which will
>> +    then be mapped to a block device.
>> +
>> +    """
>> +    if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
>> +      raise errors.ProgrammerError("Invalid configuration data %s" %
>> +                                   str(unique_id))
>> +
>> +    # Call the External Storage's create script,
>> +    # to provision a new Volume inside the External Storage
>> +    _ExtStorageAction(constants.ES_ACTION_CREATE, unique_id, str(size))
>> +
>> +    return ExtStorageDevice(unique_id, children, size, params)
>> +
>> +  def Remove(self):
>> +    """Remove the extstorage device.
>> +
>> +    """
>> +    if not self.minor and not self.Attach():
>> +      # The extstorage device doesn't exist.
>> +      return
>> +
>> +    # First shutdown the device (remove mappings).
>> +    self.Shutdown()
>> +
>> +    # Call the External Storage's remove script,
>> +    # to remove the Volume from the External Storage
>> +    _ExtStorageAction(constants.ES_ACTION_REMOVE, self.unique_id)
>> +
>> +  def Rename(self, new_id):
>> +    """Rename this device.
>> +
>> +    """
>> +    pass
>> +
>> +  def Attach(self):
>> +    """Attach to an existing extstorage device.
>> +
>> +    This method maps the extstorage volume that matches our name with
>> +    a corresponding block device and then attaches to this device.
>> +
>> +    """
>> +    self.attached = False
>> +
>> +    # Call the External Storage's attach script,
>> +    # to attach an existing Volume to a block device under /dev
>> +    self.dev_path = _ExtStorageAction(constants.ES_ACTION_ATTACH,
>> +                                      self.unique_id)
>> +
>> +    try:
>> +      st = os.stat(self.dev_path)
>> +    except OSError, err:
>> +      logging.error("Error stat()'ing %s: %s", self.dev_path, str(err))
>> +      return False
>> +
>> +    if not stat.S_ISBLK(st.st_mode):
>> +      logging.error("%s is not a

>> The option is needed because during gnt-instance modify parameters'
>> validation, we do not know the type of the disk template
>> (whereas in gnt-instance add we find it from the -t option).

>> The option is called 'arbit' and not 'ext' params because it may be
>> useful in other contexts too, in the future.

>> The corresponding prereq checks for the existence of the 'provider'
>> parameter in case of an 'ext' template are also taken care off in
>> this commit. It serves as a general introduction of the 'ext'
>> template in gnt-instance modify.

>> Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>

>> Conflicts:

>>        lib/cmdlib.py
> Conflicts? :)

> This is not a review of the patch, but just reading the commit message,
> I find "--allow-arbit-params" too hard to read. What about
> "--flexible-params"?

> >>>>Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>
> >>>>---
> >>>>  doc/design-shared-storage.rst |  204 ++++++++++++++++++++++------------------
> >>>>  1 files changed, 112 insertions(+), 92 deletions(-)

> >>>>diff --git a/doc/design-shared-storage.rst b/doc/design-shared-storage.rst
> >>>>index c175476..7080182 100644
> >>>>--- a/doc/design-shared-storage.rst
> >>>>+++ b/doc/design-shared-storage.rst
> >>>>@@ -64,15 +64,11 @@ The design addresses the following procedures:
> >>>>    filesystems.
> >>>>  - Introduction of shared block device disk template with device
> >>>>    adoption.
> >>>>+- Introduction of an External Storage Interface.
> >>>>  Additionally, mid- to long-term goals include:
> >>>>  - Support for external “storage pools”.
> >>>>-- Introduction of an interface for communicating with external scripts,
> >>>>-  providing methods for the various stages of a block device's and
> >>>>-  instance's life-cycle. In order to provide storage provisioning
> >>>>-  capabilities for various SAN appliances, external helpers in the form
> >>>>-  of a “storage driver” will be possibly introduced as well.
> >>>>  Refactoring of all code referring to constants.DTS_NET_MIRROR
> >>>>  =============================================================
> >>>>@@ -159,6 +155,104 @@ The shared block device template will make the following assumptions:
> >>>>  - The device will be available with the same path under all nodes in the
> >>>>    node group.
> >>>>+Introduction of an External Storage Interface
> >>>>+==============================================
> >>>>+Overview
> >>>>+--------
> >>>>+
> >>>>+To extend the shared block storage template and give Ganeti the ability
> >>>>+to control and manipulate external storage (provisioning, removal,
> >>>>+growing, etc.) we need a more generic approach. The generic method for
> >>>>+supporting external shared storage in Ganeti will be to have an
> >>>>+ExtStorage provider for each external shared storage hardware type. The
> >>>>+ExtStorage provider will be a set of files (executable scripts and text
> >>>>+files), contained inside a directory which will be named after the
> >>>>+provider. This directory must be present across all nodes of a nodegroup
> >>>>+(Ganeti doesn't replicate it), in order for the provider to be usable by
> >>>>+Ganeti for this nodegroup (valid).
> >>>How will Ganeti behave if they are not consistent? Report errors? (in
> >>>cluster verify?) Ignore the provider? Etc.
> >>The ExtStorage code follows exactly the behavior of the code
> >>handling OS definitions: It produces appropriate error messages
> >>and also comes with `gnt-storage {diagnose, info}' similarly to
> >>`gnt-os {diagnose, info}'.

> >>There is only one difference compared to the way OS defs are
> >>handled:

> >>The ExtStorage diagnose code calculates the validity of each provider
> >>for each nodegroup in the cmdlib logic rather than in the client.
> >>This was marked as 'TODO'' inside cmdlib for OS diagnose.

> >>This gives you the flexibility to do neat things easily, such as running
> >>the LU from inside cluster verify and producing validity statuses
> >>each provider-nodegroup combination. So, presumably this can also
> >>be used inside `gnt-cluster verify' in the future.
> >Sounds very good, thanks!

> >>>>The external shared storage hardware
> >>>>+should also be accessible by all nodes of this nodegroup too.
> >>>>+
> >>>>+An “ExtStorage provider” will have to provide the following methods:
> >>>>+
> >>>>+- Create a disk
> >>>>+- Remove a disk
> >>>>+- Grow a disk
> >>>>+- Attach a disk to a given node
> >>>>+- Detach a disk from a given node
> >>>>+- Verify its supported parameters
> >>>>+
> >>>>+The proposed ExtStorage interface borrows heavily from the OS
> >>>>+interface and follows a one-script-per-function approach. An ExtStorage
> >>>>+provider is expected to provide the following scripts:
> >>>>+
> >>>>+- `create`
> >>>>+- `remove`
> >>>>+- `grow`
> >>>>+- `attach`
> >>>>+- `detach`
> >>>>+- `verify`
> >>>>+
> >>>>+All scripts will be called with no arguments and get their input via
> >>>>+environment variables. A common set of variables will be exported for
> >>>>+all commands, and some of them might have extra ones.
> >>>>+
> >>>>+- `VOL_NAME`: The name of the volume. This is unique for Ganeti and it
> >>>>+  uses it to refer to a specific volume inside the external storage.
> >>>>+- `VOL_SIZE`: The volume's size in mebibytes.
> >>>>+- `VOL_NEW_SIZE`: Available only to the `grow` script. It declares the
> >>>>+  new size of the volume after grow (in mebibytes).
> >>>>+- `EXTP_name`: ExtStorage parameter, where `name` is the parameter in
> >>>>+  upper-case (same as OS interface's `OSP_*` parameters).
> >>>>+
> >>>>+All scripts except `attach` should return 0 on success and non-zero on
> >>>>+error, accompanied by an appropriate error message on stderr. The
> >>>>+`attach` script should return a string on stdout on success, which is
> >>>>+the block device's full path, after it has been successfully attached to
> >>>>+the host node. On error it should return non-zero.
> >>>>+
> >>>>+Implementation
> >>>>+--------------
> >>>>+
> >>>>+To support the ExtStorage interface, we will introduce a new disk
> >>>>+template called `ext`. This template will implement the existing Ganeti
> >>>>+disk interface in `lib/bdev.py` (create, remove, attach, assemble,
> >>>>+shutdown, grow), and will simultaneously pass control to the external
> >>>>+scripts to actually handle the above actions. The `ext` disk template
> >>>>+will act as a translation layer between the current Ganeti disk
> >>>>+interface and the ExtStorage providers.
> >>>>+
> >>>>+We will also introduce a new IDISK_PARAM called `IDISK_PROVIDER =
> >>>>+provider`, which will be used at the command line to select the desired
> >>>>+ExtStorage provider. This parameter will be valid only for template
> >>>>+`ext` e.g.::
> >>>>+
> >>>>+ gnt-instance add -t ext --disk=0:size=2G,provider=sample_provider1
> >>>>+
> >>>>+The Extstorage interface will support different disks to be created by
> >>>>+different providers. e.g.::
> >>>>+
> >>>>+ gnt-instance add -t ext --disk=0:size=2G,provider=sample_provider1
> >>>>+                         --disk=1:size=1G,provider=sample_provider2
> >>>>+                         --disk=2:size=3G,provider=sample_provider1
> >>>This (also in the context of your other design changes) makes me a bit
> >>>uneasy, with regards to coordinating changes across multiple providers
> >>>in live migration and similar changes (even startup). Have you thought
> >>>about this?
> >>I'm not sure I can understand your point completely. Given the
> >>diagnose functionality described above, are you concerned providers
> >>are going to be in inconsistent state among nodes? Is it a matter
> >>of how the allocator decides the target node given different providers?
> >Ah no, see below.

> >>Can you expand on the "coordinating changes across multiple providers
> >>in live migration and similar changes (even startup)" part of your
> >>question? Perhaps with some examples?
> >I'll try :)

> OK. now its clear, thanks.

> >I have a _very slight_ worry on that handling "complex" instances will
> >become more tricky if the behaviour of different storage providers or
> >disk templates (this is in the context of the other designs) differs.

> >For example, let's say we have an instance with first disk DRBD, second
> >disk ext,provider=p1, third disk ext,provider=p2.

> >We know we can live migrate an instance across node groups for DRBD, and
> >we now we can migrate ext providers if they are available in both
> >groups. But combining all these checks across multiple disks is just 2%
> >more tricky: we need to move from "disk_template in
> >constants.DTS_MIRRORED" to something like "does all instance disks allow
> >migration/failover/move from (nodegroup A, nodes [a,b]) to (nodegroup B,
> >nodes [c,d])" (where A could be equal to B)?

> >This is doable, just means that a lot of decisions about the instance
> >behaviour (can be moved, can be live migrated, etc.) will move away from
> >the instance level (disk_template) and become an aggregate of the
> >instance's disk capabilities.

> Exactly! Wrt the ExtStorage patchset, we won't need to make changes
> in the decision making because everything still stays at instance level,
> even though we have different providers on different disks. All we have
> to do, is make sure all providers are present at the node/nodegroup we
> want to migrate/failover/move (I have tested live migrations of instances
> with let's say disk0 ext,provider=p1, disk1 ext,provider=p2 without
> changing anything in the current allocation logic).

> When we introduce Storage Pools and the ability to have different
> disks of an instance residing in different Storage Pools, then we will
> have to do exactly as you are saying (and is also written in the design
> doc).

> >>The ExtStorage Interface provides Ganeti with the ability to interact
> >>with externally connected shared storage pools, visible by all
> >>VM-capable nodes. This means that Ganeti is able to handle VM disks
> >>that reside inside a NAS/SAN or any distributed block storage provider.

> >>The ExtStorage Interface provides a clear API, heavily inspired by the
> >>gnt-os-interface API, that can be used by storage vendors or sysadmins
> >>to write simple ExtStorage Providers (correlated to gnt-os-interface's
> >>OS Definitions). Those Providers will glue externally attached shared
> >>storage with Ganeti, without the need of preprovisioned block devices
> >>on Ganeti VM-capable nodes as confined be the current `blockdev' disk
> >>template.

> >>To do so, we implement a new disk template called `ext' (of type
> >>DTS_EXT_MIRROR) that passes control to externally provided scripts
> >>(the ExtStorage Provider) for the template's basic functions:

> >>  create / attach / detach / remove / grow

> >>The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
> >>and only one ExtStorage Provider is supported called `ext'.

> >>The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
> >>is generated as in disk template `plain' and X is the disk's index.
> >A few general comments, or rather questions. Some of the are rather
> >design level, but they become more apparent when reading the code only,
> >sorry.

> No problem.

> >I'm not sure creating one log file per operation is a good idea. While
> >installing/renaming an instance is a (somewhat) rare operation,
> >activating the disks of an instance is a much often one. I know that
> >right now in attach you don't create a log, but that seems to be the
> >intention (hence the TODO). Thoughts?

> Indeed, creating a log file during attach was the initial intention, however
> I understand your point since specifically attach is run multiple times
> even during a single instance addition. On the other hand, I think creation
> and removal of a disk should reside in different files because they do not
> happen so often. Furthermore, these log files are the only place where the
> admin actually can see what happens, when something goes wrong with
> the external hardware and are also very helpful when writing custom
> ExtStorage providers.

> That's right.

> >This means that only the ganeti configuration has the mapping instance
> >name to disks, and that losing the ganeti configuration would mean
> >instantly losing all instance data since we can't map it back to a VM.
> >For 'plain' disks, we explicitly tag the LV names with the name of the
> >instance, to safeguard against such problems. Do you think this is not a
> >problem?

> To be honest, I haven't thought about this case, but I understand
> your point.

> >  Or could we add the instance name to the external provider
> >create call, such that providers could do such an association
> >themselves, if they care?

> That would be fine, but I can't see an easy way to do that, since the
> instance object doesn't find its way inside bdev, and it wouldn't be clean
> to add new parameters in the Create call. What we could do though,
> would be to prepend the volume name with the instance's name.
> Much like you do with 'plain' disks. Now the disk name has the following
> format:

> '04859fac4.ext.diskX'   where X is the index.

> We could change that to 'instancename-04859fac4.ext.diskX' and
> document it appropriately, so that the mapping will be there for the
> providers to use it, if they care. Would that be OK?

> >>diff --git a/Makefile.am b/Makefile.am
> >>index 58daa7a..99c1bc2 100644
> >>--- a/Makefile.am
> >>+++ b/Makefile.am
> >>@@ -1175,6 +1175,7 @@ lib/_autoconf.py: Makefile | stamp-directories
> >>     echo "SSH_CONFIG_DIR = '$(SSH_CONFIG_DIR)'"; \
> >>     echo "EXPORT_DIR = '$(EXPORT_DIR)'"; \
> >>     echo "OS_SEARCH_PATH = [$(OS_SEARCH_PATH)]"; \
> >>+    echo "ES_SEARCH_PATH = [$(ES_SEARCH_PATH)]"; \
> >>     echo "XEN_BOOTLOADER = '$(XEN_BOOTLOADER)'"; \
> >>     echo "XEN_KERNEL = '$(XEN_KERNEL)'"; \
> >>     echo "XEN_INITRD = '$(XEN_INITRD)'"; \
> >>diff --git a/configure.ac b/configure.ac
> >>index 5bb14af..a697e31 100644
> >>--- a/configure.ac
> >>+++ b/configure.ac
> >>@@ -60,6 +60,18 @@ AC_ARG_WITH([os-search-path],
> >>    [os_search_path="'/srv/ganeti/os'"])
> >>  AC_SUBST(OS_SEARCH_PATH, $os_search_path)
> >>+# --with-extstorage-search-path=...
> >>+# same black sed magic for quoting of the strings in the list
> >>+AC_ARG_WITH([extstorage-search-path],
> >>+  [AS_HELP_STRING([--with-extstorage-search-path=LIST],
> >>+    [comma separated list of directories to]
> >>+    [ search for External Storage Providers]
> >>+    [ (default is /srv/ganeti/extstorage)]
> >>+  )],
> >>+  [es_search_path=`echo -n "$withval" | sed -e "s/\([[^,]]*\)/'\1'/g"`],
> >>+  [es_search_path="'/srv/ganeti/extstorage'"])
> >>+AC_SUBST(ES_SEARCH_PATH, $es_search_path)
> >>+
> >>  # --with-iallocator-search-path=...
> >>  # do a bit of black sed magic to for quoting of the strings in the list
> >>  AC_ARG_WITH([iallocator-search-path],
> >>diff --git a/lib/bdev.py b/lib/bdev.py
> >>index d6c1a66..b0ed7c0 100644
> >>--- a/lib/bdev.py
> >>+++ b/lib/bdev.py
> >>@@ -33,6 +33,7 @@ import logging
> >>  from ganeti import utils
> >>  from ganeti import errors
> >>  from ganeti import constants
> >>+from ganeti import pathutils
> >>  from ganeti import objects
> >>  from ganeti import compat
> >>  from ganeti import netutils
> >>@@ -2648,11 +2649,328 @@ class RADOSBlockDevice(BlockDev):
> >>                    result.fail_reason, result.output)
> >>+class ExtStorageDevice(BlockDev):
> >>+  """A block device provided by an ExtStorage Provider.
> >>+
> >>+  This class implements the External Storage Interface, which means
> >>+  handling of the externally provided block devices.
> >>+
> >>+  """
> >>+  def __init__(self, unique_id, children, size, params):
> >>+    """Attaches to an extstorage block device.
> >>+
> >>+    """
> >>+    super(ExtStorageDevice, self).__init__(unique_id, children, size, params)
> >>+    if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
> >>+      raise ValueError("Invalid configuration data %s" % str(unique_id))
> >>+
> >>+    self.driver, self.vol_name = unique_id
> >>+
> >>+    self.major = self.minor = None
> >>+    self.Attach()
> >>+
> >>+  @classmethod
> >>+  def Create(cls, unique_id, children, size, params):
> >>+    """Create a new extstorage device.
> >>+
> >>+    Provision a new volume using an extstorage provider, which will
> >>+    then be mapped to a block device.
> >>+
> >>+    """
> >>+    if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
> >>+      raise errors.ProgrammerError("Invalid configuration data %s" %
> >>+                                   str(unique_id))
> >>+
> >>+    # Call the External Storage's create script,
> >>+    # to provision a new Volume inside the External Storage
> >>+    _ExtStorageAction(constants.ES_ACTION_CREATE, unique_id, str(size))
> >>+
> >>+    return ExtStorageDevice(unique_id, children, size, params)
> >>+
> >>+  def Remove(self):
> >>+    """Remove the extstorage device.
> >>+
> >>+    """
> >>+    if not self.minor and not self.Attach():
> >>+      # The extstorage device doesn't exist.
> >>+      return
> >>+
> >>+    # First shutdown the device (remove mappings).
> >>+    self.Shutdown()
> >>+
> >>+    # Call the External Storage's remove script,
> >>+    # to remove the Volume from the External Storage
> >>+  

> >>The option is needed because during gnt-instance modify parameters'
> >>validation, we do not know the type of the disk template
> >>(whereas in gnt-instance add we find it from the -t option).

> >>The option is called 'arbit' and not 'ext' params because it may be
> >>useful in other contexts too, in the future.

> >>The corresponding prereq checks for the existence of the 'provider'
> >>parameter in case of an 'ext' template are also taken care off in
> >>this commit. It serves as a general introduction of the 'ext'
> >>template in gnt-instance modify.

> >>Signed-off-by: Constantinos Venetsanopoulos <c...@grnet.gr>

> >>Conflicts:

> >>   lib/cmdlib.py
> >Conflicts? :)

> Oooops :)
> I've been rebasing a hundred times (always resolving conflicts) the last
> two weeks to catch up with you guys so this must have slipped.
> Could you please remove it when applying?

> I think "allow" should be in there. Maybe it becomes more clear when
> you see the code. For "arbit" I'm not sure either. In any case, we can
> change that to "--flexible-params" during the review if you think that
> fits best at the end.

>>>> The ExtStorage Interface provides Ganeti with the ability to interact
>>>> with externally connected shared storage pools, visible by all
>>>> VM-capable nodes. This means that Ganeti is able to handle VM disks
>>>> that reside inside a NAS/SAN or any distributed block storage provider.

>>>> The ExtStorage Interface provides a clear API, heavily inspired by the
>>>> gnt-os-interface API, that can be used by storage vendors or sysadmins
>>>> to write simple ExtStorage Providers (correlated to gnt-os-interface's
>>>> OS Definitions). Those Providers will glue externally attached shared
>>>> storage with Ganeti, without the need of preprovisioned block devices
>>>> on Ganeti VM-capable nodes as confined be the current `blockdev' disk
>>>> template.

>>>> To do so, we implement a new disk template called `ext' (of type
>>>> DTS_EXT_MIRROR) that passes control to externally provided scripts
>>>> (the ExtStorage Provider) for the template's basic functions:

>>>>   create / attach / detach / remove / grow

>>>> The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
>>>> and only one ExtStorage Provider is supported called `ext'.

>>>> The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
>>>> is generated as in disk template `plain' and X is the disk's index.
>>> A few general comments, or rather questions. Some of the are rather
>>> design level, but they become more apparent when reading the code only,
>>> sorry.
>> No problem.

>>> I'm not sure creating one log file per operation is a good idea. While
>>> installing/renaming an instance is a (somewhat) rare operation,
>>> activating the disks of an instance is a much often one. I know that
>>> right now in attach you don't create a log, but that seems to be the
>>> intention (hence the TODO). Thoughts?
>> Indeed, creating a log file during attach was the initial intention, however
>> I understand your point since specifically attach is run multiple times
>> even during a single instance addition. On the other hand, I think creation
>> and removal of a disk should reside in different files because they do not
>> happen so often. Furthermore, these log files are the only place where the
>> admin actually can see what happens, when something goes wrong with
>> the external hardware and are also very helpful when writing custom
>> ExtStorage providers.
> I'm not sure I understand this. Do you mean about debug/informational
> messages? I believe that actual errors will simply propagate to ganeti
> as failures in activation/other ops.

>> What do you think? Another option would be to keep the log files for
>> create/remove/grow and do not log the attach/detach. Or keep them
>> for now to see the workflow and if they produce a lot of unnecessary
>> info that we do not want to keep, merge all actions in a single log file
>> for each disk (but this needs code refactoring).
> Indeed.

> Maybe a separate option would be, as you say, log only
> create/remove/grow and recommend that attach/detach options log to
> syslog? Or request that all logging goes to syslog?

>>> This means that only the ganeti configuration has the mapping instance
>>> name to disks, and that losing the ganeti configuration would mean
>>> instantly losing all instance data since we can't map it back to a VM.
>>> For 'plain' disks, we explicitly tag the LV names with the name of the
>>> instance, to safeguard against such problems. Do you think this is not a
>>> problem?
>> To be honest, I haven't thought about this case, but I understand
>> your point.

>>>   Or could we add the instance name to the external provider
>>> create call, such that providers could do such an association
>>> themselves, if they care?
>> That would be fine, but I can't see an easy way to do that, since the
>> instance object doesn't find its way inside bdev, and it wouldn't be clean
>> to add new parameters in the Create call. What we could do though,
>> would be to prepend the volume name with the instance's name.
>> Much like you do with 'plain' disks. Now the disk name has the following
>> format:

>> '04859fac4.ext.diskX'   where X is the index.

>> We could change that to 'instancename-04859fac4.ext.diskX' and
>> document it appropriately, so that the mapping will be there for the
>> providers to use it, if they care. Would that be OK?
> Hmm, not really. The instance name can and will change, so having it as
> part of the disk ID is not good.

> But note that the instance "info" is already passed to BlockDevCreate,
> and while not passed to the actual Create call, is passed to
> BlockDevice.SetInfo()? Could we reuse that here too?

>>>> diff --git a/Makefile.am b/Makefile.am
>>>> index 58daa7a..99c1bc2 100644
>>>> --- a/Makefile.am
>>>> +++ b/Makefile.am
>>>> @@ -1175,6 +1175,7 @@ lib/_autoconf.py: Makefile | stamp-directories
>>>>          echo "SSH_CONFIG_DIR = '$(SSH_CONFIG_DIR)'"; \
>>>>          echo "EXPORT_DIR = '$(EXPORT_DIR)'"; \
>>>>          echo "OS_SEARCH_PATH = [$(OS_SEARCH_PATH)]"; \
>>>> +        echo "ES_SEARCH_PATH = [$(ES_SEARCH_PATH)]"; \
>>>>          echo "XEN_BOOTLOADER = '$(XEN_BOOTLOADER)'"; \
>>>>          echo "XEN_KERNEL = '$(XEN_KERNEL)'"; \
>>>>          echo "XEN_INITRD = '$(XEN_INITRD)'"; \
>>>> diff --git a/configure.ac b/configure.ac
>>>> index 5bb14af..a697e31 100644
>>>> --- a/configure.ac
>>>> +++ b/configure.ac
>>>> @@ -60,6 +60,18 @@ AC_ARG_WITH([os-search-path],
>>>>     [os_search_path="'/srv/ganeti/os'"])
>>>>   AC_SUBST(OS_SEARCH_PATH, $os_search_path)
>>>> +# --with-extstorage-search-path=...
>>>> +# same black sed magic for quoting of the strings in the list
>>>> +AC_ARG_WITH([extstorage-search-path],
>>>> +  [AS_HELP_STRING([--with-extstorage-search-path=LIST],
>>>> +    [comma separated list of directories to]
>>>> +    [ search for External Storage Providers]
>>>> +    [ (default is /srv/ganeti/extstorage)]
>>>> +  )],
>>>> +  [es_search_path=`echo -n "$withval" | sed -e "s/\([[^,]]*\)/'\1'/g"`],
>>>> +  [es_search_path="'/srv/ganeti/extstorage'"])
>>>> +AC_SUBST(ES_SEARCH_PATH, $es_search_path)
>>>> +
>>>>   # --with-iallocator-search-path=...
>>>>   # do a bit of black sed magic to for quoting of the strings in the list
>>>>   AC_ARG_WITH([iallocator-search-path],
>>>> diff --git a/lib/bdev.py b/lib/bdev.py
>>>> index d6c1a66..b0ed7c0 100644
>>>> --- a/lib/bdev.py
>>>> +++ b/lib/bdev.py
>>>> @@ -33,6 +33,7 @@ import logging
>>>>   from ganeti import utils
>>>>   from ganeti import errors
>>>>   from ganeti import constants
>>>> +from ganeti import pathutils
>>>>   from ganeti import objects
>>>>   from ganeti import compat
>>>>   from ganeti import netutils
>>>> @@ -2648,11 +2649,328 @@ class RADOSBlockDevice(BlockDev):

> >>>>The ExtStorage Interface provides Ganeti with the ability to interact
> >>>>with externally connected shared storage pools, visible by all
> >>>>VM-capable nodes. This means that Ganeti is able to handle VM disks
> >>>>that reside inside a NAS/SAN or any distributed block storage provider.

> >>>>The ExtStorage Interface provides a clear API, heavily inspired by the
> >>>>gnt-os-interface API, that can be used by storage vendors or sysadmins
> >>>>to write simple ExtStorage Providers (correlated to gnt-os-interface's
> >>>>OS Definitions). Those Providers will glue externally attached shared
> >>>>storage with Ganeti, without the need of preprovisioned block devices
> >>>>on Ganeti VM-capable nodes as confined be the current `blockdev' disk
> >>>>template.

> >>>>To do so, we implement a new disk template called `ext' (of type
> >>>>DTS_EXT_MIRROR) that passes control to externally provided scripts
> >>>>(the ExtStorage Provider) for the template's basic functions:

> >>>>  create / attach / detach / remove / grow

> >>>>The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
> >>>>and only one ExtStorage Provider is supported called `ext'.

> >>>>The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
> >>>>is generated as in disk template `plain' and X is the disk's index.
> >>>A few general comments, or rather questions. Some of the are rather
> >>>design level, but they become more apparent when reading the code only,
> >>>sorry.
> >>No problem.

> >>>I'm not sure creating one log file per operation is a good idea. While
> >>>installing/renaming an instance is a (somewhat) rare operation,
> >>>activating the disks of an instance is a much often one. I know that
> >>>right now in attach you don't create a log, but that seems to be the
> >>>intention (hence the TODO). Thoughts?
> >>Indeed, creating a log file during attach was the initial intention, however
> >>I understand your point since specifically attach is run multiple times
> >>even during a single instance addition. On the other hand, I think creation
> >>and removal of a disk should reside in different files because they do not
> >>happen so often. Furthermore, these log files are the only place where the
> >>admin actually can see what happens, when something goes wrong with
> >>the external hardware and are also very helpful when writing custom
> >>ExtStorage providers.
> >I'm not sure I understand this. Do you mean about debug/informational
> >messages? I believe that actual errors will simply propagate to ganeti
> >as failures in activation/other ops.

> Actual errors do propagate to ganeti. However the log doesn't
> (except the "n" last lines of the logfile). The user has to go back to
> the log file to see what happened exactly. From my experience writing
> ExtStorage providers, it really helps being able to look at the attach
> script's stderr.

> >Maybe a separate option would be, as you say, log only
> >create/remove/grow and recommend that attach/detach options log to
> >syslog? Or request that all logging goes to syslog?

> After more consideration, I think that not logging attach/detach ops is
> not the right thing to do.

> Currently no logging for attach happens, due to the issue with RunResult().
> When this gets fixed, I think it would be best to log everything related
> to a single disk in the same log file, so we don't miss anything, keep
> related things together, and do not create too many small files. As an
> example the file would contain a series of
> [create,attach,attach,attach,detach,attach,...,grow,...,detach,remove] ops.

> If you are OK with this approach, I will contribute the relevant patches as
> soon as the code gets merged and the issue in the TODO comment gets fixed.

> >>>This means that only the ganeti configuration has the mapping instance
> >>>name to disks, and that losing the ganeti configuration would mean
> >>>instantly losing all instance data since we can't map it back to a VM.
> >>>For 'plain' disks, we explicitly tag the LV names with the name of the
> >>>instance, to safeguard against such problems. Do you think this is not a
> >>>problem?
> >>To be honest, I haven't thought about this case, but I understand
> >>your point.

> >>>  Or could we add the instance name to the external provider
> >>>create call, such that providers could do such an association
> >>>themselves, if they care?
> >>That would be fine, but I can't see an easy way to do that, since the
> >>instance object doesn't find its way inside bdev, and it wouldn't be clean
> >>to add new parameters in the Create call. What we could do though,
> >>would be to prepend the volume name with the instance's name.
> >>Much like you do with 'plain' disks. Now the disk name has the following
> >>format:

> >>'04859fac4.ext.diskX'   where X is the index.

> >>We could change that to 'instancename-04859fac4.ext.diskX' and
> >>document it appropriately, so that the mapping will be there for the
> >>providers to use it, if they care. Would that be OK?
> >Hmm, not really. The instance name can and will change, so having it as
> >part of the disk ID is not good.

> >But note that the instance "info" is already passed to BlockDevCreate,
> >and while not passed to the actual Create call, is passed to
> >BlockDevice.SetInfo()? Could we reuse that here too?

> If I understand correctly, you prefer disk names to be independent of
> the instance name, then go and tag the disk object itself, e.g., the lv in
> the case of lvm.

>>>>>> The ExtStorage Interface provides Ganeti with the ability to interact
>>>>>> with externally connected shared storage pools, visible by all
>>>>>> VM-capable nodes. This means that Ganeti is able to handle VM disks
>>>>>> that reside inside a NAS/SAN or any distributed block storage provider.

>>>>>> The ExtStorage Interface provides a clear API, heavily inspired by the
>>>>>> gnt-os-interface API, that can be used by storage vendors or sysadmins
>>>>>> to write simple ExtStorage Providers (correlated to gnt-os-interface's
>>>>>> OS Definitions). Those Providers will glue externally attached shared
>>>>>> storage with Ganeti, without the need of preprovisioned block devices
>>>>>> on Ganeti VM-capable nodes as confined be the current `blockdev' disk
>>>>>> template.

>>>>>> To do so, we implement a new disk template called `ext' (of type
>>>>>> DTS_EXT_MIRROR) that passes control to externally provided scripts
>>>>>> (the ExtStorage Provider) for the template's basic functions:

>>>>>>   create / attach / detach / remove / grow

>>>>>> The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
>>>>>> and only one ExtStorage Provider is supported called `ext'.

>>>>>> The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
>>>>>> is generated as in disk template `plain' and X is the disk's index.
>>>>> A few general comments, or rather questions. Some of the are rather
>>>>> design level, but they become more apparent when reading the code only,
>>>>> sorry.
>>>> No problem.

>>>>> I'm not sure creating one log file per operation is a good idea. While
>>>>> installing/renaming an instance is a (somewhat) rare operation,
>>>>> activating the disks of an instance is a much often one. I know that
>>>>> right now in attach you don't create a log, but that seems to be the
>>>>> intention (hence the TODO). Thoughts?
>>>> Indeed, creating a log file during attach was the initial intention, however
>>>> I understand your point since specifically attach is run multiple times
>>>> even during a single instance addition. On the other hand, I think creation
>>>> and removal of a disk should reside in different files because they do not
>>>> happen so often. Furthermore, these log files are the only place where the
>>>> admin actually can see what happens, when something goes wrong with
>>>> the external hardware and are also very helpful when writing custom
>>>> ExtStorage providers.
>>> I'm not sure I understand this. Do you mean about debug/informational
>>> messages? I believe that actual errors will simply propagate to ganeti
>>> as failures in activation/other ops.
>> Actual errors do propagate to ganeti. However the log doesn't
>> (except the "n" last lines of the logfile). The user has to go back to
>> the log file to see what happened exactly. From my experience writing
>> ExtStorage providers, it really helps being able to look at the attach
>> script's stderr.
> I see.

>>>> What do you think? Another option would be to keep the log files for
>>>> create/remove/grow and do not log the attach/detach. Or keep them
>>>> for now to see the workflow and if they produce a lot of unnecessary
>>>> info that we do not want to keep, merge all actions in a single log file
>>>> for each disk (but this needs code refactoring).
>>> Indeed.

>>> Maybe a separate option would be, as you say, log only
>>> create/remove/grow and recommend that attach/detach options log to
>>> syslog? Or request that all logging goes to syslog?
>> After more consideration, I think that not logging attach/detach ops is
>> not the right thing to do.

>> Currently no logging for attach happens, due to the issue with RunResult().
>> When this gets fixed, I think it would be best to log everything related
>> to a single disk in the same log file, so we don't miss anything, keep
>> related things together, and do not create too many small files. As an
>> example the file would contain a series of
>> [create,attach,attach,attach,detach,attach,...,grow,...,detach,remove] ops.

>> If you are OK with this approach, I will contribute the relevant patches as
>> soon as the code gets merged and the issue in the TODO comment gets fixed.
> Sounds good. Thanks for taking the time to think this through!

>>>>> Also, it seems by reading the code that Ganeti generates an UUID for the
>>>>> volume, and it asks the provider to create one such drive; basically,
>>>>> the provider will get "please create disk f0559a.0 of size 500G", right?
>>>> That's right.

>>>>> This means that only the ganeti configuration has the mapping instance
>>>>> name to disks, and that losing the ganeti configuration would mean
>>>>> instantly losing all instance data since we can't map it back to a VM.
>>>>> For 'plain' disks, we explicitly tag the LV names with the name of the
>>>>> instance, to safeguard against such problems. Do you think this is not a
>>>>> problem?
>>>> To be honest, I haven't thought about this case, but I understand
>>>> your point.

>>>>>   Or could we add the instance name to the external provider
>>>>> create call, such that providers could do such an association
>>>>> themselves, if they care?
>>>> That would be fine, but I can't see an easy way to do that, since the
>>>> instance object doesn't find its way inside bdev, and it wouldn't be clean
>>>> to add new parameters in the Create call. What we could do though,
>>>> would be to prepend the volume name with the instance's name.
>>>> Much like you do with 'plain' disks. Now the disk name has the following
>>>> format:

>>>> '04859fac4.ext.diskX'   where X is the index.

>>>> We could change that to 'instancename-04859fac4.ext.diskX' and
>>>> document it appropriately, so that the mapping will be there for the
>>>> providers to use it, if they care. Would that be OK?
>>> Hmm, not really. The instance name can and will change, so having it as
>>> part of the disk ID is not good.

>>> But note that the instance "info" is already passed to BlockDevCreate,
>>> and while not passed to the actual Create call, is passed to
>>> BlockDevice.SetInfo()? Could we reuse that here too?
>> If I understand correctly, you prefer disk names to be independent of
>> the instance name, then go and tag the disk object itself, e.g., the lv in
>> the case of lvm.
> Yes.

>> A similar approach would be for me to implement the
>> ExtStorageDevice.SetInfo() call which runs an external "setinfo" script.
>> The setinfo script would mark the disk with the instance name or other
>> metadata in a provider-specific way. For example the volumes of a NAS
>> would be marked with vendor specific metadata and the admin would be
>> able to recover instance data by examining these.
> Yes, exactly.

>> However, even if this happens, it seems I can only find code setting lv
>> tags during blockdev creation and not instance rename. So, how do
>> lv tags stay consistent after renames? Thus, even we follow the approach
>> you propose, the admin will still have problems tracking down instance
>> disks after loosing the ganeti config. Thoughts?
> That is actually a current bug (only internally raised so far). We do
> plan to fix the rename LU to include this, by exposing SetInfo via an
> rpc call.

> So yes, let's take this approach, and fix the rename issue separately.

> >>>>>>The ExtStorage Interface provides Ganeti with the ability to interact
> >>>>>>with externally connected shared storage pools, visible by all
> >>>>>>VM-capable nodes. This means that Ganeti is able to handle VM disks
> >>>>>>that reside inside a NAS/SAN or any distributed block storage provider.

> >>>>>>The ExtStorage Interface provides a clear API, heavily inspired by the
> >>>>>>gnt-os-interface API, that can be used by storage vendors or sysadmins
> >>>>>>to write simple ExtStorage Providers (correlated to gnt-os-interface's
> >>>>>>OS Definitions). Those Providers will glue externally attached shared
> >>>>>>storage with Ganeti, without the need of preprovisioned block devices
> >>>>>>on Ganeti VM-capable nodes as confined be the current `blockdev' disk
> >>>>>>template.

> >>>>>>To do so, we implement a new disk template called `ext' (of type
> >>>>>>DTS_EXT_MIRROR) that passes control to externally provided scripts
> >>>>>>(the ExtStorage Provider) for the template's basic functions:

> >>>>>>  create / attach / detach / remove / grow

> >>>>>>The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
> >>>>>>and only one ExtStorage Provider is supported called `ext'.

> >>>>>>The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
> >>>>>>is generated as in disk template `plain' and X is the disk's index.
> >>>>>A few general comments, or rather questions. Some of the are rather
> >>>>>design level, but they become more apparent when reading the code only,
> >>>>>sorry.
> >>>>No problem.

> >>>>>I'm not sure creating one log file per operation is a good idea. While
> >>>>>installing/renaming an instance is a (somewhat) rare operation,
> >>>>>activating the disks of an instance is a much often one. I know that
> >>>>>right now in attach you don't create a log, but that seems to be the
> >>>>>intention (hence the TODO). Thoughts?
> >>>>Indeed, creating a log file during attach was the initial intention, however
> >>>>I understand your point since specifically attach is run multiple times
> >>>>even during a single instance addition. On the other hand, I think creation
> >>>>and removal of a disk should reside in different files because they do not
> >>>>happen so often. Furthermore, these log files are the only place where the
> >>>>admin actually can see what happens, when something goes wrong with
> >>>>the external hardware and are also very helpful when writing custom
> >>>>ExtStorage providers.
> >>>I'm not sure I understand this. Do you mean about debug/informational
> >>>messages? I believe that actual errors will simply propagate to ganeti
> >>>as failures in activation/other ops.
> >>Actual errors do propagate to ganeti. However the log doesn't
> >>(except the "n" last lines of the logfile). The user has to go back to
> >>the log file to see what happened exactly. From my experience writing
> >>ExtStorage providers, it really helps being able to look at the attach
> >>script's stderr.
> >I see.

> >>>>What do you think? Another option would be to keep the log files for
> >>>>create/remove/grow and do not log the attach/detach. Or keep them
> >>>>for now to see the workflow and if they produce a lot of unnecessary
> >>>>info that we do not want to keep, merge all actions in a single log file
> >>>>for each disk (but this needs code refactoring).
> >>>Indeed.

> >>>Maybe a separate option would be, as you say, log only
> >>>create/remove/grow and recommend that attach/detach options log to
> >>>syslog? Or request that all logging goes to syslog?
> >>After more consideration, I think that not logging attach/detach ops is
> >>not the right thing to do.

> >>Currently no logging for attach happens, due to the issue with RunResult().
> >>When this gets fixed, I think it would be best to log everything related
> >>to a single disk in the same log file, so we don't miss anything, keep
> >>related things together, and do not create too many small files. As an
> >>example the file would contain a series of
> >>[create,attach,attach,attach,detach,attach,...,grow,...,detach,remove] ops.

> >>If you are OK with this approach, I will contribute the relevant patches as
> >>soon as the code gets merged and the issue in the TODO comment gets fixed.
> >Sounds good. Thanks for taking the time to think this through!

> >>>>>Also, it seems by reading the code that Ganeti generates an UUID for the
> >>>>>volume, and it asks the provider to create one such drive; basically,
> >>>>>the provider will get "please create disk f0559a.0 of size 500G", right?
> >>>>That's right.

> >>>>>This means that only the ganeti configuration has the mapping instance
> >>>>>name to disks, and that losing the ganeti configuration would mean
> >>>>>instantly losing all instance data since we can't map it back to a VM.
> >>>>>For 'plain' disks, we explicitly tag the LV names with the name of the
> >>>>>instance, to safeguard against such problems. Do you think this is not a
> >>>>>problem?
> >>>>To be honest, I haven't thought about this case, but I understand
> >>>>your point.

> >>>>>  Or could we add the instance name to the external provider
> >>>>>create call, such that providers could do such an association
> >>>>>themselves, if they care?
> >>>>That would be fine, but I can't see an easy way to do that, since the
> >>>>instance object doesn't find its way inside bdev, and it wouldn't be clean
> >>>>to add new parameters in the Create call. What we could do though,
> >>>>would be to prepend the volume name with the instance's name.
> >>>>Much like you do with 'plain' disks. Now the disk name has the following
> >>>>format:

> >>>>'04859fac4.ext.diskX'   where X is the index.

> >>>>We could change that to 'instancename-04859fac4.ext.diskX' and
> >>>>document it appropriately, so that the mapping will be there for the
> >>>>providers to use it, if they care. Would that be OK?
> >>>Hmm, not really. The instance name can and will change, so having it as
> >>>part of the disk ID is not good.

> >>>But note that the instance "info" is already passed to BlockDevCreate,
> >>>and while not passed to the actual Create call, is passed to
> >>>BlockDevice.SetInfo()? Could we reuse that here too?
> >>If I understand correctly, you prefer disk names to be independent of
> >>the instance name, then go and tag the disk object itself, e.g., the lv in
> >>the case of lvm.
> >Yes.

> >>A similar approach would be for me to implement the
> >>ExtStorageDevice.SetInfo() call which runs an external "setinfo" script.
> >>The setinfo script would mark the disk with the instance name or other
> >>metadata in a provider-specific way. For example the volumes of a NAS
> >>would be marked with vendor specific metadata and the admin would be
> >>able to recover instance data by examining these.
> >Yes, exactly.

> >>However, even if this happens, it seems I can only find code setting lv
> >>tags during blockdev creation and not instance rename. So, how do
> >>lv tags stay consistent after renames? Thus, even we follow the approach
> >>you propose, the admin will still have problems tracking down instance
> >>disks after loosing the ganeti config. Thoughts?
> >That is actually a current bug (only internally raised so far). We do
> >plan to fix the rename LU to include this, by exposing SetInfo via an
> >rpc call.

> >So yes, let's take this approach, and fix the rename issue separately.

> OK. So, to sum up, I'll implement the SetInfo functionality, fix all the
> small issues we discussed in previous emails and send an interdiff.

> When we fix the RunResult TODO and the code gets merged, I'll send
> a patch that will merge all log files into a single one for each disk.

> Finally, after the merge, I will also send a separate patch that
> documents the SetInfo functionality in the design doc and man pages.

> Is that OK?