[PATCH master 4/7] Add --allow-arbit-params to 'gnt-instance modify'

[Constantinos Venetsanopoulos]

The option is added to allow us the passing of arbitrary ext-params
during disk addition (gnt-instance modify --disk add: ...), same as
we do during instance add.

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 <cv...@grnet.gr>

Conflicts:

lib/cmdlib.py

Signed-off-by: Constantinos Venetsanopoulos <cv...@grnet.gr>
---
lib/cli.py | 8 +++++
lib/client/gnt_instance.py | 6 ++-
lib/cmdlib.py | 71 +++++++++++++++++++++++++++++++++++++------
lib/opcodes.py | 55 ++++++++++++++++++++++++++++++++++
4 files changed, 128 insertions(+), 12 deletions(-)

diff --git a/lib/cli.py b/lib/cli.py
index 91ea9f6..5f003d1 100644
--- a/lib/cli.py
+++ b/lib/cli.py
@@ -202,6 +202,7 @@ __all__ = [
"HV_STATE_OPT",
"IGNORE_IPOLICY_OPT",
"INSTANCE_POLICY_OPTS",
+ "ALLOW_ARBITPARAMS_OPT",
# Generic functions for CLI programs
"ConfirmOperation",
"CreateIPolicyFromOpts",
@@ -1440,6 +1441,13 @@ ABSOLUTE_OPT = cli_option("--absolute", dest="absolute",
help="Marks the grow as absolute instead of the"
" (default) relative mode")

+ALLOW_ARBITPARAMS_OPT = cli_option("--allow-arbit-params",
+ dest="allow_arbit_params",
+ action="store_true", default=None,
+ help="Allow arbitrary params to be passed"
+ " to --disk(s) option (used by ExtStorage)")
+
+
#: Options provided by all commands
COMMON_OPTS = [DEBUG_OPT]

diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py
index fceb359..841ccc7 100644
--- a/lib/client/gnt_instance.py
+++ b/lib/client/gnt_instance.py
@@ -1422,7 +1422,8 @@ def SetInstanceParams(opts, args):
force=opts.force,
wait_for_sync=opts.wait_for_sync,
offline=offline,
- ignore_ipolicy=opts.ignore_ipolicy)
+ ignore_ipolicy=opts.ignore_ipolicy,
+ allow_arbit_params=opts.allow_arbit_params)

# even if here we process the result, we allow submit only
result = SubmitOrSend(op, opts)
@@ -1608,7 +1609,8 @@ commands = {
[BACKEND_OPT, DISK_OPT, FORCE_OPT, HVOPTS_OPT, NET_OPT, SUBMIT_OPT,
DISK_TEMPLATE_OPT, SINGLE_NODE_OPT, OS_OPT, FORCE_VARIANT_OPT,
OSPARAMS_OPT, DRY_RUN_OPT, PRIORITY_OPT, NWSYNC_OPT, OFFLINE_INST_OPT,
- ONLINE_INST_OPT, IGNORE_IPOLICY_OPT, RUNTIME_MEM_OPT],
+ ONLINE_INST_OPT, IGNORE_IPOLICY_OPT, RUNTIME_MEM_OPT,
+ ALLOW_ARBITPARAMS_OPT],
"<instance>", "Alters the parameters of an instance"),
"shutdown": (
GenericManyOps("shutdown", _ShutdownInstance), [ArgInstance()],
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 406ae2f..4554752 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -12241,7 +12241,10 @@ class LUInstanceSetParams(LogicalUnit):
for (op, _, params) in mods:
assert ht.TDict(params)

- utils.ForceDictType(params, key_types)
+ # If key_types is an empty dict, we assume we have an 'ext' template
+ # and thus do not ForceDictType
+ if key_types:
+ utils.ForceDictType(params, key_types)

if op == constants.DDM_REMOVE:
if params:
@@ -12277,9 +12280,18 @@ class LUInstanceSetParams(LogicalUnit):

params[constants.IDISK_SIZE] = size

- elif op == constants.DDM_MODIFY and constants.IDISK_SIZE in params:
- raise errors.OpPrereqError("Disk size change not possible, use"
- " grow-disk", errors.ECODE_INVAL)
+ elif op == constants.DDM_MODIFY:
+ if constants.IDISK_SIZE in params:
+ raise errors.OpPrereqError("Disk size change not possible, use"
+ " grow-disk", errors.ECODE_INVAL)
+ if constants.IDISK_MODE not in params:
+ raise errors.OpPrereqError("Disk 'mode' is the only kind of"
+ " modification supported, but missing",
+ errors.ECODE_NOENT)
+ if len(params) > 1:
+ raise errors.OpPrereqError("Disk modification doesn't support"
+ " additional arbitrary parameters",
+ errors.ECODE_INVAL)

@staticmethod
def _VerifyNicModification(op, params):
@@ -12330,14 +12342,26 @@ class LUInstanceSetParams(LogicalUnit):
if self.op.hvparams:
_CheckGlobalHvParams(self.op.hvparams)

- self.op.disks = self._UpgradeDiskNicMods(
- "disk", self.op.disks, opcodes.OpInstanceSetParams.TestDiskModifications)
- self.op.nics = self._UpgradeDiskNicMods(
- "NIC", self.op.nics, opcodes.OpInstanceSetParams.TestNicModifications)
+ if self.op.allow_arbit_params:
+ self.op.disks = \
+ self._UpgradeDiskNicMods("disk", self.op.disks,
+ opcodes.OpInstanceSetParams.TestExtDiskModifications)
+ else:
+ self.op.disks = \
+ self._UpgradeDiskNicMods("disk", self.op.disks,
+ opcodes.OpInstanceSetParams.TestDiskModifications)
+
+ self.op.nics = \
+ self._UpgradeDiskNicMods("NIC", self.op.nics,
+ opcodes.OpInstanceSetParams.TestNicModifications)

# Check disk modifications
- self._CheckMods("disk", self.op.disks, constants.IDISK_PARAMS_TYPES,
- self._VerifyDiskModification)
+ if self.op.allow_arbit_params:
+ self._CheckMods("disk", self.op.disks, {},
+ self._VerifyDiskModification)
+ else:
+ self._CheckMods("disk", self.op.disks, constants.IDISK_PARAMS_TYPES,
+ self._VerifyDiskModification)

if self.op.disks and self.op.disk_template is not None:
raise errors.OpPrereqError("Disk template conversion and other disk"
@@ -12491,6 +12515,33 @@ class LUInstanceSetParams(LogicalUnit):
self.diskmod = PrepareContainerMods(self.op.disks, None)
self.nicmod = PrepareContainerMods(self.op.nics, _InstNicModPrivate)

+ # Check the validity of the `provider' parameter
+ if instance.disk_template in constants.DT_EXT:
+ for mod in self.diskmod:
+ ext_provider = mod[2].get(constants.IDISK_PROVIDER, None)
+ if mod[0] == constants.DDM_ADD:
+ if ext_provider is None:
+ raise errors.OpPrereqError("Instance template is '%s' and parameter"
+ " '%s' missing, during disk add" %
+ (constants.DT_EXT,
+ constants.IDISK_PROVIDER),
+ errors.ECODE_NOENT)
+ elif mod[0] == constants.DDM_MODIFY:
+ if ext_provider:
+ raise errors.OpPrereqError("Parameter '%s' is invalid during disk"
+ " modification" %
+ constants.IDISK_PROVIDER,
+ errors.ECODE_INVAL)
+ else:
+ for mod in self.diskmod:
+ ext_provider = mod[2].get(constants.IDISK_PROVIDER, None)
+ if ext_provider is not None:
+ raise errors.OpPrereqError("Parameter '%s' is only valid for"
+ " instances of type '%s'" %
+ (constants.IDISK_PROVIDER,
+ constants.DT_EXT),
+ errors.ECODE_INVAL)
+
# OS change
if self.op.os_name and not self.op.force:
_CheckNodeHasOS(self, instance.primary_node, self.op.os_name,
diff --git a/lib/opcodes.py b/lib/opcodes.py
index 961192f..92e7798 100644
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -1536,6 +1536,7 @@ class OpInstanceSetParams(OpCode):
"""
TestNicModifications = _TestInstSetParamsModList(_TestNicDef)
TestDiskModifications = _TestInstSetParamsModList(_TDiskParams)
+ TestExtDiskModifications = _TestInstSetParamsModList(_TExtDiskParams)

OP_DSC_FIELD = "instance_name"
OP_PARAMS = [
@@ -1569,9 +1570,63 @@ class OpInstanceSetParams(OpCode):
("wait_for_sync", True, ht.TBool,
"Whether to wait for the disk to synchronize, when changing template"),
("offline", None, ht.TMaybeBool, "Whether to mark instance as offline"),
+ ("allow_arbit_params", None, ht.TMaybeBool,
+ "Whether to allow the passing of arbitrary parameters to --disk(s)"),
]
OP_RESULT = _TSetParamsResult

+ def Validate(self, set_defaults):
+ """Validate opcode parameters, optionally setting default values.
+
+ @type set_defaults: bool
+ @param set_defaults: Whether to set default values
+ @raise errors.OpPrereqError: When a parameter value doesn't match
+ requirements
+
+ """
+ # pylint: disable=E1101
+ # as OP_ID has been dynamically defined
+
+ # Check if the template is DT_EXT
+ allow_arbitrary_params = False
+ for (attr_name, _, _, _) in self.GetAllParams():
+ if hasattr(self, attr_name):
+ if attr_name == "allow_arbit_params" and \
+ getattr(self, attr_name) == True:
+ allow_arbitrary_params = True
+
+ for (attr_name, default, test, _) in self.GetAllParams():
+ assert test == ht.NoType or callable(test)
+
+ if not hasattr(self, attr_name):
+ if default == ht.NoDefault:
+ raise errors.OpPrereqError("Required parameter '%s.%s' missing" %
+ (self.OP_ID, attr_name),
+ errors.ECODE_INVAL)
+ elif set_defaults:
+ if callable(default):
+ dval = default()
+ else:
+ dval = default
+ setattr(self, attr_name, dval)
+
+ # If `allow_arbit_params' is set, use ExtStorage's test method for disks
+ if allow_arbitrary_params and attr_name == "disks":
+ test = OpInstanceSetParams.TestExtDiskModifications
+
+ if test == ht.NoType:
+ # no tests here
+ continue
+
+ if set_defaults or hasattr(self, attr_name):
+ attr_val = getattr(self, attr_name)
+ if not test(attr_val):
+ logging.error("OpCode %s, parameter %s, has invalid type %s/value %s",
+ self.OP_ID, attr_name, type(attr_val), attr_val)
+ raise errors.OpPrereqError("Parameter '%s.%s' fails validation" %
+ (self.OP_ID, attr_name),
+ errors.ECODE_INVAL)
+

class OpInstanceGrowDisk(OpCode):
"""Grow a disk of an instance."""
--
1.7.2.5

[Constantinos Venetsanopoulos]

With this commit we introduce the External Storage Interface
to Ganeti, abbreviated: ExtStorage Interface.

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.

Signed-off-by: Constantinos Venetsanopoulos <cv...@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 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), grow=str(new_size))
+
+
+def _ExtStorageAction(action, unique_id, size=None, grow=None):
+ """Take an External Storage action.
+
+ Take an External Storage action concerning or affecting
+ a specific Volume inside the External Storage.
+
+ @type action: string
+ @param action: which action to perform. One of:
+ create / remove / grow / attach / detach
+ @type unique_id: tuple (driver, vol_name)
+ @param unique_id: a tuple containing the type of ExtStorage (driver)
+ and the Volume name
+ @type size: integer
+ @param size: the size of the Volume in mebibytes
+ @type grow: integer
+ @param grow: the new size in mebibytes (after grow)
+ @rtype: None or a block device path (during attach)
+
+ """
+ driver, vol_name = unique_id
+
+ # Create an External Storage instance of type `driver'
+ status, inst_es = ExtStorageFromDisk(driver)
+ if not status:
+ _ThrowError("%s" % inst_es)
+
+ # Create the basic environment for the driver's scripts
+ create_env = _ExtStorageEnvironment(unique_id, size, grow)
+
+ # Do not use log file for action `attach' as we need
+ # to get the output from RunResult
+ # TODO: find a way to have a log file for attach too
+ logfile = None
+ if action is not constants.ES_ACTION_ATTACH:
+ logfile = _VolumeLogName(action, driver, vol_name)
+
+ # Find out which external script to run according the given action
+ script_name = action + "_script"
+ script = getattr(inst_es, script_name)
+
+ # Run the external script
+ result = utils.RunCmd([script], env=create_env,
+ cwd=inst_es.path, output=logfile,)
+ if result.failed:
+ logging.error("External storage's %s command '%s' returned"
+ " error: %s, logfile: %s, output: %s",
+ action, result.cmd, result.fail_reason,
+ logfile, result.output)
+
+ # If logfile is 'None' (during attach), it breaks TailFile
+ # TODO: have a log file for attach too
+ if action is not constants.ES_ACTION_ATTACH:
+ lines = [utils.SafeEncode(val)
+ for val in utils.TailFile(logfile, lines=20)]
+ _ThrowError("External storage's %s script failed (%s), last"
+ " lines in the log file:\n%s",
+ action, result.fail_reason, "\n".join(lines))
+ else:
+ _ThrowError("External storage's %s script failed (%s)",
+ action, result.fail_reason)
+
+ if action == constants.ES_ACTION_ATTACH:
+ return result.stdout
+
+
+def ExtStorageFromDisk(name, base_dir=None):
+ """Create an ExtStorage instance from disk.
+
+ This function will return an ExtStorage instance
+ if the given name is a valid ExtStorage name.
+
+ @type base_dir: string
+ @keyword base_dir: Base directory containing ExtStorage installations.
+ Defaults to a search in all the ES_SEARCH_PATH dirs.
+ @rtype: tuple
+ @return: True and the ExtStorage instance if we find a valid one, or
+ False and the diagnose message on error
+
+ """
+ if base_dir is None:
+ es_dir = utils.FindFile(name, pathutils.ES_SEARCH_PATH, os.path.isdir)
+ else:
+ es_dir = utils.FindFile(name, [base_dir], os.path.isdir)
+
+ if es_dir is None:
+ return False, ("Directory for External Storage Provider %s not"
+ " found in search path" % name)
+
+ # ES Files dictionary, we will populate it with the absolute path
+ # names; if the value is True, then it is a required file, otherwise
+ # an optional one
+ es_files = dict.fromkeys(constants.ES_SCRIPTS, True)
+
+ for filename in es_files:
+ es_files[filename] = utils.PathJoin(es_dir, filename)
+
+ try:
+ st = os.stat(es_files[filename])
+ except EnvironmentError, err:
+ return False, ("File '%s' under path '%s' is missing (%s)" %
+ (filename, es_dir, utils.ErrnoOrStr(err)))
+
+ if not stat.S_ISREG(stat.S_IFMT(st.st_mode)):
+ return False, ("File '%s' under path '%s' is not a regular file" %
+ (filename, es_dir))
+
+ if filename in constants.ES_SCRIPTS:
+ if stat.S_IMODE(st.st_mode) & stat.S_IXUSR != stat.S_IXUSR:
+ return False, ("File '%s' under path '%s' is not executable" %
+ (filename, es_dir))
+
+ es_obj = \
+ objects.ExtStorage(name=name, path=es_dir,
+ create_script=es_files[constants.ES_SCRIPT_CREATE],
+ remove_script=es_files[constants.ES_SCRIPT_REMOVE],
+ grow_script=es_files[constants.ES_SCRIPT_GROW],
+ attach_script=es_files[constants.ES_SCRIPT_ATTACH],
+ detach_script=es_files[constants.ES_SCRIPT_DETACH])
+ return True, es_obj
+
+
+def _ExtStorageEnvironment(unique_id, size=None, grow=None):
+ """Calculate the environment for an External Storage script.
+
+ @type unique_id: tuple (driver, vol_name)
+ @param unique_id: ExtStorage pool and name of the Volume
+ @type size: integer
+ @param size: size of the Volume in mebibytes
+ @rtype: dict
+ @return: dict of environment variables
+
+ """
+ vol_name = unique_id[1]
+
+ result = {}
+ result['VOL_NAME'] = vol_name
+
+ if size is not None:
+ result['VOL_SIZE'] = size
+
+ if grow is not None:
+ result['VOL_NEW_SIZE'] = grow
+
+ return result
+
+
+def _VolumeLogName(kind, es_name, volume):
+ """Compute the ExtStorage log filename for a given Volume and operation.
+
+ @type kind: string
+ @param kind: the operation type (e.g. create, remove etc.)
+ @type es_name: string
+ @param es_name: the ExtStorage name
+ @type volume: string
+ @param volume: the name of the Volume inside the External Storage
+
+ """
+ # Check if the extstorage log dir is a valid dir
+ if not os.path.isdir(pathutils.LOG_ES_DIR):
+ _ThrowError("Cannot find log directory: %s", pathutils.LOG_ES_DIR)
+
+ # TODO: Use tempfile.mkstemp to create unique filename
+ base = ("%s-%s-%s-%s.log" %
+ (kind, es_name, volume, utils.TimestampForFilename()))
+ return utils.PathJoin(pathutils.LOG_ES_DIR, base)
+
+
DEV_MAP = {
constants.LD_LV: LogicalVolume,
constants.LD_DRBD8: DRBD8,
constants.LD_BLOCKDEV: PersistentBlockDevice,
constants.LD_RBD: RADOSBlockDevice,
+ constants.LD_EXT: ExtStorageDevice,
}

if constants.ENABLE_FILE_STORAGE or constants.ENABLE_SHARED_FILE_STORAGE:
diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
index 2759844..dc625ac 100644
--- a/lib/client/gnt_cluster.py
+++ b/lib/client/gnt_cluster.py
@@ -454,6 +454,8 @@ def ShowClusterConfig(opts, args):
ToStdout(" - primary ip version: %d", result["primary_ip_version"])
ToStdout(" - preallocation wipe disks: %s", result["prealloc_wipe_disks"])
ToStdout(" - OS search path: %s", utils.CommaJoin(pathutils.OS_SEARCH_PATH))
+ ToStdout(" - ExtStorage Providers search path: %s",
+ utils.CommaJoin(pathutils.ES_SEARCH_PATH))

ToStdout("Default node parameters:")
_PrintGroupedParams(result["ndparams"], roman=opts.roman_integers)
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 2068882..90b2117 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8591,9 +8591,9 @@ class TLMigrateInstance(Tasklet):
self._GoReconnect(False)
self._WaitUntilSync()

- # If the instance's disk template is `rbd' and there was a successful
- # migration, unmap the device from the source node.
- if self.instance.disk_template == constants.DT_RBD:
+ # If the instance's disk template is `rbd' or `ext' and there was a
+ # successful migration, unmap the device from the source node.
+ if self.instance.disk_template in (constants.DT_RBD, constants.DT_EXT):
disks = _ExpandCheckDisks(instance, instance.disks)
self.feedback_fn("* unmapping instance's disks from %s" % source_node)
for disk in disks:
@@ -8843,6 +8843,7 @@ def _GenerateDRBD8Branch(lu, primary, secondary, size, vgnames, names,
_DISK_TEMPLATE_NAME_PREFIX = {
constants.DT_PLAIN: "",
constants.DT_RBD: ".rbd",
+ constants.DT_EXT: ".ext",
}


@@ -8852,6 +8853,7 @@ _DISK_TEMPLATE_DEVICE_TYPE = {
constants.DT_SHARED_FILE: constants.LD_FILE,
constants.DT_BLOCK: constants.LD_BLOCKDEV,
constants.DT_RBD: constants.LD_RBD,
+ constants.DT_EXT: constants.LD_EXT,
}


@@ -8931,6 +8933,8 @@ def _GenerateDiskTemplate(
disk[constants.IDISK_ADOPT])
elif template_name == constants.DT_RBD:
logical_id_fn = lambda idx, _, disk: ("rbd", names[idx])
+ elif template_name == constants.DT_EXT:
+ logical_id_fn = lambda idx, _, disk: ("ext", names[idx])
else:
raise errors.ProgrammerError("Unknown disk template '%s'" % template_name)

@@ -10017,6 +10021,9 @@ class LUInstanceCreate(LogicalUnit):
# Any function that checks prerequisites can be placed here.
# Check if there is enough space on the RADOS cluster.
_CheckRADOSFreeSpace()
+ elif self.op.disk_template == constants.DT_EXT:
+ # FIXME: Function that checks prereqs if needed
+ pass
else:
# Check lv size requirements, if not adopting
req_sizes = _ComputeDiskSizePerVG(self.op.disk_template, self.disks)
@@ -11725,7 +11732,8 @@ class LUInstanceGrowDisk(LogicalUnit):

if instance.disk_template not in (constants.DT_FILE,
constants.DT_SHARED_FILE,
- constants.DT_RBD):
+ constants.DT_RBD,
+ constants.DT_EXT):
# TODO: check the free disk space for file, when that feature will be
# supported
_CheckNodesFreeDiskPerVG(self, nodenames,
diff --git a/lib/constants.py b/lib/constants.py
index 45b3f41..1cb7196 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -343,19 +343,21 @@ DT_FILE = "file"
DT_SHARED_FILE = "sharedfile"
DT_BLOCK = "blockdev"
DT_RBD = "rbd"
+DT_EXT = "ext"

# the set of network-mirrored disk templates
DTS_INT_MIRROR = frozenset([DT_DRBD8])

# the set of externally-mirrored disk templates (e.g. SAN, NAS)
-DTS_EXT_MIRROR = frozenset([DT_SHARED_FILE, DT_BLOCK, DT_RBD])
+DTS_EXT_MIRROR = frozenset([DT_SHARED_FILE, DT_BLOCK, DT_RBD, DT_EXT])

# the set of non-lvm-based disk templates
DTS_NOT_LVM = frozenset([DT_DISKLESS, DT_FILE, DT_SHARED_FILE,
- DT_BLOCK, DT_RBD])
+ DT_BLOCK, DT_RBD, DT_EXT])

# the set of disk templates which can be grown
-DTS_GROWABLE = frozenset([DT_PLAIN, DT_DRBD8, DT_FILE, DT_SHARED_FILE, DT_RBD])
+DTS_GROWABLE = frozenset([DT_PLAIN, DT_DRBD8, DT_FILE, DT_SHARED_FILE,
+ DT_RBD, DT_EXT])

# the set of disk templates that allow adoption
DTS_MAY_ADOPT = frozenset([DT_PLAIN, DT_BLOCK])
@@ -375,15 +377,17 @@ LD_DRBD8 = "drbd8"
LD_FILE = "file"
LD_BLOCKDEV = "blockdev"
LD_RBD = "rbd"
+LD_EXT = "ext"
LOGICAL_DISK_TYPES = frozenset([
LD_LV,
LD_DRBD8,
LD_FILE,
LD_BLOCKDEV,
LD_RBD,
+ LD_EXT,
])

-LDS_BLOCK = frozenset([LD_LV, LD_DRBD8, LD_BLOCKDEV, LD_RBD])
+LDS_BLOCK = frozenset([LD_LV, LD_DRBD8, LD_BLOCKDEV, LD_RBD, LD_EXT])

# drbd constants
DRBD_HMAC_ALG = "md5"
@@ -482,7 +486,8 @@ DISK_TEMPLATES = frozenset([
DT_FILE,
DT_SHARED_FILE,
DT_BLOCK,
- DT_RBD
+ DT_RBD,
+ DT_EXT
])

FILE_DRIVER = frozenset([FD_LOOP, FD_BLKTAP])
@@ -604,6 +609,26 @@ OS_PARAMETERS_FILE = "parameters.list"
OS_VALIDATE_PARAMETERS = "parameters"
OS_VALIDATE_CALLS = frozenset([OS_VALIDATE_PARAMETERS])

+# External Storage (ES) related constants
+ES_ACTION_CREATE = "create"
+ES_ACTION_REMOVE = "remove"
+ES_ACTION_GROW = "grow"
+ES_ACTION_ATTACH = "attach"
+ES_ACTION_DETACH = "detach"
+
+ES_SCRIPT_CREATE = ES_ACTION_CREATE
+ES_SCRIPT_REMOVE = ES_ACTION_REMOVE
+ES_SCRIPT_GROW = ES_ACTION_GROW
+ES_SCRIPT_ATTACH = ES_ACTION_ATTACH
+ES_SCRIPT_DETACH = ES_ACTION_DETACH
+ES_SCRIPTS = frozenset([
+ ES_SCRIPT_CREATE,
+ ES_SCRIPT_REMOVE,
+ ES_SCRIPT_GROW,
+ ES_SCRIPT_ATTACH,
+ ES_SCRIPT_DETACH
+ ])
+
# ssh constants
SSH = "ssh"
SCP = "scp"
@@ -1821,6 +1846,8 @@ DISK_LD_DEFAULTS = {
LD_RBD: {
LDP_POOL: "rbd"
},
+ LD_EXT: {
+ },
}

# readability shortcuts
@@ -1854,6 +1881,8 @@ DISK_DT_DEFAULTS = {
DT_RBD: {
RBD_POOL: DISK_LD_DEFAULTS[LD_RBD][LDP_POOL]
},
+ DT_EXT: {
+ },
}

# we don't want to export the shortcuts
@@ -2013,6 +2042,7 @@ VALID_ALLOC_POLICIES = [

# Temporary external/shared storage parameters
BLOCKDEV_DRIVER_MANUAL = "manual"
+EXTSTORAGE_SAMPLE_PROVIDER = "rbd"

# qemu-img path, required for ovfconverter
QEMUIMG_PATH = _autoconf.QEMUIMG_PATH
diff --git a/lib/masterd/instance.py b/lib/masterd/instance.py
index 83e7424..d99f4d8 100644
--- a/lib/masterd/instance.py
+++ b/lib/masterd/instance.py
@@ -1630,6 +1630,7 @@ def ComputeDiskSize(disk_template, disks):
constants.DT_SHARED_FILE: sum(d[constants.IDISK_SIZE] for d in disks),
constants.DT_BLOCK: 0,
constants.DT_RBD: sum(d[constants.IDISK_SIZE] for d in disks),
+ constants.DT_EXT: sum(d[constants.IDISK_SIZE] for d in disks),
}

if disk_template not in req_size_dict:
diff --git a/lib/objects.py b/lib/objects.py
index 4be25ab..da5bc5b 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -598,7 +598,8 @@ class Disk(ConfigObject):

"""
if self.dev_type in [constants.LD_LV, constants.LD_FILE,
- constants.LD_BLOCKDEV, constants.LD_RBD]:
+ constants.LD_BLOCKDEV, constants.LD_RBD,
+ constants.LD_EXT]:
result = [node]
elif self.dev_type in constants.LDS_DRBD:
result = [self.logical_id[0], self.logical_id[1]]
@@ -674,7 +675,7 @@ class Disk(ConfigObject):

"""
if self.dev_type in (constants.LD_LV, constants.LD_FILE,
- constants.LD_RBD):
+ constants.LD_RBD, constants.LD_EXT):
self.size += amount
elif self.dev_type == constants.LD_DRBD8:
if self.children:
@@ -1231,6 +1232,21 @@ class OS(ConfigObject):
return cls.SplitNameVariant(name)[1]


+class ExtStorage(ConfigObject):
+ """Config object representing an External Storage Provider.
+
+ """
+ __slots__ = [
+ "name",
+ "path",
+ "create_script",
+ "remove_script",
+ "grow_script",
+ "attach_script",
+ "detach_script",
+ ]
+
+
class NodeHvState(ConfigObject):
"""Hypvervisor state on a node.

diff --git a/lib/pathutils.py b/lib/pathutils.py
index 738c661..7983f48 100644
--- a/lib/pathutils.py
+++ b/lib/pathutils.py
@@ -30,6 +30,7 @@ DEFAULT_FILE_STORAGE_DIR = _autoconf.FILE_STORAGE_DIR
DEFAULT_SHARED_FILE_STORAGE_DIR = _autoconf.SHARED_FILE_STORAGE_DIR
EXPORT_DIR = _autoconf.EXPORT_DIR
OS_SEARCH_PATH = _autoconf.OS_SEARCH_PATH
+ES_SEARCH_PATH = _autoconf.ES_SEARCH_PATH
SSH_CONFIG_DIR = _autoconf.SSH_CONFIG_DIR
SYSCONFDIR = _autoconf.SYSCONFDIR
TOOLSDIR = _autoconf.TOOLSDIR
@@ -106,6 +107,7 @@ MASTER_SOCKET = SOCKET_DIR + "/ganeti-master"
QUERY_SOCKET = SOCKET_DIR + "/ganeti-query"

LOG_OS_DIR = LOG_DIR + "/os"
+LOG_ES_DIR = LOG_DIR + "/extstorage"

# Job queue paths
JOB_QUEUE_LOCK_FILE = QUEUE_DIR + "/lock"
diff --git a/tools/burnin b/tools/burnin
index 11a7042..adbf43b 100755
--- a/tools/burnin
+++ b/tools/burnin
@@ -463,6 +463,7 @@ class Burner(object):
constants.DT_PLAIN,
constants.DT_DRBD8,
constants.DT_RBD,
+ constants.DT_EXT,
)
if options.disk_template not in supported_disk_templates:
Err("Unknown disk template '%s'" % options.disk_template)
--
1.7.2.5

[Constantinos Venetsanopoulos]

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

htools/Ganeti/HTools/Cluster.hs | 6 ++++++
htools/Ganeti/HTools/Instance.hs | 1 +
htools/Ganeti/HTools/Types.hs | 2 ++
htools/Ganeti/Objects.hs | 12 ++++++++++++
4 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/htools/Ganeti/HTools/Cluster.hs b/htools/Ganeti/HTools/Cluster.hs
index deb56dd..ec3c00f 100644
--- a/htools/Ganeti/HTools/Cluster.hs
+++ b/htools/Ganeti/HTools/Cluster.hs
@@ -937,6 +937,12 @@ nodeEvacInstance nl il mode inst@(Instance.Instance
failOnSecondaryChange mode dt >>
evacOneNodeOnly nl il inst gdx avail_nodes

+nodeEvacInstance nl il mode inst@(Instance.Instance
+ {Instance.diskTemplate = dt@DTExt})
+ gdx avail_nodes =
+ failOnSecondaryChange mode dt >>
+ evacOneNodeOnly nl il inst gdx avail_nodes
+
nodeEvacInstance nl il ChangePrimary
inst@(Instance.Instance {Instance.diskTemplate = DTDrbd8})
_ _ =
diff --git a/htools/Ganeti/HTools/Instance.hs b/htools/Ganeti/HTools/Instance.hs
index 9d13556..d211d5a 100644
--- a/htools/Ganeti/HTools/Instance.hs
+++ b/htools/Ganeti/HTools/Instance.hs
@@ -144,6 +144,7 @@ movableDiskTemplates =
, T.DTBlock
, T.DTSharedFile
, T.DTRbd
+ , T.DTExt
]

-- | A simple name for the int, instance association list.
diff --git a/htools/Ganeti/HTools/Types.hs b/htools/Ganeti/HTools/Types.hs
index 31b3d70..4fc6b3f 100644
--- a/htools/Ganeti/HTools/Types.hs
+++ b/htools/Ganeti/HTools/Types.hs
@@ -123,6 +123,7 @@ $(THH.declareSADT "DiskTemplate"
, ("DTBlock", 'C.dtBlock)
, ("DTDrbd8", 'C.dtDrbd8)
, ("DTRbd", 'C.dtRbd)
+ , ("DTExt", 'C.dtExt)
])
$(THH.makeJSONInstance ''DiskTemplate)

@@ -141,6 +142,7 @@ templateMirrorType DTPlain = MirrorNone
templateMirrorType DTBlock = MirrorExternal
templateMirrorType DTDrbd8 = MirrorInternal
templateMirrorType DTRbd = MirrorExternal
+templateMirrorType DTExt = MirrorExternal

-- | The Group allocation policy type.
--
diff --git a/htools/Ganeti/Objects.hs b/htools/Ganeti/Objects.hs
index 2a3207d..ea7e177 100644
--- a/htools/Ganeti/Objects.hs
+++ b/htools/Ganeti/Objects.hs
@@ -200,6 +200,7 @@ $(declareSADT "DiskType"
, ("LD_FILE", 'C.ldFile)
, ("LD_BLOCKDEV", 'C.ldBlockdev)
, ("LD_RADOS", 'C.ldRbd)
+ , ("LD_EXT", 'C.ldExt)
])
$(makeJSONInstance ''DiskType)

@@ -231,6 +232,7 @@ data DiskLogicalId
| LIDFile FileDriver String -- ^ Driver, path
| LIDBlockDev BlockDriver String -- ^ Driver, path (must be under /dev)
| LIDRados String String -- ^ Unused, path
+ | LIDExt String String -- ^ ExtProvider, unique name
deriving (Read, Show, Eq)

-- | Mapping from a logical id to a disk type.
@@ -240,6 +242,7 @@ lidDiskType (LIDDrbd8 {}) = LD_DRBD8
lidDiskType (LIDFile {}) = LD_FILE
lidDiskType (LIDBlockDev {}) = LD_BLOCKDEV
lidDiskType (LIDRados {}) = LD_RADOS
+lidDiskType (LIDExt {}) = LD_EXT

-- | Builds the extra disk_type field for a given logical id.
lidEncodeType :: DiskLogicalId -> [(String, JSValue)]
@@ -254,6 +257,7 @@ encodeDLId (LIDDrbd8 nodeA nodeB port minorA minorB key) =
encodeDLId (LIDRados pool name) = JSArray [showJSON pool, showJSON name]
encodeDLId (LIDFile driver name) = JSArray [showJSON driver, showJSON name]
encodeDLId (LIDBlockDev driver name) = JSArray [showJSON driver, showJSON name]
+encodeDLId (LIDExt extprovider name) = JSArray [showJSON extprovider, showJSON name]

-- | Custom encoder for DiskLogicalId, composing both the logical id
-- and the extra disk_type field.
@@ -305,6 +309,13 @@ decodeDLId obj lid = do
path' <- readJSON path
return $ LIDRados driver' path'
_ -> fail "Can't read logical_id for rdb type"
+ LD_EXT ->
+ case lid of
+ JSArray [extprovider, name] -> do
+ extprovider' <- readJSON extprovider
+ name' <- readJSON name
+ return $ LIDExt extprovider' name'
+ _ -> fail "Can't read logical_id for extstorage type"

-- | Disk data structure.
--
@@ -353,6 +364,7 @@ $(declareSADT "DiskTemplate"
, ("DTBlock", 'C.dtBlock)
, ("DTDrbd8", 'C.dtDrbd8)
, ("DTRados", 'C.dtRbd)
+ , ("DTExt", 'C.dtExt)
])
$(makeJSONInstance ''DiskTemplate)

--
1.7.2.5

[Constantinos Venetsanopoulos]

Hello team,

This is the ExtStorage Interface patch series rebased on top of
master branch's commit 0e1e0b6. It passes all current lint/unit
tests.

The first commit (design doc) has been already reviewed by Iustin
and the only question was about separate providers between disks,
which I think has been answered, after the discussion concerning
Storage Pools.

You can find the previous discussion with Iustin here:
https://groups.google.com/forum/?fromgroups=#!search/Update$20the$20shared$20storage$20design$20document/ganeti-devel/2qO1xGUQQwA/aPsQYU3TG6wJ

I'll be also sending a sample ExtStorage provider, which you can
use to test the patch series, without the need of setting up a
NAS and writing custom scripts.

Kind Regards,
Constantinos

[Constantinos Venetsanopoulos]

Add support for passing parameters to the ext template (ext-params).
Take advantage of disk-params, that don't seem to make much sense in
this template (ExtStorage Providers are not predefined and we don't
know their needs) and use them to pass the ext-params dynamically to
the template.

ext-params are correlated with gnt-os-interface's os-params.
All ext-params are exported to the ExtStorage Provider through it's
environment, with variables prefixed with 'EXTP_' (similarly to the
OS interface's 'OSP_' params).

ext-params are passed through the --disk option. If the disk template
is of type `ext' during instance add, then any additional options that
are not in IDISK_PARAMS given to --disk are considered ext-params
e.g.:

gnt-instance add -t ext --disk=0:size=2G,param1=value1,param2=value2

Finally, we introduce a new IDISK_PARAM called IDISK_PROVIDER, that is
mandatory for template `ext' and is used to select the desired
ExtStorage Provider. This parameter is not valid for other template
types.

The IDISK_PROVIDER parameter becomes the first element of the
disk's unique_id tuple e.g.:

unique_id = ('sample_provider1', 'UUID.ext.diskX')

Example selecting different ExtStorage Providers for each disk and
passing different ext-params to them:

-t ext --disk=0:size=2G,provider=sample_provider1,param1=value1
--disk=1:size=3G,provider=sample_provider2,param2=value2

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

lib/bdev.py | 47 +++++++++++++++++++++++++++++++++---------
lib/cmdlib.py | 45 ++++++++++++++++++++++++++++++++++++++--
lib/constants.py | 9 +++++++-
lib/objects.py | 5 ++++
lib/opcodes.py | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 151 insertions(+), 14 deletions(-)

diff --git a/lib/bdev.py b/lib/bdev.py
index b0ed7c0..ffb88a1 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -2665,6 +2665,7 @@ class ExtStorageDevice(BlockDev):

raise ValueError("Invalid configuration data %s" % str(unique_id))

self.driver, self.vol_name = unique_id
+ self.ext_params = params

self.major = self.minor = None

self.Attach()
@@ -2680,10 +2681,12 @@ class ExtStorageDevice(BlockDev):

if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:

raise errors.ProgrammerError("Invalid configuration data %s" %

str(unique_id))
+ ext_params = params

# 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))
+ _ExtStorageAction(constants.ES_ACTION_CREATE, unique_id,
+ ext_params, str(size))

return ExtStorageDevice(unique_id, children, size, params)

@@ -2700,7 +2703,8 @@ class ExtStorageDevice(BlockDev):

# Call the External Storage's remove script,

# to remove the Volume from the External Storage

- _ExtStorageAction(constants.ES_ACTION_REMOVE, self.unique_id)
+ _ExtStorageAction(constants.ES_ACTION_REMOVE, self.unique_id,
+ self.ext_params)

def Rename(self, new_id):
"""Rename this device.
@@ -2720,7 +2724,7 @@ class ExtStorageDevice(BlockDev):

# 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)
+ self.unique_id, self.ext_params)

try:
st = os.stat(self.dev_path)
@@ -2754,7 +2758,8 @@ class ExtStorageDevice(BlockDev):

# 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)
+ _ExtStorageAction(constants.ES_ACTION_DETACH, self.unique_id,
+ self.ext_params)

self.minor = None
self.dev_path = None
@@ -2795,10 +2800,10 @@ class ExtStorageDevice(BlockDev):

# 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), grow=str(new_size))
+ self.ext_params, str(self.size), grow=str(new_size))


-def _ExtStorageAction(action, unique_id, size=None, grow=None):
+def _ExtStorageAction(action, unique_id, ext_params, size=None, grow=None):

"""Take an External Storage action.

Take an External Storage action concerning or affecting

@@ -2810,6 +2815,8 @@ def _ExtStorageAction(action, unique_id, size=None, grow=None):

@type unique_id: tuple (driver, vol_name)

@param unique_id: a tuple containing the type of ExtStorage (driver)

and the Volume name
+ @type ext_params: dict
+ @type ext_params: ExtStorage parameters
@type size: integer

@param size: the size of the Volume in mebibytes

@type grow: integer
@@ -2825,7 +2832,7 @@ def _ExtStorageAction(action, unique_id, size=None, grow=None):
_ThrowError("%s" % inst_es)

# Create the basic environment for the driver's scripts

- create_env = _ExtStorageEnvironment(unique_id, size, grow)
+ create_env = _ExtStorageEnvironment(unique_id, ext_params, size, grow)

# Do not use log file for action `attach' as we need

# to get the output from RunResult

@@ -2891,7 +2898,9 @@ def ExtStorageFromDisk(name, base_dir=None):
# an optional one
es_files = dict.fromkeys(constants.ES_SCRIPTS, True)

- for filename in es_files:
+ es_files[constants.ES_PARAMETERS_FILE] = True
+
+ for (filename, _) in es_files.items():

es_files[filename] = utils.PathJoin(es_dir, filename)

try:
@@ -2909,21 +2918,35 @@ def ExtStorageFromDisk(name, base_dir=None):

return False, ("File '%s' under path '%s' is not executable" %

(filename, es_dir))

+ parameters = []
+ if constants.ES_PARAMETERS_FILE in es_files:
+ parameters_file = es_files[constants.ES_PARAMETERS_FILE]
+ try:
+ parameters = utils.ReadFile(parameters_file).splitlines()
+ except EnvironmentError, err:
+ return False, ("Error while reading the EXT parameters file at %s: %s" %
+ (parameters_file, utils.ErrnoOrStr(err)))
+ parameters = [v.split(None, 1) for v in parameters]
+
es_obj = \
objects.ExtStorage(name=name, path=es_dir,
create_script=es_files[constants.ES_SCRIPT_CREATE],
remove_script=es_files[constants.ES_SCRIPT_REMOVE],
grow_script=es_files[constants.ES_SCRIPT_GROW],
attach_script=es_files[constants.ES_SCRIPT_ATTACH],
- detach_script=es_files[constants.ES_SCRIPT_DETACH])
+ detach_script=es_files[constants.ES_SCRIPT_DETACH],
+ verify_script=es_files[constants.ES_SCRIPT_VERIFY],
+ supported_parameters=parameters)
return True, es_obj


-def _ExtStorageEnvironment(unique_id, size=None, grow=None):
+def _ExtStorageEnvironment(unique_id, ext_params, size=None, grow=None):

"""Calculate the environment for an External Storage script.

@type unique_id: tuple (driver, vol_name)

@param unique_id: ExtStorage pool and name of the Volume

+ @type ext_params: dict
+ @param ext_params: the EXT parameters
@type size: integer

@param size: size of the Volume in mebibytes

@rtype: dict
@@ -2935,6 +2958,10 @@ def _ExtStorageEnvironment(unique_id, size=None, grow=None):
result = {}
result['VOL_NAME'] = vol_name

+ # EXT params
+ for pname, pvalue in ext_params.items():
+ result["EXTP_%s" % pname.upper()] = str(pvalue)

+
if size is not None:

result['VOL_SIZE'] = size

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 90b2117..406ae2f 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -7071,6 +7071,7 @@ class LUInstanceRecreateDisks(LogicalUnit):
# TODO: Implement support changing VG while recreating
constants.IDISK_VG,
constants.IDISK_METAVG,
+ constants.IDISK_PROVIDER,
]))

def _RunAllocator(self):
@@ -8934,13 +8935,26 @@ def _GenerateDiskTemplate(

elif template_name == constants.DT_RBD:
logical_id_fn = lambda idx, _, disk: ("rbd", names[idx])

elif template_name == constants.DT_EXT:
- logical_id_fn = lambda idx, _, disk: ("ext", names[idx])
+ def logical_id_fn(idx, _, disk):
+ provider = disk.get(constants.IDISK_PROVIDER, None)
+ if provider is None:
+ raise errors.ProgrammerError("Disk template is %s, but '%s' is"
+ " not found", constants.DT_EXT,
+ constants.IDISK_PROVIDER)
+ return (provider, names[idx])

else:
raise errors.ProgrammerError("Unknown disk template '%s'" % template_name)

dev_type = _DISK_TEMPLATE_DEVICE_TYPE[template_name]

for idx, disk in enumerate(disk_info):
+ params = {}
+ # Only for the Ext template add disk_info to params
+ if template_name == constants.DT_EXT:
+ params[constants.IDISK_PROVIDER] = disk[constants.IDISK_PROVIDER]
+ for key in disk:
+ if key not in constants.IDISK_PARAMS:
+ params[key] = disk[key]
disk_index = idx + base_index
size = disk[constants.IDISK_SIZE]
feedback_fn("* disk %s, size %s" %
@@ -8949,7 +8963,7 @@ def _GenerateDiskTemplate(
logical_id=logical_id_fn(idx, disk_index, disk),
iv_name="disk/%d" % disk_index,
mode=disk[constants.IDISK_MODE],
- params={}))
+ params=params))

return disks

@@ -9298,7 +9312,8 @@ class LUInstanceCreate(LogicalUnit):
# check disks. parameter names and consistent adopt/no-adopt strategy
has_adopt = has_no_adopt = False
for disk in self.op.disks:
- utils.ForceDictType(disk, constants.IDISK_PARAMS_TYPES)
+ if self.op.disk_template != constants.DT_EXT:
+ utils.ForceDictType(disk, constants.IDISK_PARAMS_TYPES)
if constants.IDISK_ADOPT in disk:
has_adopt = True
else:
@@ -9893,16 +9908,37 @@ class LUInstanceCreate(LogicalUnit):
raise errors.OpPrereqError("Invalid disk size '%s'" % size,
errors.ECODE_INVAL)

+ ext_provider = disk.get(constants.IDISK_PROVIDER, None)
+ if ext_provider and self.op.disk_template != constants.DT_EXT:
+ raise errors.OpPrereqError("The '%s' option is only valid for the %s"
+ " disk template, not %s" %
+ (constants.IDISK_PROVIDER, constants.DT_EXT,
+ self.op.disk_template), errors.ECODE_INVAL)
+
data_vg = disk.get(constants.IDISK_VG, default_vg)
new_disk = {
constants.IDISK_SIZE: size,
constants.IDISK_MODE: mode,
constants.IDISK_VG: data_vg,
}
+
if constants.IDISK_METAVG in disk:
new_disk[constants.IDISK_METAVG] = disk[constants.IDISK_METAVG]
if constants.IDISK_ADOPT in disk:
new_disk[constants.IDISK_ADOPT] = disk[constants.IDISK_ADOPT]
+
+ # For extstorage, demand the `provider' option and add any
+ # additional parameters (ext-params) to the dict
+ if self.op.disk_template == constants.DT_EXT:
+ if ext_provider:
+ new_disk[constants.IDISK_PROVIDER] = ext_provider
+ for key in disk:
+ if key not in constants.IDISK_PARAMS:
+ new_disk[key] = disk[key]
+ else:
+ raise errors.OpPrereqError("Missing provider for template '%s'" %
+ constants.DT_EXT, errors.ECODE_INVAL)
+
self.disks.append(new_disk)

if self.op.mode == constants.INSTANCE_IMPORT:
@@ -10107,6 +10143,9 @@ class LUInstanceCreate(LogicalUnit):

_CheckNicsBridgesExist(self, self.nics, self.pnode.name)

+ #TODO: _CheckExtParams (remotely)
+ # Check parameters for extstorage
+
# memory check on primary node
#TODO(dynmem): use MINMEM for checking
if self.op.start:
diff --git a/lib/constants.py b/lib/constants.py
index 1cb7196..4847f43 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -615,20 +615,25 @@ ES_ACTION_REMOVE = "remove"
ES_ACTION_GROW = "grow"
ES_ACTION_ATTACH = "attach"
ES_ACTION_DETACH = "detach"
+ES_ACTION_VERIFY = "verify"

ES_SCRIPT_CREATE = ES_ACTION_CREATE
ES_SCRIPT_REMOVE = ES_ACTION_REMOVE
ES_SCRIPT_GROW = ES_ACTION_GROW
ES_SCRIPT_ATTACH = ES_ACTION_ATTACH
ES_SCRIPT_DETACH = ES_ACTION_DETACH
+ES_SCRIPT_VERIFY = ES_ACTION_VERIFY
ES_SCRIPTS = frozenset([
ES_SCRIPT_CREATE,
ES_SCRIPT_REMOVE,
ES_SCRIPT_GROW,
ES_SCRIPT_ATTACH,
- ES_SCRIPT_DETACH
+ ES_SCRIPT_DETACH,
+ ES_SCRIPT_VERIFY
])

+ES_PARAMETERS_FILE = "parameters.list"

+
# ssh constants
SSH = "ssh"
SCP = "scp"

@@ -1085,12 +1090,14 @@ IDISK_MODE = "mode"
IDISK_ADOPT = "adopt"
IDISK_VG = "vg"
IDISK_METAVG = "metavg"
+IDISK_PROVIDER = "provider"
IDISK_PARAMS_TYPES = {
IDISK_SIZE: VTYPE_SIZE,
IDISK_MODE: VTYPE_STRING,
IDISK_ADOPT: VTYPE_STRING,
IDISK_VG: VTYPE_STRING,
IDISK_METAVG: VTYPE_STRING,
+ IDISK_PROVIDER: VTYPE_STRING,
}
IDISK_PARAMS = frozenset(IDISK_PARAMS_TYPES.keys())

diff --git a/lib/objects.py b/lib/objects.py
index da5bc5b..601b817 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -905,6 +905,9 @@ class Disk(ConfigObject):
constants.LDP_POOL: dt_params[constants.RBD_POOL]
}))

+ elif disk_template == constants.DT_EXT:
+ result.append(constants.DISK_LD_DEFAULTS[constants.LD_EXT])
+
return result


@@ -1244,6 +1247,8 @@ class ExtStorage(ConfigObject):
"grow_script",
"attach_script",
"detach_script",
+ "verify_script",
+ "supported_parameters",
]


diff --git a/lib/opcodes.py b/lib/opcodes.py
index e4ad078..961192f 100644
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -201,6 +201,12 @@ _TDiskParams = \
ht.Comment("Disk parameters")(ht.TDictOf(ht.TElemOf(constants.IDISK_PARAMS),
ht.TOr(ht.TNonEmptyString, ht.TInt)))

+#: Same as _TDiskParams but with NonEmptyString in the place of IDISK_PARAMS
+_TExtDiskParams = \
+ ht.Comment("ExtStorage Disk parameters")(ht.TDictOf(ht.TNonEmptyString,
+ ht.TOr(ht.TNonEmptyString,
+ ht.TInt)))
+
_TQueryRow = \
ht.TListOf(ht.TAnd(ht.TIsLength(2),
ht.TItems([ht.TElemOf(constants.RS_ALL),
@@ -1207,6 +1213,59 @@ class OpInstanceCreate(OpCode):
]
OP_RESULT = ht.Comment("instance nodes")(ht.TListOf(ht.TNonEmptyString))

+ def Validate(self, set_defaults):
+ """Validate opcode parameters, optionally setting default values.
+
+ @type set_defaults: bool
+ @param set_defaults: Whether to set default values
+ @raise errors.OpPrereqError: When a parameter value doesn't match
+ requirements
+
+ """
+ # pylint: disable=E1101
+ # as OP_ID has been dynamically defined

+
+ # Check if the template is DT_EXT
+ is_ext = False

+ for (attr_name, _, _, _) in self.GetAllParams():
+ if hasattr(self, attr_name):

+ if attr_name == "disk_template" and \
+ getattr(self, attr_name) == constants.DT_EXT:
+ is_ext = True

+
+ for (attr_name, default, test, _) in self.GetAllParams():
+ assert test == ht.NoType or callable(test)
+
+ if not hasattr(self, attr_name):
+ if default == ht.NoDefault:
+ raise errors.OpPrereqError("Required parameter '%s.%s' missing" %
+ (self.OP_ID, attr_name),
+ errors.ECODE_INVAL)
+ elif set_defaults:
+ if callable(default):
+ dval = default()
+ else:
+ dval = default
+ setattr(self, attr_name, dval)
+

+ # If the template is DT_EXT and attr_name = disks
+ # set a new test method that allows passing of unknown parameters
+ if is_ext and attr_name == "disks":
+ test = ht.TListOf(_TExtDiskParams)

+
+ if test == ht.NoType:
+ # no tests here
+ continue
+
+ if set_defaults or hasattr(self, attr_name):
+ attr_val = getattr(self, attr_name)
+ if not test(attr_val):
+ logging.error("OpCode %s, parameter %s, has invalid type %s/value %s",
+ self.OP_ID, attr_name, type(attr_val), attr_val)
+ raise errors.OpPrereqError("Parameter '%s.%s' fails validation" %
+ (self.OP_ID, attr_name),
+ errors.ECODE_INVAL)
+

class OpInstanceReinstall(OpCode):
"""Reinstall an instance's OS."""
--
1.7.2.5

[Constantinos Venetsanopoulos]

* ganeti-extstorage-interface man page
* gnt-storage man page

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

.gitignore | 1 +
Makefile.am | 2 +
man/footer.rst | 11 +-
man/ganeti-extstorage-interface.rst | 212 +++++++++++++++++++++++++++++++++++
man/gnt-storage.rst | 63 ++++++++++
5 files changed, 284 insertions(+), 5 deletions(-)
create mode 100644 man/ganeti-extstorage-interface.rst
create mode 100644 man/gnt-storage.rst

diff --git a/.gitignore b/.gitignore
index 393495e..516d1de 100644
--- a/.gitignore
+++ b/.gitignore
@@ -103,6 +103,7 @@
/scripts/gnt-job
/scripts/gnt-node
/scripts/gnt-os
+/scripts/gnt-storage

# htools-specific rules
/htools/apidoc
diff --git a/Makefile.am b/Makefile.am
index 215c739..1a49879 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -743,6 +743,7 @@ man_MANS = \
man/ganeti-masterd.8 \
man/ganeti-noded.8 \
man/ganeti-os-interface.7 \
+ man/ganeti-extstorage-interface.7 \
man/ganeti-rapi.8 \
man/ganeti-watcher.8 \
man/gnt-backup.8 \
@@ -753,6 +754,7 @@ man_MANS = \
man/gnt-job.8 \
man/gnt-node.8 \
man/gnt-os.8 \
+ man/gnt-storage.8 \
man/hail.1 \
man/hbal.1 \
man/hcheck.1 \
diff --git a/man/footer.rst b/man/footer.rst
index a774f05..00416e6 100644
--- a/man/footer.rst
+++ b/man/footer.rst
@@ -9,14 +9,15 @@ SEE ALSO
--------

Ganeti overview and specifications: **ganeti**(7) (general overview),
-**ganeti-os-interface**(7) (guest OS definitions).
+**ganeti-os-interface**(7) (guest OS definitions),
+**ganeti-extstorage-interface**(7) (external storage providers).

Ganeti commands: **gnt-cluster**(8) (cluster-wide commands),
**gnt-job**(8) (job-related commands), **gnt-node**(8) (node-related
-commands), **gnt-instance**(8) (instance commands), **gnt-os**(8)
-(guest OS commands), **gnt-group**(8) (node group commands),
-**gnt-backup**(8) (instance import/export commands), **gnt-debug**(8)
-(debug commands).
+commands), **gnt-instance**(8) (instance commands), **gnt-os**(8) (guest
+OS commands), **gnt-storage**(8) (storage commands), **gnt-group**(8)
+(node group commands), **gnt-backup**(8) (instance import/export
+commands), **gnt-debug**(8) (debug commands).

Ganeti daemons: **ganeti-watcher**(8) (automatic instance restarter),
**ganeti-cleaner**(8) (job queue cleaner), **ganeti-noded**(8) (node
diff --git a/man/ganeti-extstorage-interface.rst b/man/ganeti-extstorage-interface.rst
new file mode 100644
index 0000000..7266ee4
--- /dev/null
+++ b/man/ganeti-extstorage-interface.rst
@@ -0,0 +1,212 @@
+ganeti-extstorage-interface(7) Ganeti | Version @GANETI_VERSION@
+================================================================
+
+Name
+----
+
+ganeti-extstorage-interface - Specifications for ExtStorage providers
+
+DESCRIPTION
+-----------
+
+The method for supporting external shared storage in Ganeti is to have
+an ExtStorage provider for each external shared storage hardware type.
+The ExtStorage provider is a set of files (executable scripts and text
+files), contained inside a directory which is 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). The external shared storage hardware should
+also be accessible by all nodes of this nodegroup too.
+
+REFERENCE
+---------
+
+There are seven required files: *create*, *attach*, *detach*, *remove*,
+*grow*, *verify* (executables) and *parameters.list* (text file).
+
+Common environment
+~~~~~~~~~~~~~~~~~~
+
+All commands will 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. Note that all counts are zero-based.
+
+Since Ganeti version 2.5, the environment will be cleaned up before
+being passed to scripts, therefore they will not inherit the environment
+in with which the ganeti node daemon was started. If you depend on any
+environment variables (non-Ganeti), then you will need to define or
+source them appropriately.
+
+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. Its
+ format is ``UUID.ext.diskX`` where ``UUID`` is produced by Ganeti
+ and is unique inside the Ganeti context. ``X`` is the number of the
+ disk count.
+
+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). To find the amount of grow,
+ the scipt should calculate the number VOL_NEW_SIZE - VOL_SIZE.
+
+EXTP_*name*
+ Each ExtStorage parameter (see below) will be exported in its own
+ variable, prefixed with ``EXTP_``, and upper-cased. For example, a
+ ``fromsnap`` parameter will be exported as ``EXTP_FROMSNAP``.
+
+EXECUTABLE SCRIPTS
+------------------
+
+
+create
+~~~~~~
+
+The **create** command is used for creating a new volume inside the
+external storage. The ``VOL_NAME`` denotes the volume's name, which
+should be unique. After creation, Ganeti will refer to this volume by
+this name for all other actions.
+
+Ganeti produces this name dynamically and ensures its uniqueness inside
+the Ganeti context. Therefore, you should make sure not to provision
+manually additional volumes inside the external storage with this type
+of name, because this will lead to conflicts and possible loss of data.
+
+The ``VOL_SIZE`` variable denotes the size of the new volume to be
+created in mebibytes.
+
+If the script ends successfully, a new volume of size ``VOL_SIZE``
+should exist inside the external storage. e.g:: a lun inside a NAS
+appliance.
+
+The script returns ``0`` on success.
+
+attach
+~~~~~~
+
+This command is used in order to make an already created volume visible
+to the physical node which will host the instance. This is done by
+mapping the already provisioned volume to a block device inside the host
+node.
+
+The ``VOL_NAME`` variable denotes the volume to be mapped.
+
+After successful attachment the script returns to its stdout a string,
+which is the full path of the block device to which the volume is
+mapped. e.g:: /dev/dummy1
+
+When attach returns, this path should be a valid block device on the
+host node.
+
+The attach script should be idempotent if the volume is already mapped.
+If the requested volume is already mapped, then the script should just
+return to its stdout the path which is already mapped to.
+
+detach
+~~~~~~
+
+This command is used in order to unmap an already mapped volume from the
+host node. Detach undoes everything attach did. This is done by
+unmapping the requested volume from the block device it is mapped to.
+
+The ``VOL_NAME`` variable denotes the volume to be unmapped.
+
+``detach`` doesn't affect the volume itself. It just unmaps it from the
+host node. The volume continues to exist inside the external storage.
+It's just not accessible by the node anymore. This script doesn't return
+anything to its stdout.
+
+The detach script should be idempotent if the volume is already
+unmapped. If the volume is not mapped, the script doesn't perform any
+action at all.
+
+The script returns ``0`` on success.
+
+remove
+~~~~~~
+
+This command is used to remove an existing volume from the external
+storage. The volume is permanently removed from inside the external
+storage along with all its data.
+
+The ``VOL_NAME`` variable denotes the volume to be removed.
+
+The script returns ``0`` on success.
+
+grow
+~~~~
+
+This command is used to grow an existing volume of the external storage.
+
+The ``VOL_NAME`` variable denotes the volume to grow.
+
+The ``VOL_SIZE`` variable denotes the current volume's size (in
+mebibytes). The ``VOL_NEW_SIZE`` variable denotes the final size after
+the volume has been grown (in mebibytes).
+
+The amount of grow can be easily calculated by the scipt and is:
+
+grow_amount = VOL_NEW_SIZE - VOL_SIZE (in mebibytes)
+
+Ganeti ensures that: ``VOL_NEW_SIZE`` > ``VOL_SIZE``
+
+If the script returns successfully, then the volume inside the external
+storage will have a new size of ``VOL_NEW_SIZE``. This isn't immediately
+reflected to the instance's disk. See ``gnt-instance grow`` for more
+details on when the running instance becomes aware of its grown disk.
+
+The script returns ``0`` on success.
+
+verify
+~~~~~~
+
+The *verify* script is used to verify consistency of the external
+parameters (ext-params) (see below). The command should take one or more
+arguments denoting what checks should be performed, and return a proper
+exit code depending on whether the validation failed or succeeded.
+
+Currently, the script is not invoked by Ganeti, but should be present
+for future use and consistency with gnt-os-interface's verify script.
+
+The script should return ``0`` on success.
+
+TEXT FILES
+----------
+
+
+parameters.list
+~~~~~~~~~~~~~~~
+
+This file declares the parameters supported by the ExtStorage provider,
+one parameter per line, with name and description (space and/or tab
+separated). For example::
+
+ fromsnap Snapshot name to create the volume from
+ nas_ip The IP of the NAS appliance
+
+The parameters can then be used during instance add as follows::
+
+ # gnt-instance add --disk=0:fromsnap="file_name",nas_ip="1.2.3.4" ...
+
+NOTES
+-----
+
+Backwards compatibility
+~~~~~~~~~~~~~~~~~~~~~~~
+
+The ExtStorage Interface was introduced in Ganeti 2.6.
+Ganeti 2.6 and up is compatible with the ExtStorage Interface.
+
+Common behaviour
+~~~~~~~~~~~~~~~~
+
+All the scripts should display an usage message when called with a wrong
+number of arguments or when the first argument is ``-h`` or ``--help``.
+
+.. vim: set textwidth=72 :
+.. Local Variables:
+.. mode: rst
+.. fill-column: 72
+.. End:
diff --git a/man/gnt-storage.rst b/man/gnt-storage.rst
new file mode 100644
index 0000000..9fb2325
--- /dev/null
+++ b/man/gnt-storage.rst
@@ -0,0 +1,63 @@
+gnt-storage(8) Ganeti | Version @GANETI_VERSION@
+================================================
+
+Name
+----
+
+gnt-storage - Ganeti storage administration
+
+Synopsis
+--------
+
+**gnt-storage** {command} [arguments...]
+
+DESCRIPTION
+-----------
+
+The **gnt-storage** is used for managing the available storage inside
+the Ganeti cluster. At the moment, it manages only external storage
+(ExtStorage).
+
+COMMANDS
+--------
+
+
+**diagnose**
+
+This command provides detailed information about the state of all
+ExtStorage providers available in the Ganeti cluster. The state of each
+provider is calculated per nodegroup. This means that a provider may be
+valid (meaning usable) for some nodegroups, and invalid (not usable) for
+some others. This command will help you see why an installed ExtStorage
+provider is not valid for a specific nodegroup. It could be that it is
+missing from a node, or is only partially installed. This command will
+show the details of all ExtStorage providers and the reasons they are or
+aren't valid for every nodegroup in the cluster.
+
+**info**
+
+This command will list detailed information about each ExtStorage
+provider found in the cluster, including its nodegroup validity, the
+supported parameters (if any) and their documentations, etc.
+
+For each ExtStorage provider only the valid nodegroups will be listed.
+
+If run with no arguments, it will display info for all ExtStorage
+providers found in the cluster. If given ExtStorage provider's names as
+arguments it will list info only for providers given.
+
+NOTES
+-----
+
+In the future **gnt-storage** can be extended to also handle internal
+storage (such as lvm, drbd, etc) and also provide diagnostics for them
+too.
+
+It can also be extended to handle internal and external storage pools,
+if/when this kind of abstraction is implemented to Ganeti.
+
+.. vim: set textwidth=72 :
+.. Local Variables:
+.. mode: rst
+.. fill-column: 72
+.. End:
--
1.7.2.5

[Constantinos Venetsanopoulos]

Update the shared storage design document to reflect the current
changes, after the implementation of the ExtStorage interface.

Signed-off-by: Constantinos Venetsanopoulos <cv...@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). 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
+
+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.
+
Long-term shared storage goals
==============================
Storage pool handling
@@ -166,7 +260,7 @@ Storage pool handling

A new cluster configuration attribute will be introduced, named
“storage_pools”, modeled as a dictionary mapping storage pools to
-external storage drivers (see below), e.g.::
+external storage providers (see below), e.g.::

{
"nas1": "foostore",
@@ -188,93 +282,19 @@ Furthermore, the storage pools will be used to indicate the availability
of storage pools to different node groups, thus specifying the
instances' “mobility domain”.

-New disk templates will also be necessary to facilitate the use of external
-storage. The proposed addition is a whole template namespace created by
-prefixing the pool names with a fixed string, e.g. “ext:”, forming names
-like “ext:nas1”, “ext:foo”.
-
-Interface to the external storage drivers
------------------------------------------
-
-In addition to external storage pools, a new interface will be
-introduced to allow external scripts to provision and manipulate shared
-storage.
-
-In order to provide storage provisioning and manipulation (e.g. growing,
-renaming) capabilities, each instance's disk template can possibly be
-associated with an external “storage driver” which, based on the
-instance's configuration and tags, will perform all supported storage
-operations using auxiliary means (e.g. XML-RPC, ssh, etc.).
-
-A “storage driver” will have to provide the following methods:
-
-- Create a disk
-- Remove a disk
-- Rename a disk
-- Resize a disk
-- Attach a disk to a given node
-- Detach a disk from a given node
-
-The proposed storage driver architecture borrows heavily from the OS
-interface and follows a one-script-per-function approach. A storage
-driver is expected to provide the following scripts:
-
-- `create`
-- `resize`
-- `rename`
-- `remove`
-- `attach`
-- `detach`
-
-These executables will be called once for each disk with no arguments
-and all required information will be passed through environment
-variables. The following environment variables will always be present on
-each invocation:
-
-- `INSTANCE_NAME`: The instance's name
-- `INSTANCE_UUID`: The instance's UUID
-- `INSTANCE_TAGS`: The instance's tags
-- `DISK_INDEX`: The current disk index.
-- `LOGICAL_ID`: The disk's logical id (if existing)
-- `POOL`: The storage pool the instance belongs to.
-
-Additional variables may be available in a per-script context (see
-below).
-
-Of particular importance is the disk's logical ID, which will act as
-glue between Ganeti and the external storage drivers; there are two
-possible ways of using a disk's logical ID in a storage driver:
-
-1. Simply use it as a unique identifier (e.g. UUID) and keep a separate,
- external database linking it to the actual storage.
-2. Encode all useful storage information in the logical ID and have the
- driver decode it at runtime.
-
-All scripts should return 0 on success and non-zero on error accompanied by
-an appropriate error message on stderr. Furthermore, the following
-special cases are defined:
-
-1. `create` In case of success, a string representing the disk's logical
- id must be returned on stdout, which will be saved in the instance's
- configuration and can be later used by the other scripts of the same
- storage driver. The logical id may be based on instance name,
- instance uuid and/or disk index.
-
- Additional environment variables present:
- - `DISK_SIZE`: The requested disk size in MiB
-
-2. `resize` In case of success, output the new disk size.
-
- Additional environment variables present:
- - `DISK_SIZE`: The requested disk size in MiB
-
-3. `rename` On success, a new logical id should be returned, which will
- replace the old one. This script is meant to rename the instance's
- backing store and update the disk's logical ID in case one of them is
- bound to the instance name.
+The pool, in which to put the new instance's disk, will be defined at
+the command line during `instance add`. This will become possible by
+replacing the IDISK_PROVIDER parameter with a new one, called `IDISK_POOL
+= pool`. The cmdlib logic will then look at the cluster-level mapping
+dictionary to determine the ExtStorage provider for the given pool.

- Additional environment variables present:
- - `NEW_INSTANCE_NAME`: The instance's new name.
+gnt-storage
+-----------

+The `gnt-storage` client can be extended to support pool management
+(creation/modification/deletion of pools, connection/disconnection of
+pools to nodegroups, etc.). It can also be extended to diagnose and
+provide information for internal disk templates too, such as lvm and
+drbd.

.. vim: set textwidth=72 :
--
1.7.2.5

[Constantinos Venetsanopoulos]

Add a new client called 'gnt-storage'.
The client interacts with the ExtStorage interface, similarly to
the way gnt-os interacts with the OS interface.

For now, only two commands are supported: 'info' and 'diagnose'.

'diagnose' calculates the node status of each provider on each node,
similarly to gnt-os diagnose. Furthermore, for every provider, it
calculates it's nodegroup validity for each nodegroup. This is done
inside the LU and not the client (marked as 'TODO' for the global
validity of gnt-os diagnose).

In the future, gnt-storage can be used to manage storage pools,
or even be extended to diagnose other storage types supported by
Ganeti, such as lvm, drbd (INT_MIRROR) or rbd (EXT_MIRROR).

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

Makefile.am | 6 +-
autotools/build-bash-completion | 4 +
lib/backend.py | 45 +++++++++
lib/cli.py | 12 ++-
lib/client/gnt_storage.py | 196 +++++++++++++++++++++++++++++++++++++++
lib/cmdlib.py | 154 ++++++++++++++++++++++++++++++
lib/constants.py | 2 +
lib/opcodes.py | 10 ++
lib/query.py | 34 +++++++
lib/rpc_defs.py | 7 +-
lib/server/noded.py | 9 ++
test/docs_unittest.py | 1 +
12 files changed, 476 insertions(+), 4 deletions(-)
create mode 100644 lib/client/gnt_storage.py

diff --git a/Makefile.am b/Makefile.am
index 99c1bc2..215c739 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -251,7 +251,8 @@ client_PYTHON = \
lib/client/gnt_instance.py \
lib/client/gnt_job.py \
lib/client/gnt_node.py \
- lib/client/gnt_os.py
+ lib/client/gnt_os.py \
+ lib/client/gnt_storage.py

hypervisor_PYTHON = \
lib/hypervisor/__init__.py \
@@ -542,7 +543,8 @@ gnt_scripts = \
scripts/gnt-instance \
scripts/gnt-job \
scripts/gnt-node \
- scripts/gnt-os
+ scripts/gnt-os \
+ scripts/gnt-storage

PYTHON_BOOTSTRAP_SBIN = \
daemons/ganeti-masterd \
diff --git a/autotools/build-bash-completion b/autotools/build-bash-completion
index 05ac04f..d14b40e 100755
--- a/autotools/build-bash-completion
+++ b/autotools/build-bash-completion
@@ -347,6 +347,8 @@ class CompletionWriter:
WriteCompReply(sw, "-W \"$(_ganeti_instances)\"", cur=cur)
elif suggest == cli.OPT_COMPL_ONE_OS:
WriteCompReply(sw, "-W \"$(_ganeti_os)\"", cur=cur)
+ elif suggest == cli.OPT_COMPL_ONE_EXTSTORAGE:
+ WriteCompReply(sw, "-W \"$(_ganeti_extstorage)\"", cur=cur)
elif suggest == cli.OPT_COMPL_ONE_IALLOCATOR:
WriteCompReply(sw, "-W \"$(_ganeti_iallocator)\"", cur=cur)
elif suggest == cli.OPT_COMPL_ONE_NODEGROUP:
@@ -453,6 +455,8 @@ class CompletionWriter:
choices = "$(_ganeti_jobs)"
elif isinstance(arg, cli.ArgOs):
choices = "$(_ganeti_os)"
+ elif isinstance(arg, cli.ArgExtStorage):
+ choices = "$(_ganeti_extstorage)"
elif isinstance(arg, cli.ArgFile):
choices = ""
compgenargs.append("-f")
diff --git a/lib/backend.py b/lib/backend.py
index 1e76c99..1864e90 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -2444,6 +2444,51 @@ def OSEnvironment(instance, inst_os, debug=0):
return result


+def DiagnoseExtStorage(top_dirs=None):
+ """Compute the validity for all ExtStorage Providers.
+
+ @type top_dirs: list
+ @param top_dirs: the list of directories in which to
+ search (if not given defaults to
+ L{pathutils.ES_SEARCH_PATH})
+ @rtype: list of L{objects.ExtStorage}
+ @return: a list of tuples (name, path, status, diagnose, parameters)
+ for all (potential) ExtStorage Providers under all
+ search paths, where:
+ - name is the (potential) ExtStorage Provider
+ - path is the full path to the ExtStorage Provider
+ - status True/False is the validity of the ExtStorage Provider
+ - diagnose is the error message for an invalid ExtStorage Provider,
+ otherwise empty
+ - parameters is a list of (name, help) parameters, if any
+
+ """
+ if top_dirs is None:
+ top_dirs = pathutils.ES_SEARCH_PATH
+
+ result = []
+ for dir_name in top_dirs:
+ if os.path.isdir(dir_name):
+ try:
+ f_names = utils.ListVisibleFiles(dir_name)
+ except EnvironmentError, err:
+ logging.exception("Can't list the ExtStorage directory %s: %s",
+ dir_name, err)
+ break
+ for name in f_names:
+ es_path = utils.PathJoin(dir_name, name)
+ status, es_inst = bdev.ExtStorageFromDisk(name, base_dir=dir_name)
+ if status:
+ diagnose = ""
+ parameters = es_inst.supported_parameters
+ else:
+ diagnose = es_inst
+ parameters = []
+ result.append((name, es_path, status, diagnose, parameters))

+
+ return result
+
+

def BlockdevGrow(disk, amount, dryrun, backingstore):
"""Grow a stack of block devices.

diff --git a/lib/cli.py b/lib/cli.py
index 5f003d1..2293ad3 100644
--- a/lib/cli.py
+++ b/lib/cli.py
@@ -250,6 +250,7 @@ __all__ = [
"ArgJobId",
"ArgNode",
"ArgOs",
+ "ArgExtStorage",
"ArgSuggest",
"ArgUnknown",
"OPT_COMPL_INST_ADD_NODES",
@@ -259,6 +260,7 @@ __all__ = [
"OPT_COMPL_ONE_NODE",
"OPT_COMPL_ONE_NODEGROUP",
"OPT_COMPL_ONE_OS",
+ "OPT_COMPL_ONE_EXTSTORAGE",
"cli_option",
"SplitNodeOption",
"CalculateOSNames",
@@ -392,6 +394,12 @@ class ArgOs(_Argument):
"""


+class ArgExtStorage(_Argument):
+ """ExtStorage argument.
+
+ """
+
+
ARGS_NONE = []
ARGS_MANY_INSTANCES = [ArgInstance()]
ARGS_MANY_NODES = [ArgNode()]
@@ -639,15 +647,17 @@ def check_maybefloat(option, opt, value): # pylint: disable=W0613
OPT_COMPL_ONE_NODE,
OPT_COMPL_ONE_INSTANCE,
OPT_COMPL_ONE_OS,
+ OPT_COMPL_ONE_EXTSTORAGE,
OPT_COMPL_ONE_IALLOCATOR,
OPT_COMPL_INST_ADD_NODES,
- OPT_COMPL_ONE_NODEGROUP) = range(100, 107)
+ OPT_COMPL_ONE_NODEGROUP) = range(100, 108)

OPT_COMPL_ALL = frozenset([
OPT_COMPL_MANY_NODES,
OPT_COMPL_ONE_NODE,
OPT_COMPL_ONE_INSTANCE,
OPT_COMPL_ONE_OS,
+ OPT_COMPL_ONE_EXTSTORAGE,
OPT_COMPL_ONE_IALLOCATOR,
OPT_COMPL_INST_ADD_NODES,
OPT_COMPL_ONE_NODEGROUP,
diff --git a/lib/client/gnt_storage.py b/lib/client/gnt_storage.py
new file mode 100644
index 0000000..09ac14e
--- /dev/null
+++ b/lib/client/gnt_storage.py
@@ -0,0 +1,196 @@
+#
+#
+
+# Copyright (C) 2012 Google Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.
+
+"""External Storage related commands"""
+
+# pylint: disable=W0401,W0613,W0614,C0103
+# W0401: Wildcard import ganeti.cli
+# W0613: Unused argument, since all functions follow the same API
+# W0614: Unused import %s from wildcard import (since we need cli)
+# C0103: Invalid name gnt-storage
+
+from ganeti.cli import *
+from ganeti import opcodes
+from ganeti import utils
+
+
+def ShowExtStorageInfo(opts, args):
+ """List detailed information about ExtStorage providers.
+
+ @param opts: the command line options selected by the user
+ @type args: list
+ @param args: empty list or list of ExtStorage providers' names
+ @rtype: int
+ @return: the desired exit code
+
+ """
+ op = opcodes.OpExtStorageDiagnose(output_fields=["name", "nodegroup_status",
+ "parameters"],
+ names=[])
+
+ result = SubmitOpCode(op, opts=opts)
+
+ if not result:
+ ToStderr("Can't get the ExtStorage providers list")
+ return 1
+
+ do_filter = bool(args)
+
+ for (name, nodegroup_data, parameters) in result:
+ if do_filter:
+ if name not in args:
+ continue
+ else:
+ args.remove(name)
+
+ nodegroups_valid = []
+ for nodegroup_name, nodegroup_status in nodegroup_data.iteritems():
+ if nodegroup_status:
+ nodegroups_valid.append(nodegroup_name)
+
+ ToStdout("%s:", name)
+
+ if nodegroups_valid != []:
+ ToStdout(" - Valid for nodegroups:")
+ for ndgrp in utils.NiceSort(nodegroups_valid):
+ ToStdout(" %s", ndgrp)
+ ToStdout(" - Supported parameters:")
+ for pname, pdesc in parameters:
+ ToStdout(" %s: %s", pname, pdesc)
+ else:
+ ToStdout(" - Invalid for all nodegroups")
+
+ ToStdout("")
+
+ if args:
+ for name in args:
+ ToStdout("%s: Not Found", name)
+ ToStdout("")
+
+ return 0
+
+
+def _ExtStorageStatus(status, diagnose):
+ """Beautifier function for ExtStorage status.
+
+ @type status: boolean
+ @param status: is the ExtStorage provider valid
+ @type diagnose: string
+ @param diagnose: the error message for invalid ExtStorages
+ @rtype: string
+ @return: a formatted status
+
+ """
+ if status:
+ return "valid"
+ else:
+ return "invalid - %s" % diagnose
+
+
+def DiagnoseExtStorage(opts, args):
+ """Analyse all ExtStorage providers.
+
+ @param opts: the command line options selected by the user
+ @type args: list
+ @param args: should be an empty list
+ @rtype: int
+ @return: the desired exit code
+
+ """
+ op = opcodes.OpExtStorageDiagnose(output_fields=["name", "node_status",
+ "nodegroup_status"],
+ names=[])
+
+ result = SubmitOpCode(op, opts=opts)
+
+ if not result:
+ ToStderr("Can't get the list of ExtStorage providers")
+ return 1
+
+ for provider_name, node_data, nodegroup_data in result:
+
+ nodes_valid = {}
+ nodes_bad = {}
+ nodegroups_valid = {}
+ nodegroups_bad = {}
+
+ # Per node diagnose
+ for node_name, node_info in node_data.iteritems():
+ if node_info: # at least one entry in the per-node list
+ (fo_path, fo_status, fo_msg, fo_params) = node_info.pop(0)
+ fo_msg = "%s (path: %s)" % (_ExtStorageStatus(fo_status, fo_msg),
+ fo_path)
+ if fo_params:
+ fo_msg += (" [parameters: %s]" %
+ utils.CommaJoin([v[0] for v in fo_params]))
+ else:
+ fo_msg += " [no parameters]"
+ if fo_status:
+ nodes_valid[node_name] = fo_msg
+ else:
+ nodes_bad[node_name] = fo_msg
+ else:
+ nodes_bad[node_name] = "ExtStorage provider not found"
+
+ # Per nodegroup diagnose
+ for nodegroup_name, nodegroup_status in nodegroup_data.iteritems():
+ status = nodegroup_status
+ if status:
+ nodegroups_valid[nodegroup_name] = "valid"
+ else:
+ nodegroups_bad[nodegroup_name] = "invalid"
+
+ def _OutputPerNodegroupStatus(msg_map):
+ map_k = utils.NiceSort(msg_map.keys())
+ for nodegroup in map_k:
+ ToStdout(" For nodegroup: %s --> %s", nodegroup,
+ msg_map[nodegroup])
+
+ def _OutputPerNodeStatus(msg_map):
+ map_k = utils.NiceSort(msg_map.keys())
+ for node_name in map_k:
+ ToStdout(" Node: %s, status: %s", node_name, msg_map[node_name])
+
+ # Print the output
+ st_msg = "Provider: %s" % provider_name
+ ToStdout(st_msg)
+ ToStdout("---")
+ _OutputPerNodeStatus(nodes_valid)
+ _OutputPerNodeStatus(nodes_bad)
+ ToStdout(" --")
+ _OutputPerNodegroupStatus(nodegroups_valid)
+ _OutputPerNodegroupStatus(nodegroups_bad)
+ ToStdout("")
+
+ return 0
+
+
+commands = {
+ "diagnose": (
+ DiagnoseExtStorage, ARGS_NONE, [PRIORITY_OPT],
+ "", "Diagnose all ExtStorage providers"),
+ "info": (
+ ShowExtStorageInfo, [ArgOs()], [PRIORITY_OPT],
+ "", "Show info about ExtStorage providers"),
+ }
+
+
+def Main():
+ return GenericMain(commands)
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 4554752..edfa1aa 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -4917,6 +4917,159 @@ class LUOsDiagnose(NoHooksLU):
return self.oq.OldStyleQuery(self)


+class _ExtStorageQuery(_QueryBase):
+ FIELDS = query.EXTSTORAGE_FIELDS
+
+ def ExpandNames(self, lu):
+ # Lock all nodes in shared mode
+ # Temporary removal of locks, should be reverted later
+ # TODO: reintroduce locks when they are lighter-weight
+ lu.needed_locks = {}
+ #self.share_locks[locking.LEVEL_NODE] = 1
+ #self.needed_locks[locking.LEVEL_NODE] = locking.ALL_SET
+
+ # The following variables interact with _QueryBase._GetNames
+ if self.names:
+ self.wanted = self.names
+ else:
+ self.wanted = locking.ALL_SET
+
+ self.do_locking = self.use_locking
+
+ def DeclareLocks(self, lu, level):
+ pass
+
+ @staticmethod
+ def _DiagnoseByProvider(rlist):
+ """Remaps a per-node return list into an a per-provider per-node dictionary
+
+ @param rlist: a map with node names as keys and ExtStorage objects as values
+
+ @rtype: dict
+ @return: a dictionary with extstorage providers as keys and as
+ value another map, with nodes as keys and tuples of
+ (path, status, diagnose, parameters) as values, eg::
+
+ {"provider1": {"node1": [(/usr/lib/..., True, "", [])]
+ "node2": [(/srv/..., False, "missing file")]
+ "node3": [(/srv/..., True, "", [])]
+ }
+
+ """
+ all_es = {}
+ # we build here the list of nodes that didn't fail the RPC (at RPC
+ # level), so that nodes with a non-responding node daemon don't
+ # make all OSes invalid
+ good_nodes = [node_name for node_name in rlist
+ if not rlist[node_name].fail_msg]
+ for node_name, nr in rlist.items():
+ if nr.fail_msg or not nr.payload:
+ continue
+ for (name, path, status, diagnose, params) in nr.payload:
+ if name not in all_es:
+ # build a list of nodes for this os containing empty lists
+ # for each node in node_list
+ all_es[name] = {}
+ for nname in good_nodes:
+ all_es[name][nname] = []
+ # convert params from [name, help] to (name, help)
+ params = [tuple(v) for v in params]
+ all_es[name][node_name].append((path, status, diagnose, params))
+ return all_es
+
+ def _GetQueryData(self, lu):
+ """Computes the list of nodes and their attributes.
+
+ """
+ # Locking is not used
+ assert not (compat.any(lu.glm.is_owned(level)
+ for level in locking.LEVELS
+ if level != locking.LEVEL_CLUSTER) or
+ self.do_locking or self.use_locking)
+
+ valid_nodes = [node.name
+ for node in lu.cfg.GetAllNodesInfo().values()
+ if not node.offline and node.vm_capable]
+ pol = self._DiagnoseByProvider(lu.rpc.call_extstorage_diagnose(valid_nodes))
+
+ data = {}
+
+ nodegroup_list = lu.cfg.GetNodeGroupList()
+
+ for (es_name, es_data) in pol.items():
+ # For every provider compute the nodegroup validity.
+ # To do this we need to check the validity of each node in es_data
+ # and then construct the corresponding nodegroup dict:
+ # { nodegroup1: status
+ # nodegroup2: status
+ # }
+ ndgrp_data = {}
+ for nodegroup in nodegroup_list:
+ ndgrp = lu.cfg.GetNodeGroup(nodegroup)
+
+ nodegroup_nodes = ndgrp.members
+ nodegroup_name = ndgrp.name
+ node_statuses = []
+
+ for node in nodegroup_nodes:
+ if node in valid_nodes:
+ if es_data[node] != []:
+ node_status = es_data[node][0][1]
+ node_statuses.append(node_status)
+ else:
+ node_statuses.append(False)
+
+ if False in node_statuses:
+ ndgrp_data[nodegroup_name] = False
+ else:
+ ndgrp_data[nodegroup_name] = True
+
+ # Compute the provider's parameters
+ parameters = set()
+ for idx, esl in enumerate(es_data.values()):
+ valid = bool(esl and esl[0][1])
+ if not valid:
+ break
+
+ node_params = esl[0][3]
+ if idx == 0:
+ # First entry
+ parameters.update(node_params)
+ else:
+ # Filter out inconsistent values
+ parameters.intersection_update(node_params)
+
+ params = list(parameters)
+
+ # Now fill all the info for this provider
+ info = query.ExtStorageInfo(name=es_name, node_status=es_data,
+ nodegroup_status=ndgrp_data,
+ parameters=params)
+
+ data[es_name] = info
+
+ # Prepare data in requested order
+ return [data[name] for name in self._GetNames(lu, pol.keys(), None)
+ if name in data]
+
+
+class LUExtStorageDiagnose(NoHooksLU):
+ """Logical unit for ExtStorage diagnose/query.
+
+ """
+ REQ_BGL = False
+
+ def CheckArguments(self):
+ self.eq = _ExtStorageQuery(qlang.MakeSimpleFilter("name", self.op.names),
+ self.op.output_fields, False)
+
+ def ExpandNames(self):
+ self.eq.ExpandNames(self)
+
+ def Exec(self, feedback_fn):
+ return self.eq.OldStyleQuery(self)
+
+
class LUNodeRemove(LogicalUnit):
"""Logical unit for removing a node.

@@ -15000,6 +15153,7 @@ _QUERY_IMPL = {
constants.QR_NODE: _NodeQuery,
constants.QR_GROUP: _GroupQuery,
constants.QR_OS: _OsQuery,
+ constants.QR_EXTSTORAGE: _ExtStorageQuery,
constants.QR_EXPORT: _ExportQuery,
}

diff --git a/lib/constants.py b/lib/constants.py
index 4847f43..8bde687 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -1607,6 +1607,7 @@ QR_GROUP = "group"
QR_OS = "os"
QR_JOB = "job"
QR_EXPORT = "export"
+QR_EXTSTORAGE = "extstorage"

#: List of resources which can be queried using L{opcodes.OpQuery}
QR_VIA_OP = frozenset([
@@ -1616,6 +1617,7 @@ QR_VIA_OP = frozenset([
QR_GROUP,
QR_OS,
QR_EXPORT,
+ QR_EXTSTORAGE,
])

#: List of resources which can be queried using Local UniX Interface
diff --git a/lib/opcodes.py b/lib/opcodes.py
index 92e7798..889ce0b 100644
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -1753,6 +1753,16 @@ class OpOsDiagnose(OpCode):
OP_RESULT = _TOldQueryResult


+# ExtStorage opcodes
+class OpExtStorageDiagnose(OpCode):
+ """Compute the list of external storage providers."""
+ OP_PARAMS = [
+ _POutputFields,
+ ("names", ht.EmptyList, ht.TListOf(ht.TNonEmptyString),
+ "Which ExtStorage Provider to diagnose"),
+ ]
+ OP_RESULT = _TOldQueryResult
+
# Exports opcodes
class OpBackupQuery(OpCode):
"""Compute the list of exported images."""
diff --git a/lib/query.py b/lib/query.py
index b922bf9..9f2daaa 100644
--- a/lib/query.py
+++ b/lib/query.py
@@ -2186,6 +2186,36 @@ def _BuildOsFields():
return _PrepareFieldList(fields, [])


+class ExtStorageInfo(objects.ConfigObject):

+ __slots__ = [
+ "name",

+ "node_status",
+ "nodegroup_status",
+ "parameters",
+ ]
+
+
+def _BuildExtStorageFields():
+ """Builds list of fields for extstorage provider queries.
+
+ """
+ fields = [
+ (_MakeField("name", "Name", QFT_TEXT, "ExtStorage provider name"),
+ None, 0, _GetItemAttr("name")),
+ (_MakeField("node_status", "NodeStatus", QFT_OTHER,
+ "Status from node"),
+ None, 0, _GetItemAttr("node_status")),
+ (_MakeField("nodegroup_status", "NodegroupStatus", QFT_OTHER,
+ "Overall Nodegroup status"),
+ None, 0, _GetItemAttr("nodegroup_status")),
+ (_MakeField("parameters", "Parameters", QFT_OTHER,
+ "ExtStorage provider parameters"),
+ None, 0, _GetItemAttr("parameters")),
+ ]
+
+ return _PrepareFieldList(fields, [])
+
+
def _JobUnavailInner(fn, ctx, (job_id, job)): # pylint: disable=W0613
"""Return L{_FS_UNAVAIL} if job is None.

@@ -2430,6 +2460,9 @@ GROUP_FIELDS = _BuildGroupFields()
#: Fields available for operating system queries
OS_FIELDS = _BuildOsFields()

+#: Fields available for extstorage provider queries
+EXTSTORAGE_FIELDS = _BuildExtStorageFields()
+
#: Fields available for job queries
JOB_FIELDS = _BuildJobFields()

@@ -2444,6 +2477,7 @@ ALL_FIELDS = {
constants.QR_LOCK: LOCK_FIELDS,
constants.QR_GROUP: GROUP_FIELDS,
constants.QR_OS: OS_FIELDS,
+ constants.QR_EXTSTORAGE: EXTSTORAGE_FIELDS,
constants.QR_JOB: JOB_FIELDS,
constants.QR_EXPORT: EXPORT_FIELDS,
}
diff --git a/lib/rpc_defs.py b/lib/rpc_defs.py
index 0b4f6a6..87c232c 100644
--- a/lib/rpc_defs.py
+++ b/lib/rpc_defs.py
@@ -434,6 +434,11 @@ _OS_CALLS = [
], None, _OsGetPostProc, "Returns an OS definition"),
]

+_EXTSTORAGE_CALLS = [
+ ("extstorage_diagnose", MULTI, None, constants.RPC_TMO_FAST, [], None, None,
+ "Request a diagnose of ExtStorage Providers"),
+ ]
+
_NODE_CALLS = [
("node_has_ip_address", SINGLE, None, constants.RPC_TMO_FAST, [
("address", None, "IP address"),
@@ -503,7 +508,7 @@ CALLS = {
"RpcClientDefault":
_Prepare(_IMPEXP_CALLS + _X509_CALLS + _OS_CALLS + _NODE_CALLS +
_FILE_STORAGE_CALLS + _MISC_CALLS + _INSTANCE_CALLS +
- _BLOCKDEV_CALLS + _STORAGE_CALLS),
+ _BLOCKDEV_CALLS + _STORAGE_CALLS + _EXTSTORAGE_CALLS),
"RpcClientJobQueue": _Prepare([
("jobqueue_update", MULTI, None, constants.RPC_TMO_URGENT, [
("file_name", None, None),
diff --git a/lib/server/noded.py b/lib/server/noded.py
index 7a41449..ca251d0 100644
--- a/lib/server/noded.py
+++ b/lib/server/noded.py
@@ -835,6 +835,15 @@ class NodeRequestHandler(http.server.HttpServerHandler):
required, name, checks, params = params
return backend.ValidateOS(required, name, checks, params)

+ # extstorage -----------------------
+
+ @staticmethod
+ def perspective_extstorage_diagnose(params):
+ """Query detailed information about existing extstorage providers.
+
+ """
+ return backend.DiagnoseExtStorage()
+
# hooks -----------------------

@staticmethod
diff --git a/test/docs_unittest.py b/test/docs_unittest.py
index d91976c..7d71fd9 100755
--- a/test/docs_unittest.py
+++ b/test/docs_unittest.py
@@ -58,6 +58,7 @@ RAPI_OPCODE_EXCLUDE = frozenset([
opcodes.OpTagsSearch,
opcodes.OpClusterActivateMasterIp,
opcodes.OpClusterDeactivateMasterIp,
+ opcodes.OpExtStorageDiagnose,

# Difficult if not impossible
opcodes.OpClusterDestroy,
--
1.7.2.5

[Iustin Pop]

How will Ganeti behave if they are not consistent? Report errors? (in
cluster verify?) Ignore the provider? Etc.

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

[Constantinos Venetsanopoulos]

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?
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?

>
>> +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 ;-).

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.

Thanks,
Constantinos

[Iustin Pop]

Sounds very good, thanks!

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 :)

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.

Which is all fine, now that I thought it through, just something that
we need to keep in mind.

> >>+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 ;-).
>
> 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.

Sounds good!

thanks a lot.

iustin

[Constantinos Venetsanopoulos]

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). We should move the decision logic from operating at instance
level, to operating at the aggregation of the instance's disks Storage
Pools. At that point, we also don't have a problem with providers,
because providers will be moved from an IDISK_PARAM (which we
need right now as a transition level) to a parameter of the Storage
Pool. Thus, the decision logic will not need to know anything about
providers as it doesn't need to know now.

As you say, we will move from:

"disk_template in constants.DTS_MIRRORED"

to:

"are all the instance's disks Storage Pools connected to the
nodegroup we want to migrate/failover/move".
For Storage Pools of disk template EXT_MIRROR that's all,
for Storage Pools of disk template INT_MIRROR (DRBD)
we will have to adjust the current code that handles the
secondary node.

>
> Which is all fine, now that I thought it through, just something that
> we need to keep in mind.

Sure. Sounds really good you find that fine, and I think that with a
little more effort in the decision logic (when we move to Storage
Pools), we will result with a very simple and unified design that will
give even more functionality to Ganeti.

Thanks,
Constantinos

[Iustin Pop]

On Wed, Sep 26, 2012 at 05:38:18PM +0300, Constantinos Venetsanopoulos wrote:
> With this commit we introduce the External Storage Interface
> to Ganeti, abbreviated: ExtStorage Interface.
>
> 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.

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?

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?
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? 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?

This function makes no check that action is a valid ES_ACTION_*
constant, and as such this indexing into the inst_es could fail.

> + # Run the external script
> + result = utils.RunCmd([script], env=create_env,
> + cwd=inst_es.path, output=logfile,)
> + if result.failed:
> + logging.error("External storage's %s command '%s' returned"
> + " error: %s, logfile: %s, output: %s",
> + action, result.cmd, result.fail_reason,
> + logfile, result.output)
> +
> + # If logfile is 'None' (during attach), it breaks TailFile
> + # TODO: have a log file for attach too
> + if action is not constants.ES_ACTION_ATTACH:
> + lines = [utils.SafeEncode(val)
> + for val in utils.TailFile(logfile, lines=20)]
> + _ThrowError("External storage's %s script failed (%s), last"
> + " lines in the log file:\n%s",
> + action, result.fail_reason, "\n".join(lines))
> + else:
> + _ThrowError("External storage's %s script failed (%s)",
> + action, result.fail_reason)

Couldn't you use result.out here and thus make a single _ThrowError
call?

if action … :
lines = tailfile(…)
else:
lines = result.output[-20:]
_ThrowError(…)

> + if action == constants.ES_ACTION_ATTACH:
> + return result.stdout
> +
> +
> +def ExtStorageFromDisk(name, base_dir=None):
> + """Create an ExtStorage instance from disk.
> +
> + This function will return an ExtStorage instance
> + if the given name is a valid ExtStorage name.
> +
> + @type base_dir: string
> + @keyword base_dir: Base directory containing ExtStorage installations.
> + Defaults to a search in all the ES_SEARCH_PATH dirs.
> + @rtype: tuple
> + @return: True and the ExtStorage instance if we find a valid one, or
> + False and the diagnose message on error
> +
> + """
> + if base_dir is None:
> + es_dir = utils.FindFile(name, pathutils.ES_SEARCH_PATH, os.path.isdir)
> + else:
> + es_dir = utils.FindFile(name, [base_dir], os.path.isdir)

Please call FindFile only once, outside of the if, and in the if assign
a local variable (if this is copied from the OS code, we should clean
that too, in another patch).

No ' please. All string quoting must be " (here and in any other cases).

Should this "ext" be LD_EXT? Applies to the other constants, true.

> else:
> raise errors.ProgrammerError("Unknown disk template '%s'" % template_name)
>
> @@ -10017,6 +10021,9 @@ class LUInstanceCreate(LogicalUnit):
> # Any function that checks prerequisites can be placed here.
> # Check if there is enough space on the RADOS cluster.
> _CheckRADOSFreeSpace()
> + elif self.op.disk_template == constants.DT_EXT:
> + # FIXME: Function that checks prereqs if needed
> + pass
> else:
> # Check lv size requirements, if not adopting
> req_sizes = _ComputeDiskSizePerVG(self.op.disk_template, self.disks)
> @@ -11725,7 +11732,8 @@ class LUInstanceGrowDisk(LogicalUnit):
>
> if instance.disk_template not in (constants.DT_FILE,
> constants.DT_SHARED_FILE,
> - constants.DT_RBD):
> + constants.DT_RBD,
> + constants.DT_EXT):

Hmm, I wonder if here and in similar places we shouldn't migrate to more
constants, rather than ad-hoc sets.

Rest LGTM.

thanks,
iustin

[Iustin Pop]

On Wed, Sep 26, 2012 at 05:38:20PM +0300, Constantinos Venetsanopoulos wrote:
> The option is added to allow us the passing of arbitrary ext-params
> during disk addition (gnt-instance modify --disk add: ...), same as
> we do during instance add.
>
> 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 <cv...@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"?

thanks,
iustin

[Constantinos Venetsanopoulos]

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.

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).

>
> 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?

You are right. Will change that to check correctly.

Guess I can do that. Yes.

Indeed, this is copied from the OS code. Will change it. Then, it should
be cleaned in the the OS code too, with another patch.

Yes! You are right. Must have slipped. Will fix everywhere.

I just copied the above code. In any way, this gets overwritten by the
next commit.

>> else:
>> raise errors.ProgrammerError("Unknown disk template '%s'" % template_name)
>>
>> @@ -10017,6 +10021,9 @@ class LUInstanceCreate(LogicalUnit):
>> # Any function that checks prerequisites can be placed here.
>> # Check if there is enough space on the RADOS cluster.
>> _CheckRADOSFreeSpace()
>> + elif self.op.disk_template == constants.DT_EXT:
>> + # FIXME: Function that checks prereqs if needed
>> + pass
>> else:
>> # Check lv size requirements, if not adopting
>> req_sizes = _ComputeDiskSizePerVG(self.op.disk_template, self.disks)
>> @@ -11725,7 +11732,8 @@ class LUInstanceGrowDisk(LogicalUnit):
>>
>> if instance.disk_template not in (constants.DT_FILE,
>> constants.DT_SHARED_FILE,
>> - constants.DT_RBD):
>> + constants.DT_RBD,
>> + constants.DT_EXT):
> Hmm, I wonder if here and in similar places we shouldn't migrate to more
> constants, rather than ad-hoc sets.

Seems reasonable. We can think about it as a future patch.

> Rest LGTM.
>
> thanks,
> iustin

Once we decide on the design issues in the beginning, I'll send an
interdiff which fixes all the other issues.

Thanks,
Constantinos

[Constantinos Venetsanopoulos]

On 09/27/2012 04:26 PM, Iustin Pop wrote:
> On Wed, Sep 26, 2012 at 05:38:20PM +0300, Constantinos Venetsanopoulos wrote:
>> The option is added to allow us the passing of arbitrary ext-params
>> during disk addition (gnt-instance modify --disk add: ...), same as
>> we do during instance add.
>>
>> 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 <cv...@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?

>
> 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"?

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.

Thanks,
Constantinos

> thanks,
> iustin

[Iustin Pop]

Hah, sorry, I didn't read those designs except very briefly :)

> We should move the decision logic from operating at instance
> level, to operating at the aggregation of the instance's disks Storage
> Pools. At that point, we also don't have a problem with providers,
> because providers will be moved from an IDISK_PARAM (which we
> need right now as a transition level) to a parameter of the Storage
> Pool. Thus, the decision logic will not need to know anything about
> providers as it doesn't need to know now.
>
> As you say, we will move from:
>
> "disk_template in constants.DTS_MIRRORED"
>
> to:
>
> "are all the instance's disks Storage Pools connected to the
> nodegroup we want to migrate/failover/move".
> For Storage Pools of disk template EXT_MIRROR that's all,
> for Storage Pools of disk template INT_MIRROR (DRBD)
> we will have to adjust the current code that handles the
> secondary node.

Sounds good.

> >Which is all fine, now that I thought it through, just something that
> >we need to keep in mind.
>
> Sure. Sounds really good you find that fine, and I think that with a
> little more effort in the decision logic (when we move to Storage
> Pools), we will result with a very simple and unified design that will
> give even more functionality to Ganeti.

Indeed.

thanks,
iustin

[Iustin Pop]

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?

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?

Ack.

Ah OK, sounds good then.

Thanks, sounds good.

iustin

[Iustin Pop]

On Thu, Sep 27, 2012 at 05:51:05PM +0300, Constantinos Venetsanopoulos wrote:
> On 09/27/2012 04:26 PM, Iustin Pop wrote:
> >On Wed, Sep 26, 2012 at 05:38:20PM +0300, Constantinos Venetsanopoulos wrote:
> >>The option is added to allow us the passing of arbitrary ext-params
> >>during disk addition (gnt-instance modify --disk add: ...), same as
> >>we do during instance add.
> >>
> >>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 <cv...@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?

Sure, no problem.

> >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"?
>
> 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.

Hmm, I read the code again and I am still undecided.

If I understood the patch correctly, the problem it tries to solve is
that during the _opcode_ validation, which is done outside of the LU
context, we don't have access to the disk template, hence no way to
check the parameters to the opcode. And you fix this by adding a
parameter which directs the code to use the extended disk template
specific code.

If this is correct, then I'm not sure it's the best solution. Since we
can't do actual validation at the opcode layer, then maybe we shouldn't;
what about delaying validation until we are in an LU context, and we
have access to the instance? Yes, the job will fail later, but that
would seem cleaner than "propagating" the disk template into the opcode
layer.

Thoughts?

thanks,
iustin

[Constantinos Venetsanopoulos]

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.

>
>> 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.

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.

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.

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?

[Iustin Pop]

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!

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.

thanks!
iustin

[Constantinos Venetsanopoulos]

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?

Constantinos

[Iustin Pop]

Sounds good, thanks!

iustin

[Constantinos Venetsanopoulos]

diff --git a/lib/bdev.py b/lib/bdev.py
index b0ed7c0..7a27b7b 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -2797,8 +2797,24 @@ class ExtStorageDevice(BlockDev):
_ExtStorageAction(constants.ES_ACTION_GROW, self.unique_id,
str(self.size), grow=str(new_size))

+ def SetInfo(self, text):
+ """Update metadata with info text.

-def _ExtStorageAction(action, unique_id, size=None, grow=None):
+ """
+ # Replace invalid characters
+ text = re.sub("^[^A-Za-z0-9_+.]", "_", text)
+ text = re.sub("[^-A-Za-z0-9_+.]", "_", text)
+
+ # Only up to 128 characters are allowed
+ text = text[:128]
+
+ # Call the External Storage's setinfo script,
+ # to set metadata for an existing Volume inside the External Storage
+ _ExtStorageAction(constants.ES_ACTION_SETINFO, self.unique_id,
+ metadata=text)
+
+
+def _ExtStorageAction(action, unique_id, size=None, grow=None, metadata=None):

"""Take an External Storage action.

Take an External Storage action concerning or affecting

@@ -2814,6 +2830,8 @@ def _ExtStorageAction(action, unique_id, size=None, grow=None):

@param size: the size of the Volume in mebibytes

@type grow: integer

@param grow: the new size in mebibytes (after grow)

+ @type metadata: string
+ @param metadata: metadata info of the Volume, for use by the provider

@rtype: None or a block device path (during attach)

"""

@@ -2825,7 +2843,7 @@ def _ExtStorageAction(action, unique_id, size=None, grow=None):
_ThrowError("%s" % inst_es)

# Create the basic environment for the driver's scripts

- create_env = _ExtStorageEnvironment(unique_id, size, grow)
+ create_env = _ExtStorageEnvironment(unique_id, size, grow, metadata)

# Do not use log file for action `attach' as we need

# to get the output from RunResult

@@ -2834,6 +2852,12 @@ def _ExtStorageAction(action, unique_id, size=None, grow=None):

if action is not constants.ES_ACTION_ATTACH:

logfile = _VolumeLogName(action, driver, vol_name)

+ # Make sure the given action results in a valid script
+ target_script = action
+ if target_script not in constants.ES_SCRIPTS:
+ _ThrowError("Action '%s' doesn't result in a valid ExtStorage script" %
+ action)

+
# Find out which external script to run according the given action

script_name = action + "_script"

script = getattr(inst_es, script_name)
@@ -2852,12 +2876,12 @@ def _ExtStorageAction(action, unique_id, size=None, grow=None):

if action is not constants.ES_ACTION_ATTACH:

lines = [utils.SafeEncode(val)

for val in utils.TailFile(logfile, lines=20)]

- _ThrowError("External storage's %s script failed (%s), last"
- " lines in the log file:\n%s",
- action, result.fail_reason, "\n".join(lines))
else:
- _ThrowError("External storage's %s script failed (%s)",
- action, result.fail_reason)
+ lines = result.output[-20:]
+

+ _ThrowError("External storage's %s script failed (%s), last"

+ " lines of output:\n%s",

+ action, result.fail_reason, "\n".join(lines))

if action == constants.ES_ACTION_ATTACH:
return result.stdout
@@ -2878,9 +2902,11 @@ def ExtStorageFromDisk(name, base_dir=None):

"""
if base_dir is None:
- es_dir = utils.FindFile(name, pathutils.ES_SEARCH_PATH, os.path.isdir)
+ es_base_dir = pathutils.ES_SEARCH_PATH
else:
- es_dir = utils.FindFile(name, [base_dir], os.path.isdir)
+ es_base_dir = [base_dir]
+
+ es_dir = utils.FindFile(name, es_base_dir, os.path.isdir)

if es_dir is None:

return False, ("Directory for External Storage Provider %s not"

@@ -2915,17 +2941,22 @@ def ExtStorageFromDisk(name, base_dir=None):

remove_script=es_files[constants.ES_SCRIPT_REMOVE],
grow_script=es_files[constants.ES_SCRIPT_GROW],
attach_script=es_files[constants.ES_SCRIPT_ATTACH],
- detach_script=es_files[constants.ES_SCRIPT_DETACH])
+ detach_script=es_files[constants.ES_SCRIPT_DETACH],

+ setinfo_script=es_files[constants.ES_SCRIPT_SETINFO])

return True, es_obj


-def _ExtStorageEnvironment(unique_id, size=None, grow=None):

+def _ExtStorageEnvironment(unique_id, size=None, grow=None, metadata=None):

"""Calculate the environment for an External Storage script.

@type unique_id: tuple (driver, vol_name)

@param unique_id: ExtStorage pool and name of the Volume

- @type size: integer
- @param size: size of the Volume in mebibytes
+ @type size: string
+ @param size: size of the Volume (in mebibytes)
+ @type grow: string
+ @param grow: new size of Volume after grow (in mebibytes)
+ @type metadata: string
+ @param metadata: metadata info of the Volume
@rtype: dict

@return: dict of environment variables

@@ -2933,13 +2964,16 @@ def _ExtStorageEnvironment(unique_id, size=None, grow=None):
vol_name = unique_id[1]

result = {}
- result['VOL_NAME'] = vol_name
+ result["VOL_NAME"] = vol_name

if size is not None:

- result['VOL_SIZE'] = size
+ result["VOL_SIZE"] = size

if grow is not None:

- result['VOL_NEW_SIZE'] = grow

+ result["VOL_NEW_SIZE"] = grow

+
+ if metadata is not None:
+ result["VOL_METADATA"] = metadata

return result

diff --git a/lib/constants.py b/lib/constants.py
index 1cb7196..942859b 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -615,18 +615,21 @@ ES_ACTION_REMOVE = "remove"

ES_ACTION_GROW = "grow"
ES_ACTION_ATTACH = "attach"
ES_ACTION_DETACH = "detach"

+ES_ACTION_SETINFO = "setinfo"

ES_SCRIPT_CREATE = ES_ACTION_CREATE
ES_SCRIPT_REMOVE = ES_ACTION_REMOVE
ES_SCRIPT_GROW = ES_ACTION_GROW
ES_SCRIPT_ATTACH = ES_ACTION_ATTACH
ES_SCRIPT_DETACH = ES_ACTION_DETACH

+ES_SCRIPT_SETINFO = ES_ACTION_SETINFO

ES_SCRIPTS = frozenset([
ES_SCRIPT_CREATE,
ES_SCRIPT_REMOVE,
ES_SCRIPT_GROW,
ES_SCRIPT_ATTACH,
- ES_SCRIPT_DETACH
+ ES_SCRIPT_DETACH,

+ ES_SCRIPT_SETINFO
])

# ssh constants
@@ -2042,7 +2045,6 @@ VALID_ALLOC_POLICIES = [

# Temporary external/shared storage parameters
BLOCKDEV_DRIVER_MANUAL = "manual"

-EXTSTORAGE_SAMPLE_PROVIDER = "rbd"

# qemu-img path, required for ovfconverter
QEMUIMG_PATH = _autoconf.QEMUIMG_PATH

diff --git a/lib/objects.py b/lib/objects.py
index da5bc5b..9332afc 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -1244,6 +1244,7 @@ class ExtStorage(ConfigObject):

"grow_script",
"attach_script",
"detach_script",

+ "setinfo_script",
]

[Constantinos Venetsanopoulos]

Hi iustin,

I just sent you the interdiff. After rebasing, there is a trivial conflict
when applying the next commit (2/7). I can resend it too, if you want.

Finally, I'll also update the 'shared-filer' ExtStorage provider I sent you,
to include a setinfo script.

Regards,
Constantinos

[Constantinos Venetsanopoulos]

On 10/15/2012 6:58 PM, Constantinos Venetsanopoulos wrote:
> Hi iustin,
>
> I just sent you the interdiff. After rebasing, there is a trivial
> conflict
> when applying the next commit (2/7). I can resend it too, if you want.

I meant '3/7' here: Multiple ExtStorage providers

cven

[Iustin Pop]

On Mon, Oct 15, 2012 at 08:18:33PM +0300, Constantinos Venetsanopoulos wrote:
> On 10/15/2012 6:58 PM, Constantinos Venetsanopoulos wrote:
> >Hi iustin,
> >
> >I just sent you the interdiff. After rebasing, there is a trivial
> >conflict
> >when applying the next commit (2/7). I can resend it too, if you want.
>
> I meant '3/7' here: Multiple ExtStorage providers

The interdiff looks good. I'd appreciate if you either resend both 2/7
and 3/7, or point me to a git tree where I can pull them from.

Also, should the design doc, i.e. 1/7, need to be slightly updated with
the setinfo script?

thanks!
iustin

[Iustin Pop]

I know understand better patch 4/7.

While we might end up with the interface you currently propose based on
practicality of CLI input, I would like to raise some concerns on it.

The problem is that we're mixing here two things which we shouldn't.
Basically the structure is:

{ size: Int,
mode: ReadWrite,
extp: { }
}

But at cli level (and, I believe, at opcode level too) we're flattening
this structure. This leads to the complications in 4/7 w.r.t. the
validation of disk options, and to the impossibility, for example, that
we have an extp with the name 'size' (ignoring case for the moment).

So I'm wondering if we couldn't separate, both at CLI and opcode level,
these two. Maybe having --disk=0:size=2G,params=<some other format> or
--disk 0:size=2G --dparams 0:p1=v1,p2=v2 or anything else that clearly
separates them.

Just thinking out loud…

iustin

[Constantinos Venetsanopoulos]

On 10/16/2012 03:45 PM, Iustin Pop wrote:
> On Mon, Oct 15, 2012 at 08:18:33PM +0300, Constantinos Venetsanopoulos wrote:
>> On 10/15/2012 6:58 PM, Constantinos Venetsanopoulos wrote:
>>> Hi iustin,
>>>
>>> I just sent you the interdiff. After rebasing, there is a trivial
>>> conflict
>>> when applying the next commit (2/7). I can resend it too, if you want.
>> I meant '3/7' here: Multiple ExtStorage providers
> The interdiff looks good. I'd appreciate if you either resend both 2/7
> and 3/7, or point me to a git tree where I can pull them from.

You can pull from:

https://code.grnet.gr/git/ganeti-local

branch 'master-extstorage-upstream2'

You can squash commits 0f247c10 and 8237d5eb with 2/7,
when merging. I kept them separate to easy the review.

>
> Also, should the design doc, i.e. 1/7, need to be slightly updated with
> the setinfo script?

Of course. As I already said in the previous mail (2012/10/10) I will
be sending a separate patch after everything gets merged, that will
update the design doc and corresponding man pages to include the
setinfo functionality.

Regards,
Constantinos

>
> thanks!
> iustin

[Iustin Pop]

On Tue, Oct 16, 2012 at 04:53:03PM +0300, Constantinos Venetsanopoulos wrote:
> On 10/16/2012 03:45 PM, Iustin Pop wrote:
> >On Mon, Oct 15, 2012 at 08:18:33PM +0300, Constantinos Venetsanopoulos wrote:
> >>On 10/15/2012 6:58 PM, Constantinos Venetsanopoulos wrote:
> >>>Hi iustin,
> >>>
> >>>I just sent you the interdiff. After rebasing, there is a trivial
> >>>conflict
> >>>when applying the next commit (2/7). I can resend it too, if you want.
> >>I meant '3/7' here: Multiple ExtStorage providers
> >The interdiff looks good. I'd appreciate if you either resend both 2/7
> >and 3/7, or point me to a git tree where I can pull them from.
>
> You can pull from:
>
> https://code.grnet.gr/git/ganeti-local
>
> branch 'master-extstorage-upstream2'
>
> You can squash commits 0f247c10 and 8237d5eb with 2/7,
> when merging. I kept them separate to easy the review.

Thanks, sounds good.

> >Also, should the design doc, i.e. 1/7, need to be slightly updated with
> >the setinfo script?
>
> Of course. As I already said in the previous mail (2012/10/10) I will
> be sending a separate patch after everything gets merged, that will
> update the design doc and corresponding man pages to include the
> setinfo functionality.

Ah, missed that, sorry. Sounds good.

iustin

[Constantinos Venetsanopoulos]

I had thought about these points (!) a lot of times before writing the
code and these were the reasons that got me thinking about Storage Pools
initially. It's great to see we are aligned in such a way.

As you suggest, I had also thought of the exact two solutions you propose.
However, the complication comes from the current design at opcode
and lu level, rather than the CLI. Specifically:

Currently we have the following IDISK_PARAMS:

size, mode, adopt, vg, metavg

1. 'size', 'mode' and 'adopt' are indeed separate "disk parameters"
2. 'vg' and 'metavg' are actually "template parameters"
3. 'metavg' is a "template parameter" and is already included in DRBD's "disk-params"
4. 'vg' is not included in DRBD's "disk-params", although it probably should
(as discussed in the Storage Pools thread)

The above is a bit confusing design-wise imho, and flattening already
happens at cli level; that's why I kept the ext-params there as well
(for practicality).

I've given these issues quite some thought [that's why I delayed my
reply] to find the best way to handle them, also having in mind your and
Guido's comments on Storage Pools. This is what I've come up with so far:

* Some "template-params" need to stay the same at nodegroup level
(e.g.: DRBD_NET_CUSTOM), and some need to be changed at disk/instance
level (e.g.: DRBD_DEFAULT_METAVG and 'vg')
* Also ext-params for the 'ext' template need to be set at disk level

I would propose the following:

For now:
- Keep the current ext-params interface for practicality

As a next step to pair with the Storage Pools implementation:
- Rename current "disk-params" to "template-params" to reflect their exact use.
- Make 'vg' a "template-param" for LVM and DRBD and remove it from cluster init.
(this also solves the problem that setting the vg_name during cluster init
doesn't also set the metavg correctly)
- Introduce a new variable for each template (e.g. for drbd: "DRBD_DYNAMIC_PARAMS")
modeled as a frozenset containing the template's "template-params" that can
be modifiable at disk level
(for drbd these would be the 'DRBD_DEFAULT_METAVG' = 'metavg' and 'DRBD_DEFAULT_VG(?)' = 'vg',
for rbd that could be 'RBD_POOL' inside 'RBD_DYNAMIC_PARAMS').
- Keep "template-params" inheritance intact at cluster and nodegroup level.
- Introduce Storage Pools
- After the above, IDISK_PARAMS will end up containing only the following:
size, mode, adopt, pool (which are all disk level specific)
- Allow setting ONLY the modifiable *_DYNAMIC_PARAMS at Storage Pool level
which will override the corresponding nodegroup level "template-param".
- Just connect Storage Pools to nodegroups without modifying parameters during
connect (as initially proposed by Guido).

Then we will be able to even make ext template's 'provider' a modifiable
"template-param", with the constraint that in an 'ext' Storage Pool this
parameter should ALWAYS be set.

Finally, we can rethink how to display ext-params at CLI, after we give them
more thought at the Storage Pool level, and decide exactly how they will find
their way down to bdev. Maybe then, they can get their own CLI option (e.g.,
--dparams), but I think we should re-discuss that after
the design/implementation of Storage Pools.

What do you think? Looking forward to your comments,

Constantinos