Nir Soffer has posted comments on this change. Change subject: iscsi: checking image size against VG free space ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/34940/1/src/plugins/ovirt-hosted-engine-setup/vm/image.py File src/plugins/ovirt-hosted-engine-setup/vm/image.py: Line 44: """ Line 45: Line 46: def __init__(self, context): Line 47: super(Plugin, self).__init__(context=context) Line 48: self.serv = None Why do you need this instance variable? You can get this from the environment when needed. Line 49: Line 50: @plugin.event( Line 51: stage=plugin.Stages.STAGE_INIT, Line 52: ) Line 81: ohostedcons.Stages.DIALOG_TITLES_E_VM, Line 82: ), Line 83: ) Line 84: def _disk_customization(self): Line 85: self.serv = self.environment[ohostedcons.VDSMEnv.VDS_CLI] Initializing here create temporal dependency, function using self.serv will fail if called before this function. Better initialize this in __init__, or just get it from the environment when needed. Line 86: interactive = self.environment[ Line 87: ohostedcons.StorageEnv.IMAGE_SIZE_GB Line 88: ] is None Line 89: Line 88: ] is None Line 89: Line 90: vgmaxavailable = None Line 91: if self.environment[ohostedcons.StorageEnv.DOMAIN_TYPE] in ( Line 92: ohostedcons.DomainTypes.ISCSI, Using "in" makes sense only where there is more then one value, otherwise this is over generalized and make the code harder to read for no benefit. Line 93: ): Line 94: # Can't use sparse volume on block devices Line 95: vginfo = self.serv.s.getVGInfo( Line 96: self.environment[ohostedcons.StorageEnv.VG_UUID] Line 89: Line 90: vgmaxavailable = None Line 91: if self.environment[ohostedcons.StorageEnv.DOMAIN_TYPE] in ( Line 92: ohostedcons.DomainTypes.ISCSI, Line 93: ): This block of code will more clear if you extract a method like _get_available_space() This method can get and return vgfree when working with iscsi domain, or the available space when working on nfs. Line 94: # Can't use sparse volume on block devices Line 95: vginfo = self.serv.s.getVGInfo( Line 96: self.environment[ohostedcons.StorageEnv.VG_UUID] Line 97: ) Line 100: raise RuntimeError(vginfo['status']['message']) Line 101: vgfree = int(vginfo['info']['vgfree']) Line 102: vgmaxavailable = ( Line 103: vgfree / ohostedcons.Const.MINIMUM_SPACE_STORAGEDOMAIN_MB * Line 104: ohostedcons.Const.MINIMUM_SPACE_STORAGEDOMAIN_MB Seems that you mean: vgmaxavailable = vgfree / EXTENT_SIZE * EXTENT_SIZE The name "vgmaxavailable" does not make sense - there is no range, just one value - free bytes - so there is no max or min. Since you work with GiB in the UI, why not compute the available GiB instead? This means that you don't need to hard code that extent size (unless it will be more then GiB - unlikely). available_gb = vgfree / 2**30 Line 105: ) Line 106: Line 107: valid = False Line 108: while not valid: Line 121: try: Line 122: valid = True Line 123: if vgmaxavailable is not None and int( Line 124: self.environment[ohostedcons.StorageEnv.IMAGE_SIZE_GB] Line 125: ) * pow(2, 30) > vgmaxavailable: There is no need to use pow(2, 30), use 2**30. Or if you agree with my previous suggestion: if required_gb > available_gb: ... Line 126: self.logger.warning( Line 127: _( Line 128: 'Not enough free space, ' Line 129: 'only {free:.2f} GiB are available' -- To view, visit http://gerrit.ovirt.org/34940 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17f861a053f6ac38983967ffb07ecca9ff9b8de1 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
