Change in vdsm[master]: Related to BZ#854151 - Fix disconnect iscsi connections.

Eduardo has uploaded a new change for review.

Change subject: Related to BZ#854151 - Fix disconnect iscsi connections.
......................................................................

Related to BZ#854151 - Fix disconnect iscsi connections.

Change-Id: Ic390cf1a63594a87ee03c010df31f68a432ebff4
Bug-Id: https://bugzilla.redhat.com/show_bug.cgi?id=854151
Signed-off-by: Eduardo <ewarszaw(a)redhat.com&gt;
---
M vdsm/storage/hsm.py
M vdsm/storage/storageServer.py
2 files changed, 26 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/05/8305/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index a89274a..330b7c0 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -2032,10 +2032,11 @@
             conObj = storageServer.ConnectionFactory.createConnection(conInfo)
             try:
                 conObj.disconnect()
-                status = 0
             except Exception as err:
                 self.log.error("Could not disconnect from storageServer",
exc_info=True)
                 status, _ = self._translateConnectionError(err)
+            else:
+                status = 0
 
             res.append({'id': conDef["id"], 'status': status})
 
diff --git a/vdsm/storage/storageServer.py b/vdsm/storage/storageServer.py
index 269b8eb..963a019 100644
--- a/vdsm/storage/storageServer.py
+++ b/vdsm/storage/storageServer.py
@@ -282,6 +282,7 @@
         return hash(type(self)) ^ hash(self._mountCon)
 
 class IscsiConnection(object):
+    log = logging.getLogger("StorageServer.IscsiConnection")
     @property
     def target(self):
         return self._target
@@ -314,43 +315,48 @@
             host = self._target.portal.hostname
             try:
                 ip = socket.gethostbyname(host)
+            except socket.gaierror:
+                return False
+            else:
                 if ip != portal.hostname:
                     return False
 
-            except socket.gaierror:
-                return False
-
-        if self._target.portal.port != portal.port:
+        elif self._target.portal.port != portal.port:
             return False
 
-        if self._target.tpgt != None and self._target.tpgt != target.tpgt:
+        elif self._target.tpgt != None and self._target.tpgt != target.tpgt:
             return False
 
-        if self._target.iqn != target.iqn:
+        elif self._target.iqn != target.iqn:
             return False
 
-        if self._iface.name != iface.name:
+        elif self._iface.name != iface.name:
             return False
 
-        if self._cred != cred:
+        elif self._cred != cred:
             return False
 
-        return True
+        else:
+            return True
 
     def getSessionInfo(self):
         sessions = iscsi.iterateIscsiSessions()
         try:
             info = iscsi.getSessionInfo(self._lastSessionId)
-            sessions = chain(info, sessions)
         except Exception:
-            pass
+            self.log.warning("Can't get session info, self._lastSessionId
%s", self._lastSessionId)
+        else:
+            sessions = chain(info, sessions)
 
-        for session in iscsi.iterateIscsiSessions():
+        self.log.debug("self._target: %s", self._target)
+        for session in sessions:
+            self.log.debug("session %s", session)
             if self.isSession(session):
                 self._lastSessionId = session.id
+                self.log.debug("self._lastSessionId: %s", self._lastSessionId)
                 return session
-
-        raise OSError(errno.ENOENT, "Session not found")
+        else:
+            raise OSError(errno.ENOENT, "Session not found in sessions: %s",
sessions)
 
     def isConnected(self):
         try:
@@ -362,13 +368,17 @@
             raise
 
     def disconnect(self):
+        self.log.debug("Iscsi disconnect called.")
         try:
             sid = self.getSessionInfo().id
         except OSError, e:
             if e.errno == errno.ENOENT:
+                self.log.warning("Can't get iscsi session id, ENOENT: %s",
e.errno)
                 return
+            self.log.warning("Cant get iscsi session id, errno: %s", e.errno)
             raise
 
+        self.log.debug("Disconnecting iscsi session id: %s", sid)
         iscsi.disconnectiScsiSession(sid)
 
     def __eq__(self, other):


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic390cf1a63594a87ee03c010df31f68a432ebff4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;

Itamar Heim has abandoned this change.

Change subject: Related to BZ#854151 - Fix disconnect iscsi connections.
......................................................................


Abandoned

no reply - abandoning - please restore if still relevant

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ic390cf1a63594a87ee03c010df31f68a432ebff4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)gmail.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Better Saggi <bettersaggi(a)gmail.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)gmail.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Related to BZ#854151 - Fix disconnect iscsi connections.
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 343:         sessions = iscsi.iterateIscsiSessions()
Line 344:         try:
Line 345:             info = iscsi.getSessionInfo(self._lastSessionId)
Line 346:         except Exception:
Line 347:             self.log.warning("Can't get session info,
self._lastSessionId %s", self._lastSessionId)
Can you use _lastSessionId without 'self.'  in the message?
or keep  it as debug
Line 348:         else:
Line 349:             sessions = chain(info, sessions)
Line 350: 
Line 351:         self.log.debug("self._target: %s", self._target)


Line 372:         try:
Line 373:             sid = self.getSessionInfo().id
Line 374:         except OSError, e:
Line 375:             if e.errno == errno.ENOENT:
Line 376:                 self.log.warning("Can't get iscsi session id, ENOENT:
%s", e.errno)
why %s it always  ENOENT here?
Line 377:                 return
Line 378:             self.log.warning("Cant get iscsi session id, errno: %s",
e.errno)
Line 379:             raise
Line 380: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic390cf1a63594a87ee03c010df31f68a432ebff4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;

Itamar Heim has posted comments on this change.

Change subject: Related to BZ#854151 - Fix disconnect iscsi connections.
......................................................................


Patch Set 1:

ping

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic390cf1a63594a87ee03c010df31f68a432ebff4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Better Saggi <bettersaggi(a)gmail.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-HasComments: No

Eduardo has posted comments on this change.

Change subject: Related to BZ#854151 - Fix disconnect iscsi connections.
......................................................................


Patch Set 1: (3 inline comments)

The fix was already split and merge a long time ago.
This is a related patch that improves debugging, enhance the code and does not introduce
regressions.

....................................................
File vdsm/storage/hsm.py
Line 2030:         for conDef in conList:
Line 2031:             conInfo = _connectionDict2ConnectionInfo(domType, conDef)
Line 2032:             conObj = storageServer.ConnectionFactory.createConnection(conInfo)
Line 2033:             try:
Line 2034:                 conObj.disconnect()
This make this patch more disruptive. We can do this in another patch. In addition, you
are already complaining about a too large patch.
Line 2035:             except Exception as err:
Line 2036:                 self.log.error("Could not disconnect from
storageServer", exc_info=True)
Line 2037:                 status, _ = self._translateConnectionError(err)
Line 2038:             else:


....................................................
File vdsm/storage/storageServer.py
Line 343:         sessions = iscsi.iterateIscsiSessions()
Line 344:         try:
Line 345:             info = iscsi.getSessionInfo(self._lastSessionId)
Line 346:         except Exception:
Line 347:             self.log.warning("Can't get session info,
self._lastSessionId %s", self._lastSessionId)
Why debug? Expected is that we are able to get the session info. This is irregular, in
spite that we don't raise.
Line 348:         else:
Line 349:             sessions = chain(info, sessions)
Line 350: 
Line 351:         self.log.debug("self._target: %s", self._target)


Line 372:         try:
Line 373:             sid = self.getSessionInfo().id
Line 374:         except OSError, e:
Line 375:             if e.errno == errno.ENOENT:
Line 376:                 self.log.warning("Can't get iscsi session id, ENOENT:
%s", e.errno)
Wants the number to be printed.
Line 377:                 return
Line 378:             self.log.warning("Cant get iscsi session id, errno: %s",
e.errno)
Line 379:             raise
Line 380: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic390cf1a63594a87ee03c010df31f68a432ebff4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;

Ayal Baron has posted comments on this change.

Change subject: Related to BZ#854151 - Fix disconnect iscsi connections.
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(3 inline comments)

It's impossible to understand what was actually fixed here.
Need to split this into general cleanup and actual fix.
Also, Igor's comments are correct but have gone unanswered for months...

....................................................
File vdsm/storage/hsm.py
Line 2030:         for conDef in conList:
Line 2031:             conInfo = _connectionDict2ConnectionInfo(domType, conDef)
Line 2032:             conObj = storageServer.ConnectionFactory.createConnection(conInfo)
Line 2033:             try:
Line 2034:                 conObj.disconnect()
else is redundant here.  If anything, instead of setting: status = 0
just change disconnect to return 0 (or True and return result of == True) in case of
success so you'd have:
status = conObj.disconnect()
Line 2035:             except Exception as err:
Line 2036:                 self.log.error("Could not disconnect from
storageServer", exc_info=True)
Line 2037:                 status, _ = self._translateConnectionError(err)
Line 2038:             else:


....................................................
File vdsm/storage/storageServer.py
Line 335: 
Line 336:         elif self._cred != cred:
Line 337:             return False
Line 338: 
Line 339:         else:
all of these changes are noise and have nothing to do with the real problem. please split
into 2 patches.
Line 340:             return True
Line 341: 
Line 342:     def getSessionInfo(self):
Line 343:         sessions = iscsi.iterateIscsiSessions()


Line 372:         try:
Line 373:             sid = self.getSessionInfo().id
Line 374:         except OSError, e:
Line 375:             if e.errno == errno.ENOENT:
Line 376:                 self.log.warning("Can't get iscsi session id, ENOENT:
%s", e.errno)
also, error should be "cannot disconnect session, session %s not found", sid
Line 377:                 return
Line 378:             self.log.warning("Cant get iscsi session id, errno: %s",
e.errno)
Line 379:             raise
Line 380: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic390cf1a63594a87ee03c010df31f68a432ebff4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;