Simone Tiraboschi has posted comments on this change. Change subject: vdsm: use vdscli instead of vdsClient for storage operations ......................................................................
Patch Set 3: (5 comments) 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 6 Done 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? Yes it is, on this patch I'm just focusing on storage related commands. Than I'll add a subsequent patch to remove vdsCLient at all. I'd simply prefer to focus on smaller and better testable patches. 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?! In this patch I'm simply removing vdsClient for the storage operations. I'm still using it for other operations but I'm going to remove it soon. 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 l Done 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. Sorry, I don't understand. 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 <[email protected]> Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
