Nir Soffer has posted comments on this change. Change subject: packaging: setup: asking about retriyng VG creation with force option ......................................................................
Patch Set 1: (5 comments) The goal is fine, but the way you are trying to do this is wrong. You assume that all failures to create a vg are related to vg being used, which requires the force flag to create a vg. I think that the force flag is required only if the vg is used, and the flow should be this: - Check if vg is used or free (see output of getDeviceList) - If the vg is used, warn the user before creating the vg: This LUN is used by vg xxxyyy Creating a vg on this LUN will destroy the current data on the LUN. Destroy data on this LUN? [Destroy|Cancel]: - If the user chose to destroy data, use the force flag This is basically what we use in the engine when you create a vg. Adding Daniel who knows better the engine side. http://gerrit.ovirt.org/#/c/37845/1/src/plugins/ovirt-hosted-engine-setup/storage/blockd.py File src/plugins/ovirt-hosted-engine-setup/storage/blockd.py: Line 553 Line 554 Line 555 Line 556 Line 557 For list of one item, we don't need this indentation or trailing "," - it makes sense only for list with multiple items that is likely to change in the future. Line 567: ohostedcons.StorageEnv.PROMPT_RETRY_CREATEVG Line 568: ] is None: Line 569: self.environment[ Line 570: ohostedcons.StorageEnv.PROMPT_RETRY_CREATEVG Line 571: ] = self.dialog.queryString( Why do you need to keep the result of the dialog in the environment? This should be temporary value related to the current attempt to create a vg. Line 572: name='OVEHOSTED_RETRY_CREATEVG', Line 573: note=_( Line 574: 'Error creating Volume Group: {message}\n' Line 575: 'The selected device was probably dirty.\n\n' Line 586: default=_('Retry'), Line 587: ).lower() == _('Retry').lower() Line 588: if self.environment[ Line 589: ohostedcons.StorageEnv.PROMPT_RETRY_CREATEVG Line 590: ]: Using the environment for temporary variable makes this code hard to follow. Line 591: dom = self.cli.createVG( Line 592: self.environment[ohostedcons.StorageEnv.SD_UUID], Line 593: [ Line 594: self.environment[ohostedcons.StorageEnv.LUN_ID], Line 593: [ Line 594: self.environment[ohostedcons.StorageEnv.LUN_ID], Line 595: ], Line 596: force=True, Line 597: ) This repeats the same code above - should move to helper function. Line 598: self.logger.debug(dom) Line 599: if dom['status']['code'] != 0: Line 600: raise RuntimeError(dom['status']['message']) Line 601: else: Line 598: self.logger.debug(dom) Line 599: if dom['status']['code'] != 0: Line 600: raise RuntimeError(dom['status']['message']) Line 601: else: Line 602: raise RuntimeError(dom['status']['message']) The code is very hard to follow. This make it hard to review and hard to maintain. I don't know why you are trying to write the code in one long unreadable expression, without using temporary variables. When you have long piece of code, using a temporary makes the intent much more clear: result = self.dialog.queryString(...) if result == "Retry": # retry... You should refactor this so the high level flow is clear, moving the uninteresting details into helpers: try: self.create_vg(force=False) except CreateVGError: if self.should_force_create_vg(): self.create_vg(force=True) else: raise CannotInstall("why...") Line 603: self.environment[ Line 604: ohostedcons.StorageEnv.VG_UUID Line 605: ] = dom['uuid'] Line 606: -- 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: 1 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@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
