Alon Bar-Lev has posted comments on this change.

Change subject: hosted-engine: WIP - allow to deploy hosted-engine
......................................................................


Patch Set 4:

(8 comments)

https://gerrit.ovirt.org/#/c/38547/4/src/plugins/ovirt-host-deploy/hosted-engine/configureha.py
File src/plugins/ovirt-host-deploy/hosted-engine/configureha.py:

Line 72:             odeploycons.HostedEngineEnv.STORAGE_DOMAIN_CONNECTION,
Line 73:             odeploycons.HostedEngineEnv.USE_SSL,
Line 74:             odeploycons.HostedEngineEnv.VM_UUID,
Line 75:         ):
Line 76:             self.environment.setdefault(env, None)
you do not need this
Line 77: 
Line 78:     @plugin.event(
Line 79:         stage=plugin.Stages.STAGE_PROGRAMS,
Line 80:     )


Line 75:         ):
Line 76:             self.environment.setdefault(env, None)
Line 77: 
Line 78:     @plugin.event(
Line 79:         stage=plugin.Stages.STAGE_PROGRAMS,
this should not be in programs but in validation
Line 80:     )
Line 81:     def _programs(self):
Line 82:         for service in (
Line 83:             odeploycons.Const.HA_AGENT_SERVICE,


Line 96:                             'deployed on a host already running those '
Line 97:                             'services.'
Line 98:                         )
Line 99:                     )
Line 100:                 else:
no need for else in exceptional pattern
Line 101:                     # Services are running by accident:
Line 102:                     # stopping them and continue the setup.
Line 103:                     # Related-To: https://bugzilla.redhat.com/1134873
Line 104:                     for service in (


Line 99:                     )
Line 100:                 else:
Line 101:                     # Services are running by accident:
Line 102:                     # stopping them and continue the setup.
Line 103:                     # Related-To: https://bugzilla.redhat.com/1134873
services should not run if there is no configuration file.
Line 104:                     for service in (
Line 105:                         odeploycons.Const.HA_AGENT_SERVICE,
Line 106:                         odeploycons.Const.HA_BROCKER_SERVICE,
Line 107:                     ):


Line 125:             'metadata_volume_UUID',
Line 126:             'sdUUID',
Line 127:             'spUUID',
Line 128:             'vm_disk_id',
Line 129:             'vmid',
just take whatever within the prefix and inject into configuration.

this will enable you to not modify this in future.

whoever set it is responsible for the values, host-deploy is only a bridge, the 
other side is not human.
Line 130:         ):
Line 131:             if uuid.UUID(
Line 132:                 self.environment[
Line 133:                     
odeploycons.HostedEngineEnv.HOSTED_ENGINE_CONFIG_PREFIX +


Line 172:                 - storage
Line 173:                 - user
Line 174:                 - vdsm_use_ssl
Line 175:                 - vm_disk_id
Line 176:                 - vmid
no need for these comments here, this is per package specific documentation.
Line 177: 
Line 178:         """
Line 179:         self.logger.info(_('Updating hosted-engine configuration'))
Line 180:         conf = {


Line 184:             ),
Line 185:             'fqdn': self.environment[
Line 186:                 odeploycons.VdsmEnv.ENGINE_HOST
Line 187:             ],
Line 188:             'service_start_time': 
odeploycons.Const.HOSTED_ENGINE_START_TIME,
I do not understand still what it is.
Line 189:         }
Line 190:         for env_key in self.environment:
Line 191:             if env_key.startswith(
Line 192:                 
odeploycons.HostedEngineEnv.HOSTED_ENGINE_CONFIG_PREFIX


https://gerrit.ovirt.org/#/c/38547/4/src/plugins/ovirt-host-deploy/hosted-engine/configureqemu.py
File src/plugins/ovirt-host-deploy/hosted-engine/configureqemu.py:

Line 62:         self.logger.info(_('Configuring libvirt'))
Line 63:         old_content = []
Line 64:         if os.path.exists(odeploycons.FileLocations.LIBVIRT_QEMU_CONF):
Line 65:             with open(odeploycons.FileLocations.LIBVIRT_QEMU_CONF, 
'r') as f:
Line 66:                 old_content = f.read().splitlines()
we still need an answer for this one, as modify this only one way will create 
abnormality if host is re-deployed.

and if can be done at runtime it much better, and as far as I know settings can 
be applied at runtime and not globally.
Line 67:         new_content = []
Line 68:         new_conf = 'lock_manager="sanlock"'
Line 69:         found = False
Line 70:         for line in old_content:


-- 
To view, visit https://gerrit.ovirt.org/38547
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia07992ccab2f745879c8d3d777e45b524bbdf6f8
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-host-deploy
Gerrit-Branch: master
Gerrit-Owner: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Martin Sivák <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to