Saggi Mizrahi has posted comments on this change.
Change subject: Fix getStorageDomainInfo() logic.
......................................................................
Patch Set 3: Code-Review-1
(3 comments)
There are no tests for any of the corner cases being handled.
Please add them before committing this code.
....................................................
File vdsm/storage/hsm.py
Line 2740: if info['role'] == sd.MASTER_DOMAIN:
Line 2741: try:
Line 2742: # Verify that the host is connected to the same pool which
Line 2743: # the SD is attached to.
Line 2744: pool = self.getPool(info['pool'][0])
Making sure that len(info['pool'] >= 1) and looking for info['pool'][0]
in the pool list would have made the comment unnecessary. It would have made the code
exception free.
You are actually using the side effects to check for things instead of writing the checks
out in code.
Line 2745: except IndexError:
Line 2746: self.log.error("Domain %s is marked as master but is not
"
Line 2747: "attached to any pool", sdUUID,
exc_info=True)
Line 2748: except se.StoragePoolUnknown:
Line 2752: exc_info=True)
Line 2753: else:
Line 2754: # make sure it's THE master of this pool
Line 2755: if pool.masterDomain.sdUUID != sdUUID:
Line 2756: self.log.error("Domain %s is marked as master but
actual "
IIRC it's also being checked *and fixed* elswhere
Also, if we are already checking. Isn't it better to fake it and tell return that the
domain is a data domain so that we don't confuse the user?
Line 2757: "master is %s",
Line 2758: sdUUID, pool.masterDomain.sdUUID)
Line 2759: else:
Line 2760: poolInfo = pool.getInfo()
Line 2755: if pool.masterDomain.sdUUID != sdUUID:
Line 2756: self.log.error("Domain %s is marked as master but
actual "
Line 2757: "master is %s",
Line 2758: sdUUID, pool.masterDomain.sdUUID)
Line 2759: else:
I would put the positive (and shorter) case as the first clause of the conditional and the
longer one at the end
Line 2760: poolInfo = pool.getInfo()
Line 2761: for key in ('lver', 'spm_id',
'master_ver'):
Line 2762: info[key] = poolInfo['info'][key]
Line 2763:
--
To view, visit
http://gerrit.ovirt.org/14671
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b0b2ad3dca19cf203d937c1a9f6a12ab0f1095f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Daniel Paikov <paikov(a)gmail.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes