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

On Thu, Nov 10, 2011 at 03:22:31PM +0000, 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.


---    2011-11-01 12:35:06.707272165 +0200
+++    2011-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_DISCOVERY = [constants.EXT_ISCSIADM, "-m", "discoverydb", "-t", 
"sendtargets", "-o", "new"]
ISCSIADM_NODE = [constants.EXT_ISCSIADM, "-m", "node"]
ISCSIADM_IFACE = [constants.EXT_ISCSIADM, "-m", "iface"]
@@ -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'
+'Set transport %s to target %s on portal %s' % (transport, iqn, 
+    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

Reply via email to