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

Reply via email to