Change in vdsm[master]: sp: move reconnection info check to StoragePool

abaron at redhat.com abaron at redhat.com
Sun Nov 24 09:26:43 UTC 2013


Ayal Baron has posted comments on this change.

Change subject: sp: move reconnection info check to StoragePool
......................................................................


Patch Set 3: Code-Review-1

(5 comments)

....................................................
File vdsm/storage/hsm.py
Line 1027:         except se.StoragePoolUnknown:
Line 1028:             pass  # pool not connected yet
Line 1029:         else:
Line 1030:             with rmanager.acquireResource(STORAGE, spUUID, rm.LockType.shared):
Line 1031:                 pool.update(hostID, scsiKey, msdUUID, masterVersion)
after acquiring the lock we can't even be sure we're connected to the same pool (chances are slim, but still)...
Line 1032:                 return True
Line 1033: 
Line 1034:         with rmanager.acquireResource(STORAGE, spUUID, rm.LockType.exclusive):
Line 1035:             try:


Line 1036:                 pool = self.getPool(spUUID)
Line 1037:             except se.StoragePoolUnknown:
Line 1038:                 pass  # pool not connected yet
Line 1039:             else:
Line 1040:                 pool.update(hostID, scsiKey, msdUUID, masterVersion)
same
Line 1041:                 return True
Line 1042: 
Line 1043:             pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng)
Line 1044:             res = pool.connect(hostID, scsiKey, msdUUID, masterVersion)


....................................................
File vdsm/storage/sp.py
Line 688:             msg = "Pools temp data dir: %s does not exist" % (
Line 689:                 self._poolsTmpDir)
Line 690:             raise se.StoragePoolConnectionError(msg)
Line 691: 
Line 692:         self._saveReconnectInformation(hostID, scsiKey, msdUUID, masterVersion)
I wonder if the mistake is not reverse than the purpose of this patch.
i.e. not that reading the reconnection info belong inside connect but rather that persisting it belongs outside.
this method has 2 different ways of getting its parameters and if someone called it and did not pass them then we fail back to the other method.
Instead, maybe we should have a separate reconnect method and whenever connect here is called, the parameters have to be passed.

The only place we actually call connectStoragePool without params is in the init of hsm.  So we could just retrieve the info there and persist it also at hsm level in connect.

Alternatively, I'm not even clear on whether this still works and engine automatically reconnects and we also support running VMs without being connected to a pool so I wonder if we shouldn't just drop all the reconnect logic and be done with it (esp. as what we're trying to do is get rid of the pool here).

We will need to automatically connect to domains later on (acquire lock space) to cut on startup time, but that is pretty much a different issue entirely.
Line 693:         self.id = hostID
Line 694:         self.scsiKey = scsiKey
Line 695:         # Make sure SDCache doesn't have stale data (it can be in case of FC)
Line 696:         sdCache.invalidateStorage()


Line 741: 
Line 742:     @unsecured
Line 743:     def getPoolParams(self, hostID, scsiKey, msdUUID, masterVersion):
Line 744:         if all(hostID, scsiKey, msdUUID, masterVersion):
Line 745:             return hostID, scsiKey, msdUUID, masterVersion
this makes things even more weird.
if user passed params - return the params right back.
if not -  read from disk and return
Line 746: 
Line 747:         self.log.info(
Line 748:             "Using saved connection parameters: hostID=%s, scsiKey=%s, "
Line 749:             "msdUUID=%s, masterVersion=%s", hostID, scsiKey, msdUUID,


Line 752:         with open(self._poolFile, "r") as f:
Line 753:             poolInfo = dict((x.strip().split("=", 1) for x in f))
Line 754: 
Line 755:         return tuple(poolInfo.get(x) for x in
Line 756:                      ("hostId", "scsiKey", "msdUUID", "masterVersion"))
same as patch set 1 or at least explain why I'm wrong?
Line 757: 
Line 758:     @unsecured
Line 759:     def createMaster(self, poolName, domain, masterVersion, leaseParams):
Line 760:         """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia57afc04db02b6c15633d09349a55b3ff5ae7fda
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list