Change in vdsm[master]: Configure iSCSI iface.net_ifacename

danken at redhat.com danken at redhat.com
Wed Sep 17 22:01:55 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: Configure iSCSI iface.net_ifacename
......................................................................


Patch Set 13: Code-Review-1

(3 comments)

http://gerrit.ovirt.org/#/c/31534/13/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 149: 
Line 150: def _resolveIscsiIface(ifaceName, initiatorName, netIfacename):
Line 151:     if not ifaceName:
Line 152:         iface = iscsi.IscsiInterface('default')
Line 153:         iface.netIfacename = netIfacename
in iscsi.py you modified the initiator to  accept netIfacename. so why don't you use it here?
Line 154:         return iface
Line 155: 
Line 156:     for iface in iscsi.iterateIscsiInterfaces():
Line 157:         if iface.name == ifaceName:


Line 156:     for iface in iscsi.iterateIscsiInterfaces():
Line 157:         if iface.name == ifaceName:
Line 158:             iface.netIfacename = netIfacename
Line 159:             if netIfacename:
Line 160:                 iface.update()
this is a no-op, since nothing has change iface._conf.

the bigger problem is the *wish* to update - I am not at all sure that _resolveIscsiIface() should ever modify and existing iscsi interface.

iterateIscsiInterfaces() should yield existing interfaces with their complete info - can net_ifacename be extracted from iscsiadm?
Line 161:             return iface
Line 162: 
Line 163:     iface = iscsi.IscsiInterface(ifaceName, initiatorName=initiatorName,
Line 164:                                  netIfacename=netIfacename)


http://gerrit.ovirt.org/#/c/31534/13/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:

Line 345:     def __init__(self, name, hardwareAddress=None, ipAddress=None,
Line 346:                  initiatorName=None, netIfacename=None):
Line 347:         self._conf = {}
Line 348:         # Only new tcp interfaces are supported for now
Line 349:         self._conf['iface.transport_name'] = 'tcp'
there is no functional change in these 3 lines - no need to touch them in this patch. it only confuses the reviewers.
Line 350: 
Line 351:         self._loaded = False
Line 352: 
Line 353:         self.name = name


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3ebb2f272669b753700b57486d869b21dd16f2d6
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv 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