Nir Soffer has posted comments on this change. Change subject: packaging: setup: asking about retriyng VG creation with force option ......................................................................
Patch Set 7: (10 comments) http://gerrit.ovirt.org/#/c/37845/7//COMMIT_MSG Commit Message: Line 8: Line 9: On dirty storage devices CreateVG could fail, force option can Line 10: prevent failures destroying any existent data. Line 11: Asking about retriyng VG creation with force option if first Line 12: attempt fails. Please update the commit message to reflect the changes in the code. Explain what was the previous behavior and what is the new flow. Since this change adds a new force option which is available externally, please explain the new option and why it is needed. Line 13: Line 14: Change-Id: I496c34e6b9f0d84443a8d5bc68d77916be6cb504 http://gerrit.ovirt.org/#/c/37845/7/src/plugins/ovirt-hosted-engine-setup/storage/blockd.py File src/plugins/ovirt-hosted-engine-setup/storage/blockd.py: Line 557 Line 558 Line 559 Line 560 Line 561 _create_vg should raise on errors, so the caller does not need the result['status']['code'] boilerplate. Then you can write: forceVG = ... try: vg_uuid = self._create_vg(forceVG) except CreateVGFailed as e: ... Line 271: prompt=True, Line 272: validValues=(_('Force'), _('Abort')), Line 273: caseSensitive=False, Line 274: default=_('Abort'), Line 275: ).lower() == _('Force').lower() If the force option is specified externally, we apply this option without asking the user, right? Line 276: Line 277: def _create_vg(self, forceVG=False): Line 278: return self.cli.createVG( Line 279: self.environment[ohostedcons.StorageEnv.SD_UUID], Line 600: forceVG = False Line 601: if self.environment[ Line 602: ohostedcons.StorageEnv.FORCE_CREATEVG Line 603: ]: Line 604: forceVG = True This is equivalent to : forceVG = self.environment[ohostedcons.StorageEnv.FORCE_CREATEVG] Assuming you initialized the environment to False, which is what you should do, instead of None. Line 605: self.logger.info(_('Creating Volume Group')) Line 606: dom = self._create_vg(forceVG) Line 607: self.logger.debug(dom) Line 608: if dom['status']['code'] != 0: Line 611: 'Error creating Volume Group: {message}' Line 612: ).format( Line 613: message=dom['status']['message'] Line 614: ) Line 615: ) This error is relevant only if forceVG was False, meaning that the vg is free and it should work without force. So it should move under the not forceVG block bellow. Line 616: if not forceVG: Line 617: # eventually retry forcing VG creation on dirty storage Line 618: self._customize_forcecreatevg() Line 619: if not self.environment[ Line 612: ).format( Line 613: message=dom['status']['message'] Line 614: ) Line 615: ) Line 616: if not forceVG: First handle the error case - It is much eaiser to follow: forceVG = ... try: vg_uuid = self._create_vg(forceVG) except CreateVGFailed as e: if forceVG: raise retry logic... Line 617: # eventually retry forcing VG creation on dirty storage Line 618: self._customize_forcecreatevg() Line 619: if not self.environment[ Line 620: ohostedcons.StorageEnv.FORCE_CREATEVG Line 618: self._customize_forcecreatevg() Line 619: if not self.environment[ Line 620: ohostedcons.StorageEnv.FORCE_CREATEVG Line 621: ]: Line 622: raise RuntimeError(dom['status']['message']) Why do you need to _customize the force flag at this stage? And why _customize_forcecreatevg return the value through the environment? I understand that you like to enable setting force externally - this is nice touch. But if we did set force externally, then we will never reach this code, since we fail if create failed *with* the force flag. So here we do not have force flag in the environment, and we like to interact with the user. We can show the dialog - which should be little different from the dialog for "used" devices, and return a boolean: if not self._should_force_create_vg(): raise # User canceled self._create_vg(force=True) No special error handling needed since _create_vg raises on errors. Line 623: else: Line 624: forceVG = True Line 625: dom = self._create_vg(forceVG) Line 626: self.logger.debug(dom) Line 620: ohostedcons.StorageEnv.FORCE_CREATEVG Line 621: ]: Line 622: raise RuntimeError(dom['status']['message']) Line 623: else: Line 624: forceVG = True Setting the temporary just to call the function is pointless. Instead, always call the function with kwarg style: force = self.environment[force_key] self._create_vg(force=force) ... self._create_vg(force=True) Line 625: dom = self._create_vg(forceVG) Line 626: self.logger.debug(dom) Line 627: if dom['status']['code'] != 0: Line 628: self.logger.error( Line 630: 'Error creating Volume Group: {message}' Line 631: ).format( Line 632: message=dom['status']['message'] Line 633: ) Line 634: ) You duplicate here not only the [status][code] but also the logging code. Why do you need to log when you raise an exception? This is common anti-pattern - if you raise, you don't need to log anyting - put the context in the error. If you log, do not raise. Line 635: raise RuntimeError(dom['status']['message']) Line 636: else: Line 637: raise RuntimeError(dom['status']['message']) Line 638: self.environment[ Line 633: ) Line 634: ) Line 635: raise RuntimeError(dom['status']['message']) Line 636: else: Line 637: raise RuntimeError(dom['status']['message']) Last, you mix words_with_underscore (_create_vg), joinedwords (_custimize_forcecreatevg) and mixedCase (forceVG). Please decide on one style and be consistent. PEP8 recommend words_with_underscore for variables and function names, and CamelCase for classes. Line 638: self.environment[ Line 639: ohostedcons.StorageEnv.VG_UUID Line 640: ] = dom['uuid'] Line 641: -- To view, visit http://gerrit.ovirt.org/37845 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I496c34e6b9f0d84443a8d5bc68d77916be6cb504 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@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-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
