Change in vdsm[master]: sp: refresh metadata on hsm when listing domains

nsoffer at redhat.com nsoffer at redhat.com
Thu Jan 2 17:35:41 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: sp: refresh metadata on hsm when listing domains
......................................................................


Patch Set 4: Code-Review-1

(7 comments)

There are 3 changes in this patch:
- Encapsulating access to meta data using new setDomainsMap and getDomainsMap - good change
- Invalidate meta data in getDomainsMap instead of in updateMonitoringThreads - looks very wrong to me.
- Removal of the @unsecured decorators from some methods - I don't understand what this change does

I suggest to split the refactoring from the behavior changes:
1. add getDomainsMap and setDomainsMap - can be merged quickly
2. Invalidation changes - if they are correct
3. Unsecured changes

....................................................
Commit Message
Line 3: AuthorDate: 2013-11-27 06:32:14 -0500
Line 4: Commit:     Federico Simoncelli <fsimonce at redhat.com>
Line 5: CommitDate: 2013-12-06 12:10:20 -0500
Line 6: 
Line 7: sp: refresh metadata on hsm when listing domains
Please explain why we need to do this, and what happen if we don't.
Line 8: 
Line 9: In this patch:
Line 10: - validatePoolSD and validateAttachedDomain are relevant only for SPM
Line 11:   operations (remove @unsecured)


Line 7: sp: refresh metadata on hsm when listing domains
Line 8: 
Line 9: In this patch:
Line 10: - validatePoolSD and validateAttachedDomain are relevant only for SPM
Line 11:   operations (remove @unsecured)
I don't have any idea what is the meaning of @unsecured, but it is clear that this change is not related to this patch.
Line 12: 
Line 13: Change-Id: I095cd0760076fb4be97a776498af78a40ff84112


....................................................
File vdsm/storage/sp.py
Line 528
Line 529
Line 530
Line 531
Line 532
Please split this change to another patch.


Line 536
Line 537
Line 538
Line 539
Line 540
Please split this change to another patch.


Line 1540
Line 1541
Line 1542
Line 1543
Line 1544
Why did is not needed here now?


Line 160:         self.setMetaParams(metaParams, __securityOverride=True)
Line 161: 
Line 162:     @unsecured
Line 163:     def getDomainsMap(self):
Line 164:         self.invalidateMetadata()
This looks very wrong - an accesor should not have side effects like this.
Line 165:         return self.getMetaParam(PMDK_DOMAINS)
Line 166: 
Line 167:     def setDomainsMap(self, domains):
Line 168:         self.setMetaParam(PMDK_DOMAINS, domains)


Line 536:             poolMeta[PMDK_MASTER_VER] = masterVersion
Line 537:             domain.changeRole(role)
Line 538: 
Line 539:     # TODO: Remove or rename this function.
Line 540:     def validatePoolSD(self, sdUUID):
Now meta data is invalidated when calling self.getDomains(). Previously it was invalidated only if sdUUID was not in the returned list.
Line 541:         if sdUUID not in self.getDomains():
Line 542:             raise se.StorageDomainNotMemberOfPool(self.spUUID, sdUUID)
Line 543:         return True
Line 544: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I095cd0760076fb4be97a776498af78a40ff84112
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer 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