On Tue, Jul 06, 2010 at 03:03:27PM +0300, Apollon Oikonomopoulos wrote:
> Hi,
>
> Following patch 06, I changed the way bdev_list is called during disk
> adoption.
> Also, gone are all DRBD-related methods (GetSyncStatus(), AttachNet(),
> DisconnectNet()).
>
> The revised patch follows.
Looks good, just a few small comments:
> diff --git a/lib/bdev.py b/lib/bdev.py
> index c321508..2734af6 100644
> --- a/lib/bdev.py
> +++ b/lib/bdev.py
> @@ -1,4 +1,4 @@
> -#
> +
> #
I think this is an extra change…
> # Copyright (C) 2006, 2007 Google Inc.
> @@ -26,6 +26,7 @@ import time
> import errno
> import pyparsing as pyp
> import os
> +import stat
> import logging
>
> from ganeti import utils
> @@ -1954,10 +1955,112 @@ class FileStorage(BlockDev):
>
> return FileStorage(unique_id, children, size)
>
> +class PersistentBlockDevice(BlockDev):
> + """A block device with persistent node
> +
> + May be either directly attached, or exposed through DM (e.g. dm-multipath).
> + udev helpers are probably required to give persistent, human-friendly
> + names.
> +
> + For the time being, pathnames are required to lie under /dev.
> +
> + """
> + def __init__(self, unique_id, children, size):
> + """Attaches to a static block device.
> +
> + The unique_id is a path under /dev.
> +
> + """
> + super(PersistentBlockDevice, self).__init__(unique_id, children, size)
> + if not isinstance(unique_id, (tuple, list)):
> + raise ValueError("Invalid configuration data %s" % str(unique_id))
> + self.dev_path = unique_id[1]
Hmm, why unique_id[1]? What does unique_id[0] contain?
> + if not os.path.realpath(self.dev_path).startswith('/dev/'):
> + raise ValueError("Full path '%s' lies outside /dev" %
> + os.path.realpath(self.dev_path))
This ('/dev/') needs to be abstracted into a constant, that should be
also used for the comments about the path verification in patch 6/14 (in
backend.py).
> diff --git a/lib/objects.py b/lib/objects.py
> index d6cf527..6d26bd7 100644
> --- a/lib/objects.py
> +++ b/lib/objects.py
> @@ -396,11 +396,13 @@ class Disk(ConfigObject):
>
> def CreateOnSecondary(self):
> """Test if this device needs to be created on a secondary node."""
> - return self.dev_type in (constants.LD_DRBD8, constants.LD_LV)
> + return self.dev_type in (constants.LD_DRBD8, constants.LD_LV,
> + constants.LD_BLOCK)
>
> def AssembleOnSecondary(self):
> """Test if this device needs to be assembled on a secondary node."""
> - return self.dev_type in (constants.LD_DRBD8, constants.LD_LV)
> + return self.dev_type in (constants.LD_DRBD8, constants.LD_LV,
> + constants.LD_BLOCK)
Not sure about these two (CreateOnSecondary and AssembleOnSecondary), as
LD_BLOCK do not have a secondary at all :)
>
> def OpenOnSecondary(self):
> """Test if this device needs to be opened on a secondary node."""
> @@ -419,6 +421,8 @@ class Disk(ConfigObject):
> """
> if self.dev_type == constants.LD_LV:
> return "/dev/%s/%s" % (self.logical_id[0], self.logical_id[1])
> + elif self.dev_type == constants.LD_BLOCK:
> + return self.logical_id[1]
Not sure about this, per the first comment.
iustin
