2:30 p.m.
Dominik Holler has uploaded a new change for review.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
storage: prepare iscsi for IPv6 targets
The used iscsiadm tool expects IPv6 addresses in square brackets if a
port is appended.
For this reason this patch encapsulates hostnames given as IPv6
addresses in square brackets.
Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Bug-Url: https://bugzilla.redhat.com/1308306
Signed-off-by: Dominik Holler <dholler(a)redhat.com>
---
M vdsm/storage/iscsi.py
1 file changed, 29 insertions(+), 7 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/96/65696/5
diff --git a/vdsm/storage/iscsi.py b/vdsm/storage/iscsi.py
index 4df844c..72320ff 100644
--- a/vdsm/storage/iscsi.py
+++ b/vdsm/storage/iscsi.py
@@ -35,6 +35,8 @@
from vdsm import supervdsm
from vdsm.config import config
from vdsm.network.netinfo.routes import getRouteDeviceTo
+from vdsm.network.errors import ConfigNetworkError
+from vdsm.network.ip.address import IPv6
from vdsm.password import ProtectedPassword
from vdsm.storage import devicemapper
from vdsm.storage import misc
@@ -53,6 +55,28 @@
_iscsiadmTransactionLock = RLock()
log = logging.getLogger('storage.ISCSI')
+
+def getTargetString(portal, tpgt=None, iqn=None):
+ hostname = portal.hostname
+ try:
+ IPv6.validateAddress(hostname)
+ hostname = "[%s]" % hostname
+ except ConfigNetworkError:
+ pass
+
+ portalStr = "%s:%d" % (hostname, portal.port)
+
+ if tpgt is None:
+ tpgtStr = ""
+ else:
+ tpgtStr = ",%d" % tpgt
+
+ if iqn is None:
+ iqnStr= ""
+ else:
+ iqnStr = " %s" % iqn
+
+ return "%s%s%s" % (portalStr, tpgtStr, iqnStr)
def getDevIscsiSessionId(dev):
@@ -189,8 +213,7 @@
# bounded iface. Explicitly specifying tpgt on iSCSI login imposes creation
# of the node record in the new style format which enables to access a
# portal through multiple ifaces for multipathing.
- portalStr = "%s:%d,%d" % (target.portal.hostname, target.portal.port,
- target.tpgt)
+ portalStr = getTargetString(target.portal, target.tpgt)
with _iscsiadmTransactionLock:
iscsiadm.node_new(iface.name, portalStr, target.iqn)
try:
@@ -216,8 +239,7 @@
# Basically this command deleting a node record (see addIscsiNode).
# Once we create a record in the new style format by specifying a tpgt,
# we delete it in the same way.
- portalStr = "%s:%d,%d" % (target.portal.hostname, target.portal.port,
- target.tpgt)
+ portalStr = getTargetString(target.portal, target.tpgt)
with _iscsiadmTransactionLock:
try:
iscsiadm.node_disconnect(iface.name, portalStr, target.iqn)
@@ -230,7 +252,7 @@
def addIscsiPortal(iface, portal, credentials=None):
discoverType = "sendtargets"
- portalStr = "%s:%d" % (portal.hostname, portal.port)
+ portalStr = getTargetString(portal)
with _iscsiadmTransactionLock:
iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
@@ -251,7 +273,7 @@
def deleteIscsiPortal(iface, portal):
discoverType = "sendtargets"
- portalStr = "%s:%d" % (portal.hostname, portal.port)
+ portalStr = getTargetString(portal)
iscsiadm.discoverydb_delete(discoverType, iface.name, portalStr)
@@ -259,7 +281,7 @@
# Because proper discovery actually has to clear the DB having multiple
# discoveries at once will cause unpredictable results
discoverType = "sendtargets"
- portalStr = "%s:%d" % (portal.hostname, portal.port)
+ portalStr = getTargetString(portal)
with _iscsiadmTransactionLock:
addIscsiPortal(iface, portal, credentials)
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
2:30 p.m.
Dominik Holler has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 5: Verified+1
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-HasComments: No
2:46 p.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 6:
* Update Tracker::#1308306::OK, status: NEW
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
12:52 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 6:
(2 comments)
https://gerrit.ovirt.org/#/c/65696/6/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
Line 35: from vdsm import supervdsm
Line 36: from vdsm.config import config
Line 37: from vdsm.network.netinfo.routes import getRouteDeviceTo
Line 38: from vdsm.network.errors import ConfigNetworkError
Line 39: from vdsm.network.ip.address import IPv6
we try hard to avoid interdependence between storage and network subtrees. You should
either move the imported function to lib/vdsm/common, or -- since what you need is so
simple -- redefine what you need right here.
Line 40: from vdsm.password import ProtectedPassword
Line 41: from vdsm.storage import devicemapper
Line 42: from vdsm.storage import misc
Line 43:
Line 56:
Line 57: log = logging.getLogger('storage.ISCSI')
Line 58:
Line 59:
Line 60: def getTargetString(portal, tpgt=None, iqn=None):
We need to improve the code coverage of tests. Please introduce a unit test for new
functions.
Line 61: hostname = portal.hostname
Line 62: try:
Line 63: IPv6.validateAddress(hostname)
Line 64: hostname = "[%s]" % hostname
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
1:17 a.m.
Edward Haas has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 6: Code-Review-1
(2 comments)
https://gerrit.ovirt.org/#/c/65696/6/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
Line 56:
Line 57: log = logging.getLogger('storage.ISCSI')
Line 58:
Line 59:
Line 60: def getTargetString(portal, tpgt=None, iqn=None):
We need to improve the code coverage of tests. Please introduce a
unit test
I think you can use or extend hosttail_join for this purpose.
Line 61: hostname = portal.hostname
Line 62: try:
Line 63: IPv6.validateAddress(hostname)
Line 64: hostname = "[%s]" % hostname
Line 59:
Line 60: def getTargetString(portal, tpgt=None, iqn=None):
Line 61: hostname = portal.hostname
Line 62: try:
Line 63: IPv6.validateAddress(hostname)
Using an exception for branching is not recommended.
A soft check for the address can be used for the branching, then there is an option to
validate it against IPv4/IPv6, but I do not think that is needed, it can explode on
usage.
Line 64: hostname = "[%s]" % hostname
Line 65: except ConfigNetworkError:
Line 66: pass
Line 67:
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
2:48 a.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 7:
* Update Tracker::#1308306::OK, status: NEW
* Check Bug-Url::OK
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
2:51 a.m.
Yaniv Kaul has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/65696/7/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
Line 57:
Line 58: def getTargetString(portal, tpgt=None, iqn=None):
Line 59: hostname = portal.hostname
Line 60: # check for IPv6 address
Line 61: if hostname.count(':') > 2:
What about ::1.2.3.4 - that's an IPv6 address with only 2 ':' chars.
Line 62: hostname = "[%s]" % hostname
Line 63:
Line 64: portalStr = "%s:%d" % (hostname, portal.port)
Line 65:
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
2:54 a.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 8:
* Update Tracker::#1308306::OK, status: NEW
* Check Bug-Url::OK
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
2:54 a.m.
Dominik Holler has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/65696/7/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
Line 57:
Line 58: def getTargetString(portal, tpgt=None, iqn=None):
Line 59: hostname = portal.hostname
Line 60: # check for IPv6 address
Line 61: if hostname.count(':') > 2:
What about ::1.2.3.4 - that's an IPv6 address with only 2
':' chars.
Thanks!
Line 62: hostname = "[%s]" % hostname
Line 63:
Line 64: portalStr = "%s:%d" % (hostname, portal.port)
Line 65:
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
4:41 a.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 9:
* Update Tracker::#1308306::OK, status: NEW
* Check Bug-Url::OK
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
5:23 a.m.
Dominik Holler has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 9: Verified+1
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7:28 a.m.
Dominik Holler has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 9: -Verified
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
9:35 a.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 10:
* Update Tracker::#1308306::OK, status: NEW
* Check Bug-Url::OK
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
10:29 a.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 11:
* Update Tracker::#1308306::OK, status: NEW
* Check Bug-Url::OK
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
3:46 p.m.
Edward Haas has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 11:
(3 comments)
https://gerrit.ovirt.org/#/c/65696/11/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
PS11, Line 59: getTargetString
How about 'portal_join'?
On another thought, are you sure this func is needed?
Looks like the func is trying to find a common usage for creating a portal, but it is not
really common when you need to use so many branches inside.
I think you can drop it and use hosttail_join at the callers.
PS11, Line 60: if tpgt is None:
: tpgtStr = ""
: else:
: tpgtStr = ",%d" % tpgt
Python using the Hungarian notation is a bit too much for me :)
anyway, this may look a bit nicer:
tag = ",%d" % tpgt if tpgt else ""
Same for iqn
PS11, Line 71: str(portal.port)),
This one does not fit the previous line?
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
2 a.m.
Dominik Holler has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 11:
(3 comments)
https://gerrit.ovirt.org/#/c/65696/11/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
PS11, Line 59: getTargetString
How about 'portal_join'?
How about 'portal_join'?
- I try to respect the coding conventions currently used.
This file does not use the underscore_case convention, but a getCamleCase-like
convention.
- The 'portal' is just the hostname:port part of the String, without tpgt and
iqn.
On another thought, are you sure this func is needed?
Your are right. Since the function is very small, it's functionality could be easily
inlined in the callers. But from my point of view it is more maintainable to have the
creation of the portal and target string in centralized in just one place. For example if
the namedtuple IscsiPortal is changed in future, the creation of the portal string, which
may be part of the target string, has to be updated only here in this function.
Looks like the func is trying to find a common usage for
creating a portal, but it is not really common when you need
to use so many branches inside.
You are right, it is possible to separate a dedicated getPortalString(portal) function,
but this would not make the getTargetString() function significant simpler.
I think you can drop it and use hosttail_join at the callers.
If you confirm, that this is more easy to maintain from your point of view, I will do
this.
PS11, Line 60: if tpgt is None:
: tpgtStr = ""
: else:
: tpgtStr = ",%d" % tpgt
Python using the Hungarian notation is a bit too much for me :)
Good idea!
PS11, Line 71: str(portal.port)),
This one does not fit the previous line?
Why not? hosttail_join
wants the port number as a string, for this reason the caller has to convert the port
number to a string.
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
2:20 a.m.
Edward Haas has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 11:
(2 comments)
https://gerrit.ovirt.org/#/c/65696/11/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
PS11, Line 59: getTargetString
> How about 'portal_join'?
The return value is
assigned to 'portalStr', that is why I proposed portal. I guess target_join or
targetJoin is also fine.
I do not know what convention storage is using, or what they prefer to have, leaving that
to this code maintainers.
PS11, Line 71: str(portal.port)),
Why not? hosttail_join wants the port number as a string, for this
reason t
I just asked if the whole hosttail_join call can be on the same line.
return "%s%s%s" % (hosttail_join(portal.hostname, str(portal.port)),
tpgtStr,
iqnStr)
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
2:41 a.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 12:
* Update Tracker::#1308306::OK, status: NEW
* Check Bug-Url::OK
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
5:14 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 12:
(1 comment)
https://gerrit.ovirt.org/#/c/65696/12/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
PS12, Line 63: str(
str() is redundant here, as
'%s' % bla
calls it anyway.
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
5:18 a.m.
Dominik Holler has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 12:
(1 comment)
https://gerrit.ovirt.org/#/c/65696/12/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
PS12, Line 63: str(
str() is redundant here, as
This is the second parameter for
hosttail_join, which needs strings as parameters(, since it works with string
concatenation internally)
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
8:35 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 12:
(1 comment)
https://gerrit.ovirt.org/#/c/65696/12/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
PS12, Line 63: str(
This is the second parameter for hosttail_join, which needs strings
as para
oh, I did not count my parentheses properly
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
10:59 a.m.
Edward Haas has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 12: Code-Review+1
I suggest changing the 'getTargetString' func to something nicer, but I leave this
to your consideration.
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
3:45 a.m.
Nir Soffer has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 12:
(1 comment)
Partial review.
https://gerrit.ovirt.org/#/c/65696/12/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
Line 198: # where the portal path is a directory containing a record file for each
Line 199: # bounded iface. Explicitly specifying tpgt on iSCSI login imposes creation
Line 200: # of the node record in the new style format which enables to access a
Line 201: # portal through multiple ifaces for multipathing.
Line 202: portalStr = getTargetString(target.portal, target.tpgt)
getTargetString looks like getPortalString, at least this is how we call that string in
this module.
If targetString is more correct, please update the module to use this term, if not, use
getPortalString.
Line 203: with _iscsiadmTransactionLock:
Line 204: iscsiadm.node_new(iface.name, portalStr, target.iqn)
Line 205: try:
Line 206: if credentials is not None:
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
11:56 a.m.
gerrit-hooks has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 13:
* Update Tracker::#1308306::OK, status: POST
* Check Bug-Url::OK
* Check Public Bug::#1308306::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
3:40 p.m.
New subject: Change in vdsm[master]: storage: Centralize serialization of iSCSI addresses
From Dan Kenigsberg <danken(a)redhat.com>:
Dan Kenigsberg has posted comments on this change.
Change subject: storage: Centralize serialization of iSCSI addresses
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/65696
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
2761
days inactive
2770
days old
vdsm-patches@lists.fedorahosted.org
24 comments
7 participants
participants (7)
-
automation＠ovirt.org -
Code Review -
Dan Kenigsberg -
dholler＠redhat.com -
edwardh＠redhat.com -
Nir Soffer -
ykaul＠redhat.com