Change in vdsm[master]: Automatic reconnection to the Storage Pool

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: Automatic reconnection to the Storage Pool
......................................................................

Automatic reconnection to the Storage Pool

Don't check if the pool directory is present to reconnect to the pool
(_connectStoragePool), in fact the pool directory was removed few lines
before by __cleanStorageRepository.

Signed-off-by: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
---
M vdsm/storage/hsm.py
1 file changed, 4 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/16/4516/1
--
To view, visit http://gerrit.ovirt.org/4516
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;

[Federico Simoncelli]

Federico Simoncelli has abandoned this change.

Change subject: Automatic reconnection to the Storage Pool
......................................................................


Patch Set 2: Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Ayal Baron has posted comments on this change.

Change subject: Automatic reconnection to the Storage Pool
......................................................................


Patch Set 2: Do not submit

(5 inline comments)

After considering this some more, I think the solution should just be to get rid of the
entire reconnect code (which is currently dead anyway and has been for quite a while so no
real backwards compatibility issues here).

Going forward we'll get rid of the master domain concept and then we won't have
all this complexity and reconnecting to domains will be simple.

....................................................
File vdsm/storage/hsm.py
Line 319:             dirList = os.listdir(_poolsTmpDir)
Line 320:             for spUUID in dirList:
Line 321:                 poolPath = os.path.join(self.storage_repository, spUUID)
Line 322:                 try:
Line 323:                     self._connectStoragePool(spUUID, None, None, None, None)
+1 to Edu's comment
Line 324:                     # TODO: once we support simultaneous connection to multiple
Line 325:                     #       pools, remove following line (break)
Line 326:                     break
Line 327:                 except Exception:


Line 321:                 poolPath = os.path.join(self.storage_repository, spUUID)
Line 322:                 try:
Line 323:                     self._connectStoragePool(spUUID, None, None, None, None)
Line 324:                     # TODO: once we support simultaneous connection to multiple
Line 325:                     #       pools, remove following line (break)
since this will never happen you can just remove the comment.
Line 326:                     break
Line 327:                 except Exception:
Line 328:                     self.log.error("Unexpected error", exc_info=True)
Line 329: 


Line 829: 
Line 830:         try:
Line 831:             self.getPool(spUUID)
Line 832:         except se.StoragePoolUnknown:
Line 833:             pass # Pool not connected yet
this try..except..else..return clause forces a funky structure where your exception
handling code is actually done after it's finished.
Either move everything after the clause into its own method so that instead of pass
you'd have a method call or get rid of this clause.
Right now it's downright confusing.
Line 834:         else:
Line 835:             with rmanager.acquireResource(STORAGE, spUUID, rm.LockType.shared):
Line 836:                 pool = self.getPool(spUUID)
Line 837:                 if not msdUUID or not masterVersion:


Line 848:         with rmanager.acquireResource(STORAGE, spUUID, rm.LockType.exclusive):
Line 849:             try:
Line 850:                 pool = self.getPool(spUUID)
Line 851:             except se.StoragePoolUnknown:
Line 852:                 pass # Pool not connected yet
same here.
Line 853:             else:
Line 854:                 if not msdUUID or not masterVersion:
Line 855:                     hostID, scsiKey, \
Line 856:                     msdUUID, masterVersion = pool.getStoredPoolInformation()


Line 850:                 pool = self.getPool(spUUID)
Line 851:             except se.StoragePoolUnknown:
Line 852:                 pass # Pool not connected yet
Line 853:             else:
Line 854:                 if not msdUUID or not masterVersion:
this should indeed be split
Line 855:                     hostID, scsiKey, \
Line 856:                     msdUUID, masterVersion = pool.getStoredPoolInformation()
Line 857: 
Line 858:                 misc.validateN(hostID, 'hostID')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Automatic reconnection to the Storage Pool
......................................................................


Patch Set 2:

Build Successful 

http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/113/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Automatic reconnection to the Storage Pool
......................................................................


Patch Set 2: No score

Build Started http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/113/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Eduardo has posted comments on this change.

Change subject: Automatic reconnection to the Storage Pool
......................................................................


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

(2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 323:                     self._connectStoragePool(spUUID, None, None, None, None)
Since this is the only entry in which pool parameters are taken from the saved state, IMHO
is better to call getStoredPoolInformation here, split the _connectStoragePooll function
and remove the option of passing None as MSD or version.

Line 822:                             masterVersion, options=None):
Removing None parameters support simplifies this function very much.
options parameter is unused and this is an internal function. Please remove.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Automatic reconnection to the Storage Pool
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 321:                     self._connectStoragePool(spUUID, None, None, None, None)
We are passing None here because connectStoragePool will load the correct values from the
persisted pool file ("/var/run/vdsm/pool/<uuid>").

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;

Ayal Baron has posted comments on this change.

Change subject: Automatic reconnection to the Storage Pool
......................................................................


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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 321:                     self._connectStoragePool(spUUID, None, None, None, None)
Why are we passing None here? we should not be deleting the file to begin with (unless
disconnectStoragePool is sent) and then use the correct values?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I682a12deff90dd78bb31b7368c003796ed241cd9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;