Nir Soffer has posted comments on this change. Change subject: vdsm: use vdscli instead of vdsClient for storage operations ......................................................................
Patch Set 3: Code-Review-1 (5 comments) Please stop using vdsClient - it is not a library and should not be used by other code. http://gerrit.ovirt.org/#/c/36798/3//COMMIT_MSG Commit Message: Line 5: CommitDate: 2015-02-02 19:56:58 +0100 Line 6: Line 7: vdsm: use vdscli instead of vdsClient for storage operations Line 8: Line 9: Code refactor dropping vdsClient usage in favor of This is not a refactoring, as you changed the behavior by eliminating the 60 seconds timeout in vdsClient. And the timeout is not the only problem - vdsClient is not a libary and should not be used in the way it was used. Line 10: vdscli API for storage operations. Line 11: vdsClients includes a non modificable 60 secs timeout Line 12: with could not be enough on long storage operations. Line 13: http://gerrit.ovirt.org/#/c/36798/3/src/ovirt_hosted_engine_setup/constants.py File src/ovirt_hosted_engine_setup/constants.py: Line 686: VDSMD_SERVICE = 'OVEHOSTED_VDSM/serviceName' Line 687: VDSM_UID = 'OVEHOSTED_VDSM/vdsmUid' Line 688: KVM_GID = 'OVEHOSTED_VDSM/kvmGid' Line 689: VDS_CLI = 'OVEHOSTED_VDSM/vdscli' Line 690: VDS_CLIENT = 'OVEHOSTED_VDSM/vdsClient' Is this used? Line 691: Line 692: @ohostedattrs( Line 693: answerfile=True, Line 694: ) http://gerrit.ovirt.org/#/c/36798/3/src/ovirt_hosted_engine_setup/mixins.py File src/ovirt_hosted_engine_setup/mixins.py: Line 60: Line 61: def _generateUserMessage(self, console_type): Line 62: displayPort = 5900 Line 63: displaySecurePort = 5901 Line 64: serv = self.environment[ohostedcons.VDSMEnv.VDS_CLIENT] Why are you still using vdsClient?! Line 65: try: Line 66: stats = serv.s.getVmStats( Line 67: self.environment[ohostedcons.VMEnv.VM_UUID] Line 68: ) http://gerrit.ovirt.org/#/c/36798/3/src/plugins/ovirt-hosted-engine-setup/storage/storage.py File src/plugins/ovirt-hosted-engine-setup/storage/storage.py: Line 526: poolName = self.environment[ Line 527: ohostedcons.StorageEnv.STORAGE_DATACENTER_NAME Line 528: ] Line 529: masterDom = sdUUID Line 530: domList = [sdUUID, ] There is no need for trailing "," for one item list. It make sens only in list formatted one item per line. Use [sdUUID] Line 531: mVer = 1 Line 532: status = self.cli.createStoragePool( Line 533: poolType, Line 534: spUUID, Line 718: ] = self.dialog.queryString( Line 719: name='OVEHOSTED_STORAGE_DATACENTER_NAME', Line 720: note=_( Line 721: 'Local storage datacenter name is an internal name ' Line 722: 'and currently will not be shown in engine\'s admin UI.\n' Is this related? Better do this in a separate patch. Line 723: 'Please enter local datacenter name [@DEFAULT@]: ' Line 724: ), Line 725: prompt=True, Line 726: caseSensitive=True, -- To view, visit http://gerrit.ovirt.org/36798 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iec7db5ccf55752266fa668a8b1e8f914cd632521 Gerrit-PatchSet: 3 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
