Change in vdsm[master]: tests: new functional tests for vdsm storage

nsoffer at redhat.com nsoffer at redhat.com
Mon Sep 8 19:33:26 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: tests: new functional tests for vdsm storage
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.ovirt.org/#/c/32496/4/tests/functional/testlib/storagebackends/iscsi.py
File tests/functional/testlib/storagebackends/iscsi.py:

Line 63:         self._testDirectory = tempfile.mkdtemp()
Line 64:         self._storageFile = os.path.join(self._testDirectory, 'testfile')
Line 65:         self._fileioBackstore = self.randomName('backfile')
Line 66:         logging.info('using %s, %s' % (self._fileioBackstore, self._storageFile))
Line 67:         self._targetcli('/backstores/fileio create %s %s 10G' % (self._fileioBackstore, self._storageFile))
We should separate the server driver from the backend, so it will be easier to create the version using tgtd, that we need for testing on el6.
Line 68:         self._targetcli('/iscsi create %s' % self._iqn)
Line 69:         self._targetcli('/iscsi/%s/tpg1/luns create /backstores/fileio/%s' % (self._iqn, self._fileioBackstore))
Line 70:         self._targetcli('/iscsi/%s/tpg1 set attribute authentication=0 demo_mode_write_protect=0' % self._iqn)
Line 71:         self._targetcli('/iscsi/%s/tpg1 set attribute generate_node_acls=1 cache_dynamic_acls=1' % self._iqn)


Line 65:         self._fileioBackstore = self.randomName('backfile')
Line 66:         logging.info('using %s, %s' % (self._fileioBackstore, self._storageFile))
Line 67:         self._targetcli('/backstores/fileio create %s %s 10G' % (self._fileioBackstore, self._storageFile))
Line 68:         self._targetcli('/iscsi create %s' % self._iqn)
Line 69:         self._targetcli('/iscsi/%s/tpg1/luns create /backstores/fileio/%s' % (self._iqn, self._fileioBackstore))
This  is unreadable, specially in gerrit.

This should be:

    self.driver.create_lun(self._iqn, self.tpg, self._disk)

The targetcli driver should make a command line from the arguments and run it:

    def create_lun(self, iqn, tgp, disk):
        path = '/iscsi/%s/%s' % (iqn, tpg)
        backstore = 'backstores/fileio/%s' % disk
        self._run(path, 'create', backstore)

Using strings does not make code more readable. Annoying lists are actually better for readability when command lines becomes long.
Line 70:         self._targetcli('/iscsi/%s/tpg1 set attribute authentication=0 demo_mode_write_protect=0' % self._iqn)
Line 71:         self._targetcli('/iscsi/%s/tpg1 set attribute generate_node_acls=1 cache_dynamic_acls=1' % self._iqn)
Line 72:         return self, Verify(self._iqn, self._volumeGroup, self.vdsm, self._volumeID)
Line 73: 


-- 
To view, visit http://gerrit.ovirt.org/32496
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1703e7c1dc223ff707775865cd14c7dd62314caf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list