From 578bfb57ac930121ecbe6d4fef11cf3f5a791b09 Mon Sep 17 00:00:00 2001 From: Siddharth Vinothkumar Date: Thu, 24 Apr 2014 11:04:47 +0100 Subject: [PATCH 01/69] CA-87112: Fix LVM space leak during VDI.create Signed-off-by: Siddharth Vinothkumar Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos --- drivers/LVHDSR.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/LVHDSR.py b/drivers/LVHDSR.py index b80c44d7..80ab2475 100755 --- a/drivers/LVHDSR.py +++ b/drivers/LVHDSR.py @@ -1313,13 +1313,18 @@ def create(self, sr_uuid, vdi_uuid, size): self.sm_config = self.sr.srcmd.params["vdi_sm_config"] self.sr._ensureSpaceAvailable(lvSize) - self.sr.lvmCache.create(self.lvname, lvSize) - if self.vdi_type == vhdutil.VDI_TYPE_RAW: - self.size = self.sr.lvmCache.getSize(self.lvname) - else: - vhdutil.create(self.path, long(size), False, lvhdutil.MSIZE_MB) - self.size = vhdutil.getSizeVirt(self.path) - self.sr.lvmCache.deactivateNoRefcount(self.lvname) + try: + self.sr.lvmCache.create(self.lvname, lvSize) + if self.vdi_type == vhdutil.VDI_TYPE_RAW: + self.size = self.sr.lvmCache.getSize(self.lvname) + else: + vhdutil.create(self.path, long(size), False, lvhdutil.MSIZE_MB) + self.size = vhdutil.getSizeVirt(self.path) + self.sr.lvmCache.deactivateNoRefcount(self.lvname) + except util.CommandException, e: + util.SMlog("Unable to create VDI"); + self.sr.lvmCache.remove(self.lvname) + raise xs_errors.XenError('VDICreate', opterr='error %d' % e.code) self.utilisation = lvSize self.sm_config["vdi_type"] = self.vdi_type From d58a4330773298c2a50fb2a25392b84207549b1f Mon Sep 17 00:00:00 2001 From: Siddharth Vinothkumar Date: Thu, 24 Apr 2014 11:14:10 +0100 Subject: [PATCH 02/69] CA-123175: Filter iSCSI dom0 boot disk LUN. Signed-off-by: Siddharth Vinothkumar Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos --- drivers/ISCSISR.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/ISCSISR.py b/drivers/ISCSISR.py index 9daf7b47..9198b544 100755 --- a/drivers/ISCSISR.py +++ b/drivers/ISCSISR.py @@ -626,13 +626,17 @@ def _getLUNbySMconfig(self, sm_config): def print_LUNs(self): self.LUNs = {} if os.path.exists(self.path): + dom0_disks = util.dom0_disks() for file in util.listdir(self.path): if file.find("LUN") != -1 and file.find("_") == -1: vdi_path = os.path.join(self.path,file) - LUNid = file.replace("LUN","") - obj = self.vdi(self.uuid) - obj._query(vdi_path, LUNid) - self.LUNs[obj.uuid] = obj + if os.path.realpath(vdi_path) in dom0_disks: + util.SMlog("Hide dom0 boot disk LUN") + else: + LUNid = file.replace("LUN","") + obj = self.vdi(self.uuid) + obj._query(vdi_path, LUNid) + self.LUNs[obj.uuid] = obj def print_entries(self, map): dom = xml.dom.minidom.Document() From 3ec8caf4ff948e584560d9b813cfdcc78bd6fb61 Mon Sep 17 00:00:00 2001 From: Si Beaumont Date: Wed, 23 Apr 2014 17:10:45 +0100 Subject: [PATCH 03/69] CA-119317: Use kenel blkback for raw LUNs Imported-by: Thanos Makatos --- drivers/RawHBASR.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/RawHBASR.py b/drivers/RawHBASR.py index 9d675b79..a596685c 100755 --- a/drivers/RawHBASR.py +++ b/drivers/RawHBASR.py @@ -334,6 +334,8 @@ def _query(self, path, id, uuid=None, scsi_id=None): sm_config = util.default(self, "sm_config", lambda: {}) sm_config['LUNid'] = str(self.LUNid) sm_config['SCSIid'] = self.SCSIid + # Make sure to use kernel blkback (not blktap3) for raw LUNs + sm_config['backend-kind'] = 'vbd' self.sm_config = sm_config def attach(self, sr_uuid, vdi_uuid): From 6fb27e11564ac6fd95764f2ab08afa6fe5dda850 Mon Sep 17 00:00:00 2001 From: Chandrika Srinivasan Date: Fri, 16 May 2014 18:10:06 +0100 Subject: [PATCH 04/69] CP 8320: Improved formatting in README and INSTALL files Signed-off-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit d60003a24ac53bfaf788ec64e511a2a671cef868) --- INSTALL | 4 +++- README.md | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/INSTALL b/INSTALL index 6a14d1ec..ac4b13c5 100644 --- a/INSTALL +++ b/INSTALL @@ -1 +1,3 @@ -Currently building this package outside of the XenServer build environment is somewhat broken. The goal is to replace it with a simple "make"/"make install" process. +Currently building this package outside of the XenServer build environment is +somewhat broken. The goal is to replace it with a simple "make"/"make install" +process. diff --git a/README.md b/README.md index e9dfede5..dcdc602f 100644 --- a/README.md +++ b/README.md @@ -3,6 +3,6 @@ Storage Manager for XenServer [![Build Status](https://travis-ci.org/xapi-project/sm.svg?branch=master)](https://travis-ci.org/xapi-project/sm) [![Coverage Status](https://coveralls.io/repos/xapi-project/sm/badge.png?branch=master)](https://coveralls.io/r/xapi-project/sm?branch=master) -This repository contains the code which forms the Storage Management layer for XenSever. It consists of a series of "plug-ins" to xapi (the Xen management layer) which are written primarily in python. - - +This repository contains the code which forms the Storage Management layer for +XenSever. It consists of a series of "plug-ins" to xapi (the Xen management +layer) which are written primarily in python. From 657d782ecc25c845a9eb0a162fee14ced03e1c21 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 20 May 2014 11:59:15 +0100 Subject: [PATCH 05/69] CAR-1442: Change NFS mount options In preparation for read caching, change actimeo=0 to acdirmin=0,acdirmax=0. Using "actimeo=0" prevents caching file attributes which means that each read requires a GETATTR call to the NFS server. This prevents read caching from being effective. Changing the mount options to "acdirmin=0,acdirmax=0" prevents caching directory attributes but allows caching file attributes. This allows read caching to be effective. This reverts the change made to the mount options in CA-63601. The issue in that ticket was a transient issue, fixed by a later build, before the change to the mount options was made. Signed-off-by: Ross Lagerwall Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit 8e505ad9d82dc66c06edf22588d0f7d8e8fac1cb) --- drivers/nfs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nfs.py b/drivers/nfs.py index ef4a5e8c..29339ff9 100644 --- a/drivers/nfs.py +++ b/drivers/nfs.py @@ -77,7 +77,7 @@ def soft_mount(mountpoint, remoteserver, remotepath, transport, timeout = 0): options = "soft,timeo=%d,retrans=%d,%s" % (timeout * 10, SOFTMOUNT_RETRANS, transport) - options += ',actimeo=0' + options += ',acdirmin=0,acdirmax=0' try: util.ioretry(lambda: From 3a49040d8646be044ffb29b1dea0d8cacf1eb283 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 20 May 2014 12:19:27 +0100 Subject: [PATCH 06/69] CAR-1442: Enable no-O_DIRECT read caching for VDIs on EXT and NFS SRs Pass the "-D" argument to tap-ctl when opening an EXTFileVDI or an NFSFileVDI to allow read caching by removing O_DIRECT. Allow disabling read caching per-SR by setting other-config:o_direct to true. Signed-off-by: Ross Lagerwall Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit 4be47e0b137652ef4799931a87bca6ea4ef0dd67) Conflicts: drivers/blktap2.py --- drivers/EXTSR.py | 2 ++ drivers/FileSR.py | 9 +++++++++ drivers/NFSSR.py | 2 ++ drivers/SRCommand.py | 3 ++- drivers/blktap2.py | 4 ++++ 5 files changed, 19 insertions(+), 1 deletion(-) diff --git a/drivers/EXTSR.py b/drivers/EXTSR.py index 1df87829..466a7b34 100755 --- a/drivers/EXTSR.py +++ b/drivers/EXTSR.py @@ -70,6 +70,8 @@ def load(self, sr_uuid): self.remotepath = os.path.join("/dev",self.vgname,sr_uuid) self.attached = self._checkmount() + self._check_o_direct() + def delete(self, sr_uuid): super(EXTSR, self).delete(sr_uuid) diff --git a/drivers/FileSR.py b/drivers/FileSR.py index fd8a6db3..b8c419a5 100755 --- a/drivers/FileSR.py +++ b/drivers/FileSR.py @@ -75,6 +75,11 @@ def handles(srtype): return srtype == 'file' handles = staticmethod(handles) + def _check_o_direct(self): + other_config = self.session.xenapi.SR.get_other_config(self.sr_ref) + o_direct = other_config.get("o_direct") + self.o_direct = o_direct is not None and o_direct == "true" + def load(self, sr_uuid): self.ops_exclusive = OPS_EXCLUSIVE self.lock = Lock(vhdutil.LOCK_TYPE_SR, self.uuid) @@ -85,6 +90,8 @@ def load(self, sr_uuid): self.path = os.path.join(SR.MOUNT_BASE, sr_uuid) self.attached = False + self._check_o_direct() + def create(self, sr_uuid, size): """ Create the SR. The path must not already exist, or if it does, it must be empty. (This accounts for the case where the user has @@ -381,6 +388,8 @@ class FileVDI(VDI.VDI): def load(self, vdi_uuid): self.lock = self.sr.lock + self.sr.srcmd.params['o_direct'] = self.sr.o_direct + if self.sr.srcmd.cmd == "vdi_create": self.vdi_type = vhdutil.VDI_TYPE_VHD if self.sr.srcmd.params.has_key("vdi_sm_config") and \ diff --git a/drivers/NFSSR.py b/drivers/NFSSR.py index 0feb3981..26d86749 100755 --- a/drivers/NFSSR.py +++ b/drivers/NFSSR.py @@ -91,6 +91,8 @@ def load(self, sr_uuid): if self.dconf.has_key('useUDP') and self.dconf['useUDP'] == 'true': self.transport = "udp" + self._check_o_direct() + def validate_remotepath(self, scan): if not self.dconf.has_key('serverpath'): diff --git a/drivers/SRCommand.py b/drivers/SRCommand.py index ce5ba1a3..d0e80459 100755 --- a/drivers/SRCommand.py +++ b/drivers/SRCommand.py @@ -185,7 +185,8 @@ def _run(self, sr, target): caching_params = dict((k, self.params.get(k)) for k in \ [blktap2.VDI.CONF_KEY_ALLOW_CACHING, blktap2.VDI.CONF_KEY_MODE_ON_BOOT, - blktap2.VDI.CONF_KEY_CACHE_SR]) + blktap2.VDI.CONF_KEY_CACHE_SR, + blktap2.VDI.CONF_KEY_O_DIRECT]) if self.cmd == 'vdi_create': # These are the fields owned by the backend, passed on the diff --git a/drivers/blktap2.py b/drivers/blktap2.py index a0a25b34..2e3a3ff6 100755 --- a/drivers/blktap2.py +++ b/drivers/blktap2.py @@ -958,6 +958,7 @@ class VDI(object): CONF_KEY_ALLOW_CACHING = "vdi_allow_caching" CONF_KEY_MODE_ON_BOOT = "vdi_on_boot" CONF_KEY_CACHE_SR = "local_cache_sr" + CONF_KEY_O_DIRECT = "o_direct" LOCK_CACHE_SETUP = "cachesetup" ATTACH_DETACH_RETRY_SECS = 120 @@ -1555,6 +1556,9 @@ def _activate(self, sr_uuid, vdi_uuid, options): # Maybe launch a tapdisk on the physical link if self.tap_wanted(): vdi_type = self.target.get_vdi_type() + o_direct = caching_params.get(self.CONF_KEY_O_DIRECT) + if o_direct is None: + o_direct = True dev_path = self._tap_activate(phy_path, vdi_type, sr_uuid, options, self._get_pool_config(sr_uuid).get("mem-pool-size")) From 589f2c3fd88ff56a681006a95f725977e257f8f8 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 27 May 2014 15:39:43 +0100 Subject: [PATCH 07/69] CA-136654: Don't rely on a XAPI session existing With static VDIs, the SR code is run without a XAPI session. In this case, set O_DIRECT to be on for safety since there is external knob to adjust it. Signed-off-by: Ross Lagerwall Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit 6c2d966ee492d756aa90dae84a0fc2298d4d4002) --- drivers/FileSR.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/FileSR.py b/drivers/FileSR.py index b8c419a5..a3b7458f 100755 --- a/drivers/FileSR.py +++ b/drivers/FileSR.py @@ -76,9 +76,12 @@ def handles(srtype): handles = staticmethod(handles) def _check_o_direct(self): - other_config = self.session.xenapi.SR.get_other_config(self.sr_ref) - o_direct = other_config.get("o_direct") - self.o_direct = o_direct is not None and o_direct == "true" + if self.sr_ref and self.session is not None: + other_config = self.session.xenapi.SR.get_other_config(self.sr_ref) + o_direct = other_config.get("o_direct") + self.o_direct = o_direct is not None and o_direct == "true" + else: + self.o_direct = True def load(self, sr_uuid): self.ops_exclusive = OPS_EXCLUSIVE From 6f49ca27d6ddba153a2a88ac741239e2f1ca41d5 Mon Sep 17 00:00:00 2001 From: Chandrika Srinivasan Date: Wed, 4 Jun 2014 17:45:38 +0100 Subject: [PATCH 08/69] CP-8514: Add xapi-plugin 'trim' to facilitate trim The xapi plugin 'trim' could be used to enable trim on LVM based SRs to free up storage space from Storage arrays. The plugin creates a LV using all of the free space in the VG, and then issue a lvremove setting the issue_discards to 1. lvremove should trigger a trim / unmap by lvm that should result in freeing up the storage space in the array Signed-off-by: Vineeth Thampi Raveendran Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit a34ef9c15f9b98c3b4c0d5cba74dafaa430f76cc) Conflicts: drivers/lvutil.py --- Makefile | 1 + drivers/lvutil.py | 28 +++++++++++++++---- drivers/trim | 70 +++++++++++++++++++++++++++++++++++++++++++++++ mk/sm.spec.in | 1 + 4 files changed, 94 insertions(+), 6 deletions(-) create mode 100755 drivers/trim diff --git a/Makefile b/Makefile index d00d4a61..938b075d 100755 --- a/Makefile +++ b/Makefile @@ -158,6 +158,7 @@ install: precheck install -m 755 drivers/tapdisk-pause $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) install -m 755 drivers/vss_control $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) install -m 755 drivers/intellicache-clean $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) + install -m 755 drivers/trim $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) ln -sf $(PLUGIN_SCRIPT_DEST)vss_control $(SM_STAGING)$(SM_DEST) install -m 755 drivers/iscsilib.py $(SM_STAGING)$(SM_DEST) mkdir -p $(SM_STAGING)$(LIBEXEC) diff --git a/drivers/lvutil.py b/drivers/lvutil.py index 43ee6ec2..67ac5792 100755 --- a/drivers/lvutil.py +++ b/drivers/lvutil.py @@ -400,18 +400,22 @@ def setActiveVG(path, active): cmd = [CMD_VGCHANGE, "-a" + val, path] text = util.pread2(cmd) -def create(name, size, vgname, tag = None): - size_mb = size / 1024 / 1024 - cmd = [CMD_LVCREATE, "-n", name, "-L", str(size_mb), vgname] +def create(name, size, vgname, tag=None, activate=True, + size_in_percentage=None): + if size_in_percentage: + cmd = [CMD_LVCREATE, "-n", name, "-l", size_in_percentage, vgname] + else: + size_mb = size / 1024 / 1024 + cmd = [CMD_LVCREATE, "-n", name, "-L", str(size_mb), vgname] if tag: cmd.extend(["--addtag", tag]) util.pread2(cmd) -def remove(path): +def remove(path, config_param=None): # see deactivateNoRefcount() for i in range(LVM_FAIL_RETRIES): try: - _remove(path) + _remove(path, config_param) break except util.CommandException, e: if i >= LVM_FAIL_RETRIES - 1: @@ -419,8 +423,11 @@ def remove(path): util.SMlog("*** lvremove failed on attempt #%d" % i) _lvmBugCleanup(path) -def _remove(path): +def _remove(path, config_param=None): + CONFIG_TAG = "--config" cmd = [CMD_LVREMOVE, "-f", path] + if config_param: + cmd.extend([CONFIG_TAG, "global{" + config_param + "}"]) ret = util.pread2(cmd) def rename(path, newName): @@ -434,6 +441,15 @@ def setReadonly(path, readonly): cmd = [CMD_LVCHANGE, path, "-p", val] ret = util.pread(cmd) +def exists(path): + cmd = [CMD_LVS, "--noheadings", path] + try: + ret = util.pread2(cmd) + return True + except util.CommandException, e: + util.SMlog("Ignoring exception for LV check: %s !" % path) + return False + #def getSize(path): # return _getLVsize(path) # #cmd = [CMD_LVS, "--noheadings", "--units", "B", path] diff --git a/drivers/trim b/drivers/trim new file mode 100755 index 00000000..e4dd9c27 --- /dev/null +++ b/drivers/trim @@ -0,0 +1,70 @@ +#!/usr/bin/python +# +# Copyright (C) Citrix Systems Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License as published +# by the Free Software Foundation; version 2.1 only. +# +# 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 Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser 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 +# +# A plugin for enabling trim on LVM based SRs to free up storage space +# in Storage arrays. + +import sys +import os +import XenAPIPlugin +sys.path.append("/opt/xensource/sm/") +import util +from lock import Lock +import lvhdutil +import vhdutil +import lvutil + +TRIM_LV_TAG = "_trim_lv" +def _vg_by_sr_uuid(sr_uuid): + return lvhdutil.VG_PREFIX + sr_uuid + +def _lvpath_by_vg_lv_name(vg_name, lv_name): + return os.path.join(lvhdutil.VG_LOCATION, vg_name, lv_name) + +def do_trim(session, args): + """Attempt to trim the given LVHDSR""" + util.SMlog("do_trim: %s" % args) + sr_uuid = args["sr_uuid"] + + # Lock SR, get vg empty space details + lock = Lock(vhdutil.LOCK_TYPE_SR, sr_uuid) + if lock.acquireNoblock(): + try: + vg_name = _vg_by_sr_uuid(sr_uuid) + lv_name = sr_uuid + TRIM_LV_TAG + lv_path = _lvpath_by_vg_lv_name(vg_name, lv_name) + + # Clean trim LV in case the previous trim attemp failed + if lvutil.exists(lv_path): + lvutil.remove(lv_path) + + # Perform a lvcreate and lvremove to trigger trim on the array + lvutil.create(lv_name, 0, vg_name, activate=True, + size_in_percentage="100%F") + lvutil.remove(lv_path, config_param="issue_discards=1") + util.SMlog("Trim on SR: %s complete. " % sr_uuid) + return str(True) + finally: + lock.release() + else: + util.SMlog("Could not complete Trim on %s, Lock unavailable !" \ + % sr_uuid) + return str(False) + + +if __name__ == "__main__": + XenAPIPlugin.dispatch({"do_trim": do_trim}) diff --git a/mk/sm.spec.in b/mk/sm.spec.in index 811acfa8..8eeaa5bc 100755 --- a/mk/sm.spec.in +++ b/mk/sm.spec.in @@ -71,6 +71,7 @@ cp -f /etc/lvm/lvm.conf.orig /etc/lvm/lvm.conf || exit $? /etc/xapi.d/plugins/testing-hooks /etc/xapi.d/plugins/vss_control /etc/xapi.d/plugins/intellicache-clean +/etc/xapi.d/plugins/trim /etc/xensource/master.d/02-vhdcleanup /opt/xensource/bin/blktap2 /opt/xensource/bin/tapdisk-cache-stats From 392ab49826b707886cb414d5399dd6b1f9c736d7 Mon Sep 17 00:00:00 2001 From: Vineeth Raveendran Date: Thu, 5 Jun 2014 14:50:45 +0000 Subject: [PATCH 09/69] CP-8514: issue_discards should be placed under devices section Signed-off-by: Vineeth Thampi Raveendran Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 6e6afbce763fff5ae135f6dfcd404b9d29696547) --- drivers/lvutil.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/lvutil.py b/drivers/lvutil.py index 67ac5792..1f314c0d 100755 --- a/drivers/lvutil.py +++ b/drivers/lvutil.py @@ -427,7 +427,7 @@ def _remove(path, config_param=None): CONFIG_TAG = "--config" cmd = [CMD_LVREMOVE, "-f", path] if config_param: - cmd.extend([CONFIG_TAG, "global{" + config_param + "}"]) + cmd.extend([CONFIG_TAG, "devices{" + config_param + "}"]) ret = util.pread2(cmd) def rename(path, newName): From 3c92d98b4d5d0b6ae0461ef079adfaff6c22032f Mon Sep 17 00:00:00 2001 From: Vineeth Raveendran Date: Mon, 16 Jun 2014 11:49:03 +0000 Subject: [PATCH 10/69] CP-8744: SR trim limited to compatible SRs Limited SR trim capability to LVHD based SRs. Also error handling was modified to return a XML string response on failure containing errcode and errmsg as a key value pair. Trim functionality was separated from the trim plugin entry point. Signed-off-by: Vineeth Raveendran Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 1ae07e0233cc021056404cafd64bffcb685a143d) Conflicts: Makefile --- Makefile | 1 + drivers/LVHDSR.py | 2 +- drivers/LVHDoHBASR.py | 8 +-- drivers/LVHDoISCSISR.py | 4 +- drivers/trim | 61 +++----------------- drivers/trim_util.py | 121 ++++++++++++++++++++++++++++++++++++++++ drivers/util.py | 15 +++++ mk/sm.spec.in | 3 + 8 files changed, 156 insertions(+), 59 deletions(-) create mode 100755 drivers/trim_util.py diff --git a/Makefile b/Makefile index 938b075d..b9456a20 100755 --- a/Makefile +++ b/Makefile @@ -56,6 +56,7 @@ SM_LIBS += lcache SM_LIBS += resetvdis SM_LIBS += B_util SM_LIBS += wwid_conf +SM_LIBS += trim_util UDEV_RULES = 40-multipath MPATH_DAEMON = sm-multipath diff --git a/drivers/LVHDSR.py b/drivers/LVHDSR.py index 80ab2475..d0fe17da 100755 --- a/drivers/LVHDSR.py +++ b/drivers/LVHDSR.py @@ -52,7 +52,7 @@ DEV_MAPPER_ROOT = os.path.join('/dev/mapper', lvhdutil.VG_PREFIX) geneology = {} -CAPABILITIES = ["SR_PROBE","SR_UPDATE", +CAPABILITIES = ["SR_PROBE","SR_UPDATE", "SR_TRIM", "VDI_CREATE","VDI_DELETE","VDI_ATTACH", "VDI_DETACH", "VDI_CLONE", "VDI_SNAPSHOT", "VDI_RESIZE", "ATOMIC_PAUSE", "VDI_RESET_ON_BOOT/2", "VDI_UPDATE"] diff --git a/drivers/LVHDoHBASR.py b/drivers/LVHDoHBASR.py index c4a1eaa7..1914b2cd 100755 --- a/drivers/LVHDoHBASR.py +++ b/drivers/LVHDoHBASR.py @@ -31,10 +31,10 @@ import glob import mpp_luncheck -CAPABILITIES = ["SR_PROBE", "SR_UPDATE", "SR_METADATA", "VDI_CREATE", - "VDI_DELETE", "VDI_ATTACH", "VDI_DETACH", - "VDI_GENERATE_CONFIG", "VDI_SNAPSHOT", "VDI_CLONE", - "VDI_RESIZE", "ATOMIC_PAUSE", "VDI_RESET_ON_BOOT/2", +CAPABILITIES = ["SR_PROBE", "SR_UPDATE", "SR_METADATA", "SR_TRIM", + "VDI_CREATE", "VDI_DELETE", "VDI_ATTACH", "VDI_DETACH", + "VDI_GENERATE_CONFIG", "VDI_SNAPSHOT", "VDI_CLONE", + "VDI_RESIZE", "ATOMIC_PAUSE", "VDI_RESET_ON_BOOT/2", "VDI_UPDATE"] CONFIGURATION = [ [ 'SCSIid', 'The scsi_id of the destination LUN' ], \ diff --git a/drivers/LVHDoISCSISR.py b/drivers/LVHDoISCSISR.py index c09244c9..d8e70bf2 100755 --- a/drivers/LVHDoISCSISR.py +++ b/drivers/LVHDoISCSISR.py @@ -29,8 +29,8 @@ import scsiutil import xml.dom.minidom -CAPABILITIES = ["SR_PROBE", "SR_UPDATE", "SR_METADATA", "VDI_CREATE", - "VDI_DELETE", "VDI_ATTACH", "VDI_DETACH", +CAPABILITIES = ["SR_PROBE", "SR_UPDATE", "SR_METADATA", "SR_TRIM", + "VDI_CREATE", "VDI_DELETE", "VDI_ATTACH", "VDI_DETACH", "VDI_GENERATE_CONFIG", "VDI_CLONE", "VDI_SNAPSHOT", "VDI_RESIZE", "ATOMIC_PAUSE", "VDI_RESET_ON_BOOT/2", "VDI_UPDATE"] diff --git a/drivers/trim b/drivers/trim index e4dd9c27..1f4726f7 100755 --- a/drivers/trim +++ b/drivers/trim @@ -2,69 +2,26 @@ # # Copyright (C) Citrix Systems Inc. # -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU Lesser General Public License as published +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License as published # by the Free Software Foundation; version 2.1 only. # -# 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 +# 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 Lesser General Public License for more details. # # You should have received a copy of the GNU Lesser 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 # -# A plugin for enabling trim on LVM based SRs to free up storage space +# A plugin for enabling trim on LVM based SRs to free up storage space # in Storage arrays. import sys -import os import XenAPIPlugin -sys.path.append("/opt/xensource/sm/") -import util -from lock import Lock -import lvhdutil -import vhdutil -import lvutil - -TRIM_LV_TAG = "_trim_lv" -def _vg_by_sr_uuid(sr_uuid): - return lvhdutil.VG_PREFIX + sr_uuid - -def _lvpath_by_vg_lv_name(vg_name, lv_name): - return os.path.join(lvhdutil.VG_LOCATION, vg_name, lv_name) - -def do_trim(session, args): - """Attempt to trim the given LVHDSR""" - util.SMlog("do_trim: %s" % args) - sr_uuid = args["sr_uuid"] - - # Lock SR, get vg empty space details - lock = Lock(vhdutil.LOCK_TYPE_SR, sr_uuid) - if lock.acquireNoblock(): - try: - vg_name = _vg_by_sr_uuid(sr_uuid) - lv_name = sr_uuid + TRIM_LV_TAG - lv_path = _lvpath_by_vg_lv_name(vg_name, lv_name) - - # Clean trim LV in case the previous trim attemp failed - if lvutil.exists(lv_path): - lvutil.remove(lv_path) - - # Perform a lvcreate and lvremove to trigger trim on the array - lvutil.create(lv_name, 0, vg_name, activate=True, - size_in_percentage="100%F") - lvutil.remove(lv_path, config_param="issue_discards=1") - util.SMlog("Trim on SR: %s complete. " % sr_uuid) - return str(True) - finally: - lock.release() - else: - util.SMlog("Could not complete Trim on %s, Lock unavailable !" \ - % sr_uuid) - return str(False) - if __name__ == "__main__": - XenAPIPlugin.dispatch({"do_trim": do_trim}) + sys.path.append("/opt/xensource/sm/") + import trim_util + XenAPIPlugin.dispatch({"do_trim": trim_util.do_trim}) diff --git a/drivers/trim_util.py b/drivers/trim_util.py new file mode 100755 index 00000000..997a3461 --- /dev/null +++ b/drivers/trim_util.py @@ -0,0 +1,121 @@ +#!/usr/bin/python +# +# Copyright (C) Citrix Systems Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License as published +# by the Free Software Foundation; version 2.1 only. +# +# 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 Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser 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 +# +# A plugin for enabling trim on LVM based SRs to free up storage space +# in Storage arrays. + +import xml +import sys +import os +import time +import util +import lock +import lvhdutil +import vhdutil +import lvutil +import xs_errors +import xmlrpclib + +TRIM_LV_TAG = "_trim_lv" +TRIM_CAP = "SR_TRIM" +LOCK_RETRY_ATTEMPTS = 3 +LOCK_RETRY_INTERVAL = 1 +ERROR_CODE_KEY = "errcode" +ERROR_MSG_KEY = "errmsg" + + +def _vg_by_sr_uuid(sr_uuid): + return lvhdutil.VG_PREFIX + sr_uuid + +def _lvpath_by_vg_lv_name(vg_name, lv_name): + return os.path.join(lvhdutil.VG_LOCATION, vg_name, lv_name) + +def to_xml(d): + + dom = xml.dom.minidom.Document() + trim_response = dom.createElement("trim_response") + dom.appendChild(trim_response) + + for key, value in sorted(d.items()): + key_value_element = dom.createElement("key_value_pair") + trim_response.appendChild(key_value_element) + + key_element = dom.createElement("key") + key_text_node = dom.createTextNode(key) + key_element.appendChild(key_text_node) + key_value_element.appendChild(key_element) + + value_element = dom.createElement("value") + value_text_mode = dom.createTextNode(value) + value_element.appendChild(value_text_mode) + key_value_element.appendChild(value_element) + + + return dom.toxml() + +def do_trim(session, args): + """Attempt to trim the given LVHDSR""" + util.SMlog("do_trim: %s" % args) + sr_uuid = args["sr_uuid"] + + if TRIM_CAP not in util.sr_get_capability(sr_uuid): + util.SMlog("Trim command ignored on unsupported SR %s" % sr_uuid) + err_msg = {ERROR_CODE_KEY: 'UnsupportedSRForTrim', + ERROR_MSG_KEY: 'Trim on [%s] not supported' % sr_uuid} + return to_xml(err_msg) + + # Lock SR, get vg empty space details + sr_lock = lock.Lock(vhdutil.LOCK_TYPE_SR, sr_uuid) + got_lock = False + for i in range(LOCK_RETRY_ATTEMPTS): + got_lock = sr_lock.acquireNoblock() + if got_lock: + break + time.sleep(LOCK_RETRY_INTERVAL) + + if got_lock: + try: + vg_name = _vg_by_sr_uuid(sr_uuid) + lv_name = sr_uuid + TRIM_LV_TAG + lv_path = _lvpath_by_vg_lv_name(vg_name, lv_name) + + # Clean trim LV in case the previous trim attemp failed + if lvutil.exists(lv_path): + lvutil.remove(lv_path) + + # Perform a lvcreate and lvremove to trigger trim on the array + lvutil.create(lv_name, 0, vg_name, activate=True, + size_in_percentage="100%F") + lvutil.remove(lv_path, config_param="issue_discards=1") + util.SMlog("Trim on SR: %s complete. " % sr_uuid) + result = str(True) + except: + err_msg = { + ERROR_CODE_KEY: 'UnknownTrimException', + ERROR_MSG_KEY: 'Unknown Exception: trim failed on SR [%s]' + % sr_uuid + } + result = to_xml(err_msg) + + sr_lock.release() + return result + else: + util.SMlog("Could not complete Trim on %s, Lock unavailable !" \ + % sr_uuid) + err_msg = {ERROR_CODE_KEY: 'SRUnavailable', + ERROR_MSG_KEY: 'Unable to get SR lock [%s]' % sr_uuid} + return to_xml(err_msg) diff --git a/drivers/util.py b/drivers/util.py index 485b7379..7dc39a50 100755 --- a/drivers/util.py +++ b/drivers/util.py @@ -314,6 +314,21 @@ def ioretry_stat(f, maxretry=IORETRY_MAX): retries += 1 raise CommandException(errno.EIO, str(f)) +def sr_get_capability(sr_uuid): + result = [] + session = get_localAPI_session() + sr_ref = session.xenapi.SR.get_by_uuid(sr_uuid) + sm_type = session.xenapi.SR.get_record(sr_ref)['type'] + sm_rec = session.xenapi.SM.get_all_records_where( \ + "field \"type\" = \"%s\"" % sm_type) + + # SM expects atleast one entry of any SR type + if len(sm_rec) > 0: + result = sm_rec.values()[0]['capabilities'] + + session.xenapi.logout() + return result + def sr_get_driver_info(driver_info): results = {} # first add in the vanilla stuff diff --git a/mk/sm.spec.in b/mk/sm.spec.in index 8eeaa5bc..2b11f993 100755 --- a/mk/sm.spec.in +++ b/mk/sm.spec.in @@ -256,6 +256,9 @@ cp -f /etc/lvm/lvm.conf.orig /etc/lvm/lvm.conf || exit $? /opt/xensource/sm/vhdutil.py /opt/xensource/sm/vhdutil.pyc /opt/xensource/sm/vhdutil.pyo +/opt/xensource/sm/trim_util.py +/opt/xensource/sm/trim_util.pyc +/opt/xensource/sm/trim_util.pyo /opt/xensource/sm/vss_control /opt/xensource/sm/xs_errors.py /opt/xensource/sm/xs_errors.pyc From a85cb7450c18cc71e7b3fc7393ff37a0f3444144 Mon Sep 17 00:00:00 2001 From: Vineeth Raveendran Date: Mon, 16 Jun 2014 11:54:46 +0000 Subject: [PATCH 11/69] CP-8744: [UnitTest] Add unittest for trim plugin Signed-off-by: Mate Lakat Reviewed-by: Vineeth Raveendran Imported-by: Thanos Makatos (cherry picked from commit b2db2a9b76c1110370fd8c9e31bdd73ba60c4f0b) --- tests/test_trim_util.py | 248 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 248 insertions(+) create mode 100644 tests/test_trim_util.py diff --git a/tests/test_trim_util.py b/tests/test_trim_util.py new file mode 100644 index 00000000..1f72a2cf --- /dev/null +++ b/tests/test_trim_util.py @@ -0,0 +1,248 @@ +import unittest +import trim_util +import testlib + +import mock + + +class AlwaysBusyLock(object): + def acquireNoblock(self): + return False + + +class AlwaysFreeLock(object): + def __init__(self): + self.acquired = False + + def acquireNoblock(self): + self.acquired = True + return True + + def release(self): + self.acquired = False + + +class TestTrimUtil(unittest.TestCase, testlib.XmlMixIn): + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_error_code_trim_not_supported(self, + context, + sr_get_capability): + sr_get_capability.return_value = [] + context.setup_error_codes() + + result = trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertXML(""" + + + + errcode + UnsupportedSRForTrim + + + errmsg + Trim on [some-uuid] not supported + + + """, result) + + @mock.patch('time.sleep') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_unable_to_obtain_lock_on_sr(self, + context, + sr_get_capability, + MockLock, + sleep): + MockLock.return_value = AlwaysBusyLock() + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + result = trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertXML(""" + + + + errcode + SRUnavailable + + + errmsg + Unable to get SR lock [some-uuid] + + + """, result) + + @mock.patch('time.sleep') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_sleeps_a_sec_and_retries_three_times(self, + context, + sr_get_capability, + MockLock, + sleep): + MockLock.return_value = AlwaysBusyLock() + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertEquals([ + mock.call(1), + mock.call(1), + mock.call(1) + ], + sleep.mock_calls + ) + + @mock.patch('trim_util.lvutil') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_creates_an_lv(self, + context, + sr_get_capability, + MockLock, + lvutil): + MockLock.return_value = AlwaysFreeLock() + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + lvutil.create.assert_called_once_with( + 'some-uuid_trim_lv', 0, 'VG_XenStorage-some-uuid', + size_in_percentage='100%F', activate=True + ) + + @mock.patch('trim_util.lvutil') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_removes_lv_no_leftover_trim_vol(self, + context, + sr_get_capability, + MockLock, + lvutil): + lvutil.exists.return_value = False + MockLock.return_value = AlwaysFreeLock() + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + lvutil.remove.assert_called_once_with( + '/dev/VG_XenStorage-some-uuid/some-uuid_trim_lv', + config_param='issue_discards=1') + + @mock.patch('trim_util.lvutil') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_releases_lock(self, + context, + sr_get_capability, + MockLock, + lvutil): + lvutil.exists.return_value = False + sr_lock = MockLock.return_value = AlwaysFreeLock() + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertFalse(sr_lock.acquired) + + @mock.patch('trim_util.lvutil') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_removes_lv_with_leftover_trim_vol(self, + context, + sr_get_capability, + MockLock, + lvutil): + lvutil.exists.return_value = True + MockLock.return_value = AlwaysFreeLock() + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertEquals([ + mock.call('/dev/VG_XenStorage-some-uuid/some-uuid_trim_lv'), + mock.call( + '/dev/VG_XenStorage-some-uuid/some-uuid_trim_lv', + config_param='issue_discards=1') + ], lvutil.remove.mock_calls) + + @mock.patch('trim_util.lvutil') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_lock_released_even_if_exception_raised(self, + context, + sr_get_capability, + MockLock, + lvutil): + lvutil.exists.side_effect = Exception('blah') + srlock = AlwaysFreeLock() + MockLock.return_value = srlock + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertFalse(srlock.acquired) + + @mock.patch('trim_util.lvutil') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_when_exception_then_returns_generic_err(self, + context, + sr_get_capability, + MockLock, + lvutil): + lvutil.exists.side_effect = Exception('blah') + srlock = AlwaysFreeLock() + MockLock.return_value = srlock + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + result = trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertXML(""" + + + + errcode + UnknownTrimException + + + errmsg + Unknown Exception: trim failed on SR [some-uuid] + + + """, result) + + @mock.patch('trim_util.lvutil') + @mock.patch('lock.Lock') + @mock.patch('util.sr_get_capability') + @testlib.with_context + def test_do_trim_when_trim_succeeded_returns_true(self, + context, + sr_get_capability, + MockLock, + lvutil): + MockLock.return_value = AlwaysFreeLock() + sr_get_capability.return_value = [trim_util.TRIM_CAP] + context.setup_error_codes() + + result = trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) + + self.assertEquals('True', result) From 32cef4164f2531ac5f1aa50dbebe8bdfda5674cc Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Fri, 21 Nov 2014 10:45:43 +0000 Subject: [PATCH 12/69] CP-8657: Remove iSL references from storage manager. - Removed all code references - Removed all CSLG error codes - Replaced CSLGXMLParse to XMLParse Signed-off-by: Siddharth Vinothkumar Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 0b1e794152b8a97746985776edc5388e1b841ee4) Methods util.get_isl_scsiids, util.get_scsiid_from_svid, and util.getCslDevPath have been kept back as they are used by OCFS. Conflicts: drivers/mpathcount.py --- drivers/HBASR.py | 8 +- drivers/LVHDoISCSISR.py | 4 - drivers/XE_SR_ERRORCODES.xml | 181 +---------------------------------- drivers/mpathcount.py | 2 +- 4 files changed, 5 insertions(+), 190 deletions(-) diff --git a/drivers/HBASR.py b/drivers/HBASR.py index 017ecd30..cb6bbcad 100755 --- a/drivers/HBASR.py +++ b/drivers/HBASR.py @@ -41,8 +41,6 @@ 'configuration': CONFIGURATION } -HBA_CLI = "/opt/Citrix/StorageLink/bin/csl_hbascan" - class HBASR(SR.SR): """HBA storage repository""" def handles(type): @@ -82,7 +80,7 @@ def _init_hba_hostname(self): nodewwnval = str(nodewwn[0].firstChild.nodeValue) break except: - raise xs_errors.XenError('CSLGXMLParse', opterr='HBA Host WWN scanning failed') + raise xs_errors.XenError('XMLParse', opterr='HBA Host WWN scanning failed') return nodewwnval def _init_hbas(self): @@ -104,7 +102,7 @@ def _init_hbas(self): devpath = str(devnames[0].firstChild.nodeValue).split('/')[-1] adt[devpath] = portval.split()[0] except: - raise xs_errors.XenError('CSLGXMLParse', \ + raise xs_errors.XenError('XMLParse', \ opterr='HBA scanning failed') return adt @@ -166,7 +164,7 @@ def _probe_hba(self): return dom.toxml() except: - raise xs_errors.XenError('CSLGXMLParse', \ + raise xs_errors.XenError('XMLParse', \ opterr='HBA probe failed') def attach(self, sr_uuid): diff --git a/drivers/LVHDoISCSISR.py b/drivers/LVHDoISCSISR.py index d8e70bf2..9ac1405d 100755 --- a/drivers/LVHDoISCSISR.py +++ b/drivers/LVHDoISCSISR.py @@ -320,12 +320,8 @@ def print_LUNs_XML(self): dom = xml.dom.minidom.Document() element = dom.createElement("iscsi-target") dom.appendChild(element) - # Omit the scsi-id used by iSL - isl_scsiids = util.get_isl_scsiids(self.session) for uuid in self.LUNs: val = self.LUNs[uuid] - if getattr(val,'SCSIid') in isl_scsiids: - continue entry = dom.createElement('LUN') element.appendChild(entry) diff --git a/drivers/XE_SR_ERRORCODES.xml b/drivers/XE_SR_ERRORCODES.xml index 181d347b..70c317ac 100755 --- a/drivers/XE_SR_ERRORCODES.xml +++ b/drivers/XE_SR_ERRORCODES.xml @@ -708,195 +708,16 @@ 203 - - CSLGConfigServerMissing - The CSLG server name or IP address is missing - 400 - - - CSLGConfigSSIDMissing - The Storage System ID is missing - 401 - - - CSLGConfigPoolIDMissing - The Storage Pool ID is missing - 402 - - - - CSLGProtocolCheck - The gssi operation to the CSLG server failed - 410 - - - CSLGLoadSR - The SR loading operation failed - 411 - - - CSLGInvalidProtocol - An invalid storage protocol was specified - 412 - - - CSLGXMLParse + XMLParse Unable to parse XML 413 - - CSLGProbe - Failed to probe SR - 414 - - - CSLGSnapClone - Snapshot/Clone failed - 416 - - - CSLGAssign - Storage assignment failed - 417 - - - CSLGUnassign - Storage un-assignment failed - 418 - - - CSLGAllocate - Storage allocation failed - 419 - - - CSLGDeallocate - Storage deallocation failed - 420 - - - CSLGHBAQuery - HBA Query failed - 421 - - - CSLGISCSIInit - IQN/ISCSI initialisation failed - 422 - - - CSLGDeviceScan - SCSI device scan failed - 423 - - - CSLGServer - Failed to connect to Target, Please verify hostname or IP address - 424 - - - CSLGConfigSVIDMissing - The Storage Node ID is missing - 425 - - - CSLGIntroduce - The VDI failed to be introduced to the database - 426 - - - CSLGNotInstalled - The CSLG software doesn't seem to be installed - 427 - - - CSLGPoolCreate - Failed to create sub-pools from parent pool - 428 - - - CSLGOldXML - Current XML definition is newer version - 429 - MultipathdCommsFailure Failed to communicate with the multipath daemon 430 - - CSL_Integrated - Error in storage adapter communication - 431 - - - CSLGConfigAdapterMissing - The adapter id is missing or unknown - 432 - - - CSLGConfigUsernameMissing - The username is missing - 433 - - - CSLGConfigPasswordMissing - The password is missing - 434 - - - CSLGInvalidSSID - An invalid storage system ID was specified - 435 - - - CSLGSysInfo - Failed to collect storage system information - 436 - - - CSLGPoolInfo - Failed to collect storage pool information - 437 - - - CSLGPoolDelete - Failed to delete storage pool - 438 - - - CSLGLunInfo - Failed to collect storage volume information - 439 - - - CSLGLunList - Failed to list storage volume - 440 - - - CSLGResizeLun - Failed to resize storage volume - 441 - - - CSLGTargetPorts - Failed to list storage target ports - 442 - - - CSLGPoolList - Failed to list storage pool - 443 - - - TapdiskFailed - The tapdisk failed - 444 - - - TapdiskAlreadyRunning The tapdisk is already running 445 diff --git a/drivers/mpathcount.py b/drivers/mpathcount.py index 966c4e78..92e8dbd2 100755 --- a/drivers/mpathcount.py +++ b/drivers/mpathcount.py @@ -24,7 +24,7 @@ import mpp_mpathutil import glob -supported = ['iscsi','lvmoiscsi','rawhba','lvmohba', 'ocfsohba', 'ocfsoiscsi', 'netapp','cslg'] +supported = ['iscsi','lvmoiscsi','rawhba','lvmohba', 'ocfsohba', 'ocfsoiscsi', 'netapp'] LOCK_TYPE_HOST = "host" LOCK_NS1 = "mpathcount1" From 451d08e8707d824517e5397063524471e1a56c0f Mon Sep 17 00:00:00 2001 From: Chandrika Srinivasan Date: Wed, 18 Jun 2014 11:47:37 +0100 Subject: [PATCH 13/69] CP-8657: Fix XE_SR_ERRORCODES.xml Signed-off-by: Chandrika Srinivasan Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 004465eb3d9506d0c0707414ef693abe76f0ad92) --- drivers/XE_SR_ERRORCODES.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/XE_SR_ERRORCODES.xml b/drivers/XE_SR_ERRORCODES.xml index 70c317ac..647b2537 100755 --- a/drivers/XE_SR_ERRORCODES.xml +++ b/drivers/XE_SR_ERRORCODES.xml @@ -718,6 +718,7 @@ Failed to communicate with the multipath daemon 430 + TapdiskAlreadyRunning The tapdisk is already running 445 From 9ae98eebbebb216e15dd68113bf5f3d3ff417eb4 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 10 Jun 2014 09:44:37 +0100 Subject: [PATCH 14/69] CA-134784: [UnitTest] Mimic rmdir, open('w') Mimic the behavior of rmdir, and also raise the appropriate exceptions if client wants to open a file in a non-existing directory. Enable the client to open files with 'w' flag. Signed-off-by: Mate Lakat Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 3090c20534f6a900cea1489f72ba1e3d729d1374) --- tests/test_testlib.py | 44 +++++++++++++++++++++++++++++++++++++++++++ tests/testlib.py | 18 +++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/tests/test_testlib.py b/tests/test_testlib.py index 881bd6b3..cd186e3c 100644 --- a/tests/test_testlib.py +++ b/tests/test_testlib.py @@ -1,6 +1,7 @@ import unittest import os import mock +import errno import testlib @@ -225,6 +226,16 @@ def test_write_a_file(self, context): ('/blah/subdir/somefile', 'hello') in list(context.generate_path_content())) + @testlib.with_context + def test_write_a_file_in_non_existing_dir(self, context): + import os + + try: + open('/blah/subdir/somefile', 'w') + raise AssertionError('No exception raised') + except IOError, e: + self.assertEquals(errno.ENOENT, e.errno) + @testlib.with_context def test_file_returns_an_object_with_fileno_callable(self, context): f = file('/file', 'w+') @@ -314,6 +325,39 @@ def somefunction(firstparam, context): self.assertEquals(original_open, os.open) + @testlib.with_context + def test_rmdir_is_replaced_with_a_fake(self, context): + self.assertEquals(context.fake_rmdir, os.rmdir) + + def test_rmdir_raises_error_if_dir_not_found(self): + context = testlib.TestContext() + + try: + context.fake_rmdir('nonexisting') + raise AssertionError('No Exception raised') + except OSError, e: + self.assertEquals(errno.ENOENT, e.errno) + + def test_rmdir_removes_dir_if_found(self): + context = testlib.TestContext() + + context.fake_makedirs('/existing_dir') + + context.fake_rmdir('/existing_dir') + + self.assertFalse(context.fake_exists('/existing_dir')) + + def test_rmdir_raises_exception_if_dir_is_not_empty(self): + context = testlib.TestContext() + + context.fake_makedirs('/existing_dir/somefile') + + try: + context.fake_rmdir('/existing_dir') + raise AssertionError('No Exception raised') + except OSError, e: + self.assertEquals(errno.ENOTEMPTY, e.errno) + class TestFilesystemFor(unittest.TestCase): def test_returns_single_item_for_root(self): diff --git a/tests/testlib.py b/tests/testlib.py index acbbae05..642ebb7a 100644 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -6,6 +6,7 @@ import string import random import textwrap +import errno PATHSEP = '/' @@ -106,10 +107,22 @@ def start(self): mock.patch('glob.glob', new=self.fake_glob), mock.patch('os.uname', new=self.fake_uname), mock.patch('subprocess.Popen', new=self.fake_popen), + mock.patch('os.rmdir', new=self.fake_rmdir), ] map(lambda patcher: patcher.start(), self.patchers) self.setup_modinfo() + def fake_rmdir(self, path): + if path not in self.get_filesystem(): + raise OSError(errno.ENOENT, 'No such file %s' % path) + + if self.fake_glob(os.path.join(path, '*')): + raise OSError(errno.ENOTEMPTY, 'Directory is not empty %s' % path) + + assert path in self._created_directories + self._created_directories = [ + d for d in self._created_directories if d != path] + def fake_makedirs(self, path): if path in self.get_filesystem(): raise OSError(path + " Already exists") @@ -164,10 +177,13 @@ def fake_open(self, fname, mode='r'): if fpath == fname: return StringIO.StringIO(contents) - if mode == 'w+': + if 'w' in mode: if os.path.dirname(fname) in self.get_created_directories(): self._path_content[fname] = '' return WriteableFile(self, fname, self._get_inc_fileno()) + error = IOError('No such file %s' % fname) + error.errno = errno.ENOENT + raise error self.log('tried to open file', fname) raise IOError(fname) From 77a24f832618d41e0ad8dcd197d013e5a4cbbe32 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 10 Jun 2014 09:45:10 +0100 Subject: [PATCH 15/69] CA-134784: [UnitTest] cover refcounter Signed-off-by: Mate Lakat Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 019f1404e60f21ff632af14c386428b1449c88e3) --- tests/test_refcouter.py | 60 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 tests/test_refcouter.py diff --git a/tests/test_refcouter.py b/tests/test_refcouter.py new file mode 100644 index 00000000..396efb0b --- /dev/null +++ b/tests/test_refcouter.py @@ -0,0 +1,60 @@ +import unittest +import testlib +import os + +import refcounter + + +class TestRefCounter(unittest.TestCase): + @testlib.with_context + def test_get_whencalled_creates_namespace(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + refcounter.RefCounter.get('not-important', False, 'somenamespace') + + self.assertEquals( + ['somenamespace'], + os.listdir(os.path.join(refcounter.RefCounter.BASE_DIR))) + + @testlib.with_context + def test_get_whencalled_returns_counters(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + result = refcounter.RefCounter.get('not-important', False, 'somenamespace') + + self.assertEquals(1, result) + + @testlib.with_context + def test_get_whencalled_creates_refcounter_file(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + refcounter.RefCounter.get('someobject', False, 'somenamespace') + + self.assertEquals( + ['someobject'], + os.listdir(os.path.join( + refcounter.RefCounter.BASE_DIR, 'somenamespace'))) + + @testlib.with_context + def test_get_whencalled_refcounter_file_contents(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + refcounter.RefCounter.get('someobject', False, 'somenamespace') + + path_to_refcounter = os.path.join( + refcounter.RefCounter.BASE_DIR, 'somenamespace', 'someobject') + + refcounter_file = open(path_to_refcounter, 'r') + contents = refcounter_file.read() + refcounter_file.close() + + self.assertEquals('1 0\n', contents) + + @testlib.with_context + def test_put_is_noop_if_already_zero(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + result = refcounter.RefCounter.put( + 'someobject', False, 'somenamespace') + + self.assertEquals(0, result) From 89ca7f82e49f2136ec4cc9cd9eee3babba7c166e Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 10 Jun 2014 01:36:01 -0400 Subject: [PATCH 16/69] CA-134784: Fix race conditions in refcounter Concurrent refcounter operations on two different refcounters sharing the same namespace can lead to race conditions: A.) By the time the refcounter file is written, the namespace directory has already been removed B.) Removing a namespace directory fails, because a a new refcounter file has been created within the same directory _writeCount now returns False if the file was not found A is solved by spinning on _writeCount until it succeeds. B is solved by ignoring the errors Signed-off-by: Mate Lakat Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit 63491d7b74e864436715e2a77d921be495445cbe) Conflicts: drivers/refcounter.py --- drivers/refcounter.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/refcounter.py b/drivers/refcounter.py index 1c732176..0e4c8952 100644 --- a/drivers/refcounter.py +++ b/drivers/refcounter.py @@ -150,9 +150,11 @@ def _set(ns, obj, count, binaryCount): if count == 0 and binaryCount == 0: RefCounter._removeObject(ns, obj) else: - RefCounter._createNamespace(ns) objFile = os.path.join(RefCounter.BASE_DIR, ns, obj) - RefCounter._writeCount(objFile, count, binaryCount) + + while not RefCounter._writeCount(objFile, count, binaryCount): + RefCounter._createNamespace(ns) + _set = staticmethod(_set) def _getSafeNames(obj, ns): @@ -185,13 +187,16 @@ def _removeObject(ns, obj): os.unlink(objFile) except OSError: raise RefCounterException("failed to remove '%s'" % objFile) - + try: os.rmdir(nsDir) except OSError, e: - # Having a listdir wont help since there could be other vdi related - # operations that could create a file in between the python calls. - if e.errno != errno.ENOTEMPTY: + namespaceAlreadyCleanedUp = e.errno == errno.ENOENT + newObjectAddedToNamespace = e.errno == errno.ENOTEMPTY + + if namespaceAlreadyCleanedUp or newObjectAddedToNamespace: + pass + else: raise RefCounterException("failed to remove '%s'" % nsDir) _removeObject = staticmethod(_removeObject) @@ -230,7 +235,11 @@ def _writeCount(fn, count, binaryCount): f = open(fn, 'w') f.write("%d %d\n" % (count, binaryCount)) f.close() + return True except IOError, e: + fileNotFound = e.errno == errno.ENOENT + if fileNotFound: + return False raise RefCounterException("failed to write '(%d %d)' to '%s': %s" \ % (count, binaryCount, fn, e)) _writeCount = staticmethod(_writeCount) From 51f3c457201a5cff0bcf8f2174ab7afa5ac23a26 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Wed, 11 Jun 2014 07:13:55 +0100 Subject: [PATCH 17/69] CA-134784: [UnitTest] Tests for refcounter Signed-off-by: Mate Lakat Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit f5693a167e271e90b09f34b10f68c571171e186a) --- tests/test_refcounter.py | 105 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 tests/test_refcounter.py diff --git a/tests/test_refcounter.py b/tests/test_refcounter.py new file mode 100644 index 00000000..c52b0cf7 --- /dev/null +++ b/tests/test_refcounter.py @@ -0,0 +1,105 @@ +import unittest +import testlib +import os +import mock +import errno + +import refcounter + + +class TestRefCounter(unittest.TestCase): + @testlib.with_context + def test_get_whencalled_creates_namespace(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + refcounter.RefCounter.get('not-important', False, 'somenamespace') + + self.assertEquals( + ['somenamespace'], + os.listdir(os.path.join(refcounter.RefCounter.BASE_DIR))) + + @testlib.with_context + def test_get_whencalled_returns_counters(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + result = refcounter.RefCounter.get( + 'not-important', False, 'somenamespace') + + self.assertEquals(1, result) + + @testlib.with_context + def test_get_whencalled_creates_refcounter_file(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + refcounter.RefCounter.get('someobject', False, 'somenamespace') + + self.assertEquals( + ['someobject'], + os.listdir(os.path.join( + refcounter.RefCounter.BASE_DIR, 'somenamespace'))) + + @testlib.with_context + def test_get_whencalled_refcounter_file_contents(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + refcounter.RefCounter.get('someobject', False, 'somenamespace') + + path_to_refcounter = os.path.join( + refcounter.RefCounter.BASE_DIR, 'somenamespace', 'someobject') + + refcounter_file = open(path_to_refcounter, 'r') + contents = refcounter_file.read() + refcounter_file.close() + + self.assertEquals('1 0\n', contents) + + @testlib.with_context + def test_put_is_noop_if_already_zero(self, context): + os.makedirs(refcounter.RefCounter.BASE_DIR) + + result = refcounter.RefCounter.put( + 'someobject', False, 'somenamespace') + + self.assertEquals(0, result) + + @testlib.with_context + def test_writeCount_returns_true_if_file_found(self, context): + os.makedirs('/existing') + + result = refcounter.RefCounter._writeCount('/existing/file', 1, 1) + + self.assertTrue(result) + + @testlib.with_context + def test_writeCount_returns_false_if_file_not_found(self, context): + result = refcounter.RefCounter._writeCount('/nonexisting/file', 1, 1) + + self.assertFalse(result) + + @mock.patch('os.rmdir') + @mock.patch('os.unlink') + @mock.patch('util.pathexists') + def test_removeObject_ignores_if_directory_already_removed(self, + pathexists, + unlink, + rmdir): + rmdir.side_effect = OSError(errno.ENOENT, 'ignored') + + refcounter.RefCounter._removeObject('namespace', 'obj') + + rmdir.assert_called_once_with( + os.path.join(refcounter.RefCounter.BASE_DIR, 'namespace')) + + @mock.patch('os.rmdir') + @mock.patch('os.unlink') + @mock.patch('util.pathexists') + def test_removeObject_ignores_if_directory_not_empty(self, + pathexists, + unlink, + rmdir): + rmdir.side_effect = OSError(errno.ENOTEMPTY, 'ignored') + + refcounter.RefCounter._removeObject('namespace', 'obj') + + rmdir.assert_called_once_with( + os.path.join(refcounter.RefCounter.BASE_DIR, 'namespace')) From eb6bfed3e0c6f7b4ad5d62fd7072cc828338d77e Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Thu, 29 May 2014 16:38:12 +0100 Subject: [PATCH 18/69] CA-135017: notify toolstack about tapdisk unpause errors in coalesce Coalesce is an asynchronous operation so if a tapdisk refresh fails the VM will lose the disk but the administrator will realise that when the VM hangs. This patch notifies the toolstack as soon as the failure occurs. Signed-off-by: Thanos Makatos Acked-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 5d3a649127036e0c2ceeeb169d55e451e22cdd91) --- drivers/cleanup.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/cleanup.py b/drivers/cleanup.py index 3ab279f0..b20b6499 100755 --- a/drivers/cleanup.py +++ b/drivers/cleanup.py @@ -497,9 +497,22 @@ def pause(self, failfast=False): self.uuid, failfast): raise util.SMException("Failed to pause VDI %s" % self) + def _report_tapdisk_unpause_error(self): + try: + xapi = self.sr.xapi.session.xenapi + sr_ref = xapi.SR.get_by_uuid(self.sr.uuid) + msg_name = "failed to unpause tapdisk" + msg_body = "Failed to unpause tapdisk for VDI %s, " \ + "VMs using this tapdisk have lost access " \ + "to the corresponding disk(s)" % self.uuid + xapi.message.create(msg_name, "4", "SR", self.sr.uuid, msg_body) + except Exception, e: + util.SMlog("failed to generate message: %s" % e) + def unpause(self): if not blktap2.VDI.tap_unpause(self.sr.xapi.session, self.sr.uuid, self.uuid): + self._report_tapdisk_unpause_error() raise util.SMException("Failed to unpause VDI %s" % self) def refresh(self, ignoreNonexistent = True): @@ -509,6 +522,7 @@ def refresh(self, ignoreNonexistent = True): try: if not blktap2.VDI.tap_refresh(self.sr.xapi.session, self.sr.uuid, self.uuid): + self._report_tapdisk_unpause_error() raise util.SMException("Failed to refresh %s" % self) except XenAPI.Failure, e: if util.isInvalidVDI(e) and ignoreNonexistent: From 8a8476139a596cb011a263283ab43bce24b67b58 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Thu, 3 Jul 2014 10:27:22 +0100 Subject: [PATCH 19/69] CA-139484: introduce TapdiskFailed exception type Signed-off-by: Thanos Makatos Reviewed-by: Siddharth Vinothkumar Imported-by: Thanos Makatos (cherry picked from commit 4b999e1e2848e4cbc48d7a7f25ae5ede5e4d19b1) Conflicts: drivers/XE_SR_ERRORCODES.xml --- drivers/XE_SR_ERRORCODES.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/XE_SR_ERRORCODES.xml b/drivers/XE_SR_ERRORCODES.xml index 647b2537..ac637062 100755 --- a/drivers/XE_SR_ERRORCODES.xml +++ b/drivers/XE_SR_ERRORCODES.xml @@ -761,4 +761,11 @@ OCFS filesystem creation error 452 + + + TapdiskFailed + tapdisk experienced an error + 453 + + From 4db2528576b4d218d95818ff51d25c20aafa465a Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Thu, 3 Jul 2014 14:58:48 +0100 Subject: [PATCH 20/69] CA-139739: encode NFS shares Signed-off-by: Thanos Makatos Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 2508b267a717391c92a2caecc6d826f600564b15) --- drivers/NFSSR.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/NFSSR.py b/drivers/NFSSR.py index 26d86749..087863cd 100755 --- a/drivers/NFSSR.py +++ b/drivers/NFSSR.py @@ -83,7 +83,7 @@ def load(self, sr_uuid): self.nosubdir = self.sm_config.get('nosubdir') == "true" if self.dconf.has_key('serverpath'): self.remotepath = os.path.join(self.dconf['serverpath'], - not self.nosubdir and sr_uuid or "") + not self.nosubdir and sr_uuid or "").encode('utf-8') self.path = os.path.join(SR.MOUNT_BASE, sr_uuid) # Test for the optional 'nfsoptions' dconf attribute @@ -182,7 +182,7 @@ def create(self, sr_uuid, size): # Set the target path temporarily to the base dir # so that we can create the target SR directory - self.remotepath = self.dconf['serverpath'] + self.remotepath = self.dconf['serverpath'].encode('utf-8') try: self.mount_remotepath(sr_uuid) except Exception, exn: @@ -218,7 +218,7 @@ def delete(self, sr_uuid): # Set the target path temporarily to the base dir # so that we can remove the target SR directory - self.remotepath = self.dconf['serverpath'] + self.remotepath = self.dconf['serverpath'].encode('utf-8') self.mount_remotepath(sr_uuid) if not self.nosubdir: newpath = os.path.join(self.path, sr_uuid) From a316755d788f532182b957cb1a29e33739fcea17 Mon Sep 17 00:00:00 2001 From: Siddharth Vinothkumar Date: Mon, 26 May 2014 10:05:39 +0100 Subject: [PATCH 21/69] CA-119888: Fix logical volume deletion error. Signed-off-by: Siddharth Vinothkumar Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 51d1ff4f18e0371aff4a4300c95a21bca32a729a) --- drivers/LVHDSR.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/drivers/LVHDSR.py b/drivers/LVHDSR.py index d0fe17da..c416268a 100755 --- a/drivers/LVHDSR.py +++ b/drivers/LVHDSR.py @@ -542,11 +542,48 @@ def delete(self, uuid): raise xs_errors.XenError('LVMMaster') cleanup.gc_force(self.session, self.uuid) + success = True + for fileName in \ + filter(lambda x: util.extractSRFromDevMapper(x) == self.uuid, \ + glob.glob(DEV_MAPPER_ROOT + '*')): + if util.doesFileHaveOpenHandles(fileName): + util.SMlog("LVHDSR.delete: The dev mapper entry %s has open " \ + "handles" % fileName) + success = False + continue + + # Now attempt to remove the dev mapper entry + if not lvutil.removeDevMapperEntry(fileName): + success = False + continue + + try: + lvname = os.path.basename(fileName.replace('-','/').\ + replace('//', '-')) + os.unlink(os.path.join(self.path, lvname)) + except Exception, e: + util.SMlog("LVHDSR.delete: failed to remove the symlink for " \ + "file %s. Error: %s" % (fileName, str(e))) + success = False + + if success: + try: + if util.pathexists(self.path): + os.rmdir(self.path) + except Exception, e: + util.SMlog("LVHDSR.delete: failed to remove the symlink " \ + "directory %s. Error: %s" % (self.path, str(e))) + success = False + self._removeMetadataVolume() self.lvmCache.refresh() if len(lvhdutil.getLVInfo(self.lvmCache)) > 0: raise xs_errors.XenError('SRNotEmpty') + if not success: + raise Exception("LVHDSR delete failed, please refer to the log " \ + "for details.") + lvutil.removeVG(self.root, self.vgname) self._cleanup() From 02d1e7b8effa0d7fe67fb2e9166547eec26872d5 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 17 Jul 2014 17:07:50 +0100 Subject: [PATCH 22/69] UnitTest: log to stdout By printing the log messages to stdout, they will be captured by nose, thus not disturbing the user. Should a test fail, the messages will still be displayed. Signed-off-by: Mate Lakat Acked-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 9a19651f8212c96a751e282025284882653e2212) --- tests/testlib.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testlib.py b/tests/testlib.py index 642ebb7a..9ff64a06 100644 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -279,7 +279,7 @@ def log(self, *args): WARNING = '\033[93m' ENDC = '\033[0m' import sys - sys.stderr.write( + sys.stdout.write( WARNING + ' '.join(str(arg) for arg in args) + ENDC From 397ecc48ec606dfd49a19ddada2fe844c61dbc8f Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 17 Jul 2014 17:10:18 +0100 Subject: [PATCH 23/69] UnitTest: Enable custom context Should the user want to use a custom context implementation, he can use the with_custom_context decorator to do that. It enables users to inherit from TestContext, extend its functionality, and use the extended version in a test. This could be useful if someone wanted to use a custom implementation for `open`. Signed-off-by: Mate Lakat Acked-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 422896397367f7832cba94e4269c9371bd835921) --- tests/testlib.py | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/tests/testlib.py b/tests/testlib.py index 9ff64a06..5b783579 100644 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -294,20 +294,27 @@ def adapter(self): return adapter +def with_custom_context(context_class): + def _with_context(func): + def decorated(self, *args, **kwargs): + context = context_class() + context.start() + try: + result = func(self, context, *args, **kwargs) + context.stop() + return result + except: + context.stop() + raise + + decorated.__name__ = func.__name__ + return decorated + return _with_context + + def with_context(func): - def decorated(self, *args, **kwargs): - context = TestContext() - context.start() - try: - result = func(self, context, *args, **kwargs) - context.stop() - return result - except: - context.stop() - raise - - decorated.__name__ = func.__name__ - return decorated + decorator = with_custom_context(TestContext) + return decorator(func) def xml_string(text): From 4f03e41d9553010ccb476eb8e7365be84d413073 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 17 Jul 2014 17:12:39 +0100 Subject: [PATCH 24/69] CA-140596: [UnitTest] Test to reproduce gc error This test will reproduce the issue that was seen in the referenced ticket. Whenever a new Lock instance is created, and the file() call fails, the instance's lockfile attribute remained uninitialised. As the instance has a custom destructor, that will be called by the garbage collector. That function assumes that the instance has a lockfile attribute, and thus prints a message. Signed-off-by: Mate Lakat Acked-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 9c077a91c338cda683111c9bbc6490c4867fda97) --- tests/test_lock.py | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 tests/test_lock.py diff --git a/tests/test_lock.py b/tests/test_lock.py new file mode 100644 index 00000000..7b5c28db --- /dev/null +++ b/tests/test_lock.py @@ -0,0 +1,44 @@ +import unittest +import mock +import os +import gc + +import testlib + +import lock + + +class FailingOpenContext(testlib.TestContext): + def fake_open(self, fname, mode='r'): + raise IOError() + + +class TestLockDestruction(unittest.TestCase): + def setUp(self): + gc.disable() + + @testlib.with_custom_context(FailingOpenContext) + def test_close_if_open_failed(self, ctx): + try: + lck = lock.Lock('somename') + raise AssertionError('An IOError was expected here') + except IOError: + pass + + locks = self.retrieve_lock_instances_from_gc() + self.assertEquals(1, len(locks)) + + lck, = locks + + lck._close() + + def retrieve_lock_instances_from_gc(self): + locks = [] + for obj in gc.get_objects(): + if isinstance(obj, lock.Lock): + locks.append(obj) + + return locks + + def tearDown(self): + gc.enable() From d7c04e0c2e84d723b914da17272566d060ccce9e Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 24 Jul 2014 17:27:15 +0100 Subject: [PATCH 25/69] CA-140596: Fix lockfile attribute in Lock As Lock's destructor was assuming that lockfile attribute exists, it printed out an AttributeError on stdout while the GC was cleaning it up. This fix initialises the lockfile field in the constructor. Signed-off-by: Mate Lakat Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 22d7e9b62ecb196cdd382776b4567b959a4336a8) --- drivers/lock.py | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/lock.py b/drivers/lock.py index 6d367b35..f7df7368 100755 --- a/drivers/lock.py +++ b/drivers/lock.py @@ -70,6 +70,7 @@ def _mknamespace(ns): _mknamespace = staticmethod(_mknamespace) def __init__(self, name, ns=None): + self.lockfile = None self.ns = Lock._mknamespace(ns) From 7262e8ba93cac88221750f49c0156458ed2f85d7 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 24 Jul 2014 17:31:26 +0100 Subject: [PATCH 26/69] CA-140596: Add seam for file call Extract the file call to a separate method, so that the extracted method could be used in tests as a seam, emulating situations like: what would happen if the file call failed. Signed-off-by: Mate Lakat Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 1d2d9084bf6b041ce639effd75b2e7aa7342fad8) --- drivers/lock.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/lock.py b/drivers/lock.py index f7df7368..190a2063 100755 --- a/drivers/lock.py +++ b/drivers/lock.py @@ -43,13 +43,17 @@ def _open(self): # the lockfile inside that namespace directory per namespace self.lockpath = os.path.join(self.nspath, self.name) - if not os.path.exists(self.lockpath): - util.SMlog("lock: creating lock file %s" % self.lockpath) - self.lockfile = file(self.lockpath, "w+") + + self._open_lockfile() fd = self.lockfile.fileno() self.lock = flock.WriteLock(fd) + def _open_lockfile(self): + """Provide a seam, so extreme situations could be tested""" + util.SMlog("lock: opening lock file %s" % self.lockpath) + self.lockfile = file(self.lockpath, "w+") + def _close(self): """Close the lock, which implies releasing the lock.""" if self.lockfile is not None: From 6257865d64733694144bcb6ad0d82f20a4e29f54 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 24 Jul 2014 09:25:51 +0100 Subject: [PATCH 27/69] CA-140596: [UnitTest] Test file not found in Lock These are test cases covering the lock module, and also testing what happens if file() raises an IOError. This can happen as a race condition, if two locks - A and B are created in the same namespace and by the time A gets to open the lockfile, B already cleaned up the namespace. Signed-off-by: Mate Lakat Acked-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit d10e3b034486a852483c22822e16b6249aeefdca) --- tests/test_lock.py | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/test_lock.py b/tests/test_lock.py index 7b5c28db..02901d60 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -2,6 +2,7 @@ import mock import os import gc +import errno import testlib @@ -13,6 +14,65 @@ def fake_open(self, fname, mode='r'): raise IOError() +class TestLock(unittest.TestCase): + @testlib.with_context + def test_lock_without_namespace_creates_nil_namespace(self, context): + lck = lock.Lock('somename') + + self.assertTrue( + os.path.exists( + os.path.join(lck.BASE_DIR, '.nil'))) + + @testlib.with_context + def test_lock_with_namespace_creates_namespace(self, context): + lck = lock.Lock('somename', ns='namespace') + + self.assertTrue( + os.path.exists( + os.path.join(lck.BASE_DIR, 'namespace'))) + + @testlib.with_context + def test_lock_without_namespace_creates_file(self, context): + lck = lock.Lock('somename') + + self.assertTrue( + os.path.exists( + os.path.join(lck.BASE_DIR, '.nil', 'somename'))) + + @testlib.with_context + def test_lock_with_namespace_creates_file(self, context): + lck = lock.Lock('somename', ns='namespace') + + self.assertTrue( + os.path.exists( + os.path.join(lck.BASE_DIR, 'namespace', 'somename'))) + + @testlib.with_context + def test_lock_file_create_fails_retried(self, context): + Lock = create_lock_class_that_fails_to_create_file(1) + lck = Lock('somename', ns='namespace') + + self.assertTrue( + os.path.exists( + os.path.join(lck.BASE_DIR, 'namespace', 'somename'))) + + +def create_lock_class_that_fails_to_create_file(number_of_failures): + + class LockThatFailsToCreateFile(lock.Lock): + _failures = number_of_failures + + def _open_lockfile(self): + if self._failures > 0: + error = IOError('No such file') + error.errno = errno.ENOENT + self._failures -= 1 + raise error + return lock.Lock._open_lockfile(self) + + return LockThatFailsToCreateFile + + class TestLockDestruction(unittest.TestCase): def setUp(self): gc.disable() From 3a366d2228099d112a2d8d7dd1179708a12323e5 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 24 Jul 2014 09:29:28 +0100 Subject: [PATCH 28/69] CA-140596: Fix file creation error If creating the lockfile failed with IOError having ENOENT error code, that means that another lock within the same namespace already cleaned up the namespace. With this change, should IOError happen with ENOENT, the lock will re-create the namespace, and the file 10 times, and only bubble up the exception if it failed all those retries. Signed-off-by: Mate Lakat Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 3021503ce0275f4c293df1785e1c95111034c6c9) --- drivers/lock.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/lock.py b/drivers/lock.py index 190a2063..52c05b94 100755 --- a/drivers/lock.py +++ b/drivers/lock.py @@ -39,12 +39,27 @@ def _open(self): # one directory per namespace self.nspath = os.path.join(Lock.BASE_DIR, self.ns) - self._mkdirs(self.nspath) # the lockfile inside that namespace directory per namespace self.lockpath = os.path.join(self.nspath, self.name) - self._open_lockfile() + number_of_enoent_retries = 10 + + while True: + self._mkdirs(self.nspath) + + try: + self._open_lockfile() + except IOError, e: + # If another lock within the namespace has already + # cleaned up the namespace by removing the directory, + # _open_lockfile raises an ENOENT, in this case we retry. + if e.errno == errno.ENOENT: + if number_of_enoent_retries > 0: + number_of_enoent_retries -= 1 + continue + raise + break fd = self.lockfile.fileno() self.lock = flock.WriteLock(fd) From 2f85bca2fc755a6abef63d4f6d54a5c6b7ae6d01 Mon Sep 17 00:00:00 2001 From: Robert Breker Date: Mon, 2 Jun 2014 16:35:29 +0100 Subject: [PATCH 29/69] UnitTest: tests for NFSSR.py, ISOSR.py and nfs.py Test mount calls. Signed-off-by: Robert Breker Signed-off-by: Mate Lakat Acked-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 27fbd54c9cd97a9b3a18a55f33aec0c9dc010b20) --- tests/test_ISOSR.py | 55 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_NFSSR.py | 55 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_nfs.py | 27 ++++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 tests/test_ISOSR.py create mode 100644 tests/test_NFSSR.py create mode 100644 tests/test_nfs.py diff --git a/tests/test_ISOSR.py b/tests/test_ISOSR.py new file mode 100644 index 00000000..9f023457 --- /dev/null +++ b/tests/test_ISOSR.py @@ -0,0 +1,55 @@ +import mock +import nfs +import ISOSR +import unittest + + +class FakeISOSR(ISOSR.ISOSR): + uuid = None + sr_ref = None + session = None + srcmd = None + + def __init__(self, srcmd, none): + self.dconf = srcmd.dconf + self.srcmd = srcmd + + +class TestISOSR(unittest.TestCase): + + def create_isosr(self, location='aServer:/aLocation', atype=None, + sr_uuid='asr_uuid'): + srcmd = mock.Mock() + srcmd.dconf = { + 'location': location + } + if atype: + srcmd.dconf.update({'type': atype}) + srcmd.params = { + 'command': 'some_command' + } + isosr = FakeISOSR(srcmd, None) + isosr.load(sr_uuid) + return isosr + + def test_load(self): + self.create_isosr() + + @mock.patch('util.gen_uuid') + @mock.patch('nfs.soft_mount') + @mock.patch('util._convertDNS') + @mock.patch('util.makedirs') + @mock.patch('ISOSR.ISOSR._checkmount') + def test_attach_nfs(self, _checkmount, makedirs, convertDNS, soft_mount, + gen_uuid): + isosr = self.create_isosr(location='aServer:/aLocation', atype='nfs', + sr_uuid='asr_uuid') + _checkmount.side_effect = [False, True] + gen_uuid.return_value = 'aUuid' + + isosr.attach(None) + + soft_mount.assert_called_once_with('/var/run/sr-mount/asr_uuid', + 'aServer', + '/aLocation', + 'tcp') diff --git a/tests/test_NFSSR.py b/tests/test_NFSSR.py new file mode 100644 index 00000000..2291a382 --- /dev/null +++ b/tests/test_NFSSR.py @@ -0,0 +1,55 @@ +import mock +import nfs +import NFSSR +import unittest + + +class FakeNFSSR(NFSSR.NFSSR): + uuid = None + sr_ref = None + session = None + srcmd = None + + def __init__(self, srcmd, none): + self.dconf = srcmd.dconf + self.srcmd = srcmd + + +class TestNFSSR(unittest.TestCase): + + def create_nfssr(self, server='aServer', serverpath='/aServerpath', + sr_uuid='asr_uuid'): + srcmd = mock.Mock() + srcmd.dconf = { + 'server': server, + 'serverpath': serverpath + } + srcmd.params = { + 'command': 'some_command' + } + nfssr = FakeNFSSR(srcmd, None) + nfssr.load(sr_uuid) + return nfssr + + @mock.patch('NFSSR.Lock') + def test_load(self, Lock): + self.create_nfssr() + + @mock.patch('util.makedirs') + @mock.patch('NFSSR.Lock') + @mock.patch('nfs.soft_mount') + @mock.patch('util._testHost') + @mock.patch('nfs.check_server_tcp') + def test_attach(self, check_server_tcp, _testhost, soft_mount, Lock, + makedirs): + nfssr = self.create_nfssr(server='aServer', serverpath='/aServerpath', + sr_uuid='UUID') + + nfssr.attach(None) + + check_server_tcp.assert_called_once_with('aServer') + soft_mount.assert_called_once_with('/var/run/sr-mount/UUID', + 'aServer', + '/aServerpath/UUID', + 'tcp', + timeout=0) diff --git a/tests/test_nfs.py b/tests/test_nfs.py new file mode 100644 index 00000000..710baaf2 --- /dev/null +++ b/tests/test_nfs.py @@ -0,0 +1,27 @@ +import unittest +import nfs +import mock +import sys + + +class Test_nfs(unittest.TestCase): + + @mock.patch('util.pread') + def test_check_server_tcp(self, pread): + nfs.check_server_tcp('aServer') + + pread.assert_called_once_with(['/usr/sbin/rpcinfo', '-t', 'aServer', + 'nfs', '3']) + + def get_soft_mount_pread(self, binary): + return ([binary, 'remoteserver:remotepath', 'mountpoint', '-o', + 'soft,timeo=600,retrans=2147483647,transport,acdirmin=0,' + 'acdirmax=0']) + + @mock.patch('util.makedirs') + @mock.patch('util.pread') + def test_soft_mount(self, pread, makedirs): + nfs.soft_mount('mountpoint', 'remoteserver', 'remotepath', 'transport', + timeout=0) + + pread.assert_called_once_with(self.get_soft_mount_pread('mount.nfs')) From 4c91cd85c188b81b8bda04be88ba28427f8eaf7a Mon Sep 17 00:00:00 2001 From: Robert Breker Date: Mon, 23 Dec 2013 10:56:38 +0000 Subject: [PATCH 30/69] CA-135612: Add experimental support for NFSv4 This enables the specification of the NFS version (3,4) in the device-config of ISOSRs(type=nfs) and NFSSRs. For example: device-config:nfsversion=4 Signed-off-by: Robert Breker Signed-off-by: Mate Lakat Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 7fa6b8c65d0a8fc72d14f39ab7f56a87aacbf3c9) --- drivers/ISOSR.py | 11 ++++++++--- drivers/NFSSR.py | 15 +++++++++------ drivers/nfs.py | 30 +++++++++++++++++++++++++----- 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/drivers/ISOSR.py b/drivers/ISOSR.py index 6081e267..450801c3 100755 --- a/drivers/ISOSR.py +++ b/drivers/ISOSR.py @@ -31,8 +31,9 @@ [ [ 'location', 'path to mount (required) (e.g. server:/path)' ], [ 'options', 'extra options to pass to mount (deprecated) (e.g. \'-o ro\')' ], - [ 'type','cifs or nfs'] ] - + [ 'type','cifs or nfs'], + nfs.NFS_VERSION] + DRIVER_INFO = { 'name': 'ISO', 'description': 'Handles CD images stored as files in iso format', @@ -209,6 +210,9 @@ def load(self, sr_uuid): else: self.path = self.mountpoint + # Handle optional dconf attributes + self.nfsversion = nfs.validate_nfsversion(self.dconf.get('nfsversion')) + # Some info we need: self.sr_vditype = 'phy' self.credentials = None @@ -261,7 +265,8 @@ def attach(self, sr_uuid): # to the process waiting. if self.dconf.has_key('type') and self.dconf['type']!='cifs': serv_path = location.split(':') - nfs.soft_mount(self.mountpoint, serv_path[0], serv_path[1], 'tcp') + nfs.soft_mount(self.mountpoint, serv_path[0], serv_path[1], + 'tcp', nfsversion=self.nfsversion) else: util.pread(mountcmd, True) except util.CommandException, inst: diff --git a/drivers/NFSSR.py b/drivers/NFSSR.py index 087863cd..cbed9875 100755 --- a/drivers/NFSSR.py +++ b/drivers/NFSSR.py @@ -35,8 +35,9 @@ "VDI_GENERATE_CONFIG", "VDI_RESET_ON_BOOT/2", "ATOMIC_PAUSE"] -CONFIGURATION = [ [ 'server', 'hostname or IP address of NFS server (required)' ], \ - [ 'serverpath', 'path on remote server (required)' ] ] +CONFIGURATION = [['server', 'hostname or IP address of NFS server (required)'], + ['serverpath', 'path on remote server (required)'], + nfs.NFS_VERSION] DRIVER_INFO = { @@ -86,10 +87,11 @@ def load(self, sr_uuid): not self.nosubdir and sr_uuid or "").encode('utf-8') self.path = os.path.join(SR.MOUNT_BASE, sr_uuid) - # Test for the optional 'nfsoptions' dconf attribute + # Handle optional dconf attributes self.transport = DEFAULT_TRANSPORT if self.dconf.has_key('useUDP') and self.dconf['useUDP'] == 'true': self.transport = "udp" + self.nfsversion = nfs.validate_nfsversion(self.dconf.get('nfsversion')) self._check_o_direct() @@ -108,7 +110,7 @@ def validate_remotepath(self, scan): def check_server(self): try: - nfs.check_server_tcp(self.remoteserver) + nfs.check_server_tcp(self.remoteserver, self.nfsversion) except nfs.NfsException, exc: raise xs_errors.XenError('NFSVersion', opterr=exc.errstr) @@ -116,8 +118,9 @@ def check_server(self): def mount(self, mountpoint, remotepath, timeout = 0): try: - nfs.soft_mount(mountpoint, self.remoteserver, remotepath, - self.transport, timeout) + nfs.soft_mount( + mountpoint, self.remoteserver, remotepath, self.transport, + timeout=timeout, nfsversion=self.nfsversion) except nfs.NfsException, exc: raise xs_errors.XenError('NFSMount', opterr=exc.errstr) diff --git a/drivers/nfs.py b/drivers/nfs.py index 29339ff9..301506a9 100644 --- a/drivers/nfs.py +++ b/drivers/nfs.py @@ -43,25 +43,40 @@ RPCINFO_BIN = "/usr/sbin/rpcinfo" SHOWMOUNT_BIN = "/usr/sbin/showmount" +DEFAULT_NFSVERSION = '3' + +NFS_VERSION = [ + 'nfsversion', 'for type=nfs, NFS protocol version - 3, 4 (optional)'] + class NfsException(Exception): def __init__(self, errstr): self.errstr = errstr -def check_server_tcp(server): +def check_server_tcp(server, nfsversion=DEFAULT_NFSVERSION): """Make sure that NFS over TCP/IP V3 is supported on the server. Returns True if everything is OK, False otherwise.""" try: util.ioretry(lambda: util.pread([RPCINFO_BIN,"-t", - "%s" % server, "nfs","3"]), + "%s" % server, "nfs", nfsversion]), errlist=[errno.EPERM], maxretry=2, nofail=True) except util.CommandException, inst: raise NfsException("rpcinfo failed or timed out: return code %d" % inst.code) -def soft_mount(mountpoint, remoteserver, remotepath, transport, timeout = 0): +def validate_nfsversion(nfsversion): + if not nfsversion: + nfsversion = DEFAULT_NFSVERSION + else: + if nfsversion not in ['3', '4']: + raise NfsException("Invalid nfsversion.") + return nfsversion + + +def soft_mount(mountpoint, remoteserver, remotepath, transport, timeout=0, + nfsversion=DEFAULT_NFSVERSION): """Mount the remote NFS export at 'mountpoint'. The 'timeout' param here is in seconds""" try: @@ -71,17 +86,22 @@ def soft_mount(mountpoint, remoteserver, remotepath, transport, timeout = 0): raise NfsException("Failed to make directory: code is %d" % inst.code) + mountcommand = 'mount.nfs' + if nfsversion == '4': + mountcommand = 'mount.nfs4' + if timeout < 1: timeout = SOFTMOUNT_TIMEOUT - options = "soft,timeo=%d,retrans=%d,%s" % (timeout * 10, + options = "soft,timeo=%d,retrans=%d,proto=%s" % ( + timeout * 10, SOFTMOUNT_RETRANS, transport) options += ',acdirmin=0,acdirmax=0' try: util.ioretry(lambda: - util.pread(["mount.nfs", "%s:%s" + util.pread([mountcommand, "%s:%s" % (remoteserver, remotepath), mountpoint, "-o", options]), errlist=[errno.EPIPE, errno.EIO], From 334bdb368354509c305d352985a4ec86c1f497a1 Mon Sep 17 00:00:00 2001 From: Robert Breker Date: Sat, 21 Jun 2014 20:55:17 +0100 Subject: [PATCH 31/69] CA-135612: [UnitTest] Tests for nfsversions Test NFSSR, ISOSR and nfs Signed-off-by: Robert Breker Reviewed-by: Mate Lakat Acked-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 7c13c87c8f14d0ff24f4b8b27cdbceef0e4ea6dc) --- tests/test_ISOSR.py | 36 ++++++++++++++++++++++++++++++++---- tests/test_NFSSR.py | 40 +++++++++++++++++++++++++++++++++++----- tests/test_nfs.py | 41 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 106 insertions(+), 11 deletions(-) diff --git a/tests/test_ISOSR.py b/tests/test_ISOSR.py index 9f023457..b132152d 100644 --- a/tests/test_ISOSR.py +++ b/tests/test_ISOSR.py @@ -18,13 +18,15 @@ def __init__(self, srcmd, none): class TestISOSR(unittest.TestCase): def create_isosr(self, location='aServer:/aLocation', atype=None, - sr_uuid='asr_uuid'): + sr_uuid='asr_uuid', nfsversion=None): srcmd = mock.Mock() srcmd.dconf = { 'location': location } if atype: srcmd.dconf.update({'type': atype}) + if nfsversion: + srcmd.dconf.update({'nfsversion': nfsversion}) srcmd.params = { 'command': 'some_command' } @@ -35,13 +37,38 @@ def create_isosr(self, location='aServer:/aLocation', atype=None, def test_load(self): self.create_isosr() + @mock.patch('nfs.validate_nfsversion') + def test_load_validate_nfsversion_called(self, validate_nfsversion): + isosr = self.create_isosr(nfsversion='aNfsversion') + + validate_nfsversion.assert_called_once_with('aNfsversion') + + @mock.patch('NFSSR.Lock') + @mock.patch('nfs.validate_nfsversion') + def test_load_validate_nfsversion_returnused(self, validate_nfsversion, + Lock): + validate_nfsversion.return_value = 'aNfsversion' + + self.assertEquals(self.create_isosr().nfsversion, 'aNfsversion') + + @mock.patch('NFSSR.Lock') + @mock.patch('nfs.validate_nfsversion') + def test_load_validate_nfsversion_exceptionraised(self, + validate_nfsversion, + Lock): + validate_nfsversion.side_effect = nfs.NfsException('aNfsException') + + self.assertRaises(nfs.NfsException, self.create_isosr) + @mock.patch('util.gen_uuid') @mock.patch('nfs.soft_mount') @mock.patch('util._convertDNS') + @mock.patch('nfs.validate_nfsversion') @mock.patch('util.makedirs') @mock.patch('ISOSR.ISOSR._checkmount') - def test_attach_nfs(self, _checkmount, makedirs, convertDNS, soft_mount, - gen_uuid): + def test_attach_nfs(self, _checkmount, makedirs, validate_nfsversion, + convertDNS, soft_mount, gen_uuid): + validate_nfsversion.return_value = 'aNfsversionChanged' isosr = self.create_isosr(location='aServer:/aLocation', atype='nfs', sr_uuid='asr_uuid') _checkmount.side_effect = [False, True] @@ -52,4 +79,5 @@ def test_attach_nfs(self, _checkmount, makedirs, convertDNS, soft_mount, soft_mount.assert_called_once_with('/var/run/sr-mount/asr_uuid', 'aServer', '/aLocation', - 'tcp') + 'tcp', + nfsversion='aNfsversionChanged') diff --git a/tests/test_NFSSR.py b/tests/test_NFSSR.py index 2291a382..49524ff7 100644 --- a/tests/test_NFSSR.py +++ b/tests/test_NFSSR.py @@ -18,12 +18,14 @@ def __init__(self, srcmd, none): class TestNFSSR(unittest.TestCase): def create_nfssr(self, server='aServer', serverpath='/aServerpath', - sr_uuid='asr_uuid'): + sr_uuid='asr_uuid', nfsversion=None): srcmd = mock.Mock() srcmd.dconf = { 'server': server, 'serverpath': serverpath } + if nfsversion: + srcmd.dconf.update({'nfsversion': nfsversion}) srcmd.params = { 'command': 'some_command' } @@ -35,21 +37,49 @@ def create_nfssr(self, server='aServer', serverpath='/aServerpath', def test_load(self, Lock): self.create_nfssr() + @mock.patch('NFSSR.Lock') + @mock.patch('nfs.validate_nfsversion') + def test_load_validate_nfsversion_called(self, validate_nfsversion, Lock): + nfssr = self.create_nfssr(nfsversion='aNfsversion') + + validate_nfsversion.assert_called_once_with('aNfsversion') + + @mock.patch('NFSSR.Lock') + @mock.patch('nfs.validate_nfsversion') + def test_load_validate_nfsversion_returnused(self, validate_nfsversion, + Lock): + validate_nfsversion.return_value = 'aNfsversion' + + self.assertEquals(self.create_nfssr().nfsversion, "aNfsversion") + + @mock.patch('NFSSR.Lock') + @mock.patch('nfs.validate_nfsversion') + def test_load_validate_nfsversion_exceptionraised(self, + validate_nfsversion, + Lock): + validate_nfsversion.side_effect = nfs.NfsException('aNfsException') + + self.assertRaises(nfs.NfsException, self.create_nfssr) + @mock.patch('util.makedirs') @mock.patch('NFSSR.Lock') @mock.patch('nfs.soft_mount') @mock.patch('util._testHost') @mock.patch('nfs.check_server_tcp') - def test_attach(self, check_server_tcp, _testhost, soft_mount, Lock, - makedirs): + @mock.patch('nfs.validate_nfsversion') + def test_attach(self, validate_nfsversion, check_server_tcp, _testhost, + soft_mount, Lock, makedirs): + validate_nfsversion.return_value = "aNfsversionChanged" nfssr = self.create_nfssr(server='aServer', serverpath='/aServerpath', sr_uuid='UUID') nfssr.attach(None) - check_server_tcp.assert_called_once_with('aServer') + check_server_tcp.assert_called_once_with('aServer', + 'aNfsversionChanged') soft_mount.assert_called_once_with('/var/run/sr-mount/UUID', 'aServer', '/aServerpath/UUID', 'tcp', - timeout=0) + timeout=0, + nfsversion='aNfsversionChanged') diff --git a/tests/test_nfs.py b/tests/test_nfs.py index 710baaf2..ac683f98 100644 --- a/tests/test_nfs.py +++ b/tests/test_nfs.py @@ -13,10 +13,17 @@ def test_check_server_tcp(self, pread): pread.assert_called_once_with(['/usr/sbin/rpcinfo', '-t', 'aServer', 'nfs', '3']) + @mock.patch('util.pread') + def test_check_server_tcp_nfsversion(self, pread): + nfs.check_server_tcp('aServer', 'aNfsversion') + + pread.assert_called_once_with(['/usr/sbin/rpcinfo', '-t', 'aServer', + 'nfs', 'aNfsversion']) + def get_soft_mount_pread(self, binary): return ([binary, 'remoteserver:remotepath', 'mountpoint', '-o', - 'soft,timeo=600,retrans=2147483647,transport,acdirmin=0,' - 'acdirmax=0']) + 'soft,timeo=600,retrans=2147483647,proto=transport,acdirmin=0' + ',acdirmax=0']) @mock.patch('util.makedirs') @mock.patch('util.pread') @@ -25,3 +32,33 @@ def test_soft_mount(self, pread, makedirs): timeout=0) pread.assert_called_once_with(self.get_soft_mount_pread('mount.nfs')) + + @mock.patch('util.makedirs') + @mock.patch('util.pread') + def test_soft_mount_nfsversion_3(self, pread, makedirs): + nfs.soft_mount('mountpoint', 'remoteserver', 'remotepath', 'transport', + timeout=0, nfsversion='3') + + pread.assert_called_once_with(self.get_soft_mount_pread('mount.nfs')) + + @mock.patch('util.makedirs') + @mock.patch('util.pread') + def test_soft_mount_nfsversion_4(self, pread, makedirs): + nfs.soft_mount('mountpoint', 'remoteserver', 'remotepath', 'transport', + timeout=0, nfsversion='4') + + pread.assert_called_once_with(self.get_soft_mount_pread('mount.nfs4')) + + def test_validate_nfsversion_invalid(self): + for thenfsversion in ['2', '4.1']: + self.assertRaises(nfs.NfsException, nfs.validate_nfsversion, + thenfsversion) + + def test_validate_nfsversion_default(self): + for thenfsversion in ['', None]: + self.assertEquals(nfs.validate_nfsversion(thenfsversion), '3') + + def test_validate_nfsversion_valid(self): + for thenfsversion in ['3', '4']: + self.assertEquals(nfs.validate_nfsversion(thenfsversion), + thenfsversion) From 235e3ee4920a963febd864f278c2781314dafd6a Mon Sep 17 00:00:00 2001 From: Bob Ball Date: Tue, 29 Jul 2014 15:37:52 +0100 Subject: [PATCH 32/69] CA-141945: Add reporting of TRIM usage to SR Once a TRIM successfully completed, note the time it was run on the SR for reporting purposes. Initially useful for TaaS statistics to show when features have been used. Signed-off-by: Bob Ball Reviewed-by: Robert Breker Reviewed-by: Mate Lakat Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit c5a2382f233f3f7b317e5d5b00565507ec99682d) --- drivers/trim_util.py | 16 ++++++++++++++++ tests/test_trim_util.py | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/drivers/trim_util.py b/drivers/trim_util.py index 997a3461..847d1a54 100755 --- a/drivers/trim_util.py +++ b/drivers/trim_util.py @@ -37,6 +37,7 @@ ERROR_CODE_KEY = "errcode" ERROR_MSG_KEY = "errmsg" +TRIM_LAST_TRIGGERED_KEY = "trim_last_triggered" def _vg_by_sr_uuid(sr_uuid): return lvhdutil.VG_PREFIX + sr_uuid @@ -67,6 +68,19 @@ def to_xml(d): return dom.toxml() +# Note: This function is expected to be called from a context where +# the SR is locked by the thread calling the function; therefore removing +# any risk of a race condition updating the LAST_TRIGGERED value. +def _log_last_triggered(session, sr_uuid): + try: + sr_ref = session.xenapi.SR.get_by_uuid(sr_uuid) + other_config = session.xenapi.SR.get_other_config(sr_ref) + if other_config.has_key(TRIM_LAST_TRIGGERED_KEY): + session.xenapi.SR.remove_from_other_config(sr_ref, TRIM_LAST_TRIGGERED_KEY) + session.xenapi.SR.add_to_other_config(sr_ref, TRIM_LAST_TRIGGERED_KEY, str(time.time())) + except: + util.logException("Unable to set other-config:%s" % TRIM_LAST_TRIGGERED_KEY) + def do_trim(session, args): """Attempt to trim the given LVHDSR""" util.SMlog("do_trim: %s" % args) @@ -111,6 +125,8 @@ def do_trim(session, args): } result = to_xml(err_msg) + _log_last_triggered(session, sr_uuid) + sr_lock.release() return result else: diff --git a/tests/test_trim_util.py b/tests/test_trim_util.py index 1f72a2cf..bee2bf93 100644 --- a/tests/test_trim_util.py +++ b/tests/test_trim_util.py @@ -246,3 +246,44 @@ def test_do_trim_when_trim_succeeded_returns_true(self, result = trim_util.do_trim(None, {'sr_uuid': 'some-uuid'}) self.assertEquals('True', result) + + @mock.patch('trim_util.time.time') + def test_log_last_triggered_no_key(self, mock_time): + session = mock.Mock() + mock_time.return_value = 0 + session.xenapi.SR.get_by_uuid.return_value = 'sr_ref' + session.xenapi.SR.get_other_config.return_value = {} + + trim_util._log_last_triggered(session, 'sr_uuid') + + session.xenapi.SR.add_to_other_config.assert_called_with( + 'sr_ref', trim_util.TRIM_LAST_TRIGGERED_KEY, '0') + self.assertEqual(0, session.xenapi.SR.remove_from_other_config.call_count) + + @mock.patch('trim_util.time.time') + def test_log_last_triggered_has_key(self, mock_time): + session = mock.Mock() + mock_time.return_value = 0 + session.xenapi.SR.get_by_uuid.return_value = 'sr_ref' + other_config = {trim_util.TRIM_LAST_TRIGGERED_KEY: '0'} + session.xenapi.SR.get_other_config.return_value = other_config + + trim_util._log_last_triggered(session, 'sr_uuid') + + session.xenapi.SR.remove_from_other_config.assert_called_with( + 'sr_ref', trim_util.TRIM_LAST_TRIGGERED_KEY) + session.xenapi.SR.add_to_other_config.assert_called_with( + 'sr_ref', trim_util.TRIM_LAST_TRIGGERED_KEY, '0') + + @mock.patch('trim_util.time.time') + @mock.patch('trim_util.util.logException') + def test_log_last_triggered_exc_logged(self, mock_log_exc, mock_time): + session = mock.Mock() + mock_time.return_value = 0 + session.xenapi.SR.get_by_uuid.side_effect = Exception() + + # This combination ensures that an exception does not cause the log + # function to throw, but the exception is still logged + trim_util._log_last_triggered(session, 'sr_uuid') + + self.assertEqual(1, mock_log_exc.call_count) From c91fa4302bc324cbf009bb30730082f0273df97b Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Wed, 20 Aug 2014 16:36:36 +0100 Subject: [PATCH 33/69] CA-142451: Unit tests Covering changes related to CA-142451. Signed-off-by: Flavien Quesnel Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 2ddae13dad02850e8a8295baea3cf80c05431cd1) --- tests/test_ISCSISR.py | 92 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/tests/test_ISCSISR.py b/tests/test_ISCSISR.py index 88d299ce..13730534 100644 --- a/tests/test_ISCSISR.py +++ b/tests/test_ISCSISR.py @@ -59,6 +59,33 @@ def __init__(self, extra_dconf=None): self.dconf.update(extra_dconf or {}) +class NonInitingMultiLUNISCSISR(ISCSISR.ISCSISR): + def __init__(self, node1, node2): + self.mpath = "false" + self.dconf = { + 'target': node1['ip'], + 'localIQN': 'localIQN', + 'targetIQN': node1['iqn'] + } + + self.dconf.update({}) + self.target = node1['ip'] + self.port = node1['port'] + self.targetIQN = node1['iqn'] + self.attached = True + self.multihomed = True + extra_adapter = "%s:%d" % (node2['ip'], node2['port']) + self.adapter = { + extra_adapter: None + } + + def _synchroniseAddrList(self, *args, **kwargs): + pass + + def _init_adapters(self): + pass + + class TestVdiTypeSetting(TestBase): @mock.patch('ISCSISR.iscsilib.discovery') @@ -82,3 +109,68 @@ def test_vdi_type_modified_by_force_tapdisk(self): self.load_iscsi_sr(iscsi_sr=iscsi_sr) self.assertEquals('aio', iscsi_sr.sr_vditype) + + +class TestMultiLUNISCSISR(unittest.TestCase): + + def setUp(self): + self.node1 = { + 'ip': '127.0.0.1', + 'port': 3260, + 'iqn': 'IQN' + } + self.node2 = { + 'ip': '127.0.0.2', + 'port': 8080, + 'iqn': 'IQN', + 'tpgt': 'TPGT' + } + self.node_records = [( + "%s:%d" % (self.node2['ip'], self.node2['port']), + self.node2['tpgt'], + self.node2['iqn'] + )] + + def assertActiveNodeEquals(self, node, iscsi_sr): + node_ip_port = "%s:%d" % (node['ip'], node['port']) + node_path = '/dev/iscsi/%s/%s' % (node['iqn'], node_ip_port) + + self.assertEquals(node_path, iscsi_sr.path) + self.assertEquals(node_ip_port, iscsi_sr.tgtidx) + self.assertEquals(node_ip_port, iscsi_sr.address) + + @mock.patch('ISCSISR.os.path.exists') + @mock.patch('ISCSISR.iscsilib.get_node_records') + def test_initPaths_actual_path_is_active( + self, + mock_get_node_records, + mock_exists): + mock_get_node_records.return_value = self.node_records + mock_exists.return_value = True + + iscsi_sr = NonInitingMultiLUNISCSISR(self.node1, self.node2) + + iscsi_sr._initPaths() + + self.assertActiveNodeEquals(self.node1, iscsi_sr) + + @mock.patch('ISCSISR.os.path.exists') + @mock.patch('ISCSISR.iscsilib.get_node_records') + def test_initPaths_active_path_detection( + self, + mock_get_node_records, + mock_exists): + mock_get_node_records.return_value = self.node_records + + def fake_exists(path): + if self.node1['ip'] in path: + return False + return True + + mock_exists.side_effect = fake_exists + + iscsi_sr = NonInitingMultiLUNISCSISR(self.node1, self.node2) + + iscsi_sr._initPaths() + + self.assertActiveNodeEquals(self.node2, iscsi_sr) From 1ea63fb82f32b5db084e6b93552b086477014e40 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Mon, 11 Aug 2014 05:21:24 +0100 Subject: [PATCH 34/69] CA-142595: [UnitTest] support non-block devices Add testing framework enhancements to enable us to check what happens if a given (SCSI) device does not have a block subdirectory. Signed-off-by: Mate Lakat Reviewed-by: Flavien Quesnel Imported-by: Thanos Makatos (cherry picked from commit 3ae353db80a0e67a9cd139f7428af9e80bf55586) --- tests/test_devscan.py | 24 ++++++++++++++++++++--- tests/test_testlib.py | 18 +++++++++--------- tests/testlib.py | 44 +++++++++++++++++++++++++++---------------- 3 files changed, 58 insertions(+), 28 deletions(-) diff --git a/tests/test_devscan.py b/tests/test_devscan.py index 49504e12..51aea3d4 100644 --- a/tests/test_devscan.py +++ b/tests/test_devscan.py @@ -45,7 +45,7 @@ def test_scanning_empty_sr(self, context): @testlib.with_context def test_scanning_sr_with_devices(self, context): sr = create_hba_sr() - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) adapter.add_disk() sr._init_hbadict() @@ -66,7 +66,7 @@ def test_scanning_sr_with_devices(self, context): @testlib.with_context def test_scanning_sr_includes_parameters(self, context): sr = create_hba_sr() - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) adapter.add_disk() sr._init_hbadict() adapter.add_parameter('fc_host', dict(port_name='VALUE')) @@ -98,7 +98,7 @@ def test_no_adapters(self, context): @testlib.with_context def test_adapter_and_disk_added(self, context): - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) adapter.add_disk() result = devscan.adapters() @@ -117,3 +117,21 @@ def test_adapter_and_disk_added(self, context): } }, result) + + +class TestExtractDevName(unittest.TestCase): + @testlib.with_context + def test_26_kernel(self, context): + context.kernel_version = '2.6' + context.fake_makedirs('/somepath/block:sde') + result = devscan._extract_dev_name('/somepath') + + self.assertEquals('sde', result) + + @testlib.with_context + def test_3x_kernel(self, context): + context.kernel_version = '3.2' + context.fake_makedirs('/somepath/block/sde') + result = devscan._extract_dev_name('/somepath') + + self.assertEquals('sde', result) diff --git a/tests/test_testlib.py b/tests/test_testlib.py index cd186e3c..95dd9fa8 100644 --- a/tests/test_testlib.py +++ b/tests/test_testlib.py @@ -15,14 +15,14 @@ def test_generate_inventory_file(self): @testlib.with_context def test_adapter_adds_scsi_host_entry(self, context): - context.adapter() + context.add_adapter(testlib.SCSIAdapter()) self.assertEquals(['host0'], os.listdir('/sys/class/scsi_host')) @testlib.with_context def test_add_disk_adds_scsi_disk_entry(self, context): import glob - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) adapter.add_disk() self.assertEquals( @@ -32,7 +32,7 @@ def test_add_disk_adds_scsi_disk_entry(self, context): @testlib.with_context def test_add_disk_adds_scsibus_entry(self, context): import glob - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) adapter.long_id = 'HELLO' adapter.add_disk() @@ -42,7 +42,7 @@ def test_add_disk_adds_scsibus_entry(self, context): @testlib.with_context def test_add_disk_adds_device(self, context): - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) adapter.add_disk() self.assertEquals( @@ -51,7 +51,7 @@ def test_add_disk_adds_device(self, context): @testlib.with_context def test_add_disk_adds_disk_by_id_entry(self, context): - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) disk = adapter.add_disk() disk.long_id = 'SOMEID' @@ -60,21 +60,21 @@ def test_add_disk_adds_disk_by_id_entry(self, context): @testlib.with_context def test_add_disk_adds_glob(self, context): import glob - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) disk = adapter.add_disk() self.assertEquals(['/dev/disk/by-id'], glob.glob('/dev/disk/by-id')) @testlib.with_context def test_add_disk_path_exists(self, context): - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) disk = adapter.add_disk() self.assertTrue(os.path.exists('/dev/disk/by-id')) @testlib.with_context def test_add_parameter_parameter_file_exists(self, context): - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) disk = adapter.add_disk() adapter.add_parameter('fc_host', {'node_name': 'ignored'}) @@ -82,7 +82,7 @@ def test_add_parameter_parameter_file_exists(self, context): @testlib.with_context def test_add_parameter_parameter_file_contents(self, context): - adapter = context.adapter() + adapter = context.add_adapter(testlib.SCSIAdapter()) disk = adapter.add_disk() adapter.add_parameter('fc_host', {'node_name': 'value'}) diff --git a/tests/testlib.py b/tests/testlib.py index 5b783579..40110b15 100644 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -27,10 +27,19 @@ def get_error_codes(): class SCSIDisk(object): - def __init__(self): + def __init__(self, adapter): + self.adapter = adapter self.long_id = ''.join( random.choice(string.digits) for _ in range(33)) + def disk_device_paths(self, host_id, disk_id, actual_disk_letter): + yield '/sys/class/scsi_disk/%s:0:%s:0' % (host_id, disk_id) + yield '/sys/class/scsi_disk/%s:0:%s:0/device/block/sd%s' % ( + host_id, disk_id, actual_disk_letter) + yield '/dev/disk/by-scsibus/%s-%s:0:%s:0' % ( + self.adapter.long_id, host_id, disk_id) + yield '/dev/disk/by-id/%s' % (self.long_id) + class SCSIAdapter(object): def __init__(self): @@ -40,13 +49,24 @@ def __init__(self): self.parameters = [] def add_disk(self): - disk = SCSIDisk() + disk = SCSIDisk(self) self.disks.append(disk) return disk def add_parameter(self, host_class, values): self.parameters.append((host_class, values)) + def adapter_device_paths(self, host_id): + yield '/sys/class/scsi_host/host%s' % host_id + + +class AdapterWithNonBlockDevice(SCSIAdapter): + def adapter_device_paths(self, host_id): + for adapter_device_path in super(AdapterWithNonBlockDevice, + self).adapter_device_paths(host_id): + yield adapter_device_path + yield '/sys/class/fc_transport/target7:0:0/device/7:0:0:0' + class Executable(object): def __init__(self, function_to_call): @@ -238,21 +258,14 @@ def generate_path_content(self): def generate_device_paths(self): actual_disk_letter = 'a' for host_id, adapter in enumerate(self.scsi_adapters): - yield '/sys/class/scsi_host/host%s' % host_id + for adapter_device_path in adapter.adapter_device_paths(host_id): + yield adapter_device_path for disk_id, disk in enumerate(adapter.disks): - yield '/sys/class/scsi_disk/%s:0:%s:0' % ( - host_id, disk_id) - - yield '/sys/class/scsi_disk/%s:0:%s:0/device/block/sd%s' % ( - host_id, disk_id, actual_disk_letter) - + for path in disk.disk_device_paths(host_id, disk_id, + actual_disk_letter): + yield path actual_disk_letter = chr(ord(actual_disk_letter) + 1) - yield '/dev/disk/by-scsibus/%s-%s:0:%s:0' % ( - adapter.long_id, host_id, disk_id) - - yield '/dev/disk/by-id/%s' % (disk.long_id) - for path, _content in self.generate_path_content(): yield path @@ -288,8 +301,7 @@ def log(self, *args): def stop(self): map(lambda patcher: patcher.stop(), self.patchers) - def adapter(self): - adapter = SCSIAdapter() + def add_adapter(self, adapter): self.scsi_adapters.append(adapter) return adapter From 9d38ff11655fcac18ff4bafc637440fdc0e3d5c0 Mon Sep 17 00:00:00 2001 From: Wang Yanbin Date: Fri, 8 Aug 2014 15:10:24 +0800 Subject: [PATCH 35/69] CA-142595: Non-block devices causing IndexError Some device (e.g. a scsi security manager device.) has no subdir "block/" , this will cause _extract_dev_name "IndexError: list index out of range" error. The following is the traceback info: run this cmd: xe sr-probe type=lvmohba There was an SR backend failure. status: non-zero exit stdout: stderr: Traceback (most recent call last): File "/opt/xensource/sm/LVMoHBASR", line 239, in ? SRCommand.run(LVHDoHBASR, DRIVER_INFO) File "/opt/xensource/sm/SRCommand.py", line 343, in run sr = driver(cmd, cmd.sr_uuid) File "/opt/xensource/sm/SR.py", line 139, in init self.load(sr_uuid) File "/opt/xensource/sm/LVMoHBASR", line 98, in load print >>sys.stderr,self.hbasr.print_devs() File "/opt/xensource/sm/HBASR.py", line 242, in print_devs self._init_hbadict() File "/opt/xensource/sm/HBASR.py", line 63, in _init_hbadict dict = devscan.adapters(filterstr=self.type) File "/opt/xensource/sm/devscan.py", line 75, in adapters (dev, entry) = _extract_dev(dir, proc, id, lun) File "/opt/xensource/sm/devscan.py", line 235, in _extract_dev dev = _extract_dev_name(device_dir) File "/opt/xensource/sm/devscan.py", line 226, in _extract_dev_name dev = glob.glob(os.path.join(device_dir, 'block/*'))[0] IndexError: list index out of range Signed-off-by: Wang Yanbin wangyanbin@dayang.com.cn Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 426be113cccbc052c3baeccf382c61aba3a0252f) --- drivers/devscan.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/devscan.py b/drivers/devscan.py index 86d0fc0d..d05cee04 100644 --- a/drivers/devscan.py +++ b/drivers/devscan.py @@ -73,7 +73,8 @@ def adapters(filterstr="any"): else: dir = os.path.join(sysfs,lun,"device") (dev, entry) = _extract_dev(dir, proc, id, lun) - devs[dev] = entry + if dev != "": + devs[dev] = entry # for new qlogic sysfs layout (rport under device, then target) for i in filter(match_rport,os.listdir(path)): newpath = os.path.join(path, i) @@ -223,9 +224,14 @@ def _extract_dev_name(device_dir): return dev.lstrip('block:') elif kernel_version.startswith('3.'): # directory for device name lives inside block directory e.g. block/sdx - dev = glob.glob(os.path.join(device_dir, 'block/*'))[0] - # prune path to extract the device name - return os.path.basename(dev) + + #bug fixed: some device dir has no subdir "block/" , e.g. LUN 0 has no "block/" subdir, but other LUNs are OK + devs = glob.glob(os.path.join(device_dir, 'block/*')) + if len(devs): + # prune path to extract the device name + return os.path.basename(devs[0]) + else: + return "" else: msg = 'Kernel version detected: %s' % kernel_version raise xs_errors.XenError('UnsupportedKernel', msg) From 59588b0f956b4a269a624c84fdb0a06a4e924b3c Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Thu, 28 Aug 2014 11:51:47 +0100 Subject: [PATCH 36/69] CA-142595: [UnitTest] Add tests for the fix Add unit tests relating to the fix proposed by Wang Yanbin. Signed-off-by: Flavien Quesnel Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 42911bbe95829b8cf09a59850ab3300d6a52a359) --- tests/test_devscan.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_devscan.py b/tests/test_devscan.py index 51aea3d4..19fd9091 100644 --- a/tests/test_devscan.py +++ b/tests/test_devscan.py @@ -96,6 +96,24 @@ def test_no_adapters(self, context): self.assertEquals({'devs': {}, 'adt': {}}, result) + @mock.patch('devscan.match_hbadevs') + @testlib.with_context + def test_exotic_adapter_with_security_device(self, context, match_hbadevs): + adapter = context.add_adapter(testlib.AdapterWithNonBlockDevice()) + adapter.add_disk() + + match_hbadevs.return_value = 'lpfc' + result = devscan.adapters() + + self.assertEquals( + { + 'devs': {}, + 'adt': { + 'host0': 'lpfc' + } + }, + result) + @testlib.with_context def test_adapter_and_disk_added(self, context): adapter = context.add_adapter(testlib.SCSIAdapter()) @@ -135,3 +153,13 @@ def test_3x_kernel(self, context): result = devscan._extract_dev_name('/somepath') self.assertEquals('sde', result) + + @testlib.with_context + def test_extract_dev_name_from_directory_without_block_device( + self, + context): + context.kernel_version = '3.10' + + result = devscan._extract_dev_name('/nonexisting') + + self.assertEquals('', result) From 9971132d4c3541912f2fb898eb105631b9a50762 Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Wed, 3 Sep 2014 12:18:18 +0100 Subject: [PATCH 37/69] CA-142595: Refactor devscan.py * extract INVALID_DEVICE_NAME * extract update_devs_dict * extract _get_block_device_name_with_kernel_3x Signed-off-by: Flavien Quesnel Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 3b1df13580e722c6a5231c146a5b9e3da734f331) --- drivers/devscan.py | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/drivers/devscan.py b/drivers/devscan.py index d05cee04..eb4aac8e 100644 --- a/drivers/devscan.py +++ b/drivers/devscan.py @@ -29,6 +29,8 @@ DRIVER_BLACKLIST = ['^(s|p|)ata_.*', '^ahci$', '^pdc_adma$', '^iscsi_tcp$'] +INVALID_DEVICE_NAME = '' + def getManufacturer(s): (rc,stdout,stderr) = util.doexec(['/sbin/modinfo', '-d', s]) if stdout: @@ -36,6 +38,11 @@ def getManufacturer(s): else: return "Unknown" +def update_devs_dict(devs, dev, entry): + if dev != INVALID_DEVICE_NAME: + devs[dev] = entry + + def adapters(filterstr="any"): dict = {} devs = {} @@ -73,8 +80,7 @@ def adapters(filterstr="any"): else: dir = os.path.join(sysfs,lun,"device") (dev, entry) = _extract_dev(dir, proc, id, lun) - if dev != "": - devs[dev] = entry + update_devs_dict(devs, dev, entry) # for new qlogic sysfs layout (rport under device, then target) for i in filter(match_rport,os.listdir(path)): newpath = os.path.join(path, i) @@ -86,7 +92,7 @@ def adapters(filterstr="any"): continue dir = os.path.join(sysfs,lun,"device") (dev, entry) = _extract_dev(dir, proc, id, lun) - devs[dev] = entry + update_devs_dict(devs, dev, entry) # for new mptsas sysfs entries, check for phy* node for i in filter(match_phy,os.listdir(path)): @@ -98,7 +104,7 @@ def adapters(filterstr="any"): continue dir = os.path.join(sysfs,lun,"device") (dev, entry) = _extract_dev(dir, proc, id, lun) - devs[dev] = entry + update_devs_dict(devs, dev, entry) if path.startswith(SYSFS_PATH2): os.path.join(path,"device","block:*") dev = _extract_dev_name(os.path.join(path, 'device')) @@ -107,7 +113,7 @@ def adapters(filterstr="any"): hbtl = os.path.basename(path) (h,b,t,l) = hbtl.split(':') entry = {'procname':proc, 'host':id, 'target':l} - devs[dev] = entry + update_devs_dict(devs, dev, entry) dict['devs'] = devs dict['adt'] = adt @@ -225,17 +231,19 @@ def _extract_dev_name(device_dir): elif kernel_version.startswith('3.'): # directory for device name lives inside block directory e.g. block/sdx - #bug fixed: some device dir has no subdir "block/" , e.g. LUN 0 has no "block/" subdir, but other LUNs are OK - devs = glob.glob(os.path.join(device_dir, 'block/*')) - if len(devs): - # prune path to extract the device name - return os.path.basename(devs[0]) - else: - return "" + return _get_block_device_name_with_kernel_3x(device_dir) else: msg = 'Kernel version detected: %s' % kernel_version raise xs_errors.XenError('UnsupportedKernel', msg) +def _get_block_device_name_with_kernel_3x(device_dir): + devs = glob.glob(os.path.join(device_dir, 'block/*')) + if len(devs): + # prune path to extract the device name + return os.path.basename(devs[0]) + else: + return INVALID_DEVICE_NAME + def _extract_dev(device_dir, procname, host, target): """Returns device name and creates dictionary entry for it""" dev = _extract_dev_name(device_dir) From f6c4a4c97e7669bbaf020809fc927554a2f28a11 Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Wed, 3 Sep 2014 12:19:44 +0100 Subject: [PATCH 38/69] CA-142595: [UnitTest] cover devscan.py Add unit tests, following the refactoring of devscan.py Signed-off-by: Flavien Quesnel Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 95bc8c8e46066f5b679662378095e8db2f89db63) --- tests/test_devscan.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/test_devscan.py b/tests/test_devscan.py index 19fd9091..b7786b96 100644 --- a/tests/test_devscan.py +++ b/tests/test_devscan.py @@ -162,4 +162,24 @@ def test_extract_dev_name_from_directory_without_block_device( result = devscan._extract_dev_name('/nonexisting') - self.assertEquals('', result) + self.assertEquals(devscan.INVALID_DEVICE_NAME, result) + + +class TestUpdateDevsDict(unittest.TestCase): + def test_whencalled_updates_dict(self): + devices = {} + dev = 'dev' + entry = 'entry' + + devscan.update_devs_dict(devices, dev, entry) + + self.assertEquals({'dev': 'entry'}, devices) + + def test_whencalled_with_empty_key_does_not_update_dict(self): + devices = {} + dev = devscan.INVALID_DEVICE_NAME + entry = 'entry' + + devscan.update_devs_dict(devices, dev, entry) + + self.assertEquals({}, devices) From 20eb87a6c4be2596caef67cd9f06db12b51779b4 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 2 Sep 2014 13:02:31 -0400 Subject: [PATCH 39/69] CA-138452: [UnitTest] Tests for cleanup.SR.lock Signed-off-by: Mate Lakat Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit 5423e46446d8613fe8e0861a73706934c869cbd7) --- tests/test_cleanup.py | 141 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 tests/test_cleanup.py diff --git a/tests/test_cleanup.py b/tests/test_cleanup.py new file mode 100644 index 00000000..f35ce36d --- /dev/null +++ b/tests/test_cleanup.py @@ -0,0 +1,141 @@ +import unittest +import mock + +import cleanup + +import util + + +class FakeXapi(object): + def __init__(self): + self.srRecord = { + 'name_label': 'dummy' + } + + def isPluggedHere(self): + return True + + def isMaster(self): + return True + + +class AlwaysLockedLock(object): + def acquireNoblock(self): + return False + + +class AlwaysFreeLock(object): + def acquireNoblock(self): + return True + + +class IrrelevantLock(object): + pass + + +def create_cleanup_sr(): + xapi = FakeXapi() + return cleanup.SR(uuid=None, xapi=xapi, createLock=False, force=False) + + +class TestSR(unittest.TestCase): + def setUp(self): + self.sleep_patcher = mock.patch('cleanup.time.sleep') + self.sleep_patcher.start() + + def tearDown(self): + self.sleep_patcher.stop() + + def setup_abort_flag(self, ipc_mock, should_abort=False): + flag = mock.Mock() + flag.test = mock.Mock(return_value=should_abort) + + ipc_mock.return_value = flag + + def test_lock_if_already_locked(self): + """ + Given an already locked SR, a lock call increments the lock counter + """ + + sr = create_cleanup_sr() + sr._srLock = IrrelevantLock() + sr._locked = 1 + + sr.lock() + + self.assertEquals(2, sr._locked) + + def test_lock_if_no_locking_is_used(self): + """ + Given no srLock present, the lock operations don't touch the counter + """ + + sr = create_cleanup_sr() + sr._srLock = None + + sr.lock() + + self.assertEquals(0, sr._locked) + + @mock.patch('cleanup.IPCFlag') + def test_lock_succeeds_if_lock_is_acquired( + self, + mock_ipc_flag): + """ + After performing a lock, the counter equals to 1 + """ + + self.setup_abort_flag(mock_ipc_flag) + sr = create_cleanup_sr() + sr._srLock = AlwaysFreeLock() + + sr.lock() + + self.assertEquals(1, sr._locked) + + @mock.patch('cleanup.IPCFlag') + def test_lock_raises_exception_if_abort_requested( + self, + mock_ipc_flag): + """ + If IPC abort was requested, lock raises AbortException + """ + + self.setup_abort_flag(mock_ipc_flag, should_abort=True) + sr = create_cleanup_sr() + sr._srLock = AlwaysLockedLock() + + self.assertRaises(cleanup.AbortException, sr.lock) + + @mock.patch('cleanup.IPCFlag') + def test_lock_raises_exception_if_unable_to_acquire_lock( + self, + mock_ipc_flag): + """ + If the lock is busy, SMException is raised + """ + + self.setup_abort_flag(mock_ipc_flag) + sr = create_cleanup_sr() + sr._srLock = AlwaysLockedLock() + + self.assertRaises(util.SMException, sr.lock) + + @mock.patch('cleanup.IPCFlag') + def test_lock_leaves_sr_consistent_if_unable_to_acquire_lock( + self, + mock_ipc_flag): + """ + If the lock is busy, the lock counter is not incremented + """ + + self.setup_abort_flag(mock_ipc_flag) + sr = create_cleanup_sr() + sr._srLock = AlwaysLockedLock() + + try: + sr.lock() + except: + pass + + self.assertEquals(0, sr._locked) From 24ffd6767a44e9bcdd786ff8324bae450b8d6014 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 2 Sep 2014 13:13:29 -0400 Subject: [PATCH 40/69] CA-138452: Fix SR locking If lock acquisition failed, SR._locked would still report that one lock is acquired, thus leaving the SR object in an inconsistent state. Signed-off-by: Mate Lakat Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit 97d503b49027f0f3cc2934c469b7f9bbf10a7e51) --- drivers/cleanup.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/cleanup.py b/drivers/cleanup.py index b20b6499..31ccb507 100755 --- a/drivers/cleanup.py +++ b/drivers/cleanup.py @@ -1653,18 +1653,18 @@ def lock(self): if not self._srLock: return - self._locked += 1 - if self._locked > 1: - return + if self._locked == 0 : + abortFlag = IPCFlag(self.uuid) + for i in range(SR.LOCK_RETRY_ATTEMPTS_LOCK): + if self._srLock.acquireNoblock(): + self._locked += 1 + return + if abortFlag.test(FLAG_TYPE_ABORT): + raise AbortException("Abort requested") + time.sleep(SR.LOCK_RETRY_INTERVAL) + raise util.SMException("Unable to acquire the SR lock") - abortFlag = IPCFlag(self.uuid) - for i in range(SR.LOCK_RETRY_ATTEMPTS_LOCK): - if self._srLock.acquireNoblock(): - return - if abortFlag.test(FLAG_TYPE_ABORT): - raise AbortException("Abort requested") - time.sleep(SR.LOCK_RETRY_INTERVAL) - raise util.SMException("Unable to acquire the SR lock") + self._locked += 1 def unlock(self): if not self._srLock: From e69b174e6aebd43aefe4a9461bc15ee87e1fd06f Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Fri, 19 Sep 2014 16:41:24 +0200 Subject: [PATCH 41/69] CA-146822: fix intermittent test failures As the lock test was querying the GC for the collected locks, it might be possible that when the test starts, there is already an uncollected lock in the GC. To avoid this situation, run the GC, so only the garbage created by the test will be collected. Signed-off-by: Mate Lakat Reviewed-by: Flavien Quesnel Imported-by: Thanos Makatos (cherry picked from commit 02ac5351c01aaa8806c4c5a26717fc5a47cd605d) --- tests/test_lock.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_lock.py b/tests/test_lock.py index 02901d60..7a1510b7 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -75,6 +75,9 @@ def _open_lockfile(self): class TestLockDestruction(unittest.TestCase): def setUp(self): + gc.collect() + locks = self.retrieve_lock_instances_from_gc() + assert 0 == len(locks) gc.disable() @testlib.with_custom_context(FailingOpenContext) From fd2ca22e4f3272eec59a071f5038362ec6628240 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Tue, 16 Sep 2014 14:19:09 +0100 Subject: [PATCH 42/69] CA-146138: don't refresh slaves for local SRs Don't try to synchronise logical volumes on other slaves for a SR that is local to the host, it doesn't make any sense and can cause all sorts of problems. Signed-off-by: Thanos Makatos Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit f33332e77cefba55f24afe2a54792f2f83cc8607) --- drivers/LVHDSR.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/LVHDSR.py b/drivers/LVHDSR.py index c416268a..1c41fe3d 100755 --- a/drivers/LVHDSR.py +++ b/drivers/LVHDSR.py @@ -1159,8 +1159,9 @@ def _undoAllInflateJournals(self): lvhdutil.deflate(self.lvmCache, vdi.lvname, int(val)) if vdi.readonly: self.lvmCache.setReadonly(vdi.lvname, True) - lvhdutil.lvRefreshOnAllSlaves(self.session, self.uuid, - self.vgname, vdi.lvname, uuid) + if "true" == self.session.xenapi.SR.get_shared(self.sr_ref): + lvhdutil.lvRefreshOnAllSlaves(self.session, self.uuid, + self.vgname, vdi.lvname, uuid) self.journaler.remove(lvhdutil.JRN_INFLATE, uuid) delattr(self,"vdiInfo") delattr(self,"allVDIs") From bc604644657b0162d88f125b55be22d0fa1a6751 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Fri, 19 Sep 2014 13:05:54 +0100 Subject: [PATCH 43/69] CA-146138: improve error message when srcmd.params is malformed Signed-off-by: Thanos Makatos Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 1384e58cbd3f5c8feec18f6aa734b2944db9c0e2) --- drivers/SR.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/SR.py b/drivers/SR.py index 41c6ce77..699524e2 100755 --- a/drivers/SR.py +++ b/drivers/SR.py @@ -24,6 +24,7 @@ import xs_errors import XenAPI, xmlrpclib, util import copy, os +import traceback MOUNT_BASE = '/var/run/sr-mount' DEFAULT_TAP = 'vhd' @@ -111,6 +112,8 @@ def __init__(self, srcmd, sr_uuid): if self.dconf.get("SRmaster") == "true": os.environ['LVM_SYSTEM_DIR'] = MASTER_LVM_CONF + except TypeError: + raise Exception(traceback.format_exc()) except Exception, e: raise e raise xs_errors.XenError('SRBadXML') From 89278ccdc1835f08a064057a7eebceb84b6521f2 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Fri, 19 Sep 2014 13:04:49 +0100 Subject: [PATCH 44/69] CA-146138: unit test for LV refresh on other hosts Signed-off-by: Thanos Makatos Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit 6508d6602ee7af9a9d23acd601ee29c45b7434de) --- tests/test_LVHDSR.py | 56 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 tests/test_LVHDSR.py diff --git a/tests/test_LVHDSR.py b/tests/test_LVHDSR.py new file mode 100644 index 00000000..801370a9 --- /dev/null +++ b/tests/test_LVHDSR.py @@ -0,0 +1,56 @@ +import unittest +import mock +import LVHDSR +import journaler +import lvhdutil + +class SMlog(object): + def __call__(self, *args): + print args[0] + +class FakeLVHDSR(LVHDSR.LVHDSR): + + def __init__self(self, srcmd, uuid): + super(FakeLVHDSR, self).__init__(srcmd, uuid) + +class TestLVHDSR(unittest.TestCase): + + def create_LVHDSR(self): + srcmd = mock.Mock() + srcmd.dconf = {'device': '/dev/bar'} + srcmd.params = {'command': 'foo', 'session_ref': 'some session ref'} + return FakeLVHDSR(srcmd, "some SR UUID") + + @mock.patch('lvhdutil.getVDIInfo') + def test_loadvids(self, mock_getVDIInfo): + vdi_uuid = 'some VDI UUID' + mock_getVDIInfo.return_value = {vdi_uuid: lvhdutil.VDIInfo(vdi_uuid)} + sr = self.create_LVHDSR() + sr._loadvdis() + + @mock.patch('XenAPI.xapi_local') + @mock.patch('journaler.Journaler.remove') + @mock.patch('lvhdutil.lvRefreshOnAllSlaves') + @mock.patch('util.zeroOut') + @mock.patch('lvmcache.LVMCache') + @mock.patch('util.SMlog', new_callable=SMlog) + @mock.patch('lvhdutil.deflate') + @mock.patch('lvhdutil.getVDIInfo') + @mock.patch('journaler.Journaler.getAll') + def test_undoAllInflateJournals(self, mock_getAll, mock_getVDIInfo, + mock_lvhdutil_deflate, mock_smlog, mock_lvmcache, + mock_util_zeroOut, mock_lvhdutil_lvRefreshOnAllSlaves, + mock_journal_remove, mock_xapi): + """Test that cleaning up a journal on a local LVHD SR doesn't result in LV refresh calls in other hosts.""" + + vdi_uuid = 'some VDI UUID' + + mock_getAll.return_value = {vdi_uuid: '0'} + mock_getVDIInfo.return_value = {vdi_uuid: lvhdutil.VDIInfo(vdi_uuid)} + + sr = self.create_LVHDSR() + + sr._undoAllInflateJournals() + self.assertEquals(0, mock_lvhdutil_lvRefreshOnAllSlaves.call_count, + "cleaning up journals on a local SR shouldn't result in any " + "update on other hosts") From 4729b77813dc39015234d621d5104f6b74842992 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Fri, 19 Sep 2014 20:53:09 +0200 Subject: [PATCH 45/69] CA-146138: various improvements over unit test Signed-off-by: Mate Lakat Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit 071789ceca90e38b061a8a102cc622a3f667169f) --- tests/test_LVHDSR.py | 64 +++++++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/tests/test_LVHDSR.py b/tests/test_LVHDSR.py index 801370a9..015012cd 100644 --- a/tests/test_LVHDSR.py +++ b/tests/test_LVHDSR.py @@ -4,44 +4,68 @@ import journaler import lvhdutil -class SMlog(object): + +class SMLog(object): def __call__(self, *args): - print args[0] + print args + + +class Stubs(object): + def init_stubs(self): + self._stubs = [] + + def stubout(self, *args, **kwargs): + patcher = mock.patch(*args, **kwargs) + self._stubs.append(patcher) + patcher.start() + + def remove_stubs(self): + for patcher in self._stubs: + patcher.stop() -class FakeLVHDSR(LVHDSR.LVHDSR): - def __init__self(self, srcmd, uuid): - super(FakeLVHDSR, self).__init__(srcmd, uuid) +class TestLVHDSR(unittest.TestCase, Stubs): -class TestLVHDSR(unittest.TestCase): + def setUp(self): + self.init_stubs() + + def tearDown(self): + self.remove_stubs() def create_LVHDSR(self): srcmd = mock.Mock() srcmd.dconf = {'device': '/dev/bar'} srcmd.params = {'command': 'foo', 'session_ref': 'some session ref'} - return FakeLVHDSR(srcmd, "some SR UUID") + return LVHDSR.LVHDSR(srcmd, "some SR UUID") @mock.patch('lvhdutil.getVDIInfo') def test_loadvids(self, mock_getVDIInfo): + """sr.allVDIs populated by _loadvdis""" + vdi_uuid = 'some VDI UUID' mock_getVDIInfo.return_value = {vdi_uuid: lvhdutil.VDIInfo(vdi_uuid)} sr = self.create_LVHDSR() + sr._loadvdis() - @mock.patch('XenAPI.xapi_local') - @mock.patch('journaler.Journaler.remove') + self.assertEquals([vdi_uuid], sr.allVDIs.keys()) + @mock.patch('lvhdutil.lvRefreshOnAllSlaves') - @mock.patch('util.zeroOut') - @mock.patch('lvmcache.LVMCache') - @mock.patch('util.SMlog', new_callable=SMlog) - @mock.patch('lvhdutil.deflate') @mock.patch('lvhdutil.getVDIInfo') @mock.patch('journaler.Journaler.getAll') - def test_undoAllInflateJournals(self, mock_getAll, mock_getVDIInfo, - mock_lvhdutil_deflate, mock_smlog, mock_lvmcache, - mock_util_zeroOut, mock_lvhdutil_lvRefreshOnAllSlaves, - mock_journal_remove, mock_xapi): - """Test that cleaning up a journal on a local LVHD SR doesn't result in LV refresh calls in other hosts.""" + def test_undoAllInflateJournals( + self, + mock_getAll, + mock_getVDIInfo, + mock_lvhdutil_lvRefreshOnAllSlaves): + """No LV refresh on slaves when Cleaning up local LVHD SR's journal""" + + self.stubout('XenAPI.xapi_local') + self.stubout('journaler.Journaler.remove') + self.stubout('util.zeroOut') + self.stubout('lvhdutil.deflate') + self.stubout('util.SMlog', new_callable=SMLog) + self.stubout('lvmcache.LVMCache') vdi_uuid = 'some VDI UUID' @@ -51,6 +75,4 @@ def test_undoAllInflateJournals(self, mock_getAll, mock_getVDIInfo, sr = self.create_LVHDSR() sr._undoAllInflateJournals() - self.assertEquals(0, mock_lvhdutil_lvRefreshOnAllSlaves.call_count, - "cleaning up journals on a local SR shouldn't result in any " - "update on other hosts") + self.assertEquals(0, mock_lvhdutil_lvRefreshOnAllSlaves.call_count) From 739c2ebecda9bbb975260bc1e6a143a786ffd325 Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Mon, 22 Sep 2014 19:30:53 +0100 Subject: [PATCH 46/69] CA-146822: Remove TestLockDestruction Remove TestLockDestruction to avoid intermittent test failures Signed-off-by: Flavien Quesnel Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit ad3bc91bbb8dc96adfde6c84da0a23abcee4135b) --- tests/test_lock.py | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/tests/test_lock.py b/tests/test_lock.py index 7a1510b7..7e065687 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -71,37 +71,3 @@ def _open_lockfile(self): return lock.Lock._open_lockfile(self) return LockThatFailsToCreateFile - - -class TestLockDestruction(unittest.TestCase): - def setUp(self): - gc.collect() - locks = self.retrieve_lock_instances_from_gc() - assert 0 == len(locks) - gc.disable() - - @testlib.with_custom_context(FailingOpenContext) - def test_close_if_open_failed(self, ctx): - try: - lck = lock.Lock('somename') - raise AssertionError('An IOError was expected here') - except IOError: - pass - - locks = self.retrieve_lock_instances_from_gc() - self.assertEquals(1, len(locks)) - - lck, = locks - - lck._close() - - def retrieve_lock_instances_from_gc(self): - locks = [] - for obj in gc.get_objects(): - if isinstance(obj, lock.Lock): - locks.append(obj) - - return locks - - def tearDown(self): - gc.enable() From c49a60daff43a5e913200067c12368a12b2f9a04 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Fri, 6 Jun 2014 15:52:41 +0100 Subject: [PATCH 47/69] CP-8514: [UnitTest] Add fake LVM subsystem This test helper makes it possible to fake the lvm layer. This first patch emulates the lvcreate command. Signed-off-by: Mate Lakat Reviewed-by: Vineeth Thampi Raveendran Imported-by: Thanos Makatos (cherry picked from commit 5e5faced801dd6cb3d4d18cc9048b5c73948ca39) --- tests/lvmlib.py | 78 +++++++++++++++++++++++++++++++++++ tests/test_lvmlib.py | 98 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 tests/lvmlib.py create mode 100644 tests/test_lvmlib.py diff --git a/tests/lvmlib.py b/tests/lvmlib.py new file mode 100644 index 00000000..7e61da37 --- /dev/null +++ b/tests/lvmlib.py @@ -0,0 +1,78 @@ +import optparse + + +class LogicalVolume(object): + def __init__(self, vg, name, size_mb, tag, active, zeroed): + self.name = name + self.size_mb = size_mb + self.volume_group = vg + self.tag = tag + self.active = active + self.zeroed = zeroed + + +class VolumeGroup(object): + def __init__(self, name): + self.name = name + self.volumes = [] + + def add_volume(self, name, size_mb, tag=None, active=True, zeroed=True): + self.volumes.append( + LogicalVolume(self, name, size_mb, tag, active, zeroed)) + + +class LVSubsystem(object): + def __init__(self, logger, executable_injector): + self.logger = logger + self.lv_calls = [] + self._volume_groups = [] + executable_injector('/usr/sbin/lvcreate', self.fake_lvcreate) + + def add_volume_group(self, name): + self._volume_groups.append(VolumeGroup(name)) + + def get_logical_volumes_with_name(self, name): + result = [] + for vg in self._volume_groups: + for lv in vg.volumes: + if name == lv.name: + result.append(lv) + return result + + def get_volume_group(self, vgname): + for vg in self._volume_groups: + if vg.name == vgname: + return vg + + def fake_lvcreate(self, args, stdin): + self.logger('lvcreate', repr(args), stdin) + parser = optparse.OptionParser() + parser.add_option("-n", dest='name') + parser.add_option("-L", dest='size_mb') + parser.add_option("--addtag", dest='tag') + parser.add_option("--inactive", dest='inactive', action='store_true') + parser.add_option("--zero", dest='zero', default='y') + try: + options, args = parser.parse_args(args[1:]) + except SystemExit, e: + self.logger("LVCREATE OPTION PARSING FAILED") + return (1, '', str(e)) + + vgname, = args + + if self.get_volume_group(vgname) is None: + self.logger("volume group does not exist:", vgname) + return (1, '', ' Volume group "%s" not found\n' % vgname) + + active = not options.inactive + assert options.zero in ['y', 'n'] + zeroed = options.zero == 'y' + + self.get_volume_group(vgname).add_volume( + options.name, + int(options.size_mb), + options.tag, + active, + zeroed) + + return 0, '', '' diff --git a/tests/test_lvmlib.py b/tests/test_lvmlib.py new file mode 100644 index 00000000..414884e1 --- /dev/null +++ b/tests/test_lvmlib.py @@ -0,0 +1,98 @@ +import unittest +import mock + +import lvmlib + + +class TestLVSubSystem(unittest.TestCase): + def test_lvcreate_is_mocked(self): + executable_injector = mock.Mock() + + lvsubsystem = lvmlib.LVSubsystem(None, executable_injector) + + executable_injector.assert_called_once_with( + '/usr/sbin/lvcreate', lvsubsystem.fake_lvcreate + ) + + def test_add_volume_group(self): + lvsubsystem = lvmlib.LVSubsystem(None, mock.Mock()) + + lvsubsystem.add_volume_group('vg') + vg = lvsubsystem.get_volume_group('vg') + + self.assertEquals('vg', vg.name) + + def test_fake_lvcreate_creates_volume(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + vg = lvsubsystem.add_volume_group('vg') + + exec_result = lvsubsystem.fake_lvcreate( + "someprog -n name -L 100 vg".split(), '') + + lv, = lvsubsystem.get_logical_volumes_with_name('name') + + self.assertEquals('name', lv.name) + self.assertEquals(lvsubsystem.get_volume_group('vg'), lv.volume_group) + self.assertTrue(lv.active) + self.assertTrue(lv.zeroed) + self.assertEquals(None, lv.tag) + self.assertEquals(100, lv.size_mb) + + def test_fake_lvcreate_with_tags(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + lvsubsystem.add_volume_group('vg') + + exec_result = lvsubsystem.fake_lvcreate( + "someprog -n name --addtag tagg -L 100 vg".split(), '') + + lv, = lvsubsystem.get_logical_volumes_with_name('name') + self.assertEquals('tagg', lv.tag) + + def test_fake_lvcreate_inactive(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + lvsubsystem.add_volume_group('vg') + + exec_result = lvsubsystem.fake_lvcreate( + "someprog -n name --inactive -L 100 vg".split(), '') + + lv, = lvsubsystem.get_logical_volumes_with_name('name') + self.assertFalse(lv.active) + + def test_fake_lvcreate_non_zeroed(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + lvsubsystem.add_volume_group('vg') + + exec_result = lvsubsystem.fake_lvcreate( + "someprog -n name --zero n -L 100 vg".split(), '') + + lv, = lvsubsystem.get_logical_volumes_with_name('name') + + self.assertFalse(lv.zeroed) + self.assertExecutionSucceeded(exec_result) + + def test_fake_lvcreate_called_with_wrong_params(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + lvsubsystem.add_volume_group('vg') + + exec_result = lvsubsystem.fake_lvcreate( + "someprog --something-stupid -n name n -L 100 vg".split(), '') + + self.assertExecutionFailed(exec_result) + + def test_fake_lvcreate_fails_if_no_volume_group_found(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + + exec_result = lvsubsystem.fake_lvcreate( + "someprog -n name -L 100 nonexisting".split(), '') + + self.assertExecutionFailed(exec_result) + + def assertExecutionSucceeded(self, exec_result): + returncode, stdout, stderr = exec_result + + self.assertEquals(0, returncode) + + def assertExecutionFailed(self, exec_result): + returncode, stdout, stderr = exec_result + + self.assertEquals(1, returncode) From c4a046e82dae8e588ef4c0b3778a88e845706fb1 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Fri, 6 Jun 2014 15:56:44 +0100 Subject: [PATCH 48/69] CP-8514: [UnitTest] Add tests for lvutil.create These changes test the lvutil.create method just before the TRIM functionality was added. Signed-off-by: Mate Lakat Reviewed-by: Vineeth Thampi Raveendran Imported-by: Thanos Makatos (cherry picked from commit f1218936e3adde34729140f1fbbf7ccbb49c25b0) --- tests/test_lvutil.py | 96 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 tests/test_lvutil.py diff --git a/tests/test_lvutil.py b/tests/test_lvutil.py new file mode 100644 index 00000000..bfdc5cc5 --- /dev/null +++ b/tests/test_lvutil.py @@ -0,0 +1,96 @@ +import unittest +import testlib +import lvmlib + +import lvutil + + +ONE_MEGABYTE=1*1024*1024 + + +def with_lvm_subsystem(func): + @testlib.with_context + def decorated(self, context, *args, **kwargs): + lvsystem = lvmlib.LVSubsystem(context.log, context.add_executable) + return func(self, lvsystem, *args, **kwargs) + + decorated.__name__ = func.__name__ + return decorated + + +class TestCreate(unittest.TestCase): + @with_lvm_subsystem + def test_create_volume_size(self, lvsystem): + lvsystem.add_volume_group('vgroup') + + lvutil.create('volume', 100 * ONE_MEGABYTE, 'vgroup') + + created_lv, = lvsystem.get_logical_volumes_with_name('volume') + + self.assertEquals(100, created_lv.size_mb) + + @with_lvm_subsystem + def test_create_volume_is_in_the_right_volume_group(self, lvsystem): + lvsystem.add_volume_group('vgroup') + + lvutil.create('volume', 100 * ONE_MEGABYTE, 'vgroup') + + created_lv, = lvsystem.get_logical_volumes_with_name('volume') + + self.assertEquals(100, created_lv.size_mb) + + self.assertEquals('vgroup', created_lv.volume_group.name) + self.assertTrue(created_lv.active) + self.assertTrue(created_lv.zeroed) + + @with_lvm_subsystem + def test_create_volume_is_active(self, lvsystem): + lvsystem.add_volume_group('vgroup') + + lvutil.create('volume', 100 * ONE_MEGABYTE, 'vgroup') + + created_lv, = lvsystem.get_logical_volumes_with_name('volume') + + self.assertEquals(100, created_lv.size_mb) + + self.assertTrue(created_lv.active) + self.assertTrue(created_lv.zeroed) + + @with_lvm_subsystem + def test_create_volume_is_zeroed(self, lvsystem): + lvsystem.add_volume_group('vgroup') + + lvutil.create('volume', 100 * ONE_MEGABYTE, 'vgroup') + + created_lv, = lvsystem.get_logical_volumes_with_name('volume') + + self.assertEquals(100, created_lv.size_mb) + + self.assertTrue(created_lv.zeroed) + + @with_lvm_subsystem + def test_create_creates_logical_volume_with_tags(self, lvsystem): + lvsystem.add_volume_group('vgroup') + + lvutil.create('volume', ONE_MEGABYTE, 'vgroup', tag='hello') + + created_lv, = lvsystem.get_logical_volumes_with_name('volume') + self.assertEquals('hello', created_lv.tag) + + @with_lvm_subsystem + def test_create_creates_logical_volume_inactive(self, lvsystem): + lvsystem.add_volume_group('vgroup') + + lvutil.create('volume', ONE_MEGABYTE, 'vgroup', activate=False) + + created_lv, = lvsystem.get_logical_volumes_with_name('volume') + self.assertFalse(created_lv.active) + + @with_lvm_subsystem + def test_create_inactive_volume_is_not_zeroed(self, lvsystem): + lvsystem.add_volume_group('vgroup') + + lvutil.create('volume', ONE_MEGABYTE, 'vgroup', activate=False) + + created_lv, = lvsystem.get_logical_volumes_with_name('volume') + self.assertFalse(created_lv.zeroed) From 8676065e19aec2f1e5189e625cb3f2bd3546e833 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Fri, 6 Jun 2014 18:04:43 +0100 Subject: [PATCH 49/69] CP-8514: [UnitTest] Mock out os.stat As the LVM operations use os.stat to detect the existence of a given path, it needs to be mocked out. This change adds a simple implementation, which does not return the expected stat information, just raises an OSError if the file does not exists. Signed-off-by: Mate Lakat Reviewed-by: Vineeth Thampi Raveendran Imported-by: Thanos Makatos (cherry picked from commit e3b03973c8a00ec0aef6edd13408bb3d48bc99ce) --- tests/test_testlib.py | 12 ++++++++++++ tests/testlib.py | 5 +++++ 2 files changed, 17 insertions(+) diff --git a/tests/test_testlib.py b/tests/test_testlib.py index 95dd9fa8..e562ce28 100644 --- a/tests/test_testlib.py +++ b/tests/test_testlib.py @@ -135,6 +135,18 @@ def test_exists_returns_false_for_non_existing(self, context): def test_exists_returns_true_for_root(self, context): self.assertTrue(os.path.exists('/')) + @testlib.with_context + def test_stat_nonexistent_file_throws_oserror(self, context): + self.assertRaises( + OSError, + lambda: os.stat('/nonexistingstuff')) + + @testlib.with_context + def test_stat_does_not_fail_with_existing_file(self, context): + os.makedirs('/existingstuff') + + os.stat('/existingstuff') + @testlib.with_context def test_error_codes_read(self, context): context.setup_error_codes() diff --git a/tests/testlib.py b/tests/testlib.py index 40110b15..3d6ea6aa 100644 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -128,6 +128,7 @@ def start(self): mock.patch('os.uname', new=self.fake_uname), mock.patch('subprocess.Popen', new=self.fake_popen), mock.patch('os.rmdir', new=self.fake_rmdir), + mock.patch('os.stat', new=self.fake_stat), ] map(lambda patcher: patcher.start(), self.patchers) self.setup_modinfo() @@ -143,6 +144,10 @@ def fake_rmdir(self, path): self._created_directories = [ d for d in self._created_directories if d != path] + def fake_stat(self, path): + if not self.fake_exists(path): + raise OSError() + def fake_makedirs(self, path): if path in self.get_filesystem(): raise OSError(path + " Already exists") From c8aad997427545c3f88d86a1c3d3800a3475bc0f Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Fri, 6 Jun 2014 18:07:41 +0100 Subject: [PATCH 50/69] CP-8514: [UnitTest] Mock out lvremove and dmsetup To test lvutil.remove, the /usr/bin/lvremove and /sbin/dmsetup system utilities mocked out with a primitive implementation. dmsetup will just return 0 with empty stdout and stderr, which seems to be enough for now. Signed-off-by: Mate Lakat Reviewed-by: Vineeth Thampi Raveendran Imported-by: Thanos Makatos (cherry picked from commit 4651b3d7c46ef2c1410638e18dd5c01e6a91d393) --- tests/lvmlib.py | 29 ++++++++++++++++ tests/test_lvmlib.py | 81 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 101 insertions(+), 9 deletions(-) diff --git a/tests/lvmlib.py b/tests/lvmlib.py index 7e61da37..f09a0397 100644 --- a/tests/lvmlib.py +++ b/tests/lvmlib.py @@ -20,6 +20,9 @@ def add_volume(self, name, size_mb, tag=None, active=True, zeroed=True): self.volumes.append( LogicalVolume(self, name, size_mb, tag, active, zeroed)) + def delete_volume(self, volume): + self.volumes = [vol for vol in self.volumes if vol != volume] + class LVSubsystem(object): def __init__(self, logger, executable_injector): @@ -27,6 +30,8 @@ def __init__(self, logger, executable_injector): self.lv_calls = [] self._volume_groups = [] executable_injector('/usr/sbin/lvcreate', self.fake_lvcreate) + executable_injector('/usr/sbin/lvremove', self.fake_lvremove) + executable_injector('/sbin/dmsetup', self.fake_dmsetup) def add_volume_group(self, name): self._volume_groups.append(VolumeGroup(name)) @@ -76,3 +81,27 @@ def fake_lvcreate(self, args, stdin): zeroed) return 0, '', '' + + def fake_lvremove(self, args, stdin): + self.logger('lvremove', repr(args), stdin) + parser = optparse.OptionParser() + parser.add_option( + "-f", "--force", dest='force', action='store_true', default=False) + self.logger(args, stdin) + try: + options, args = parser.parse_args(args[1:]) + except SystemExit, e: + self.logger("LVREMOVE OPTION PARSING FAILED") + return (1, '', str(e)) + + lvpath, = args + + for vg in self._volume_groups: + for lv in vg.volumes: + if '/'.join([vg.name, lv.name]) == lvpath: + vg.delete_volume(lv) + + return 0, '', '' + + def fake_dmsetup(self, args, stdin): + return 0, '', '' diff --git a/tests/test_lvmlib.py b/tests/test_lvmlib.py index 414884e1..36b783e9 100644 --- a/tests/test_lvmlib.py +++ b/tests/test_lvmlib.py @@ -4,14 +4,47 @@ import lvmlib -class TestLVSubSystem(unittest.TestCase): +class ExecResultMixIn(object): + def assertExecutionSucceeded(self, exec_result): + returncode, stdout, stderr = exec_result + + self.assertEquals(0, returncode) + + def assertExecutionFailed(self, exec_result): + returncode, stdout, stderr = exec_result + + self.assertEquals(1, returncode) + + +class TestLVSubSystem(unittest.TestCase, ExecResultMixIn): def test_lvcreate_is_mocked(self): executable_injector = mock.Mock() lvsubsystem = lvmlib.LVSubsystem(None, executable_injector) - executable_injector.assert_called_once_with( - '/usr/sbin/lvcreate', lvsubsystem.fake_lvcreate + self.assertTrue( + mock.call('/usr/sbin/lvcreate', lvsubsystem.fake_lvcreate) + in executable_injector.mock_calls + ) + + def test_lvremove_is_mocked(self): + executable_injector = mock.Mock() + + lvsubsystem = lvmlib.LVSubsystem(None, executable_injector) + + self.assertTrue( + mock.call('/usr/sbin/lvremove', lvsubsystem.fake_lvremove) + in executable_injector.mock_calls + ) + + def test_dmsetup_is_mocked(self): + executable_injector = mock.Mock() + + lvsubsystem = lvmlib.LVSubsystem(None, executable_injector) + + self.assertTrue( + mock.call('/sbin/dmsetup', lvsubsystem.fake_dmsetup) + in executable_injector.mock_calls ) def test_add_volume_group(self): @@ -87,12 +120,42 @@ def test_fake_lvcreate_fails_if_no_volume_group_found(self): self.assertExecutionFailed(exec_result) - def assertExecutionSucceeded(self, exec_result): - returncode, stdout, stderr = exec_result + def test_fake_lvremove(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + lvsubsystem.add_volume_group('vg') + lvsubsystem.get_volume_group('vg').add_volume('lv', 100) - self.assertEquals(0, returncode) + exec_result = lvsubsystem.fake_lvremove( + "someprog vg/lv".split(), '') + + self.assertExecutionSucceeded(exec_result) + + def test_fake_lvremove_with_force(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + lvsubsystem.add_volume_group('vg') + lvsubsystem.get_volume_group('vg').add_volume('lv', 100) + + exec_result = lvsubsystem.fake_lvremove( + "someprog -f vg/lv".split(), '') + + self.assertExecutionSucceeded(exec_result) + + def test_fake_lvremove_with_bad_params(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + lvsubsystem.add_volume_group('vg') + lvsubsystem.get_volume_group('vg').add_volume('lv', 100) + + exec_result = lvsubsystem.fake_lvremove( + "someprog -f vg/lv --stupid-parameter".split(), '') + + self.assertExecutionFailed(exec_result) + + def test_fake_dmsetup_status_returns_zero(self): + lvsubsystem = lvmlib.LVSubsystem(mock.Mock(), mock.Mock()) + + exec_result = lvsubsystem.fake_dmsetup( + "someprog status".split(), '') + + self.assertExecutionSucceeded(exec_result) - def assertExecutionFailed(self, exec_result): - returncode, stdout, stderr = exec_result - self.assertEquals(1, returncode) From 936a1736848be6942c185847a6417ee713689fa7 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Fri, 6 Jun 2014 18:13:02 +0100 Subject: [PATCH 51/69] CP-8514: [UnitTest] Cover lvutil.remove Testing the behavior of lvutil.remove. Signed-off-by: Mate Lakat Reviewed-by: Vineeth Thampi Raveendran Imported-by: Thanos Makatos (cherry picked from commit b41370f2155c281b89ca0dbdf4b511bf1bd63932) --- tests/test_lvutil.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/test_lvutil.py b/tests/test_lvutil.py index bfdc5cc5..d51f0f79 100644 --- a/tests/test_lvutil.py +++ b/tests/test_lvutil.py @@ -94,3 +94,14 @@ def test_create_inactive_volume_is_not_zeroed(self, lvsystem): created_lv, = lvsystem.get_logical_volumes_with_name('volume') self.assertFalse(created_lv.zeroed) + + +class TestRemove(unittest.TestCase): + @with_lvm_subsystem + def test_remove_removes_volume(self, lvsystem): + lvsystem.add_volume_group('vgroup') + lvsystem.get_volume_group('vgroup').add_volume('volume', 100) + + lvutil.remove('vgroup/volume') + + self.assertEquals([], lvsystem.get_logical_volumes_with_name('volume')) From d56008aac4612f28f5c3b24b302d300aaca24b33 Mon Sep 17 00:00:00 2001 From: Vineeth Raveendran Date: Tue, 10 Jun 2014 16:24:51 +0000 Subject: [PATCH 52/69] CP-8514: [UnitTest] lvutil.lvcreate and lvremove Test functionality for lvcreate size_in_percentage, additional config_param arg for lvremove Signed-off-by: Vineeth Raveendran Reviewed-by: Mate Lakat Imported-by: Thanos Makatos (cherry picked from commit bf822ba084d353e4d59fc6c86304419c51d6b12f) --- tests/test_lvutil.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_lvutil.py b/tests/test_lvutil.py index d51f0f79..c96bdd1f 100644 --- a/tests/test_lvutil.py +++ b/tests/test_lvutil.py @@ -1,6 +1,7 @@ import unittest import testlib import lvmlib +import mock import lvutil @@ -95,6 +96,14 @@ def test_create_inactive_volume_is_not_zeroed(self, lvsystem): created_lv, = lvsystem.get_logical_volumes_with_name('volume') self.assertFalse(created_lv.zeroed) + @mock.patch('util.pread2') + def test_create_percentage_has_precedence_over_size(self, mock_pread2): + lvutil.create('volume', ONE_MEGABYTE, 'vgroup', + size_in_percentage="10%F") + + mock_pread2.assert_called_once_with( + [lvutil.CMD_LVCREATE] + "-n volume -l 10%F vgroup".split()) + class TestRemove(unittest.TestCase): @with_lvm_subsystem @@ -105,3 +114,12 @@ def test_remove_removes_volume(self, lvsystem): lvutil.remove('vgroup/volume') self.assertEquals([], lvsystem.get_logical_volumes_with_name('volume')) + + @mock.patch('lvutil._lvmBugCleanup') + @mock.patch('util.pread2') + def test_remove_additional_config_param(self, mock_pread2, _bugCleanup): + lvutil.remove('vgroup/volume', config_param="blah") + mock_pread2.assert_called_once_with( + [lvutil.CMD_LVREMOVE] + + "-f vgroup/volume --config devices{blah}".split() + ) From aac3df8c0eea3626a77f3e02970b5643396f96a6 Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Thu, 25 Sep 2014 18:44:40 +0100 Subject: [PATCH 53/69] CA-145685: Create a tap disk to pass the host DVD drive to a PV guest More robust version. Supersedes commit 050d45de Signed-off-by: Flavien Quesnel Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit a121c3063488f8e22a536ea17c14c2f95eea9f15) --- drivers/blktap2.py | 16 +++++++++++++++- drivers/udevSR.py | 16 ++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/drivers/blktap2.py b/drivers/blktap2.py index 2e3a3ff6..4dcd59a9 100755 --- a/drivers/blktap2.py +++ b/drivers/blktap2.py @@ -999,6 +999,7 @@ def _tap_type(vdi_type): 'iso' : 'aio', # for ISO SR 'aio' : 'aio', # for LVHD 'file' : 'aio', + 'phy' : 'aio' } [vdi_type] def get_tap_type(self): @@ -1038,7 +1039,15 @@ def tap_wanted(self): raise self.UnexpectedVDIType(vdi_type, self.target.vdi) - if plug_type == 'tap': return True + if plug_type == 'tap': + return True + elif vdi_type == 'phy': + sm_config = util.default( + self.target.vdi.sr, + "sm_config", + self.get_sr_sm_config) + if sm_config.get('type') == 'cd': + return True # 2. Otherwise, there may be more reasons # @@ -1046,6 +1055,11 @@ def tap_wanted(self): return False + def get_sr_sm_config(self): + sr = self.target.vdi.session.xenapi.SR + sr_ref = sr.get_by_uuid(self.target.vdi.sr.uuid) + return sr.get_sm_config(sr_ref) + class TargetDriver: """Safe target driver access.""" diff --git a/drivers/udevSR.py b/drivers/udevSR.py index ae22db53..ff9a0427 100755 --- a/drivers/udevSR.py +++ b/drivers/udevSR.py @@ -57,8 +57,20 @@ def type(self, sr_uuid): def vdi(self, uuid): util.SMlog("params = %s" % (self.srcmd.params.keys())) - return udevVDI(self, self.srcmd.params['vdi_location']) - + + if 'vdi_location' in self.srcmd.params: + vdi_location = self.srcmd.params['vdi_location'] + else: + vdi_location = self.get_vdi_location(uuid) + + return udevVDI(self, vdi_location) + + def get_vdi_location(self, uuid): + import XenAPI + vdi = self.session.xenapi.VDI + vdi_ref = vdi.get_by_uuid(uuid) + return vdi.get_location(vdi_ref) + def load(self, sr_uuid): # First of all, check we've got the correct keys in dconf if not self.dconf.has_key('location'): From 55f9d15e5d225d83f69661bee0ee5605ed772d55 Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Thu, 25 Sep 2014 18:46:35 +0100 Subject: [PATCH 54/69] CA-145685: [UnitTest] Add unit tests Supersedes commit 801f9e9c Signed-off-by: Flavien Quesnel Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 3060073a8dbb9396452cf38d654f70b580c4c1c4) --- tests/test_blktap2.py | 37 +++++++++++++++++++++++++++++++++++++ tests/test_udevSR.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 tests/test_blktap2.py create mode 100644 tests/test_udevSR.py diff --git a/tests/test_blktap2.py b/tests/test_blktap2.py new file mode 100644 index 00000000..4a2d303c --- /dev/null +++ b/tests/test_blktap2.py @@ -0,0 +1,37 @@ +import unittest +import blktap2 +import mock + + +class TestVDI(unittest.TestCase): + @mock.patch('blktap2.VDI.TargetDriver') + def setUp(self, mock_target): + mock_target.get_vdi_type.return_value = 'phy' + mock_target.vdi.sr.sm_config = {'type': 'cd'} + + self.vdi = blktap2.VDI('uuid', mock_target, None) + self.vdi.target = mock_target + + def test_tap_wanted_returns_true_for_physical_dvd_drive(self): + result = self.vdi.tap_wanted() + + self.assertEquals(True, result) + + @mock.patch('blktap2.VDI.get_sr_sm_config') + def test_tap_wanted_returns_true_for_dvd_drive_if_sm_config_deleted( + self, + mock_sr_sm_config): + mock_sr_sm_config.return_value = {'type': 'cd'} + del self.vdi.target.vdi.sr.sm_config + + sm_config_exists = hasattr(self.vdi.target.vdi.sr, 'sm_config') + self.assertEquals(False, sm_config_exists) + + result = self.vdi.tap_wanted() + + self.assertEquals(True, result) + + def test_get_tap_type_returns_aio_for_physical_dvd_drive(self): + result = self.vdi.get_tap_type() + + self.assertEquals('aio', result) diff --git a/tests/test_udevSR.py b/tests/test_udevSR.py new file mode 100644 index 00000000..5935ebdd --- /dev/null +++ b/tests/test_udevSR.py @@ -0,0 +1,28 @@ +import unittest +import udevSR +import SRCommand +import mock + + +VDI_LOCATION = '/path/to/vdi' + + +class TestVdi(unittest.TestCase): + + @mock.patch('udevSR.udevSR.get_vdi_location') + @mock.patch('udevSR.udevSR.load') + def test_vdi_succeeds_if_vdi_location_not_in_params_dictionary( + self, + mock_load, + mock_get_vdi_location): + mock_get_vdi_location.return_value = VDI_LOCATION + srcmd = SRCommand.SRCommand('driver_info') + srcmd.params = {'command': 'cmd'} + sr_uuid = 'sr_uuid' + udev_sr = udevSR.udevSR(srcmd, sr_uuid) + + self.assertEquals(None, udev_sr.srcmd.params.get('vdi_location')) + + udev_vdi = udev_sr.vdi('vdi_uuid') + + self.assertEquals(VDI_LOCATION, udev_vdi.location) From 13b963b982d5707ccfc652624b9c67f82458a1fe Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Fri, 3 Oct 2014 14:38:55 +0100 Subject: [PATCH 55/69] CA-147816: Create a tap disk for every kind of udev device Commit a121c306 was about creating a tap disk when passing a physical cd device to a guest. For that purpose, we checked that the value of sr.sm_config['type'] was equal to 'cd'. With CA-147816, we also want a tap disk in case of a USB device. However, sr.sm_config['type'] is equal to 'block' in this case, which may be too general; for instance, we do not want to create a tap disk for a storage array. Moreover, there is no easy way to know that we are dealing with a USB device. According to the XenServer 6.2 administration guide, a udev SR only handles cd and usb devices. This fix relies on this assumption, since a tap disk is now created if the SR handles udev devices. Signed-off-by: Flavien Quesnel Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit ce2e4982d137600073af2e6530a62392a9638567) --- drivers/blktap2.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/drivers/blktap2.py b/drivers/blktap2.py index 4dcd59a9..0a974395 100755 --- a/drivers/blktap2.py +++ b/drivers/blktap2.py @@ -1041,13 +1041,8 @@ def tap_wanted(self): if plug_type == 'tap': return True - elif vdi_type == 'phy': - sm_config = util.default( - self.target.vdi.sr, - "sm_config", - self.get_sr_sm_config) - if sm_config.get('type') == 'cd': - return True + elif self.target.vdi.sr.handles('udev'): + return True # 2. Otherwise, there may be more reasons # @@ -1055,11 +1050,6 @@ def tap_wanted(self): return False - def get_sr_sm_config(self): - sr = self.target.vdi.session.xenapi.SR - sr_ref = sr.get_by_uuid(self.target.vdi.sr.uuid) - return sr.get_sm_config(sr_ref) - class TargetDriver: """Safe target driver access.""" From 955e4a743db9bc26f15eb3f8f9bb78e74436ad3d Mon Sep 17 00:00:00 2001 From: Flavien Quesnel Date: Fri, 3 Oct 2014 14:57:23 +0100 Subject: [PATCH 56/69] CA-147816: [UnitTest] Update and refactor unit tests Update and refactor unit tests introduced by 3060073a to reflect the changes from a121c306 to c0f26bf4. Signed-off-by: Flavien Quesnel Imported-by: Thanos Makatos (cherry picked from commit c71c96858a4e725166808e05d44c5c386ac813da) --- tests/test_blktap2.py | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/tests/test_blktap2.py b/tests/test_blktap2.py index 4a2d303c..45d1598c 100644 --- a/tests/test_blktap2.py +++ b/tests/test_blktap2.py @@ -7,31 +7,21 @@ class TestVDI(unittest.TestCase): @mock.patch('blktap2.VDI.TargetDriver') def setUp(self, mock_target): mock_target.get_vdi_type.return_value = 'phy' - mock_target.vdi.sr.sm_config = {'type': 'cd'} - self.vdi = blktap2.VDI('uuid', mock_target, None) - self.vdi.target = mock_target - - def test_tap_wanted_returns_true_for_physical_dvd_drive(self): - result = self.vdi.tap_wanted() + def mock_handles(type_str): + return type_str == 'udev' - self.assertEquals(True, result) - - @mock.patch('blktap2.VDI.get_sr_sm_config') - def test_tap_wanted_returns_true_for_dvd_drive_if_sm_config_deleted( - self, - mock_sr_sm_config): - mock_sr_sm_config.return_value = {'type': 'cd'} - del self.vdi.target.vdi.sr.sm_config + mock_target.vdi.sr.handles = mock_handles - sm_config_exists = hasattr(self.vdi.target.vdi.sr, 'sm_config') - self.assertEquals(False, sm_config_exists) + self.vdi = blktap2.VDI('uuid', mock_target, None) + self.vdi.target = mock_target + def test_tap_wanted_returns_true_for_udev_device(self): result = self.vdi.tap_wanted() self.assertEquals(True, result) - def test_get_tap_type_returns_aio_for_physical_dvd_drive(self): + def test_get_tap_type_returns_aio_for_udev_device(self): result = self.vdi.get_tap_type() self.assertEquals('aio', result) From 56e6aca1315950c8cc6fd611368c21931dd8aacf Mon Sep 17 00:00:00 2001 From: Kostas Ladopoulos Date: Thu, 9 Oct 2014 16:03:55 +0100 Subject: [PATCH 57/69] CA-129230: Output of SM is truncated in logs (by syslog) Problem: If an exception other than SR.SRException was raised in SRCommand.run(), it was only caught and logged into SMlog by LVHD. All other SR drivers let it pass to xapi, which in turn truncated the traceback / exception message, making it difficult to debug. Fix: There is now a single, driver agnostic point inside SRCommand.run() that logs all exceptions to SMlog. Traceback is not truncated. NOTE: All individual lines are limited to 989 characters. Signed-off-by: Kostas Ladopoulos Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit ca832ba7ed51bcb3257cb47b5ebc3e59e96f72b1) --- drivers/LVHDSR.py | 13 ++++--------- drivers/SRCommand.py | 22 +++++++++++++++++----- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/LVHDSR.py b/drivers/LVHDSR.py index 1c41fe3d..83de1c4c 100755 --- a/drivers/LVHDSR.py +++ b/drivers/LVHDSR.py @@ -2126,12 +2126,7 @@ def update(self, sr_uuid, vdi_uuid): self.session.xenapi.VDI.get_metadata_of_pool(vdi_ref) LVMMetadataHandler(self.sr.mdpath).updateMetadata(update_map) -try: - if __name__ == '__main__': - SRCommand.run(LVHDSR, DRIVER_INFO) - else: - SR.registerSR(LVHDSR) -except Exception: - util.logException("LVHD") - raise - +if __name__ == '__main__': + SRCommand.run(LVHDSR, DRIVER_INFO) +else: + SR.registerSR(LVHDSR) diff --git a/drivers/SRCommand.py b/drivers/SRCommand.py index d0e80459..16d1e3b8 100755 --- a/drivers/SRCommand.py +++ b/drivers/SRCommand.py @@ -348,8 +348,20 @@ def run(driver, driver_info): print util.return_nil () else: print ret - sys.exit(0) - - except SR.SRException, inst: - print inst.toxml() - sys.exit(0) + + except Exception, e: + try: + util.logException(driver_info['name']) + except KeyError: + util.SMlog('driver_info does not contain a \'name\' key.') + except: + pass + + # If exception is of type SR.SRException, + # pass to xapi, else re-raise. + if isinstance(e, SR.SRException): + print e.toxml() + else: + raise + + sys.exit(0) From 2ee924ac1daa27e7e3b5516ae51b78ce20cdb60d Mon Sep 17 00:00:00 2001 From: Kostas Ladopoulos Date: Thu, 9 Oct 2014 16:19:53 +0100 Subject: [PATCH 58/69] CA-129230: [UnitTest] Three Unit Tests regarding the fix Signed-off-by: Kostas Ladopoulos Reviewed-by: Thanos Makatos Imported-by: Thanos Makatos (cherry picked from commit fdac607a35b1ec4bde528ff424b0273156ea8a8a) --- tests/test_SRCommand.py | 133 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 tests/test_SRCommand.py diff --git a/tests/test_SRCommand.py b/tests/test_SRCommand.py new file mode 100644 index 00000000..c94bf525 --- /dev/null +++ b/tests/test_SRCommand.py @@ -0,0 +1,133 @@ +import unittest +import mock + +import SRCommand + + +class SomeException(Exception): + pass + + +class TestStandaloneFunctions(unittest.TestCase): + + @mock.patch('util.SMlog') + @mock.patch('__builtin__.reduce') + @mock.patch('SRCommand.SRCommand.run_statics') + @mock.patch('SRCommand.SRCommand.parse') + def test_run_correctly_log_all_exceptions( + self, + mock_parse, + mock_run_statics, + mock_reduce, + mock_SMlog): + + """ Assert that any arbitrary exception raised and with a big + message length is logged to SMlog. Only the first line of + the message is asserted (traceback ommited). + """ + + from random import choice + from string import ascii_letters + from DummySR import DRIVER_INFO + + MSG_LEN = 2048 + + # TestSRCommand data member to hold SMlog output. + self.smlog_out = None + + # Generate random exception message of MSG_LEN characters + rand_huge_msg = ''.join(choice(ascii_letters) for _ in range(MSG_LEN)) + + # Create function to raise exception in SRCommand.run() + mock_driver = mock.Mock(side_effect=SomeException(rand_huge_msg)) + + # MockSMlog replaces util.SMlog. Instead of printing to + # /var/log/SMlog, it writes the output to self.smlog_out. + def MockSMlog(str_arg): + self.smlog_out = str_arg.strip() + + mock_reduce.return_value = '' + mock_SMlog.side_effect = MockSMlog + + try: + SRCommand.run(mock_driver, DRIVER_INFO) + except SomeException: + # SomeException needs to be suppressed for this + # test, as it is re-raised after it is logged. + pass + + self.assertEqual(self.smlog_out, + ('***** dummy: EXCEPTION test_SRCommand.' + 'SomeException, ' + rand_huge_msg)) + + @mock.patch('util.logException') + @mock.patch('SRCommand.SRCommand.run_statics') + @mock.patch('SRCommand.SRCommand.parse') + def test_run_print_xml_error_if_SRException( + self, + mock_parse, + mock_run_statics, + mock_logException): + + """ If an SR.SRException is thrown, assert that + "print .toxml()" is called. + """ + + import sys + from StringIO import StringIO + from SR import SRException + from DummySR import DRIVER_INFO + + # Save original sys.stdout file object. + saved_stdout = sys.stdout + + # Create a mock_stdout object and assign it to sys.stdout + mock_stdout = StringIO() + sys.stdout = mock_stdout + + # Create function to raise exception in SRCommand.run() + mock_driver = mock.Mock(side_effect=SRException( + "[UnitTest] SRException thrown")) + + try: + SRCommand.run(mock_driver, DRIVER_INFO) + except SystemExit: + pass + + # Write SRCommand.run() output to variable. + actual_out = mock_stdout.getvalue() + + # Restore the original sys.stdout object. + sys.stdout = saved_stdout + + expected_out = ("\n\n\n" + "\n\nfaultCode\n" + "22\n\n\n" + "faultString\n[UnitTest] " + "SRException thrown\n\n" + "\n\n\n\n") + + self.assertEqual(actual_out, expected_out) + + @mock.patch('util.logException') + @mock.patch('SRCommand.SRCommand.run_statics') + @mock.patch('SRCommand.SRCommand.parse') + def test_run_reraise_if_not_SRException( + self, + mock_parse, + mock_run_statics, + mock_logException): + + """ If an exception other than SR.SRException + is thrown, assert that it is re-raised. + """ + + from DummySR import DRIVER_INFO + + # Create function to raise exception in SRCommand.run() + mock_driver = mock.Mock(side_effect=SomeException) + + try: + SRCommand.run(mock_driver, DRIVER_INFO) + except Exception, e: + self.assertTrue(isinstance(e, SomeException)) From 6422d9c021b410de3cef42f0b080ba044462e078 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Fri, 17 Oct 2014 16:41:46 +0100 Subject: [PATCH 59/69] CP-10069: disable read caching if license-restricted Read caching is disabled if license does not allow it. VDI.attach_from_config does not contain a XAPI session, because this is called by HA before XAPI starts. We don't use read caching for static VDIs because it does not make sense, we only occasionally write data to such VDIs. Signed-off-by: Thanos Makatos [squashed commits] Signed-off-by: Germano Percossi Reviewed-by: Felipe Franciosi Imported-by: Thanos Makatos (cherry picked from commit 494b2d7c863b49c1db84b9909bc7de0874965a6d) Conflicts: drivers/blktap2.py --- drivers/blktap2.py | 9 ++++++--- drivers/util.py | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/blktap2.py b/drivers/blktap2.py index 0a974395..ad895896 100755 --- a/drivers/blktap2.py +++ b/drivers/blktap2.py @@ -1560,9 +1560,12 @@ def _activate(self, sr_uuid, vdi_uuid, options): # Maybe launch a tapdisk on the physical link if self.tap_wanted(): vdi_type = self.target.get_vdi_type() - o_direct = caching_params.get(self.CONF_KEY_O_DIRECT) - if o_direct is None: - o_direct = True + if util.read_caching_is_restricted(self._session): + options["o_direct"] = True + else: + options["o_direct"] = options.get(self.CONF_KEY_O_DIRECT) + if options["o_direct"] is None: + options["o_direct"] = True dev_path = self._tap_activate(phy_path, vdi_type, sr_uuid, options, self._get_pool_config(sr_uuid).get("mem-pool-size")) diff --git a/drivers/util.py b/drivers/util.py index 7dc39a50..0ecb447c 100755 --- a/drivers/util.py +++ b/drivers/util.py @@ -1688,3 +1688,18 @@ def open_atomic(path, mode=None): def isInvalidVDI(exception): return exception.details[0] == "HANDLE_INVALID" or \ exception.details[0] == "UUID_INVALID" + +def get_pool_restrictions(session): + """Returns pool restrictions as a map, @session must be already + established.""" + return session.xenapi.pool.get_all_records().values()[0]['restrictions'] + +def read_caching_is_restricted(session): + """Tells whether read caching is restricted.""" + if session is None or session == "": + return True + restrictions = get_pool_restrictions(session) + if 'restrict_read_caching' in restrictions and \ + restrictions['restrict_read_caching'] == "true": + return True + return False From ba1338442e84f6906a52ae8481ada39f08abb48a Mon Sep 17 00:00:00 2001 From: Chandrika Srinivasan Date: Tue, 24 Jun 2014 15:02:52 +0100 Subject: [PATCH 60/69] CP-8633: Remove support for MPP RDAC LUNs This change makes sure that no calls to MPP RDAC drivers (which are not supported anymore) are made through the storage layer. A check for RDAC LUN always returns false but not before logging a warning if the RDAC LUN path is found to be populated. Signed-off-by: Chandrika Srinivasan Reviewed-by: Germano Percossi Imported-by: Thanos Makatos (cherry picked from commit 0f4c99242ed67af6cd203676f2a426980adad456) --- drivers/mpp_luncheck.py | 7 ++++++- drivers/updatempppathd.py | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/mpp_luncheck.py b/drivers/mpp_luncheck.py index 9771ee44..5f8cd541 100755 --- a/drivers/mpp_luncheck.py +++ b/drivers/mpp_luncheck.py @@ -17,13 +17,18 @@ import sys, os import glob +import util DEVBYMPPPATH = "/dev/disk/by-mpp" def is_RdacLun(scsi_id): path = os.path.join(DEVBYMPPPATH,"%s" % scsi_id) mpppath = glob.glob(path) if len(mpppath): - return True + # Support for RDAC LUNs discontinued + # Always return False + util.SMlog("Found unsupported RDAC LUN at %s" % mpppath, + priority=util.LOG_WARNING) + return False else: return False diff --git a/drivers/updatempppathd.py b/drivers/updatempppathd.py index 3eaed867..77f670ac 100755 --- a/drivers/updatempppathd.py +++ b/drivers/updatempppathd.py @@ -26,6 +26,7 @@ import mpath_dmp import mpp_mpathutil import gc +import mpp_luncheck DEBUG_OUT = False DAEMONISE = True @@ -67,8 +68,7 @@ def UpdatePaths(): for filename in fileList: # extract the SCSI ID from the file name. scsiid = filename.rsplit("/")[len(filename.rsplit("/")) - 1].split('-')[0] - links=glob.glob('/dev/disk/by-mpp/%s' % scsiid) - if not (len(links)): + if not (mpp_luncheck.is_RdacLun(scsiid)): continue # Get the cached value for the total and active paths for this SCSI ID From 569c3c240d3ccf3c63021f5b1ac3facf78fa6664 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 25 Nov 2014 12:23:47 +0100 Subject: [PATCH 61/69] Pin pylint's revision Signed-off-by: Mate Lakat --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index e8b88b19..b951f555 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ mock xenapi coveralls -pylint +pylint==1.2.0 From ff295acb1f266633ae530910764ed718bd8e54d6 Mon Sep 17 00:00:00 2001 From: Germano Percossi Date: Fri, 20 Jun 2014 10:47:42 +0100 Subject: [PATCH 62/69] CP-5962: Borehamwood code into main ISO and not into Supp pack Signed-off-by: Germano Percossi Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 832e9eabd2c2cf2ea755e0a7229449944043310e) --- mk/Makefile | 10 ++-------- mk/setup.py | 20 -------------------- 2 files changed, 2 insertions(+), 28 deletions(-) delete mode 100644 mk/setup.py diff --git a/mk/Makefile b/mk/Makefile index 5ca91f61..f90e8529 100644 --- a/mk/Makefile +++ b/mk/Makefile @@ -22,12 +22,11 @@ ERRORCODES_XML := XE_SR_ERRORCODES.xml SM_TESTS := $(MY_OUTPUT_DIR)/storage-manager-tests.tar SM_UNIT_TESTS := $(MY_OUTPUT_DIR)/smunittests.tar REPO := $(call git_loc,sm) -SUPPORT_PACK := $(MY_OUTPUT_DIR)/borehamwood-supp-pack.iso -BUILD := $(BUILD_NUMBER) + .PHONY: build build: sources $(SM_STAMP) $(SM_TESTS) $(SM_UNIT_TESTS) \ - $(MY_OUTPUT_DIR)/$(ERRORCODES_XML) $(SUPPORT_PACK) + $(MY_OUTPUT_DIR)/$(ERRORCODES_XML) $(MY_SOURCES)/MANIFEST: $(RPM_SRPMSDIR)/$(SM_SRPM) $(MY_SOURCES_DIRSTAMP) $(RPM_BUILD_COOKIE) rm -f $@ @@ -62,8 +61,6 @@ $(SM_STAMP): $(RPM_SRPMSDIR)/$(SM_SRPM) cp $(RPM_RPMSDIR)/$(DOMAIN0_ARCH_OPTIMIZED)/sm-*.rpm $(MY_MAIN_PACKAGES) # Deliberately omit the debuginfo RPM (sm-debuginfo-...) rm -f $(MY_MAIN_PACKAGES)/sm-debuginfo-*.rpm - # sm-rawhba is packaged as supp-pack. Skip it. - rm -f $(MY_MAIN_PACKAGES)/sm-rawhba-*.rpm touch $@ $(SM_TESTS): @@ -77,6 +74,3 @@ $(SM_UNIT_TESTS): $(MY_OUTPUT_DIR)/$(ERRORCODES_XML): rm -f $@ cp $(REPO)/drivers/$(ERRORCODES_XML) $@ - -$(SUPPORT_PACK): $(SM_STAMP) - python setup.py --out $(dir $@) --pdn $(PRODUCT_BRAND) --pdv $(PRODUCT_VERSION) --bld $(BUILD) --spn "borehamwood-supp-pack" --spd "XenServer RawHBA implementation" --rxv "6.1.0" $(RPM_RPMSDIR)/$(DOMAIN0_ARCH_OPTIMIZED)/sm-rawhba-*.$(DOMAIN0_ARCH_OPTIMIZED).rpm diff --git a/mk/setup.py b/mk/setup.py deleted file mode 100644 index 80b2a0bd..00000000 --- a/mk/setup.py +++ /dev/null @@ -1,20 +0,0 @@ -from xcp.supplementalpack import * -from optparse import OptionParser - -parser = OptionParser() -parser.add_option('--pdn', dest="product_name") -parser.add_option('--pdv', dest="product_version") -parser.add_option('--bld', dest="build") -parser.add_option('--out', dest="outdir") -parser.add_option('--spn', dest="sp_name") -parser.add_option('--spd', dest="sp_description") -parser.add_option('--rxv', dest="req_xs_version") -(options, args) = parser.parse_args() - -xs = Requires(originator='xs', name='main', test='ge', - product=options.product_name, version=options.req_xs_version) - -setup(originator='xs', name=options.sp_name, product=options.product_name, - version=options.product_version, build=options.build, vendor='Citrix Systems, Inc.', - description=options.sp_description, packages=args, requires=[xs], - outdir=options.outdir, output=['iso']) From b723309e2670987f7539b7882de963efcc4ed187 Mon Sep 17 00:00:00 2001 From: Germano Percossi Date: Tue, 1 Jul 2014 17:11:34 +0100 Subject: [PATCH 63/69] CP-5962: after rawhba installation the SR is not enabled by default Even though it is packaged in the main ISO and installed by default, this patch will prevent the SR type to be usable unless the proper script is run Signed-off-by: Germano Percossi Reviewed-by: Chandrika Srinivasan Imported-by: Thanos Makatos (cherry picked from commit 5bfb0cb95a8a388a485c319cfaf2f753bd8e9570) --- Makefile | 1 + drivers/enable-borehamwood | 54 ++++++++++++++++++++++++++++++++++++++ mk/sm.spec.in | 3 ++- 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100755 drivers/enable-borehamwood diff --git a/Makefile b/Makefile index b9456a20..14225785 100755 --- a/Makefile +++ b/Makefile @@ -159,6 +159,7 @@ install: precheck install -m 755 drivers/tapdisk-pause $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) install -m 755 drivers/vss_control $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) install -m 755 drivers/intellicache-clean $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) + install -m 755 drivers/enable-borehamwood $(SM_STAGING)$(SM_DEST) install -m 755 drivers/trim $(SM_STAGING)$(PLUGIN_SCRIPT_DEST) ln -sf $(PLUGIN_SCRIPT_DEST)vss_control $(SM_STAGING)$(SM_DEST) install -m 755 drivers/iscsilib.py $(SM_STAGING)$(SM_DEST) diff --git a/drivers/enable-borehamwood b/drivers/enable-borehamwood new file mode 100755 index 00000000..c0014acd --- /dev/null +++ b/drivers/enable-borehamwood @@ -0,0 +1,54 @@ +#!/usr/bin/python +# Copyright (C) 2014 Citrix Ltd. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License as published +# by the Free Software Foundation; version 2.1 only. +# +# 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 Lesser General Public License for more details. +# + +import os, sys +import subprocess + +DIR="/opt/xensource/sm/" +FILENAME="RawHBASR" + +def usage(): + print "Usage: %s enable" % (sys.argv[0]) + +if __name__ == "__main__": + if len(sys.argv) != 2: + usage() + sys.exit(1) + + try: + os.chdir(DIR) + os.symlink(FILENAME + ".py", FILENAME) + except OSError, e: + print "Error: %s [errno=%s]" % (e.args) + sys.exit(1) + + print "%s%s symlink created" % (DIR,FILENAME) + + try: + ret = subprocess.call(["xe-toolstack-restart"]) + except OSError, e: + print "Error: %s [errno=%s]" % (e.args) + ret = 1 # make the following block to cleanup + + if ret != 0: + print "Failed toolstack restart, rolling back symlink creation" + try: + os.unlink(FILENAME) + except OSError, e: + print "Error: %s [errno=%s], fix manually" % (e.args) + sys.exit(1) + else: + print "toolstack restarted" + + sys.exit(0) + diff --git a/mk/sm.spec.in b/mk/sm.spec.in index 2b11f993..e0bfee8a 100755 --- a/mk/sm.spec.in +++ b/mk/sm.spec.in @@ -284,10 +284,11 @@ This package adds a new rawhba SR type. This SR type allows utilization of Fiber Channel raw LUNs as separate VDIs (LUN per VDI) %files rawhba -/opt/xensource/sm/RawHBASR /opt/xensource/sm/RawHBASR.py +%exclude /opt/xensource/sm/RawHBASR /opt/xensource/sm/RawHBASR.pyc /opt/xensource/sm/RawHBASR.pyo /opt/xensource/sm/B_util.py /opt/xensource/sm/B_util.pyc /opt/xensource/sm/B_util.pyo +/opt/xensource/sm/enable-borehamwood From 80bb87bd56402e01f0d0a4f83c7aa80dc30b79c6 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Tue, 25 Nov 2014 14:00:45 +0000 Subject: [PATCH 64/69] CP-5962: don't use errno as a variable name Signed-off-by: Thanos Makatos Suggested-by: Mate Lakat --- drivers/util.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/util.py b/drivers/util.py index 0ecb447c..500ffe3d 100755 --- a/drivers/util.py +++ b/drivers/util.py @@ -287,9 +287,9 @@ def ioretry(f, errlist=[errno.EIO], maxretry=IORETRY_MAX, period=IORETRY_PERIOD, try: return f() except OSError, inst: - errno = int(inst.errno) - if not errno in errlist: - raise CommandException(errno, str(f), "OSError") + err = int(inst.errno) + if not err in errlist: + raise CommandException(err, str(f), "OSError") except CommandException, inst: if not int(inst.code) in errlist: raise From defc948ef1bd8ad7b3bab2730911b8ae1b7e347a Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Tue, 25 Nov 2014 14:38:37 +0000 Subject: [PATCH 65/69] CA-129230: [UnitTest] don't split test description If a test fails only the first line of the test description is printed. Signed-off-by: Thanos Makatos --- tests/test_SRCommand.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/test_SRCommand.py b/tests/test_SRCommand.py index c94bf525..00f7e002 100644 --- a/tests/test_SRCommand.py +++ b/tests/test_SRCommand.py @@ -21,9 +21,7 @@ def test_run_correctly_log_all_exceptions( mock_reduce, mock_SMlog): - """ Assert that any arbitrary exception raised and with a big - message length is logged to SMlog. Only the first line of - the message is asserted (traceback ommited). + """ Assert that any arbitrary exception raised and with a big message length is logged to SMlog. Only the first line of the message is asserted (traceback ommited). """ from random import choice @@ -69,8 +67,7 @@ def test_run_print_xml_error_if_SRException( mock_run_statics, mock_logException): - """ If an SR.SRException is thrown, assert that - "print .toxml()" is called. + """ If an SR.SRException is thrown, assert that print .toxml()" is called. """ import sys @@ -118,8 +115,7 @@ def test_run_reraise_if_not_SRException( mock_run_statics, mock_logException): - """ If an exception other than SR.SRException - is thrown, assert that it is re-raised. + """ If an exception other than SR.SRException is thrown, assert that it is re-raised. """ from DummySR import DRIVER_INFO From 785870ab63f2e35b14b383b1edad182c307736d6 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 25 Nov 2014 09:37:07 -0500 Subject: [PATCH 66/69] CP-5818: [UnitTest] Remove volume activation related unit tests Signed-off-by: Mate Lakat Reviewed-by: Thanos Makatos --- drivers/lvutil.py | 3 +-- tests/test_lvutil.py | 18 ------------------ 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/drivers/lvutil.py b/drivers/lvutil.py index 1f314c0d..32fa02b2 100755 --- a/drivers/lvutil.py +++ b/drivers/lvutil.py @@ -400,8 +400,7 @@ def setActiveVG(path, active): cmd = [CMD_VGCHANGE, "-a" + val, path] text = util.pread2(cmd) -def create(name, size, vgname, tag=None, activate=True, - size_in_percentage=None): +def create(name, size, vgname, tag=None, size_in_percentage=None): if size_in_percentage: cmd = [CMD_LVCREATE, "-n", name, "-l", size_in_percentage, vgname] else: diff --git a/tests/test_lvutil.py b/tests/test_lvutil.py index c96bdd1f..ab3e4898 100644 --- a/tests/test_lvutil.py +++ b/tests/test_lvutil.py @@ -78,24 +78,6 @@ def test_create_creates_logical_volume_with_tags(self, lvsystem): created_lv, = lvsystem.get_logical_volumes_with_name('volume') self.assertEquals('hello', created_lv.tag) - @with_lvm_subsystem - def test_create_creates_logical_volume_inactive(self, lvsystem): - lvsystem.add_volume_group('vgroup') - - lvutil.create('volume', ONE_MEGABYTE, 'vgroup', activate=False) - - created_lv, = lvsystem.get_logical_volumes_with_name('volume') - self.assertFalse(created_lv.active) - - @with_lvm_subsystem - def test_create_inactive_volume_is_not_zeroed(self, lvsystem): - lvsystem.add_volume_group('vgroup') - - lvutil.create('volume', ONE_MEGABYTE, 'vgroup', activate=False) - - created_lv, = lvsystem.get_logical_volumes_with_name('volume') - self.assertFalse(created_lv.zeroed) - @mock.patch('util.pread2') def test_create_percentage_has_precedence_over_size(self, mock_pread2): lvutil.create('volume', ONE_MEGABYTE, 'vgroup', From 006356a558beac0437cbc31f92af3fa30d2ecf45 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Tue, 25 Nov 2014 09:37:42 -0500 Subject: [PATCH 67/69] CA-129230: [UnitTest] Fix unit tests Serialisation of a class name has changed over the python versions, so the test needed to be adjusted. Now only the error message is searched in the log instead of expecting a full match. Signed-off-by: Mate Lakat Reviewed-by: Thanos Makatos --- tests/test_SRCommand.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_SRCommand.py b/tests/test_SRCommand.py index 00f7e002..f0313534 100644 --- a/tests/test_SRCommand.py +++ b/tests/test_SRCommand.py @@ -54,9 +54,7 @@ def MockSMlog(str_arg): # test, as it is re-raised after it is logged. pass - self.assertEqual(self.smlog_out, - ('***** dummy: EXCEPTION test_SRCommand.' - 'SomeException, ' + rand_huge_msg)) + self.assertTrue(rand_huge_msg in self.smlog_out) @mock.patch('util.logException') @mock.patch('SRCommand.SRCommand.run_statics') From cb39f5ebf8c01d9ba4c4c4427c9615876b6fa6f1 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Tue, 25 Nov 2014 14:56:44 +0000 Subject: [PATCH 68/69] CP-8744 & CP-5818: [UnitTest] remove LVM activate argument Suggested-by: Thanos Makatos Signed-off-by: Thanos Makatos --- drivers/trim_util.py | 3 +-- tests/test_trim_util.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/trim_util.py b/drivers/trim_util.py index 847d1a54..1726b5c7 100755 --- a/drivers/trim_util.py +++ b/drivers/trim_util.py @@ -112,8 +112,7 @@ def do_trim(session, args): lvutil.remove(lv_path) # Perform a lvcreate and lvremove to trigger trim on the array - lvutil.create(lv_name, 0, vg_name, activate=True, - size_in_percentage="100%F") + lvutil.create(lv_name, 0, vg_name, size_in_percentage="100%F") lvutil.remove(lv_path, config_param="issue_discards=1") util.SMlog("Trim on SR: %s complete. " % sr_uuid) result = str(True) diff --git a/tests/test_trim_util.py b/tests/test_trim_util.py index bee2bf93..000d6149 100644 --- a/tests/test_trim_util.py +++ b/tests/test_trim_util.py @@ -116,7 +116,7 @@ def test_do_trim_creates_an_lv(self, lvutil.create.assert_called_once_with( 'some-uuid_trim_lv', 0, 'VG_XenStorage-some-uuid', - size_in_percentage='100%F', activate=True + size_in_percentage='100%F' ) @mock.patch('trim_util.lvutil') From b5c7890a60b968bae4450b7f1b20cc4118e4cd32 Mon Sep 17 00:00:00 2001 From: Thanos Makatos Date: Wed, 19 Nov 2014 11:26:41 +0000 Subject: [PATCH 69/69] CA-150952: properly check for XAPI session existence Don't check if a XAPI session exists by comparing session with the empty string because if a XAPI session does exist it returns a error string, which evaluates to true. Signed-off-by: Thanos Makatos (cherry picked from commit 3fd6375cc13cb4106153b15c31c62580fd7dcc89) --- drivers/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/util.py b/drivers/util.py index 500ffe3d..caeca96c 100755 --- a/drivers/util.py +++ b/drivers/util.py @@ -1696,7 +1696,7 @@ def get_pool_restrictions(session): def read_caching_is_restricted(session): """Tells whether read caching is restricted.""" - if session is None or session == "": + if session is None or (isinstance(session, str) and session == ""): return True restrictions = get_pool_restrictions(session) if 'restrict_read_caching' in restrictions and \