Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: using cloud-init to run engine-setup ......................................................................
Patch Set 16: (8 comments) https://gerrit.ovirt.org/#/c/40546/16/src/ovirt_hosted_engine_setup/appliance_esetup.py File src/ovirt_hosted_engine_setup/appliance_esetup.py: Line 88: """ Line 89: Read a line from the (already-connected) appliance in a not blocking Line 90: way. The response is non-newline-terminated string plus a boolean Line 91: to indicate that a timeout has occurred. Line 92: Max 200 chars per line. Any reason to not allow at least 1024? We sometimes have pretty long lines. What happens if we overflow? Line 93: """ Line 94: line = '' Line 95: len = 0 Line 96: if not self._appliance_is_connected(): https://gerrit.ovirt.org/#/c/40546/16/src/ovirt_hosted_engine_setup/constants.py File src/ovirt_hosted_engine_setup/constants.py: Line 691 Line 692 Line 693 Line 694 Line 695 perhaps this in answer file too https://gerrit.ovirt.org/#/c/40546/16/src/plugins/ovirt-hosted-engine-setup/engine/health.py File src/plugins/ovirt-hosted-engine-setup/engine/health.py: Line 122: else: Line 123: self.logger.error( Line 124: 'Invalid option \'{0}\''.format(response) Line 125: ) Line 126: else: Pretty long if/else, if not breaking to functions perhaps at least add a comment here Line 127: spath = ( Line 128: ohostedcons.Const.OVIRT_HE_CHANNEL_PATH + Line 129: self.environment[ Line 130: ohostedcons.VMEnv.VM_UUID Line 142: rtimeouts = 0 Line 143: while not completed: Line 144: line, timeout = self._appliance_readline_nb(TIMEOUT) Line 145: if line: Line 146: self.dialog.note('|- ' + line + '\n') Will it show the colors? We check (otopi) whether output isatty. Not sure what will happen in this case. IMO it will be nice if we manage to show them. Line 147: if timeout: Line 148: rtimeouts += 1 Line 149: else: Line 150: rtimeouts = 0 Line 158: 'since {since} seconds ago.\n' Line 159: 'Please check its log on the appliance.\n' Line 160: ).format(since=TIMEOUT*NTIMEOUT) Line 161: ) Line 162: if 'Execution of setup completed successfully' in line: This is localized, don't think you can rely on it. Better to somehow check $? and do something simple (create a file 'failed' or 'succeeded' or output something easy to parse or whatever) Line 163: completed = True Line 164: elif 'Execution of setup failed' in line: Line 165: self.logger.error( Line 166: 'Engine setup failed on the appliance' https://gerrit.ovirt.org/#/c/40546/16/src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py File src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py: Line 264: user_data += ( Line 265: 'write_files:\n' Line 266: ' - content: |\n' Line 267: ' [environment:default]\n' Line 268: ' OVESETUP_CONFIG/adminPassword=str:{password}\n' Do we make sure that all the copies of this password are safe? Including temporary file before iso creation, iso file itself, engine vm image, file inside engine vm? Line 269: ' OVESETUP_CONFIG/fqdn=str:{fqdn}\n' Line 270: ' path: {heanswers}\n' Line 271: ' owner: root:root\n' Line 272: ' permissions: \'0640\'\n' https://gerrit.ovirt.org/#/c/40546/16/src/vdsm_hooks/hostedengine.py File src/vdsm_hooks/hostedengine.py: Line 47: def appendAgentDevice(self, path, name): Line 48: """ Line 49: <channel type='unix'> Line 50: <target type='virtio' name='org.linux-kvm.port.0'/> Line 51: <source mode='bind' path='/tmp/socket'/> What's '/tmp/socket'? Is it on host? VM? Can it be randomized? Line 52: </channel> Line 53: """ Line 54: vm_uuid_element = self.domxml.getElementsByTagName('uuid')[0] Line 55: vm_uuid = vm_uuid_element.childNodes[0].nodeValue Line 54: vm_uuid_element = self.domxml.getElementsByTagName('uuid')[0] Line 55: vm_uuid = vm_uuid_element.childNodes[0].nodeValue Line 56: devices = self.domxml.getElementsByTagName('devices')[0] Line 57: Line 58: channel = self.domxml.createElement('channel') What if it already exists? Line 59: channel.setAttribute('type', 'unix') Line 60: Line 61: target = self.domxml.createElement('target') Line 62: target.setAttribute('type', 'virtio') -- To view, visit https://gerrit.ovirt.org/40546 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I305232fc44a524fd53e2d7fbd819ab5b95d4931c Gerrit-PatchSet: 16 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
