Simone Tiraboschi has posted comments on this change. Change subject: packaging: setup: let the user configure engine VM networking ......................................................................
Patch Set 4: (15 comments) 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 Done 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 Done 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. Done 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 We discussed that a bit there: https://gerrit.ovirt.org/#/c/38811/8/src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py Indeed they could assume different values: this one is the hostname on the engine VM, while the other is the name your are going to publish it on http which could also be an alias. 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: Done 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 :-) Done 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 e I wasn't meaning Continuous Integration: We could have it only via CloudInit. 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 Yes, I agree 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 Done 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: Done 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 The dirty way (directly parsing /etc/resolv.conf) is not that complex but I'm not sure that have it in robust and portable way is that easy. 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 Done 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: Done 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 inp Adding a TODO 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. Ok 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: Simone Tiraboschi <stira...@redhat.com> 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
