Hi!
On Tue, Apr 30, 2013 at 11:38 AM, Thomas Thrainer <[email protected]>wrote: > All functionality specific to a single DRBD8 devide is now in DRBD8Dev, > whereas functionality which is valid for the whole DRBD "installation" > on a device is collected in DRBD8. > > This makes it possible to remove a couple of FIXME's and clarifies the > code in some areas. > > Signed-off-by: Thomas Thrainer <[email protected]> > > Conflicts: > lib/block/drbd.py > --- > lib/backend.py | 10 +- > lib/block/drbd.py | 280 > ++++++++++++++++++---------------- > lib/bootstrap.py | 2 +- > lib/watcher/nodemaint.py | 7 +- > test/py/ganeti.block.drbd_unittest.py | 10 +- > 5 files changed, 160 insertions(+), 149 deletions(-) > > diff --git a/lib/backend.py b/lib/backend.py > index 147f35c..dc13b02 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -66,7 +66,7 @@ from ganeti import pathutils > from ganeti import vcluster > from ganeti import ht > from ganeti.block.base import BlockDev > -from ganeti.block.drbd_info import DRBD8Info > +from ganeti.block.drbd import DRBD8 > from ganeti import hooksmaster > > > @@ -833,7 +833,7 @@ def VerifyNode(what, cluster_name): > > if constants.NV_DRBDVERSION in what and vm_capable: > try: > - drbd_version = DRBD8Info.CreateFromFile().GetVersionString() > + drbd_version = DRBD8.GetProcInfo().GetVersionString() > except errors.BlockDeviceError, err: > logging.warning("Can't get DRBD version", exc_info=True) > drbd_version = str(err) > @@ -841,7 +841,7 @@ def VerifyNode(what, cluster_name): > > if constants.NV_DRBDLIST in what and vm_capable: > try: > - used_minors = drbd.DRBD8Dev.GetUsedDevs() > + used_minors = drbd.DRBD8.GetUsedDevs() > except errors.BlockDeviceError, err: > logging.warning("Can't get used minors list", exc_info=True) > used_minors = str(err) > @@ -850,7 +850,7 @@ def VerifyNode(what, cluster_name): > if constants.NV_DRBDHELPER in what and vm_capable: > status = True > try: > - payload = drbd.DRBD8Dev.GetUsermodeHelper() > + payload = drbd.DRBD8.GetUsermodeHelper() > except errors.BlockDeviceError, err: > logging.error("Can't get DRBD usermode helper: %s", str(err)) > status = False > @@ -3692,7 +3692,7 @@ def GetDrbdUsermodeHelper(): > > """ > try: > - return drbd.DRBD8Dev.GetUsermodeHelper() > + return drbd.DRBD8.GetUsermodeHelper() > except errors.BlockDeviceError, err: > _Fail(str(err)) > > diff --git a/lib/block/drbd.py b/lib/block/drbd.py > index 3912f3d..dc1bba2 100644 > --- a/lib/block/drbd.py > +++ b/lib/block/drbd.py > @@ -42,6 +42,102 @@ from ganeti.block import drbd_cmdgen > _DEVICE_READ_SIZE = 128 * 1024 > > > +class DRBD8(object): > + """Various methods to deals with the DRBD system as a whole. > + > + This class provides a set of methods to deal with the DRBD installation > on > + the node or with uninitialized devices as opposed to a DRBD device. > + > + """ > + _USERMODE_HELPER_FILE = "/sys/module/drbd/parameters/usermode_helper" > + > + _MAX_MINORS = 255 > + > + @staticmethod > + def GetUsermodeHelper(filename=_USERMODE_HELPER_FILE): > + """Returns DRBD usermode_helper currently set. > + > + """ > + try: > + helper = utils.ReadFile(filename).splitlines()[0] > + except EnvironmentError, err: > + if err.errno == errno.ENOENT: > + base.ThrowError("The file %s cannot be opened, check if the > module" > + " is loaded (%s)", filename, str(err)) > + else: > + base.ThrowError("Can't read DRBD helper file %s: %s", > + filename, str(err)) > + if not helper: > + base.ThrowError("Can't read any data from %s", filename) > + return helper > + > + @staticmethod > + def GetProcInfo(): > + """Reads and parses information from /proc/drbd. > + > + @return: a L{DRBD8Info} instance > + > + """ > + return DRBD8Info.CreateFromFile() > + > + @staticmethod > + def GetUsedDevs(): > + """Compute the list of used DRBD devices. > + > + """ > + drbd_info = DRBD8.GetProcInfo() > + return filter(lambda m: not > drbd_info.GetMinorStatus(m).is_unconfigured, > + drbd_info.GetMinors()) > + > + @staticmethod > + def FindUnusedMinor(): > + """Find an unused DRBD device. > + > + This is specific to 8.x as the minors are allocated dynamically, > + so non-existing numbers up to a max minor count are actually free. > + > + """ > + highest = None > + drbd_info = DRBD8.GetProcInfo() > + for minor in drbd_info.GetMinors(): > + status = drbd_info.GetMinorStatus(minor) > + if not status.is_in_use: > + return minor > + highest = max(highest, minor) > + > + if highest is None: # there are no minors in use at all > + return 0 > + if highest >= DRBD8._MAX_MINORS: > + logging.error("Error: no free drbd minors!") > + raise errors.BlockDeviceError("Can't find a free DRBD minor") > + > + return highest + 1 > + > + @staticmethod > + def GetCmdGenerator(drbd_info): > add docstring + parameter description (inkl. type) please. > + version = drbd_info.GetVersion() > + if version["k_minor"] <= 3: > + return drbd_cmdgen.DRBD83CmdGenerator(version) > + else: > + return drbd_cmdgen.DRBD84CmdGenerator(version) > + > + @staticmethod > + def ShutdownAll(minor): > + """Deactivate the device. > + > + This will, of course, fail if the device is in use. > add parameter description (inkl. type) please > + > + """ > + drbd_info = DRBD8.GetProcInfo() > + cmd_gen = DRBD8.GetCmdGenerator(drbd_info) > + > + cmd = cmd_gen.GenDownCmd(minor) > + result = utils.RunCmd(cmd) > + if result.failed: > + base.ThrowError("drbd%d: can't shutdown drbd device: %s", > + minor, result.output) > + > + > class DRBD8Dev(base.BlockDev): > """DRBD v8.x block device. > > @@ -57,10 +153,6 @@ class DRBD8Dev(base.BlockDev): > """ > _DRBD_MAJOR = 147 > > - _USERMODE_HELPER_FILE = "/sys/module/drbd/parameters/usermode_helper" > - > - _MAX_MINORS = 255 > - > # timeout constants > _NET_RECONFIG_TIMEOUT = 60 > > @@ -81,7 +173,7 @@ class DRBD8Dev(base.BlockDev): > super(DRBD8Dev, self).__init__(unique_id, children, size, params) > self.major = self._DRBD_MAJOR > > - drbd_info = DRBD8Info.CreateFromFile() > + drbd_info = DRBD8.GetProcInfo() > version = drbd_info.GetVersion() > if version["k_major"] != 8: > base.ThrowError("Mismatch in DRBD kernel version and requested > ganeti" > @@ -93,7 +185,7 @@ class DRBD8Dev(base.BlockDev): > else: > self._show_info_cls = DRBD84ShowInfo > > - self._cmd_gen = self._GetCmdGenerator(drbd_info) > + self._cmd_gen = DRBD8.GetCmdGenerator(drbd_info) > > if (self._lhost is not None and self._lhost == self._rhost and > self._lport == self._rport): > @@ -101,32 +193,6 @@ class DRBD8Dev(base.BlockDev): > (unique_id,)) > self.Attach() > > - @classmethod > - def _GetCmdGenerator(cls, drbd_info): > - version = drbd_info.GetVersion() > - if version["k_minor"] <= 3: > - return drbd_cmdgen.DRBD83CmdGenerator(version) > - else: > - return drbd_cmdgen.DRBD84CmdGenerator(version) > - > - @staticmethod > - def GetUsermodeHelper(filename=_USERMODE_HELPER_FILE): > - """Returns DRBD usermode_helper currently set. > - > - """ > - try: > - helper = utils.ReadFile(filename).splitlines()[0] > - except EnvironmentError, err: > - if err.errno == errno.ENOENT: > - base.ThrowError("The file %s cannot be opened, check if the > module" > - " is loaded (%s)", filename, str(err)) > - else: > - base.ThrowError("Can't read DRBD helper file %s: %s", > - filename, str(err)) > - if not helper: > - base.ThrowError("Can't read any data from %s", filename) > - return helper > - > @staticmethod > def _DevPath(minor): > """Return the path to a drbd device for a given minor. > @@ -134,15 +200,6 @@ class DRBD8Dev(base.BlockDev): > """ > return "/dev/drbd%d" % minor > > - @classmethod > - def GetUsedDevs(cls): > - """Compute the list of used DRBD devices. > - > - """ > - drbd_info = DRBD8Info.CreateFromFile() > - return filter(lambda m: not > drbd_info.GetMinorStatus(m).is_unconfigured, > - drbd_info.GetMinors()) > - > def _SetFromMinor(self, minor): > """Set our parameters based on the given minor. > > @@ -187,56 +244,6 @@ class DRBD8Dev(base.BlockDev): > base.ThrowError("Meta device too big (%.2fMiB)", > (num_bytes / 1024 / 1024)) > > - @classmethod > - def _InitMeta(cls, minor, dev_path): > - """Initialize a meta device. > - > - This will not work if the given minor is in use. > - > - """ > - # Zero the metadata first, in order to make sure drbdmeta doesn't > - # try to auto-detect existing filesystems or similar (see > - # http://code.google.com/p/ganeti/issues/detail?id=182); we only > - # care about the first 128MB of data in the device, even though it > - # can be bigger > - result = utils.RunCmd([constants.DD_CMD, > - "if=/dev/zero", "of=%s" % dev_path, > - "bs=1048576", "count=128", "oflag=direct"]) > - if result.failed: > - base.ThrowError("Can't wipe the meta device: %s", result.output) > - > - drbd_info = DRBD8Info.CreateFromFile() > - cmd_gen = cls._GetCmdGenerator(drbd_info) > - cmd = cmd_gen.GenInitMetaCmd(minor, dev_path) > - > - result = utils.RunCmd(cmd) > - if result.failed: > - base.ThrowError("Can't initialize meta device: %s", result.output) > - > - def _FindUnusedMinor(self): > - """Find an unused DRBD device. > - > - This is specific to 8.x as the minors are allocated dynamically, > - so non-existing numbers up to a max minor count are actually free. > - > - """ > - > - highest = None > - drbd_info = DRBD8Info.CreateFromFile() > - for minor in drbd_info.GetMinors(): > - status = drbd_info.GetMinorStatus(minor) > - if not status.is_in_use: > - return minor > - highest = max(highest, minor) > - > - if highest is None: # there are no minors in use at all > - return 0 > - if highest >= self._MAX_MINORS: > - logging.error("Error: no free drbd minors!") > - raise errors.BlockDeviceError("Can't find a free DRBD minor") > - > - return highest + 1 > - > def _GetShowData(self, minor): > """Return the `drbdsetup show` data for a minor. > > @@ -400,7 +407,7 @@ class DRBD8Dev(base.BlockDev): > backend.Open() > meta.Open() > self._CheckMetaSize(meta.dev_path) > - self._InitMeta(self._FindUnusedMinor(), meta.dev_path) > + self._InitMeta(DRBD8.FindUnusedMinor(), meta.dev_path) > > self._AssembleLocal(self.minor, backend.dev_path, meta.dev_path, > self.size) > self._children = devices > @@ -506,7 +513,7 @@ class DRBD8Dev(base.BlockDev): > if self.minor is None: > base.ThrowError("drbd%d: GetStats() called while not attached", > self._aminor) > - drbd_info = DRBD8Info.CreateFromFile() > + drbd_info = DRBD8.GetProcInfo() > if not drbd_info.HasMinorStatus(self.minor): > base.ThrowError("drbd%d: can't find myself in /proc", self.minor) > return drbd_info.GetMinorStatus(self.minor) > @@ -685,7 +692,7 @@ class DRBD8Dev(base.BlockDev): > /proc). > > """ > - used_devs = self.GetUsedDevs() > + used_devs = DRBD8.GetUsedDevs() > if self._aminor in used_devs: > minor = self._aminor > else: > @@ -840,25 +847,6 @@ class DRBD8Dev(base.BlockDev): > base.ThrowError("drbd%d: can't shutdown network: %s", > minor, result.output) > > - @classmethod > - def _ShutdownAll(cls, minor): > - """Deactivate the device. > - > - This will, of course, fail if the device is in use. > - > - """ > - # FIXME: _ShutdownAll, despite being private, is used in nodemaint.py. > - # That's why we can't make it an instance method, which in turn > requires > - # us to duplicate code here (from __init__). This should be properly > fixed. > - drbd_info = DRBD8Info.CreateFromFile() > - cmd_gen = cls._GetCmdGenerator(drbd_info) > - > - cmd = cmd_gen.GenDownCmd(minor) > - result = utils.RunCmd(cmd) > - if result.failed: > - base.ThrowError("drbd%d: can't shutdown drbd device: %s", > - minor, result.output) > - > def Shutdown(self): > """Shutdown the DRBD device. > > @@ -869,7 +857,7 @@ class DRBD8Dev(base.BlockDev): > minor = self.minor > self.minor = None > self.dev_path = None > - self._ShutdownAll(minor) > + DRBD8.ShutdownAll(minor) > > def Remove(self): > """Stub remove for DRBD devices. > @@ -885,6 +873,50 @@ class DRBD8Dev(base.BlockDev): > """ > raise errors.ProgrammerError("Can't rename a drbd device") > > + def Grow(self, amount, dryrun, backingstore): > + """Resize the DRBD device and its backing storage. > + > while you are refactoring this, please add description (inkl. type) for the parameters > + """ > + if self.minor is None: > + base.ThrowError("drbd%d: Grow called while not attached", > self._aminor) > + if len(self._children) != 2 or None in self._children: > + base.ThrowError("drbd%d: cannot grow diskless device", self.minor) > + self._children[0].Grow(amount, dryrun, backingstore) > + if dryrun or backingstore: > + # DRBD does not support dry-run mode and is not backing storage, > + # so we'll return here > + return > + cmd = self._cmd_gen.GenResizeCmd(self.minor, self.size + amount) > + result = utils.RunCmd(cmd) > + if result.failed: > + base.ThrowError("drbd%d: resize failed: %s", self.minor, > result.output) > + > + @classmethod > + def _InitMeta(cls, minor, dev_path): > + """Initialize a meta device. > + > + This will not work if the given minor is in use. > same here > + > + """ > + # Zero the metadata first, in order to make sure drbdmeta doesn't > + # try to auto-detect existing filesystems or similar (see > + # http://code.google.com/p/ganeti/issues/detail?id=182); we only > + # care about the first 128MB of data in the device, even though it > + # can be bigger > + result = utils.RunCmd([constants.DD_CMD, > + "if=/dev/zero", "of=%s" % dev_path, > + "bs=1048576", "count=128", "oflag=direct"]) > + if result.failed: > + base.ThrowError("Can't wipe the meta device: %s", result.output) > + > + drbd_info = DRBD8.GetProcInfo() > + cmd_gen = DRBD8.GetCmdGenerator(drbd_info) > + cmd = cmd_gen.GenInitMetaCmd(minor, dev_path) > + > + result = utils.RunCmd(cmd) > + if result.failed: > + base.ThrowError("Can't initialize meta device: %s", result.output) > + > @classmethod > def Create(cls, unique_id, children, size, params, excl_stor): > """Create a new DRBD8 device. > @@ -901,7 +933,7 @@ class DRBD8Dev(base.BlockDev): > # check that the minor is unused > aminor = unique_id[4] > > - drbd_info = DRBD8Info.CreateFromFile() > + drbd_info = DRBD8.GetProcInfo() > if drbd_info.HasMinorStatus(aminor): > status = drbd_info.GetMinorStatus(aminor) > in_use = status.is_in_use > @@ -919,24 +951,6 @@ class DRBD8Dev(base.BlockDev): > cls._InitMeta(aminor, meta.dev_path) > return cls(unique_id, children, size, params) > > - def Grow(self, amount, dryrun, backingstore): > - """Resize the DRBD device and its backing storage. > - > - """ > - if self.minor is None: > - base.ThrowError("drbd%d: Grow called while not attached", > self._aminor) > - if len(self._children) != 2 or None in self._children: > - base.ThrowError("drbd%d: cannot grow diskless device", self.minor) > - self._children[0].Grow(amount, dryrun, backingstore) > - if dryrun or backingstore: > - # DRBD does not support dry-run mode and is not backing storage, > - # so we'll return here > - return > - cmd = self._cmd_gen.GenResizeCmd(self.minor, self.size + amount) > - result = utils.RunCmd(cmd) > - if result.failed: > - base.ThrowError("drbd%d: resize failed: %s", self.minor, > result.output) > - > > def _CanReadDevice(path): > """Check if we can read from the given device. > diff --git a/lib/bootstrap.py b/lib/bootstrap.py > index fba3315..d648c77 100644 > --- a/lib/bootstrap.py > +++ b/lib/bootstrap.py > @@ -470,7 +470,7 @@ def InitCluster(cluster_name, mac_prefix, # pylint: > disable=R0913, R0914 > > if drbd_helper is not None: > try: > - curr_helper = drbd.DRBD8Dev.GetUsermodeHelper() > + curr_helper = drbd.DRBD8.GetUsermodeHelper() > except errors.BlockDeviceError, err: > raise errors.OpPrereqError("Error while checking drbd helper" > " (specify --no-drbd-storage if you are > not" > diff --git a/lib/watcher/nodemaint.py b/lib/watcher/nodemaint.py > index ed4157e..c3b82c0 100644 > --- a/lib/watcher/nodemaint.py > +++ b/lib/watcher/nodemaint.py > @@ -80,7 +80,7 @@ class NodeMaintenance(object): > """Get list of used DRBD minors. > > """ > - return drbd.DRBD8Dev.GetUsedDevs() > + return drbd.DRBD8.GetUsedDevs() > > @classmethod > def DoMaintenance(cls, role): > @@ -121,10 +121,7 @@ class NodeMaintenance(object): > logging.info("Following DRBD minors should not be active," > " shutting them down: %s", > utils.CommaJoin(drbd_running)) > for minor in drbd_running: > - # pylint: disable=W0212 > - # using the private method as is, pending enhancements to the DRBD > - # interface > - drbd.DRBD8Dev._ShutdownAll(minor) > + drbd.DRBD8.ShutdownAll(minor) > > def Exec(self): > """Check node status versus cluster desired state. > diff --git a/test/py/ganeti.block.drbd_unittest.py b/test/py/ > ganeti.block.drbd_unittest.py > index ace363f..fcba8ee 100755 > --- a/test/py/ganeti.block.drbd_unittest.py > +++ b/test/py/ganeti.block.drbd_unittest.py > @@ -295,7 +295,7 @@ class TestDRBD8Status(testutils.GanetiTestCase): > def testHelper(self): > """Test reading usermode_helper in /sys.""" > sys_drbd_helper = > testutils.TestDataFilename("sys_drbd_usermode_helper.txt") > - drbd_helper = > drbd.DRBD8Dev.GetUsermodeHelper(filename=sys_drbd_helper) > + drbd_helper = drbd.DRBD8.GetUsermodeHelper(filename=sys_drbd_helper) > self.failUnlessEqual(drbd_helper, "/bin/true") > > def testHelperIOErrors(self): > @@ -303,7 +303,7 @@ class TestDRBD8Status(testutils.GanetiTestCase): > temp_file = self._CreateTempFile() > os.unlink(temp_file) > self.failUnlessRaises(errors.BlockDeviceError, > - drbd.DRBD8Dev.GetUsermodeHelper, > filename=temp_file) > + drbd.DRBD8.GetUsermodeHelper, > filename=temp_file) > > def testMinorNotFound(self): > """Test not-found-minor in /proc""" > @@ -401,7 +401,7 @@ class TestDRBD8Construction(testutils.GanetiTestCase): > > self.test_unique_id = ("hosta.com", 123, "host2.com", 123, 0, > "secret") > > - @testutils.patch_object(drbd_info.DRBD8Info, "CreateFromFile") > + @testutils.patch_object(drbd.DRBD8, "GetProcInfo") > def testConstructionWith80Data(self, mock_create_from_file): > mock_create_from_file.return_value = self.proc80_info > > @@ -409,7 +409,7 @@ class TestDRBD8Construction(testutils.GanetiTestCase): > self.assertEqual(inst._show_info_cls, drbd_info.DRBD83ShowInfo) > self.assertTrue(isinstance(inst._cmd_gen, > drbd_cmdgen.DRBD83CmdGenerator)) > > - @testutils.patch_object(drbd_info.DRBD8Info, "CreateFromFile") > + @testutils.patch_object(drbd.DRBD8, "GetProcInfo") > def testConstructionWith83Data(self, mock_create_from_file): > mock_create_from_file.return_value = self.proc83_info > > @@ -417,7 +417,7 @@ class TestDRBD8Construction(testutils.GanetiTestCase): > self.assertEqual(inst._show_info_cls, drbd_info.DRBD83ShowInfo) > self.assertTrue(isinstance(inst._cmd_gen, > drbd_cmdgen.DRBD83CmdGenerator)) > > - @testutils.patch_object(drbd_info.DRBD8Info, "CreateFromFile") > + @testutils.patch_object(drbd.DRBD8, "GetProcInfo") > def testConstructionWith84Data(self, mock_create_from_file): > mock_create_from_file.return_value = self.proc84_info > > -- > 1.8.2.1 > > Cheers, Helga
