Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: let the user configure engine VM networking ......................................................................
Patch Set 4: (15 comments) Nice work! Some comments inside. https://gerrit.ovirt.org/#/c/41357/4/src/ovirt_hosted_engine_setup/constants.py File src/ovirt_hosted_engine_setup/constants.py: Line 719 Line 720 Line 721 Line 722 Line 723 Perhaps we should move all these to their own class, the names are getting longer... https://gerrit.ovirt.org/#/c/41357/4/src/ovirt_hosted_engine_setup/util.py File src/ovirt_hosted_engine_setup/util.py: Line 1 Line 2 Line 3 2015 Line 71: def check_is_pingable(address): Line 72: """ Line 73: Ensure that an address is pingable Line 74: """ Line 75: if os.system("ping -c 1 " + address) == 0: I'd rather use plugin.execute as before. No need for try/raise/except - you can get rc from it just as you did here. Line 76: return True Line 77: return False Line 78: Line 79: https://gerrit.ovirt.org/#/c/41357/4/src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py File src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py: Line 159 Line 160 Line 161 Line 162 Line 163 Sorry for not seeing this earlier: Why do we ask both this and engine fqdn (in engine/fqdn.py)? If it's because we want to allow them to be different, please change the text accordingly, e.g.: Please provide the hostname for the engine VM. This should probably be the same as the engine FQDN. It should be different from the name of the host, or any other machine. Also not sure about using 'engine appliance' vs 'engine VM', here and elsewhere. Perhaps consider using 'appliance' only if it's really our appliance. Line 157: static = self.dialog.queryString( Line 158: name='CI_VM_STATIC_NETWORKING', Line 159: note=_( Line 160: 'Rely on DHCP to configure the network for the ' Line 161: 'engine VM?\n' Perhaps: How should the engine VM network should be configured (DHCP, Static) [DHCP]? Line 162: '(@VALUES@)[@DEFAULT@]? ' Line 163: ), Line 164: prompt=True, Line 165: validValues=(_('DHCP'), _('Static')), Line 165: validValues=(_('DHCP'), _('Static')), Line 166: caseSensitive=False, Line 167: default=_('DHCP') Line 168: ) == _('Static').lower() Line 169: if static: # Static Comment not needed :-) Line 170: default_cidr = self._getFreeIPAddress( Line 171: self._getMyIPAddress() Line 172: ) Line 173: ipcidr = self.dialog.queryString( Line 170: default_cidr = self._getFreeIPAddress( Line 171: self._getMyIPAddress() Line 172: ) Line 173: ipcidr = self.dialog.queryString( Line 174: name='CI_CIDR', What's 'CI'? IMO it's not only for CI. Also please use longer names (also elsewhere). Line 175: note=_( Line 176: 'Please enter the IP address/prefix len ' Line 177: 'to be used for the engine VM\n' Line 178: '[@DEFAULT@] ' Line 175: note=_( Line 176: 'Please enter the IP address/prefix len ' Line 177: 'to be used for the engine VM\n' Line 178: '[@DEFAULT@] ' Line 179: ), I think that the prefix can be taken from the host Line 180: prompt=True, Line 181: caseSensitive=False, Line 182: default=str(default_cidr), Line 183: ) Line 200: else: # DHCP Line 201: self.environment[ Line 202: ohostedcons.VMEnv.CLOUD_INIT_VM_CIDR Line 203: ] = False Line 204: else: # Not interactive Line 205: if self.environment[ Line 206: ohostedcons.VMEnv.CLOUD_INIT_VM_CIDR Line 207: ]: Line 208: valid_ip = self._validate_ip_cidr( Line 241: 'Please provide the list of DNS you would ' Line 242: 'like to use for the engine appliance.\n' Line 243: 'Note: You could provide more than one ' Line 244: 'address separating them with a ,\n' Line 245: 'Engine VM DNS (leave it empty to skip): ' Perhaps: Please provide a comma-separated list of IP addresses of domain name servers for the engine VM: Line 246: ), Line 247: prompt=True, Line 248: default='', Line 249: ) Line 244: 'address separating them with a ,\n' Line 245: 'Engine VM DNS (leave it empty to skip): ' Line 246: ), Line 247: prompt=True, Line 248: default='', Any chance to have default same as host? If too much work, please add TODO comment Line 249: ) Line 250: if not dns: Line 251: self.environment[ Line 252: ohostedcons.VMEnv.CLOUD_INIT_VM_DNS Line 264: self.environment[ Line 265: ohostedcons.VMEnv.CLOUD_INIT_VM_DNS Line 266: ] = ','.join(dnslist) Line 267: valid = True Line 268: else: # Not interactive Line 269: if self.environment[ohostedcons.VMEnv.CLOUD_INIT_VM_DNS]: Line 270: for d in self.environment[ Line 271: ohostedcons.VMEnv.CLOUD_INIT_VM_DNS Line 272: ].split(','): Line 530: note=_( Line 531: 'Automatically push a line for this host ' Line 532: 'under /etc/hosts on the engine VM\n' Line 533: 'Note: ensuring that this host could resolve the ' Line 534: 'engine VM hostname is still up to you\n' Perhaps: Add to /etc/hosts on the engine VM a line for this host? Also, we can add a line for the engine to our /etc/hosts - we know all details... Line 535: '(@VALUES@)[@DEFAULT@]? ' Line 536: ), Line 537: prompt=True, Line 538: validValues=(_('Yes'), _('No')), Line 572: 'bootcmd:\n' Line 573: ' - echo "{myip} {myfqdn}" >> /etc/hosts' Line 574: ).format( Line 575: myip=self._getMyIPAddress().ip, Line 576: myfqdn=socket.gethostname(), IMO we should replace all calls to gethostname with env and ask 'Please input FQDN of this host', similar to engine fqdn. If not doing that now, please add a TODO comment. Line 577: ) Line 578: Line 579: if self.environment[ohostedcons.VMEnv.CLOUD_INIT_EXECUTE_ESETUP]: Line 580: org = 'Test' Line 645: domain = self.environment[ Line 646: ohostedcons.NetworkEnv.OVIRT_HOSTED_ENGINE_FQDN Line 647: ].split('.', 1)[1] Line 648: user_data += ( Line 649: ' domain: {domain}\n' Please ask about this if you want. Do not guess. Line 650: ).format( Line 651: domain=domain Line 652: ) Line 653: -- To view, visit https://gerrit.ovirt.org/41357 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic09f4d92e44f1bf810f06ada703e533b8c5e2061 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
