On 09/27/2012 04:17 PM, Iustin Pop wrote:
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.
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?
Signed-off-by: Constantinos Venetsanopoulos <[email protected]>
---
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)
This function makes no check that action is a valid ES_ACTION_*
constant, and as such this indexing into the inst_es could fail.
You are right. Will change that to check correctly.
+ # 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(…)
Guess I can do that. Yes.
+ 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).
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.
+ 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
No ' please. All string quoting must be " (here and in any other cases).
Yes! You are right. Must have slipped. Will fix everywhere.
+ 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])
Should this "ext" be LD_EXT? Applies to the other constants, true.
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
