Change in vdsm[master]: getStorageDomainInfo: SP keys when disconnected.

abaron at redhat.com abaron at redhat.com
Tue Nov 26 21:46:46 UTC 2013


Ayal Baron has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................


Patch Set 2: Code-Review-1

(6 comments)

1. No clear motivation for this patch.
2. It is breaking encapsulation

....................................................
Commit Message
Line 12: directly the pool MD for any MSD.
Line 13: This changes the previous behaviour.
Line 14: 
Line 15: TODO: pool related information should not be returned as part of
Line 16: this API response.
+1
Line 17: 
Line 18: Change-Id: I8b7323d4ccaaaec0f39d5590a06879faa7fc999e


....................................................
File vdsm/storage/hsm.py
Line 2735:         # This, of-course breaks backward compatibility.
Line 2736:         # These keys are not likely to change (also because of
Line 2737:         # BC) so it's not that horrible. In any case please
Line 2738:         # remove this when we can stop supporting this API.
Line 2739:         if info['role'] != sd.MASTER_DOMAIN:
remove this if
Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]


Line 2737:         # BC) so it's not that horrible. In any case please
Line 2738:         # remove this when we can stop supporting this API.
Line 2739:         if info['role'] != sd.MASTER_DOMAIN:
Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
change else to: if info['role'] == sd.MASTER_DOMAIN:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
Line 2745:             info['spm_id'] = -1


Line 2738:         # remove this when we can stop supporting this API.
Line 2739:         if info['role'] != sd.MASTER_DOMAIN:
Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
instead of accessing internals, you should be adding a method that returns the info you need or something
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
Line 2745:             info['spm_id'] = -1
Line 2746:             try:


Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
I see no reason to return this info if getPool fails?
i.e. move both lines down to join the spm_id line
Line 2745:             info['spm_id'] = -1
Line 2746:             try:
Line 2747:                 # Verify that the host is connected to the same pool which
Line 2748:                 # the SD is attached to.


Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
Line 2745:             info['spm_id'] = -1
remove this line (spm_id = -1) as with above changes it is redundant
Line 2746:             try:
Line 2747:                 # Verify that the host is connected to the same pool which
Line 2748:                 # the SD is attached to.
Line 2749:                 pool = self.getPool(info['pool'][0])


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8b7323d4ccaaaec0f39d5590a06879faa7fc999e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw 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: Elad Ben Aharon <eladba1990 at gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Gadi Ickowicz <gickowic at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list