Simone Tiraboschi 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. We simply print it splitted on two lines on hosted-engine side. No real issue there but it's also not a problem to use a bigger value. 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 I'm always a bit uncertain about saving passwords in the answer-file. Sandro? 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 co Done 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 w It's currently colorless but well formatted. No need for that if we are going to switch to machine dialect. 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. Yes, it's localized but I'm also executing engine-setup on a freshly deployed appliance so I'm also almost secure that it will speak English cause I'm not forcing any other language. On my opinion the better solution is to complete the machine dialect parser within otopi and rely on it. 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 te Temporary file got destroyed after ISO creation. The ISO itself will get wiped at STAGE_CLEANUP. This answerfile will be still available on the appliance but it's under /root/; we could simply add another command to destroy it just after engine-setup 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? No, it's just a comment to provide an example of what I'm going to add. The real socket name will include the VM UUID 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? We simply append another channel. Indeed we already have two of them: one for VDSM and the other for the guest agent. 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
