Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: let the user configure engine VM networking ......................................................................
Patch Set 25: (3 comments) https://gerrit.ovirt.org/#/c/41357/25/src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py File src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py: Line 101: return None Line 102: nprefix = '/32' Line 103: if str(naip.ipv6()) == str(ip): Line 104: nprefix = '/128' Line 105: return self._validate_ip_cidr(str(ip) + nprefix) return self._validate_ip_cidr( str(ip) + ('/128' if str(naip.ipv6()) == str(ip) else '/32') ) Line 106: Line 107: def _getMyIPAddress(self): Line 108: device = ( Line 109: self.environment[ohostedcons.NetworkEnv.BRIDGE_NAME] Line 169: elem = _('this host') Line 170: elif type == 'g': Line 171: elem = _('the default gateway') Line 172: else: Line 173: elem = _('the other host') Can't find where you call it with type not in 'h','g'. Is this a preparation for HC? Perhaps better add a type for that and abort otherwise. Line 174: if not netaddr.IPAddress(v_ip) in netaddr.IPNetwork(proposed_cidr): Line 175: return _( Line 176: 'The Engine VM ({engine}) and {elem} ' Line 177: '({host}) will not be in the same IP subnet.\n' Line 241: proposed_cidr = proposed_ip + '/' + str(my_ip.prefixlen) Line 242: msg = self._msg_validate_ip_cidr(proposed_cidr) Line 243: if msg: Line 244: self.logger.error(msg) Line 245: continue Perhaps add: def _error_if_msg(self, msg, interactive): if interactive: self.logger.error(msg) return msg else: self.logger.error(msg) raise RuntimeError(msg) and then: if self._error_if_msg(self._msg_validate_ip_cidr(proposed_cidr), True): continue Actually, when asking to reduce code duplication, I had in mind something like e.g. network/gateway.py, where the 'if interactive: query' is inside the while loop: While not valid: If interactive: query for address if test1: if interactive error and continue else raise ... Not that important. But it will be nicer in the long run if we had a function (or a few) to query for things, which will include everything, including calling tests, looping or raising, etc., so that _customize_vm_addressing will just be a single long call to that function, without any interaction logic. Line 246: msg = self._msg__validate_ip_cidr_subnet( Line 247: proposed_cidr, Line 248: my_ip, Line 249: 'h' -- 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: 25 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Lev Veyde <lve...@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-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
