Simone Tiraboschi has posted comments on this change. Change subject: iscsi: checking image size against VG free space ......................................................................
Patch Set 6: (4 comments) http://gerrit.ovirt.org/#/c/34940/6/src/ovirt_hosted_engine_setup/constants.py File src/ovirt_hosted_engine_setup/constants.py: Line 269: Line 270: @util.export Line 271: @util.codegen Line 272: class Const(object): Line 273: MINIMUM_SPACE_STORAGEDOMAIN_MB = 20480 > This value needs explanation. As far as I know it's stated in product specification Line 274: FIRST_HOST_ID = 1 Line 275: HA_AGENT_SERVICE = 'ovirt-ha-agent' Line 276: HA_BROCKER_SERVICE = 'ovirt-ha-broker' Line 277: HOSTED_ENGINE_VM_NAME = 'HostedEngine' Line 274: FIRST_HOST_ID = 1 Line 275: HA_AGENT_SERVICE = 'ovirt-ha-agent' Line 276: HA_BROCKER_SERVICE = 'ovirt-ha-broker' Line 277: HOSTED_ENGINE_VM_NAME = 'HostedEngine' Line 278: OHD_OVERHEAD_GIB = 1 > This is not clear - what is OHD, and what is the mysterious overhead? Done Line 279: METADATA_CHUNK_SIZE = 4096 Line 280: MAX_HOST_ID = 250 Line 281: HA_NOTIF_SMTP_SERVER = 'smtp-server' Line 282: HA_NOTIF_SMTP_PORT = 'smtp-port' http://gerrit.ovirt.org/#/c/34940/6/src/plugins/ovirt-hosted-engine-setup/vm/image.py File src/plugins/ovirt-hosted-engine-setup/vm/image.py: Line 94: # could be gathered from vginfo but at this point Line 95: # it still has to be created Line 96: available_gb = int(self.environment[ Line 97: ohostedcons.StorageEnv.BDEVICE_SIZE_GB Line 98: ]) - int(ohostedcons.Const.OHD_OVERHEAD_GIB) > This is not available_gb, you don't know the availale gb at this point. The > When you know the actual size available, you can display it, without any > guesswork. Unfortunately I cannot, that is the point. For rolling back and safety issues, otopi based code is structured into different plugins that are loaded at different stages. By design all the customization process (including selecting the LUN and choosing the image size) should be completed at stage=plugin.Stages.STAGE_CUSTOMIZATION and validated at stage=plugin.Stages.STAGE_VALIDATION while further action like creating the VG and than the image should be performed ad further stages so we need to know and validate the image size before creating the VG. Instead of hardcoding this value in different places, we could get it at runtime from VDSM APIs, in this case we don't introduce any reverse dependencies. Line 99: Line 100: valid = False Line 101: while not valid: Line 102: if interactive: Line 196: self.logger.debug(vginfo) Line 197: if vginfo['status']['code'] != 0: Line 198: raise RuntimeError(vginfo['status']['message']) Line 199: vgfree = int(vginfo['info']['vgfree']) Line 200: available_gb = vgfree / pow(2, 30) > Use 2**30, or a constant - using pow() is the worst possible way to do this This is the built-in pow function witch, as specified in the python docs, is equivalent to the power operator (**); I'd get a different behavior with the math.pow external function but this is not the case. https://docs.python.org/2.7/library/functions.html#pow Line 201: if self.environment[ Line 202: ohostedcons.StorageEnv.IMAGE_SIZE_GB Line 203: ] > available_gb: Line 204: raise ohosteddomains.InsufficientSpaceError( -- 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: 6 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: Simone Tiraboschi <stira...@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
