Change in vdsm[master]: sp: deactivateSd - remove domain from pending for upgrade list

laravot at redhat.com laravot at redhat.com
Tue Sep 8 08:06:10 UTC 2015


Liron Aravot has posted comments on this change.

Change subject: sp: deactivateSd - remove domain from pending for upgrade list
......................................................................


Patch Set 1:

(9 comments)

https://gerrit.ovirt.org/#/c/45773/1/vdsm/storage/sp.py
File vdsm/storage/sp.py:

Line 156
Line 157
Line 158
Line 159
Line 160
> You also reversed the lock order - please revert these changes.
the lock order was changed intentionally to avoid a deadlock with deactivateSd(), in hsm.py deactivateStorageDomain() we take a lock for the sdUUID (without the _update prefix), keeping the order the same here will open a possibility to a deadlock.

regardless of that, this lock order makes more sense, because other operations that use the domain take the lock by the sdUUID, so in most cases we'll succeed here to the the _upgrade lock and then fail.


Line 158
Line 159
Line 160
Line 161
Line 162
> What we need to do is to take the same locks used here, in the same order w
see previous comment, that's the reason i've changed the order here.


Line 1094
Line 1095
Line 1096
Line 1097
Line 1098
> I think that the only change needed for this patch is this:
1, the lock for the sdUUID is already taken at hsm.py -deactivateSd(), please take a look there.

2. we shouldn't block, we use here acquireResource - on the worst case the domain deactivation will fail with is the right behavior (if you were lucky enough to deactivate a domain on the exact time it's being upgraded..).

3. the removal from self._domainsToUpgrade should be done after the domain is actually deactivated in the added _deactivateSd(), otherwise you might be left with domain that we failed to deactivate (failed update the pool/domain metadata on storage) but will never be updated (as it was removed from self._domainsToUpgrade).


Line 1094
Line 1095
Line 1096
Line 1097
Line 1098
> When removing uuid from _domainsToUpgrade, it may become empty. This is han
I'll handle add that


Line 152:         except:
Line 153:             self.log.error("Error while preparing domain `%s` upgrade", sdUUID,
Line 154:                            exc_info=True)
Line 155:             return
Line 156:                 
> Please remove this unneeded whitespace change
Done
Line 157:         with rmanager.acquireResource(STORAGE, sdUUID,
Line 158:                                       rm.LockType.exclusive):
Line 159:             with rmanager.acquireResource(STORAGE, "upgrade_" + sdUUID,
Line 160:                                           rm.LockType.exclusive):


Line 154:                            exc_info=True)
Line 155:             return
Line 156:                 
Line 157:         with rmanager.acquireResource(STORAGE, sdUUID,
Line 158:                                       rm.LockType.exclusive):
> Previously we took exclusive lock for each domain, so we can upgrade severa
The taken lock is is per domain id so there shouldn't be such problem, perhaps you confused it with a lock by the pool id?
Line 159:             with rmanager.acquireResource(STORAGE, "upgrade_" + sdUUID,
Line 160:                                           rm.LockType.exclusive):
Line 161:                 if sdUUID not in self._domainsToUpgrade:
Line 162:                     return


Line 1097:                     self.masterMigrate(sdUUID, newMsdUUID, masterVersion)
Line 1098:                     self._deactivateSD(domList, sdUUID)
Line 1099:             else:
Line 1100:                 with rmanager.acquireResource(STORAGE, "upgrade_" + self.spUUID,
Line 1101:                                               rm.LockType.exclusive):
> This will not work if the upgrade thread is holding the resource, for examp
if the upgrade thread is trying to upgrade at the exact time, you should fail here. in the worst case you'll have to try again which makes sense 0 there's an operation running or the domain. do you have suggestion for a better behavior on that case?
Line 1102:                     masterDir = os.path.join(dom.domaindir, sd.MASTER_FS_DIR)
Line 1103:                     try:
Line 1104:                         m = mount.getMountFromTarget(masterDir)
Line 1105:                     except OSError as e:


Line 1113:                         except mount.MountError:
Line 1114:                             self.log.error("Can't umount masterDir %s for domain "
Line 1115:                                         "%s", masterDir, dom)
Line 1116:                     self._deactivateSD(domList, sdUUID)
Line 1117:                     self._domainsToUpgrade.remove(sdUUID)
> You don't handle the case when self._domainsToUpgrade becomes empty.
Right, will add it.
Line 1118: 
Line 1119:     def _deactivateSD(self, domList, sdUUID):
Line 1120:         domList[sdUUID] = sd.DOM_ATTACHED_STATUS
Line 1121:         self._backend.setDomainsMap(domList)


Line 1115:                                         "%s", masterDir, dom)
Line 1116:                     self._deactivateSD(domList, sdUUID)
Line 1117:                     self._domainsToUpgrade.remove(sdUUID)
Line 1118: 
Line 1119:     def _deactivateSD(self, domList, sdUUID):
> This change is not needed for removing the domain from _domainsToUpgrade.
Its needed, see previous comment. firstly the metadata updates should be made.
Line 1120:         domList[sdUUID] = sd.DOM_ATTACHED_STATUS
Line 1121:         self._backend.setDomainsMap(domList)
Line 1122:         self.updateMonitoringThreads()
Line 1123: 


-- 
To view, visit https://gerrit.ovirt.org/45773
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4451b348b8837dd83d95aea2be4a4536b33cdd99
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list