Nir Soffer has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 37: (17 comments) http://gerrit.ovirt.org/#/c/34294/37/vdsm/v2v.py File vdsm/v2v.py: Line 29: from vdsm import libvirtconnection Line 30: from vdsm.constants import EXT_VIRT_V2V Line 31: Line 32: Line 33: __jobs = [] Why use a list? Jobs do not have order, and we need an easy way to access them by job id. Looks like the best data for keeping jobs is a mapping from job_id to job. Line 34: Line 35: Line 36: class InvalidVMConfiguration(ValueError): Line 37: ''' Unexpected error while parsing libvirt domain xml ''' Line 87: for job in __jobs: Line 88: ret[job.job_id()] = {} Line 89: ret[job.job_id()]['status'] = job.status() Line 90: ret[job.job_id()]['progress'] = job.progress() Line 91: return ret This list will not include the finished jobs, if they failed or succeeded. How are you going to manage the jobs when you don't have their finish status? Line 92: Line 93: Line 94: class _import_vm_thread(threading.Thread): Line 95: """ Line 106: self._jobId = jobId Line 107: self._status = 'initializing' Line 108: self._progress = 0 Line 109: Line 110: def job_id(self): Use: @property def job_id(self): return self._job_id Line 111: return self._jobId Line 112: Line 113: def status(self): Line 114: return self._status Line 109: Line 110: def job_id(self): Line 111: return self._jobId Line 112: Line 113: def status(self): Use property Line 114: return self._status Line 115: Line 116: def progress(self): Line 117: return self._progress Line 112: Line 113: def status(self): Line 114: return self._status Line 115: Line 116: def progress(self): Use propperty Line 117: return self._progress Line 118: Line 119: def run(self): Line 120: self._ovf = tempfile.mktemp() Line 115: Line 116: def progress(self): Line 117: return self._progress Line 118: Line 119: def run(self): This function is much too big and unsafe (see the comments bellow). It should be broken into: def run(self): try: _import_vm() # Put all logic there except Exception: logging of unhandled exceptions finally: cleanup code... Line 120: self._ovf = tempfile.mktemp() Line 121: logging.info('import vm, (jobId %s) started', self._jobId) Line 122: self._status = 'starting' Line 123: cmd = ['-ic', self._uri, '-o', 'vdsm', '-of', 'raw', Line 116: def progress(self): Line 117: return self._progress Line 118: Line 119: def run(self): Line 120: self._ovf = tempfile.mktemp() - This is unsafe, use tempfile.mkstemp() - When do you clean this temporary file? Line 121: logging.info('import vm, (jobId %s) started', self._jobId) Line 122: self._status = 'starting' Line 123: cmd = ['-ic', self._uri, '-o', 'vdsm', '-of', 'raw', Line 124: '--vdsm-image-uuid', self._vmProperties['imageID'], Line 128: '--machine-readable', Line 129: '-os', self._vmProperties['path'], Line 130: self._vmProperties['vmName']] Line 131: proc = pexpect.spawn(EXT_VIRT_V2V, cmd, timeout=6000, Line 132: env={'LIBGUESTFS_BACKEND': 'direct'}) Not sure pexpect is safe to use in mulltithreaded application like vdsm. This may lead to deadlock. You should use CPopen for running sub processes - see utils.execCmd(sync=False) This is also too long to be here, you should extract a _start_v2v_process() helper to make the logic here easy to understand. Line 133: # virt-v2v expect the password from the stdin twice, Line 134: # once for logging to libvirt and one for curl to fetch the images Line 135: # TODO: virt-v2v in rhel 7.2 will enable reading password from file. Line 136: # https://bugzilla.redhat.com/1158526 Line 134: # once for logging to libvirt and one for curl to fetch the images Line 135: # TODO: virt-v2v in rhel 7.2 will enable reading password from file. Line 136: # https://bugzilla.redhat.com/1158526 Line 137: proc.sendline(self._password) Line 138: proc.sendline(self._password) Extract to _send_input() helper Line 139: Line 140: re_copy_disk = re.compile('.*(Copying disk \d+/\d+).*') Line 141: re_progress = re.compile(' \((\d+).*') Line 142: while proc.isalive(): Line 137: proc.sendline(self._password) Line 138: proc.sendline(self._password) Line 139: Line 140: re_copy_disk = re.compile('.*(Copying disk \d+/\d+).*') Line 141: re_progress = re.compile(' \((\d+).*') - The leading whitespace is too fragile, use "\s+" for one or more whitespace characters. - Use raw string for regular expressions: r' \((\d+).*' - your re will not work like this. Line 142: while proc.isalive(): Line 143: line = proc.readline() Line 144: if 'Copying disk' in line: Line 145: m = re_copy_disk.match(line) Line 140: re_copy_disk = re.compile('.*(Copying disk \d+/\d+).*') Line 141: re_progress = re.compile(' \((\d+).*') Line 142: while proc.isalive(): Line 143: line = proc.readline() Line 144: if 'Copying disk' in line: What is the point of this check? You have a regex for matching the line? Line 145: m = re_copy_disk.match(line) Line 146: if m is not None: Line 147: self._status = m.groups()[0] Line 148: self._progress = 0 Line 153: "section: " + line) Line 154: while True: Line 155: buf = proc.read(1) Line 156: while '\r' not in buf: Line 157: buf += proc.read(1) You should read a chunk, and then split the lines using '\r', and handle each line contents. Line 158: m = re_progress.match(buf) Line 159: if m is not None: Line 160: try: Line 161: self._progress = int(m.groups()[0]) Line 169: raise V2VProcParsingException("error parsing" Line 170: "progress:%r" Line 171: % line) Line 172: elif line == '': Line 173: break This should be the first check, meaning that the stdout of the process was closed. You can simplify this nested while block by separating reading the first line "Copying disk..." from the progress: _read_header_message() _watch_progress() Line 174: Line 175: if (proc.isalive()): Line 176: proc.wait() Line 177: Line 171: % line) Line 172: elif line == '': Line 173: break Line 174: Line 175: if (proc.isalive()): Should log here that the process is done and we are waiting for the process to exit. Line 176: proc.wait() Line 177: Line 178: logging.info('import vm ended, exit status %d', Line 179: proc.exitstatus) Line 172: elif line == '': Line 173: break Line 174: Line 175: if (proc.isalive()): Line 176: proc.wait() We must have a timeout here, or we will wait for ever - utils.execCmd provides a wait with a timeout. Line 177: Line 178: logging.info('import vm ended, exit status %d', Line 179: proc.exitstatus) Line 180: __jobs.remove(self) Line 176: proc.wait() Line 177: Line 178: logging.info('import vm ended, exit status %d', Line 179: proc.exitstatus) Line 180: __jobs.remove(self) This will be skipped if an error is raised before this. This must use try-finally block: try: do stuff except Exception: log.exception("Description...") finally: _jobs.remove(self) Current code will leak jobs that will never be removed from _jobs list. Line 181: if proc.exitstatus != 0: Line 182: raise V2VProcParsingException("convert endded with status: %r" Line 183: % str(proc.exitstatus)) Line 184: return self._ovf Line 180: __jobs.remove(self) Line 181: if proc.exitstatus != 0: Line 182: raise V2VProcParsingException("convert endded with status: %r" Line 183: % str(proc.exitstatus)) Line 184: return self._ovf The caller is responsible for cleaning up the ovf file? But I don't see who can get the return value of run() - this value will simply disappear. Line 185: Line 186: Line 187: def _mem_to_mib(size, unit): Line 188: lunit = unit.lower() -- To view, visit http://gerrit.ovirt.org/34294 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I737f2c70fc6b8fec9ee5210cdb1d8274d3bd22aa Gerrit-PatchSet: 37 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
