Marcin Mirecki has posted comments on this change. Change subject: vdsm hooks: report hook stderr to Engine if it fails an action ......................................................................
Patch Set 15: (5 comments) https://gerrit.ovirt.org/#/c/42592/15/lib/vdsm/define.py File lib/vdsm/define.py: Line 180: 'message': 'Error unregistering Libvirt secret'}}, Line 181: 'recovery': {'status': { Line 182: 'code': 99, Line 183: 'message': 'Recovering from crash or Initializing'}}, Line 184: 'hookError': {'status': { > 99 should always be the last (or at least violating this constraints requir 75 is taken, using 78 Line 185: 'code': 100, Line 186: 'message': 'Hook error'}}, Line 187: } Line 188: https://gerrit.ovirt.org/#/c/42592/15/vdsm/API.py File vdsm/API.py: Line 376: 'Hook error during migration: ' + str(e)) Line 377: Line 378: stats = v.getStats().copy() Line 379: if runHooks: Line 380: stats = hooks.after_get_vm_stats([stats])[0] > why isn't this hook protected like the above? The after_* hooks do not raise exceptions when they fail. after_get_vm_stats has raiseError set to False Line 381: return {'status': doneCode, 'statsList': [stats]} Line 382: Line 383: def hibernate(self, hibernationVolHandle): Line 384: """ https://gerrit.ovirt.org/#/c/42592/15/vdsm/virt/migration.py File vdsm/virt/migration.py: Line 88: self._downtime = kwargs.get('downtime') or \ Line 89: config.get('vars', 'migration_downtime') Line 90: self._autoConverge = autoConverge Line 91: self._compressed = compressed Line 92: self._progress = 0 > unneeded change, please remove Decided in a discussion that it should stay like it is Line 93: self.status = { Line 94: 'status': { Line 95: 'code': 0, Line 96: 'message': 'Migration in progress'}, Line 121: self.status['migrationStats']['progress'] = self._progress Line 122: Line 123: stat = self._vm._dom.jobStats(libvirt.VIR_DOMAIN_JOB_STATS_COMPLETED) Line 124: if 'downtime_net' in stat: Line 125: self.status['migrationStats']['downtime'] = stat['downtime_net'] > why is 'migrationStats' needed at all? this can break Engine. migrationStats is part of how the code worked before. It was moved from API.py To be moved to another patch Line 126: return self.status Line 127: Line 128: def _createClient(self, port): Line 129: sslctx = sslutils.create_ssl_context() Line 342: if response.is_failure(result): Line 343: self._last_status = result Line 344: hookErrorCode = errCode['hookError']['status']['code'] Line 345: if result['status']['code'] == hookErrorCode: Line 346: raise hooks.HookError(result['status']['message']) > can we achieve the same result without raising HookError? This is more elegant. Decided in a discusion that it should stay like this. Line 347: raise RuntimeError('migration destination error: ' + Line 348: result['status']['message']) Line 349: if config.getboolean('vars', 'ssl'): Line 350: transport = 'tls' -- To view, visit https://gerrit.ovirt.org/42592 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibe2d5eb52cf2c8855d9d7d5e0ff1628a6cf1dc51 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Marcin Mirecki <mmire...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Feng Yang <yangf...@cloud-times.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Marcin Mirecki <mmire...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
