On Wed, Apr 24, 2013 at 4:59 PM, Thomas Thrainer <[email protected]>wrote:
> As the DRBD8 class got bigger due to the previous merge of BaseDRBD, now > parts of it were ripped out into DRBD8Info. This new class parses s/were/are/ > /proc/drbd and exposes the information in an easily accessible way. This > allowed to simplify some methods in DRBD8 and do make the tests more > concise. > > Signed-off-by: Thomas Thrainer <[email protected]> > --- > lib/backend.py | 2 +- > lib/block/drbd.py | 271 > ++++++++++++++++++---------------- > lib/watcher/nodemaint.py | 2 +- > test/data/proc_drbd80-emptyline.txt | 1 + > test/py/ganeti.block.bdev_unittest.py | 60 ++++---- > 5 files changed, 176 insertions(+), 160 deletions(-) > > diff --git a/lib/backend.py b/lib/backend.py > index 5c83471..d7ed407 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -832,7 +832,7 @@ def VerifyNode(what, cluster_name): > > if constants.NV_DRBDLIST in what and vm_capable: > try: > - used_minors = drbd.DRBD8.GetUsedDevs().keys() > + used_minors = drbd.DRBD8.GetUsedDevs() > except errors.BlockDeviceError, err: > logging.warning("Can't get used minors list", exc_info=True) > used_minors = str(err) > diff --git a/lib/block/drbd.py b/lib/block/drbd.py > index 92e5b81..5f1d47b 100644 > --- a/lib/block/drbd.py > +++ b/lib/block/drbd.py > @@ -42,11 +42,11 @@ from ganeti.block import base > _DEVICE_READ_SIZE = 128 * 1024 > > > -class DRBD8Status(object): > +class DRBD8Status(object): # pylint: disable=R0902 > Are we sure we really want to disable the check for the number of attributes in the class? Cannot something else be refactored a bit to stay below the limit? > """A DRBD status representation class. > > Note that this class is meant to be used to parse one of the entries > returned > - from L{DRBD8._JoinProcDataPerMinor}. > + from L{DRBD8Info._JoinLinesPerMinor}. > Why changing the name of this function? And if you really want to do it, I would suggest to do such a thing in its own patch, instead of doing it as a "side effect" of the restructuring of this class. It would make reviewing the change much easier :-) > """ > UNCONF_RE = re.compile(r"\s*[0-9]+:\s*cs:Unconfigured$") > @@ -119,6 +119,7 @@ class DRBD8Status(object): > self.is_standalone = self.cstatus == self.CS_STANDALONE > self.is_wfconn = self.cstatus == self.CS_WFCONNECTION > self.is_connected = self.cstatus == self.CS_CONNECTED > + self.is_unconfigured = self.cstatus == self.CS_UNCONFIGURED > self.is_primary = self.lrole == self.RO_PRIMARY > self.is_secondary = self.lrole == self.RO_SECONDARY > self.peer_primary = self.rrole == self.RO_PRIMARY > @@ -151,6 +152,119 @@ class DRBD8Status(object): > self.est_time = None > > > +class DRBD8Info(object): > + """Represents information DRBD exports (usually via /proc/drbd). > + > + An instance of this class is created by one of the CreateFrom... > methods. > + > + """ > + > + _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:\.\d+)?" > + r" \(api:(\d+)/proto:(\d+)(?:-(\d+))?\)") > + _VALID_LINE_RE = re.compile("^ *([0-9]+): cs:([^ ]+).*$") > + > + def __init__(self, lines): > + self._version = self._ParseVersion(lines) > + self._minors, self._line_per_minor = self._JoinLinesPerMinor(lines) > + > + def GetVersion(self): > + """Return the DRBD version. > + > + This will return a dict with keys: > + - k_major > + - k_minor > + - k_point > + - api > + - proto > + - proto2 (only on drbd > 8.2.X) > + > + """ > + return self._version > + > + def GetMinors(self): > + """Return a list of minor for which information is available. > + > + This list is ordered in exactly the order which was found in the > underlying > + data. > + > + """ > + return self._minors > + > + def HasMinorStatus(self, minor): > + return minor in self._line_per_minor > + > + def GetMinorStatus(self, minor): > + return DRBD8Status(self._line_per_minor[minor]) > + > + def _ParseVersion(self, lines): > + first_line = lines[0].strip() > + version = self._VERSION_RE.match(first_line) > + if not version: > + raise errors.BlockDeviceError("Can't parse DRBD version from '%s'" % > + first_line) > + > + values = version.groups() > + retval = { > + "k_major": int(values[0]), > + "k_minor": int(values[1]), > + "k_point": int(values[2]), > + "api": int(values[3]), > + "proto": int(values[4]), > + } > + if values[5] is not None: > + retval["proto2"] = values[5] > + > + return retval > + > + def _JoinLinesPerMinor(self, lines): > + """Transform the raw lines into a dictionary based on the minor. > + > + @return: a dictionary of minor: joined lines from /proc/drbd > + for that minor > + > + """ > + minors = [] > + results = {} > + old_minor = old_line = None > + for line in lines: > + if not line: # completely empty lines, as can be returned by > drbd8.0+ > + continue > + lresult = self._VALID_LINE_RE.match(line) > + if lresult is not None: > + if old_minor is not None: > + minors.append(old_minor) > + results[old_minor] = old_line > + old_minor = int(lresult.group(1)) > + old_line = line > + else: > + if old_minor is not None: > + old_line += " " + line.strip() > + # add last line > + if old_minor is not None: > + minors.append(old_minor) > + results[old_minor] = old_line > + return minors, results > + > + @staticmethod > + def CreateFromLines(lines): > + return DRBD8Info(lines) > + > + @staticmethod > + def CreateFromFile(filename=constants.DRBD_STATUS_FILE): > + try: > + lines = utils.ReadFile(filename).splitlines() > + 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 the DRBD proc file %s: %s", > + filename, str(err)) > + if not lines: > + base.ThrowError("Can't read any data from %s", filename) > + return DRBD8Info.CreateFromLines(lines) > + > + > class DRBD8(base.BlockDev): > """DRBD v8.x block device. > > @@ -164,17 +278,8 @@ class DRBD8(base.BlockDev): > is checked for valid size and is zeroed on create. > > """ > - _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:\.\d+)?" > - r" \(api:(\d+)/proto:(\d+)(?:-(\d+))?\)") > - _VALID_LINE_RE = re.compile("^ *([0-9]+): cs:([^ ]+).*$") > - _UNUSED_LINE_RE = re.compile("^ *([0-9]+): cs:Unconfigured$") > - > _DRBD_MAJOR = 147 > - _ST_UNCONFIGURED = DRBD8Status.CS_UNCONFIGURED > - _ST_WFCONNECTION = DRBD8Status.CS_WFCONNECTION > - _ST_CONNECTED = DRBD8Status.CS_CONNECTED > > - _STATUS_FILE = constants.DRBD_STATUS_FILE > _USERMODE_HELPER_FILE = "/sys/module/drbd/parameters/usermode_helper" > > _MAX_MINORS = 255 > @@ -205,7 +310,9 @@ class DRBD8(base.BlockDev): > children = [] > super(DRBD8, self).__init__(unique_id, children, size, params) > self.major = self._DRBD_MAJOR > - version = self._GetVersion(self._GetProcData()) > + > + drbd_info = DRBD8Info.CreateFromFile() > + version = drbd_info.GetVersion() > if version["k_major"] != 8: > base.ThrowError("Mismatch in DRBD kernel version and requested > ganeti" > " usage: kernel is %s.%s, ganeti wants 8.x", > @@ -218,83 +325,6 @@ class DRBD8(base.BlockDev): > self.Attach() > > @staticmethod > - def _GetProcData(filename=_STATUS_FILE): > - """Return data from /proc/drbd. > - > - """ > - try: > - data = utils.ReadFile(filename).splitlines() > - 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 the DRBD proc file %s: %s", > - filename, str(err)) > - if not data: > - base.ThrowError("Can't read any data from %s", filename) > - return data > - > - @classmethod > - def _JoinProcDataPerMinor(cls, data): > - """Transform the output of _GetProdData into a nicer form. > - > - @return: a dictionary of minor: joined lines from /proc/drbd > - for that minor > - > - """ > - results = {} > - old_minor = old_line = None > - for line in data: > - if not line: # completely empty lines, as can be returned by > drbd8.0+ > - continue > - lresult = cls._VALID_LINE_RE.match(line) > - if lresult is not None: > - if old_minor is not None: > - results[old_minor] = old_line > - old_minor = int(lresult.group(1)) > - old_line = line > - else: > - if old_minor is not None: > - old_line += " " + line.strip() > - # add last line > - if old_minor is not None: > - results[old_minor] = old_line > - return results > - > - @classmethod > - def _GetVersion(cls, proc_data): > - """Return the DRBD version. > - > - This will return a dict with keys: > - - k_major > - - k_minor > - - k_point > - - api > - - proto > - - proto2 (only on drbd > 8.2.X) > - > - """ > - first_line = proc_data[0].strip() > - version = cls._VERSION_RE.match(first_line) > - if not version: > - raise errors.BlockDeviceError("Can't parse DRBD version from '%s'" % > - first_line) > - > - values = version.groups() > - retval = { > - "k_major": int(values[0]), > - "k_minor": int(values[1]), > - "k_point": int(values[2]), > - "api": int(values[3]), > - "proto": int(values[4]), > - } > - if values[5] is not None: > - retval["proto2"] = values[5] > - > - return retval > - > - @staticmethod > def GetUsermodeHelper(filename=_USERMODE_HELPER_FILE): > """Returns DRBD usermode_helper currently set. > > @@ -324,20 +354,9 @@ class DRBD8(base.BlockDev): > """Compute the list of used DRBD devices. > > """ > - data = cls._GetProcData() > - > - used_devs = {} > - for line in data: > - match = cls._VALID_LINE_RE.match(line) > - if not match: > - continue > - minor = int(match.group(1)) > - state = match.group(2) > - if state == cls._ST_UNCONFIGURED: > - continue > - used_devs[minor] = state, line > - > - return used_devs > + 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. > @@ -406,30 +425,28 @@ class DRBD8(base.BlockDev): > if result.failed: > base.ThrowError("Can't initialize meta device: %s", result.output) > > - @classmethod > - def _FindUnusedMinor(cls): > + 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. > > """ > - data = cls._GetProcData() > > highest = None > - for line in data: > - match = cls._UNUSED_LINE_RE.match(line) > - if match: > - return int(match.group(1)) > - match = cls._VALID_LINE_RE.match(line) > - if match: > - minor = int(match.group(1)) > - highest = max(highest, minor) > + 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 >= cls._MAX_MINORS: > + 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 > > @classmethod > @@ -607,7 +624,8 @@ class DRBD8(base.BlockDev): > if size: > args.extend(["-d", "%sm" % size]) > > - version = self._GetVersion(self._GetProcData()) > + drbd_info = DRBD8Info.CreateFromFile() > + version = drbd_info.GetVersion() > vmaj = version["k_major"] > vmin = version["k_minor"] > vrel = version["k_point"] > @@ -822,8 +840,7 @@ class DRBD8(base.BlockDev): > self._ShutdownLocal(self.minor) > self._children = [] > > - @classmethod > - def _SetMinorSyncParams(cls, minor, params): > + def _SetMinorSyncParams(self, minor, params): > """Set the parameters of the DRBD syncer. > > This is the low-level implementation. > @@ -837,9 +854,10 @@ class DRBD8(base.BlockDev): > > """ > > - args = ["drbdsetup", cls._DevPath(minor), "syncer"] > + args = ["drbdsetup", self._DevPath(minor), "syncer"] > if params[constants.LDP_DYNAMIC_RESYNC]: > - version = cls._GetVersion(cls._GetProcData()) > + drbd_info = DRBD8Info.CreateFromFile() > + version = drbd_info.GetVersion() > vmin = version["k_minor"] > vrel = version["k_point"] > > @@ -929,10 +947,10 @@ class DRBD8(base.BlockDev): > if self.minor is None: > base.ThrowError("drbd%d: GetStats() called while not attached", > self._aminor) > - proc_info = self._JoinProcDataPerMinor(self._GetProcData()) > - if self.minor not in proc_info: > + drbd_info = DRBD8Info.CreateFromFile() > + if not drbd_info.HasMinorStatus(self.minor): > base.ThrowError("drbd%d: can't find myself in /proc", self.minor) > - return DRBD8Status(proc_info[self.minor]) > + return drbd_info.GetMinorStatus(self.minor) > > def GetSyncStatus(self): > """Returns the sync status of the device. > @@ -1312,9 +1330,10 @@ class DRBD8(base.BlockDev): > " exclusive_storage") > # check that the minor is unused > aminor = unique_id[4] > - proc_info = cls._JoinProcDataPerMinor(cls._GetProcData()) > - if aminor in proc_info: > - status = DRBD8Status(proc_info[aminor]) > + > + drbd_info = DRBD8Info.CreateFromFile() > + if drbd_info.HasMinorStatus(aminor): > + status = drbd_info.GetMinorStatus(aminor) > in_use = status.is_in_use > else: > in_use = False > diff --git a/lib/watcher/nodemaint.py b/lib/watcher/nodemaint.py > index bfe761e..da8f095 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.DRBD8.GetUsedDevs().keys() > + return drbd.DRBD8.GetUsedDevs() > > @classmethod > def DoMaintenance(cls, role): > diff --git a/test/data/proc_drbd80-emptyline.txt > b/test/data/proc_drbd80-emptyline.txt > index d7c3d58..03fafae 100644 > --- a/test/data/proc_drbd80-emptyline.txt > +++ b/test/data/proc_drbd80-emptyline.txt > @@ -1,3 +1,4 @@ > +version: 8.0.12 (api:86/proto:86) > GIT-hash: 5c9f89594553e32adb87d9638dce591782f947e3 build by > [email protected], 2009-05-22 12:47:52 > 0: cs:Connected st:Primary/Secondary ds:UpToDate/UpToDate C r--- > ns:78728316 nr:0 dw:77675644 dr:1277039 al:254 bm:270 lo:0 pe:0 ua:0 > ap:0 > diff --git a/test/py/ganeti.block.bdev_unittest.py b/test/py/ > ganeti.block.bdev_unittest.py > index 1ec838d..4867683 100755 > --- a/test/py/ganeti.block.bdev_unittest.py > +++ b/test/py/ganeti.block.bdev_unittest.py > @@ -71,7 +71,8 @@ class TestDRBD8(testutils.GanetiTestCase): > } > ] > for d,r in zip(data, result): > - self.assertEqual(drbd.DRBD8._GetVersion(d), r) > + info = drbd.DRBD8Info.CreateFromLines(d) > + self.assertEqual(info.GetVersion(), r) > > > class TestDRBD8Runner(testutils.GanetiTestCase): > @@ -244,26 +245,21 @@ class TestDRBD8Status(testutils.GanetiTestCase): > proc83_sync_data = testutils.TestDataFilename("proc_drbd83_sync.txt") > proc83_sync_krnl_data = \ > testutils.TestDataFilename("proc_drbd83_sync_krnl2.6.39.txt") > - self.proc_data = drbd.DRBD8._GetProcData(filename=proc_data) > - self.proc80e_data = drbd.DRBD8._GetProcData(filename=proc80e_data) > - self.proc83_data = drbd.DRBD8._GetProcData(filename=proc83_data) > - self.proc83_sync_data = > drbd.DRBD8._GetProcData(filename=proc83_sync_data) > - self.proc83_sync_krnl_data = \ > - drbd.DRBD8._GetProcData(filename=proc83_sync_krnl_data) > - self.mass_data = drbd.DRBD8._JoinProcDataPerMinor(self.proc_data) > - self.mass80e_data = > drbd.DRBD8._JoinProcDataPerMinor(self.proc80e_data) > - self.mass83_data = drbd.DRBD8._JoinProcDataPerMinor(self.proc83_data) > - self.mass83_sync_data = \ > - drbd.DRBD8._JoinProcDataPerMinor(self.proc83_sync_data) > - self.mass83_sync_krnl_data = \ > - drbd.DRBD8._JoinProcDataPerMinor(self.proc83_sync_krnl_data) > + > + self.drbd_info = drbd.DRBD8Info.CreateFromFile(filename=proc_data) > + self.drbd_info80e = > drbd.DRBD8Info.CreateFromFile(filename=proc80e_data) > + self.drbd_info83 = drbd.DRBD8Info.CreateFromFile(filename=proc83_data) > + self.drbd_info83_sync = \ > + drbd.DRBD8Info.CreateFromFile(filename=proc83_sync_data) > + self.drbd_info83_sync_krnl = \ > + drbd.DRBD8Info.CreateFromFile(filename=proc83_sync_krnl_data) > > def testIOErrors(self): > """Test handling of errors while reading the proc file.""" > temp_file = self._CreateTempFile() > os.unlink(temp_file) > self.failUnlessRaises(errors.BlockDeviceError, > - drbd.DRBD8._GetProcData, filename=temp_file) > + drbd.DRBD8Info.CreateFromFile, > filename=temp_file) > > def testHelper(self): > """Test reading usermode_helper in /sys.""" > @@ -280,9 +276,9 @@ class TestDRBD8Status(testutils.GanetiTestCase): > > def testMinorNotFound(self): > """Test not-found-minor in /proc""" > - self.failUnless(9 not in self.mass_data) > - self.failUnless(9 not in self.mass83_data) > - self.failUnless(3 not in self.mass80e_data) > + self.failUnless(not self.drbd_info.HasMinorStatus(9)) > + self.failUnless(not self.drbd_info83.HasMinorStatus(9)) > + self.failUnless(not self.drbd_info80e.HasMinorStatus(3)) > > def testLineNotMatch(self): > """Test wrong line passed to drbd.DRBD8Status""" > @@ -290,30 +286,30 @@ class TestDRBD8Status(testutils.GanetiTestCase): > > def testMinor0(self): > """Test connected, primary device""" > - for data in [self.mass_data, self.mass83_data]: > - stats = drbd.DRBD8Status(data[0]) > + for info in [self.drbd_info, self.drbd_info83]: > + stats = info.GetMinorStatus(0) > self.failUnless(stats.is_in_use) > self.failUnless(stats.is_connected and stats.is_primary and > stats.peer_secondary and stats.is_disk_uptodate) > > def testMinor1(self): > """Test connected, secondary device""" > - for data in [self.mass_data, self.mass83_data]: > - stats = drbd.DRBD8Status(data[1]) > + for info in [self.drbd_info, self.drbd_info83]: > + stats = info.GetMinorStatus(1) > self.failUnless(stats.is_in_use) > self.failUnless(stats.is_connected and stats.is_secondary and > stats.peer_primary and stats.is_disk_uptodate) > > def testMinor2(self): > """Test unconfigured device""" > - for data in [self.mass_data, self.mass83_data, self.mass80e_data]: > - stats = drbd.DRBD8Status(data[2]) > + for info in [self.drbd_info, self.drbd_info83, self.drbd_info80e]: > + stats = info.GetMinorStatus(2) > self.failIf(stats.is_in_use) > > def testMinor4(self): > """Test WFconn device""" > - for data in [self.mass_data, self.mass83_data]: > - stats = drbd.DRBD8Status(data[4]) > + for info in [self.drbd_info, self.drbd_info83]: > + stats = info.GetMinorStatus(4) > self.failUnless(stats.is_in_use) > self.failUnless(stats.is_wfconn and stats.is_primary and > stats.rrole == "Unknown" and > @@ -321,28 +317,28 @@ class TestDRBD8Status(testutils.GanetiTestCase): > > def testMinor6(self): > """Test diskless device""" > - for data in [self.mass_data, self.mass83_data]: > - stats = drbd.DRBD8Status(data[6]) > + for info in [self.drbd_info, self.drbd_info83]: > + stats = info.GetMinorStatus(6) > self.failUnless(stats.is_in_use) > self.failUnless(stats.is_connected and stats.is_secondary and > stats.peer_primary and stats.is_diskless) > > def testMinor8(self): > """Test standalone device""" > - for data in [self.mass_data, self.mass83_data]: > - stats = drbd.DRBD8Status(data[8]) > + for info in [self.drbd_info, self.drbd_info83]: > + stats = info.GetMinorStatus(8) > self.failUnless(stats.is_in_use) > self.failUnless(stats.is_standalone and > stats.rrole == "Unknown" and > stats.is_disk_uptodate) > > def testDRBD83SyncFine(self): > - stats = drbd.DRBD8Status(self.mass83_sync_data[3]) > + stats = self.drbd_info83_sync.GetMinorStatus(3) > self.failUnless(stats.is_in_resync) > self.failUnless(stats.sync_percent is not None) > > def testDRBD83SyncBroken(self): > - stats = drbd.DRBD8Status(self.mass83_sync_krnl_data[3]) > + stats = self.drbd_info83_sync_krnl.GetMinorStatus(3) > self.failUnless(stats.is_in_resync) > self.failUnless(stats.sync_percent is not None) > > -- > 1.8.2.1 > > There is something wrong in this patch: if you run "make check" after applying it, the test "proc_drbd80-emptyline" of "Block/Drbd/Parser" fails. Usually we want to keep the repository in a clean state after each patch, even inside a patch set. If you are fixing it later in this patch set and you cannot avoid it, it might be ok, but at least specify it in the commit message. Thanks, Michele
