Change in vdsm[master]: Apply storage QoS on running VM

fromani at redhat.com fromani at redhat.com
Mon Feb 29 16:30:44 UTC 2016


Francesco Romani has posted comments on this change.

Change subject: Apply storage QoS on running VM
......................................................................


Patch Set 14: Code-Review-1

(3 comments)

Mostly OK, but please check iniline comments, especially about IDs.
-1 for visiblity.

https://gerrit.ovirt.org/#/c/52746/14//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: Apply storage QoS on running VM
Line 8: 
Line 9: Modify updateVmPolicy command to accept disk ids and not its path.
Line 10: Add functions to access io tune info from MoM.
Could you please explain why those two changes (modify updateVolume and add functions in MOM) need to stay in the same patch?
I think it is better to have each one in its own patch, of course in one series.
Line 11: 
Line 12: Change-Id: I3f5fcb8705c974f1d91c90cdb3158d2e8dd314f8
Line 13: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1201482


https://gerrit.ovirt.org/#/c/52746/14/lib/api/vdsmapi-schema.json
File lib/api/vdsmapi-schema.json:

Line 2583: # @poolID:      #optional PoolId
Line 2584: #
Line 2585: # @imageID:     #optional ImageId
Line 2586: #
Line 2587: # @volumeID:    #optional VolumeId
this is OK since AFAIK we don't have a way in the schema to express that one *group* of parameters is optional. But we can maybe add a comment about it (see also comments in vm.py).
Line 2588: #
Line 2589: # @guaranteed:  IO tune parameters - guaranteed values
Line 2590: #
Line 2591: # @maximum:     IO tune parameters - the hard limits


https://gerrit.ovirt.org/#/c/52746/14/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2474:                     disk = self._findDriveByUUIDs({
Line 2475:                         'domainID': ioTune.get("domainID"),
Line 2476:                         'poolID': ioTune.get("poolID"),
Line 2477:                         'imageID': ioTune.get("imageID"),
Line 2478:                         'volumeID': ioTune.get("volumeID")})
ok, I think I made my mind: we should fail loudly if we don't have all of those fours IDs, so please make sure we do.
Unless I missed a valid reason why one or two of them could be missing and we could get away with that. In this case, please add a comment around line 2473 to remind us why this happens.
Line 2479: 
Line 2480:                     self.log.debug("Device path: %s", disk.path)
Line 2481:                     ioTune["name"] = disk.name
Line 2482:                     ioTune["path"] = disk.path


-- 
To view, visit https://gerrit.ovirt.org/52746
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f5fcb8705c974f1d91c90cdb3158d2e8dd314f8
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Andrej Krejcir <akrejcir at redhat.com>
Gerrit-Reviewer: Andrej Krejcir <akrejcir at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: Roy Golan <rgolan at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list