Re: patch for vdsm to fix discovery and allow iser discovery

[Saggi Mizrahi]

On 11/11/2011 09:21 AM, Roi Dayan wrote:

Forgot to answer everything on prev mail. sorry

Roi

On 10 בנוב 2011, at 22:10, "Dan Kenigsberg" > wrote:



On Thu, Nov 10, 2011 at 03:22:31PM +, Roi Dayan wrote:

Roi, I'm thrilled to see communitee contributions to Vdsm. Thanks. 
However,

please notice Federico's requests.


We use an rpm version: vdsm-4.9-106.el6.x86_64


Nonetheless, please send patches against current master branch. 
Otherwise,
someone that is not as familiar as you with your code would have to 
do the

rebasing, which is not healthy.



First change makes target login to try transport iSER and if fails 
to try transport iSCSI.
Second change is adding '-o new' to the discovery command so already 
discovered targets settings won't be overridden

with default settings.


Would you elaborate on that? I am a bit worried about the changed 
semantics. why

is it related to the iSER patch?



Thanks,
Roi


--- iscsi.py2011-11-01 12:35:06.707272165 +0200
+++ iscsi.py2011-11-09 11:52:01.935067375 +0200
@@ -25,7 +25,7 @@
import storage_exception as se
import devicemapper

-SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", 
"discoverydb", "-t", "sendtargets"]
+SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", 
"discoverydb", "-t", "sendtargets", "-o", "new"]

ISCSIADM_NODE = [constants.EXT_ISCSIADM, "-m", "node"]
ISCSIADM_IFACE = [constants.EXT_ISCSIADM, "-m", "iface"]
ISCSI_DEFAULT_PORT = "3260"
@@ -376,6 +376,28 @@

return rc, out

+def setNodeTransport(portal, iqn, transport='tcp'):
+"""Configure a node transport
+transport :tcp, iser
+"""
+if transport not in ['tcp', 'iser']:
raise error, change from list to tuple, put in constant 
SUPPORTED_ISCSI_TRANSPORT

+transport = 'tcp'
+
+log.info('Set transport %s to target %s on portal %s' % 
(transport, iqn, portal))

this is not info level information, change to debug

+
+cmds = [
+[constants.EXT_ISCSIADM] + '-m node -o update -n 
node.conn[0].iscsi.HeaderDigest -v None'.split(),
+[constants.EXT_ISCSIADM] + ('-m node -o update -n 
iface.transport_name -v '+transport).split()


Why do the splitting on runtime? Better spell out the list in the code.


I'll change that but I'm pretty sure python compiler handles that and 
after pyc is created its the same.

It's doesn't matter what the compiler does or doesn't do.
Sending a naively splitted string to Popen is bad practice and might 
cause bugs down the road.





+]
+
+cmds[0] += ['-p', portal, '-T', iqn]
+cmds[1] += ['-p', portal, '-T', iqn]

use cmds[x].extend() and use tuples instead of lists

+
+for cmd in cmds:
+rc = misc.execCmd(cmd)[0]
+if rc != 0:
+raise se.iSCSILoginError(portal)
+
def remiSCSIPortal(ip, port):
"""
Removes iSCSI portal from discovery list
@@ -467,9 +489,16 @@
raise se.SetiSCSIPasswdError(portal)

# Finally instruct the iscsi initiator to login to the target
+   # try iser first and then tcp
Comment what you mean and not what you do. Especially comment on why you 
use iser before tcp.

+   setNodeTransport(portal, iqn, 'iser')


This looks like a whitespace damage


I'll check that. It's probably from the copy paste since python breaks 
on bad spaces and everything is running fine.





cmd = cmdt + ["-l", "-p", portal]
rc = misc.execCmd(cmd)[0]
if rc != 0:
+# now try iSCSI
+setNodeTransport(portal, iqn, 'tcp')
+cmd = cmdt + ["-l", "-p", portal]
+rc = misc.execCmd(cmd)[0]


Setting the node trasport first, and doing discovery later is race-prone.
Doesn't iscsiadm support expressing the transport on the discovery 
command?
What is the span of setNodeTransport? Would it affect things other 
than login?


From what I've checked iscsiadm doesn't tell you the transport in 
discovery.





+if rc != 0:
raise se.iSCSILoginError(portal)
except se.StorageException:
exc_class, exc, tb = sys.exc_info()



___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-devel


___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-devel

Re: patch for vdsm to fix discovery and allow iser discovery

[Roi Dayan]

Forgot to answer everything on prev mail. sorry

Roi

On 10 בנוב 2011, at 22:10, "Dan Kenigsberg" 
mailto:dan...@redhat.com>> wrote:

On Thu, Nov 10, 2011 at 03:22:31PM +, Roi Dayan wrote:

Roi, I'm thrilled to see communitee contributions to Vdsm. Thanks. However,
please notice Federico's requests.

We use an rpm version: vdsm-4.9-106.el6.x86_64

Nonetheless, please send patches against current master branch. Otherwise,
someone that is not as familiar as you with your code would have to do the
rebasing, which is not healthy.


First change makes target login to try transport iSER and if fails to try 
transport iSCSI.
Second change is adding '-o new' to the discovery command so already discovered 
targets settings won't be overridden
with default settings.

Would you elaborate on that? I am a bit worried about the changed semantics. why
is it related to the iSER patch?


Thanks,
Roi


--- iscsi.py2011-11-01 12:35:06.707272165 +0200
+++ iscsi.py2011-11-09 11:52:01.935067375 +0200
@@ -25,7 +25,7 @@
import storage_exception as se
import devicemapper

-SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", "discoverydb", "-t", 
"sendtargets"]
+SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", "discoverydb", "-t", 
"sendtargets", "-o", "new"]
ISCSIADM_NODE = [constants.EXT_ISCSIADM, "-m", "node"]
ISCSIADM_IFACE = [constants.EXT_ISCSIADM, "-m", "iface"]
ISCSI_DEFAULT_PORT = "3260"
@@ -376,6 +376,28 @@

return rc, out

+def setNodeTransport(portal, iqn, transport='tcp'):
+"""Configure a node transport
+transport :tcp, iser
+"""
+if transport not in ['tcp', 'iser']:
+transport = 'tcp'
+
+log.info('Set transport %s to target %s on portal %s' % (transport, iqn, 
portal))
+
+cmds = [
+[constants.EXT_ISCSIADM] + '-m node -o update -n 
node.conn[0].iscsi.HeaderDigest -v None'.split(),
+[constants.EXT_ISCSIADM] + ('-m node -o update -n iface.transport_name 
-v '+transport).split()

Why do the splitting on runtime? Better spell out the list in the code.

I'll change that but I'm pretty sure python compiler handles that and after pyc 
is created its the same.


+]
+
+cmds[0] += ['-p', portal, '-T', iqn]
+cmds[1] += ['-p', portal, '-T', iqn]
+
+for cmd in cmds:
+rc = misc.execCmd(cmd)[0]
+if rc != 0:
+raise se.iSCSILoginError(portal)
+
def remiSCSIPortal(ip, port):
"""
Removes iSCSI portal from discovery list
@@ -467,9 +489,16 @@
raise se.SetiSCSIPasswdError(portal)

# Finally instruct the iscsi initiator to login to the target
+   # try iser first and then tcp
+   setNodeTransport(portal, iqn, 'iser')

This looks like a whitespace damage

I'll check that. It's probably from the copy paste since python breaks on bad 
spaces and everything is running fine.


cmd = cmdt + ["-l", "-p", portal]
rc = misc.execCmd(cmd)[0]
if rc != 0:
+# now try iSCSI
+setNodeTransport(portal, iqn, 'tcp')
+cmd = cmdt + ["-l", "-p", portal]
+rc = misc.execCmd(cmd)[0]

Setting the node trasport first, and doing discovery later is race-prone.
Doesn't iscsiadm support expressing the transport on the discovery command?
What is the span of setNodeTransport? Would it affect things other than login?

>From what I've checked iscsiadm doesn't tell you the transport in discovery.


+if rc != 0:
raise se.iSCSILoginError(portal)
except se.StorageException:
exc_class, exc, tb = sys.exc_info()
___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-devel

Re: patch for vdsm to fix discovery and allow iser discovery

[Roi Dayan]

Roi

On 10 בנוב 2011, at 22:10, "Dan Kenigsberg" 
mailto:dan...@redhat.com>> wrote:

On Thu, Nov 10, 2011 at 03:22:31PM +, Roi Dayan wrote:

Roi, I'm thrilled to see communitee contributions to Vdsm. Thanks. However,
please notice Federico's requests.

We use an rpm version: vdsm-4.9-106.el6.x86_64

Nonetheless, please send patches against current master branch. Otherwise,
someone that is not as familiar as you with your code would have to do the
rebasing, which is not healthy.

Ok I'll check it and do that.



First change makes target login to try transport iSER and if fails to try 
transport iSCSI.
Second change is adding '-o new' to the discovery command so already discovered 
targets settings won't be overridden
with default settings.

Would you elaborate on that? I am a bit worried about the changed semantics. why
is it related to the iSER patch?

It's related also to iSER since to login to an iSER target we need to update 
iface.transport_name first from tcp to iser.
Now when in RHEV you choose to add another target or does a discovery again 
which changes the value for old connected targets back to tcp.
So iSER targets won't reconnect when get disconnected.



Thanks,
Roi


--- iscsi.py2011-11-01 12:35:06.707272165 +0200
+++ iscsi.py2011-11-09 11:52:01.935067375 +0200
@@ -25,7 +25,7 @@
import storage_exception as se
import devicemapper

-SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", "discoverydb", "-t", 
"sendtargets"]
+SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", "discoverydb", "-t", 
"sendtargets", "-o", "new"]
ISCSIADM_NODE = [constants.EXT_ISCSIADM, "-m", "node"]
ISCSIADM_IFACE = [constants.EXT_ISCSIADM, "-m", "iface"]
ISCSI_DEFAULT_PORT = "3260"
@@ -376,6 +376,28 @@

return rc, out

+def setNodeTransport(portal, iqn, transport='tcp'):
+"""Configure a node transport
+transport :tcp, iser
+"""
+if transport not in ['tcp', 'iser']:
+transport = 'tcp'
+
+log.info('Set transport %s to target %s on portal %s' % (transport, iqn, 
portal))
+
+cmds = [
+[constants.EXT_ISCSIADM] + '-m node -o update -n 
node.conn[0].iscsi.HeaderDigest -v None'.split(),
+[constants.EXT_ISCSIADM] + ('-m node -o update -n iface.transport_name 
-v '+transport).split()

Why do the splitting on runtime? Better spell out the list in the code.

+]
+
+cmds[0] += ['-p', portal, '-T', iqn]
+cmds[1] += ['-p', portal, '-T', iqn]
+
+for cmd in cmds:
+rc = misc.execCmd(cmd)[0]
+if rc != 0:
+raise se.iSCSILoginError(portal)
+
def remiSCSIPortal(ip, port):
"""
Removes iSCSI portal from discovery list
@@ -467,9 +489,16 @@
raise se.SetiSCSIPasswdError(portal)

# Finally instruct the iscsi initiator to login to the target
+   # try iser first and then tcp
+   setNodeTransport(portal, iqn, 'iser')

This looks like a whitespace damage

cmd = cmdt + ["-l", "-p", portal]
rc = misc.execCmd(cmd)[0]
if rc != 0:
+# now try iSCSI
+setNodeTransport(portal, iqn, 'tcp')
+cmd = cmdt + ["-l", "-p", portal]
+rc = misc.execCmd(cmd)[0]

Setting the node trasport first, and doing discovery later is race-prone.
Doesn't iscsiadm support expressing the transport on the discovery command?
What is the span of setNodeTransport? Would it affect things other than login?

+if rc != 0:
raise se.iSCSILoginError(portal)
except se.StorageException:
exc_class, exc, tb = sys.exc_info()
___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-devel

Re: patch for vdsm to fix discovery and allow iser discovery

[Dan Kenigsberg]

On Thu, Nov 10, 2011 at 03:22:31PM +, Roi Dayan wrote:

Roi, I'm thrilled to see communitee contributions to Vdsm. Thanks. However,
please notice Federico's requests.

> We use an rpm version: vdsm-4.9-106.el6.x86_64

Nonetheless, please send patches against current master branch. Otherwise,
someone that is not as familiar as you with your code would have to do the
rebasing, which is not healthy.

> 
> First change makes target login to try transport iSER and if fails to try 
> transport iSCSI.
> Second change is adding '-o new' to the discovery command so already 
> discovered targets settings won't be overridden
> with default settings.

Would you elaborate on that? I am a bit worried about the changed semantics. why
is it related to the iSER patch?

> 
> Thanks,
> Roi
> 
> 
> --- iscsi.py2011-11-01 12:35:06.707272165 +0200
> +++ iscsi.py2011-11-09 11:52:01.935067375 +0200
> @@ -25,7 +25,7 @@
>  import storage_exception as se
>  import devicemapper
> 
> -SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", "discoverydb", "-t", 
> "sendtargets"]
> +SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m", "discoverydb", "-t", 
> "sendtargets", "-o", "new"]
>  ISCSIADM_NODE = [constants.EXT_ISCSIADM, "-m", "node"]
>  ISCSIADM_IFACE = [constants.EXT_ISCSIADM, "-m", "iface"]
>  ISCSI_DEFAULT_PORT = "3260"
> @@ -376,6 +376,28 @@
> 
>  return rc, out
> 
> +def setNodeTransport(portal, iqn, transport='tcp'):
> +"""Configure a node transport
> +transport :tcp, iser
> +"""
> +if transport not in ['tcp', 'iser']:
> +transport = 'tcp'
> +
> +log.info('Set transport %s to target %s on portal %s' % (transport, iqn, 
> portal))
> +
> +cmds = [
> +[constants.EXT_ISCSIADM] + '-m node -o update -n 
> node.conn[0].iscsi.HeaderDigest -v None'.split(),
> +[constants.EXT_ISCSIADM] + ('-m node -o update -n 
> iface.transport_name -v '+transport).split()

Why do the splitting on runtime? Better spell out the list in the code.

> +]
> +
> +cmds[0] += ['-p', portal, '-T', iqn]
> +cmds[1] += ['-p', portal, '-T', iqn]
> +
> +for cmd in cmds:
> +rc = misc.execCmd(cmd)[0]
> +if rc != 0:
> +raise se.iSCSILoginError(portal)
> +
>  def remiSCSIPortal(ip, port):
>  """
>  Removes iSCSI portal from discovery list
> @@ -467,9 +489,16 @@
>  raise se.SetiSCSIPasswdError(portal)
> 
>  # Finally instruct the iscsi initiator to login to the target
> +   # try iser first and then tcp
> +   setNodeTransport(portal, iqn, 'iser')

This looks like a whitespace damage

>  cmd = cmdt + ["-l", "-p", portal]
>  rc = misc.execCmd(cmd)[0]
>  if rc != 0:
> +# now try iSCSI
> +setNodeTransport(portal, iqn, 'tcp')
> +cmd = cmdt + ["-l", "-p", portal]
> +rc = misc.execCmd(cmd)[0]

Setting the node trasport first, and doing discovery later is race-prone.
Doesn't iscsiadm support expressing the transport on the discovery command?
What is the span of setNodeTransport? Would it affect things other than login?

> +if rc != 0:
>  raise se.iSCSILoginError(portal)
>  except se.StorageException:
>  exc_class, exc, tb = sys.exc_info()
___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-devel

Re: patch for vdsm to fix discovery and allow iser discovery

[Andrew Cathrow]

- Original Message -
> From: "Roi Dayan" 
> To: vdsm-devel@lists.fedorahosted.org
> Sent: Thursday, November 10, 2011 10:22:31 AM
> Subject: patch for vdsm to fix discovery and allow iser discovery
> 
> We use an rpm version: vdsm-4.9-106.el6.x86_64
> 
> First change makes target login to try transport iSER and if fails to
> try transport iSCSI.
> Second change is adding '-o new' to the discovery command so already
> discovered targets settings won't be overridden
> with default settings.
> 

So on a non-RDMA iSCSI host what will happen will it fail and then fall back to 
tcp?

If so I know that will work but since most iSCSI customers will be using TCP do 
we really want the majority of connections to fail?

A long term option would be specifying the connection option in the call to 
VDSM, but that obviously needs support in the RHEV backend as well.

Maybe as a short term solution we should have the list of valid transports 
specified in vdsm.conf, that way we could add iser to the configuration if it's 
required.

> Thanks,
> Roi
> 
> 
> --- iscsi.py2011-11-01 12:35:06.707272165 +0200
> +++ iscsi.py2011-11-09 11:52:01.935067375 +0200
> @@ -25,7 +25,7 @@
>  import storage_exception as se
>  import devicemapper
> 
> -SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m",
> "discoverydb", "-t", "sendtargets"]
> +SENDTARGETS_DISCOVERY = [constants.EXT_ISCSIADM, "-m",
> "discoverydb", "-t", "sendtargets", "-o", "new"]
>  ISCSIADM_NODE = [constants.EXT_ISCSIADM, "-m", "node"]
>  ISCSIADM_IFACE = [constants.EXT_ISCSIADM, "-m", "iface"]
>  ISCSI_DEFAULT_PORT = "3260"
> @@ -376,6 +376,28 @@
> 
>  return rc, out
> 
> +def setNodeTransport(portal, iqn, transport='tcp'):
> +"""Configure a node transport
> +transport :tcp, iser
> +"""
> +if transport not in ['tcp', 'iser']:
> +transport = 'tcp'
> +
> +log.info('Set transport %s to target %s on portal %s' %
> (transport, iqn, portal))
> +
> +cmds = [
> +[constants.EXT_ISCSIADM] + '-m node -o update -n
> node.conn[0].iscsi.HeaderDigest -v None'.split(),
> +[constants.EXT_ISCSIADM] + ('-m node -o update -n
> iface.transport_name -v '+transport).split()
> +]
> +
> +cmds[0] += ['-p', portal, '-T', iqn]
> +cmds[1] += ['-p', portal, '-T', iqn]
> +
> +for cmd in cmds:
> +rc = misc.execCmd(cmd)[0]
> +if rc != 0:
> +raise se.iSCSILoginError(portal)
> +
>  def remiSCSIPortal(ip, port):
>  """
>  Removes iSCSI portal from discovery list
> @@ -467,9 +489,16 @@
>  raise se.SetiSCSIPasswdError(portal)
> 
>  # Finally instruct the iscsi initiator to login to the
>  target
> +   # try iser first and then tcp
> +   setNodeTransport(portal, iqn, 'iser')
>  cmd = cmdt + ["-l", "-p", portal]
>  rc = misc.execCmd(cmd)[0]
>  if rc != 0:
> +# now try iSCSI
> +setNodeTransport(portal, iqn, 'tcp')
> +cmd = cmdt + ["-l", "-p", portal]
> +rc = misc.execCmd(cmd)[0]
> +if rc != 0:
>  raise se.iSCSILoginError(portal)
>  except se.StorageException:
>  exc_class, exc, tb = sys.exc_info()
> 
> 
> ___
> vdsm-devel mailing list
> vdsm-devel@lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/vdsm-devel
> 
___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-devel

Re: patch for vdsm to fix discovery and allow iser discovery

[Federico Simoncelli]

Hi Roi,
thanks for your contribution!

It would be better if you could direct your patches to vdsm-patches instead
of vdsm-devel.

If you could send the patch in the format generated by "git format-patch" [1]
it will be easier for us to review and include your patch, moreover it will
automatically carry the correct author (you).

[1] see also "git send-email"

-- 
Federico

- Original Message -
> From: "Roi Dayan" 
> To: vdsm-devel@lists.fedorahosted.org
> Sent: Thursday, November 10, 2011 4:22:31 PM
> Subject: patch for vdsm to fix discovery and allow iser discovery
> 
> We use an rpm version: vdsm-4.9-106.el6.x86_64
> 
> First change makes target login to try transport iSER and if fails to
> try transport iSCSI.
> Second change is adding '-o new' to the discovery command so already
> discovered targets settings won't be overridden
> with default settings.
> 
> Thanks,
> Roi
___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-devel