Nir Soffer has posted comments on this change. Change subject: vm: enable disk stats collection on recovery ......................................................................
Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/32406/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2895: else: Line 2896: for drive in devices[DISK_DEVICES]: Line 2897: if drive['device'] == 'disk' and isVdsmImage(drive): Line 2898: self.sdIds.append(drive['domainID']) Line 2899: self.startDisksStatsCollection() > You are just right. This code is already too messy, let's take this chance But note that in prepare path, we prepare the volumes, and startDiskStatsCollection is actually a lie - it just set set._volumesPrepared to True, it does not stat any disk stats collections :-) In the recovery path, we don't prepare any disk, and actually we cannot prepare them since when recovery is done, irs may not be ready yet. So calling self.startDisksStatsCollection() here is wrong. I think the real fix would be: - when recovery is done, set a flag in cif - It think this already exists - when both irs is ready, and recovery is done, we may need to check if all volumes are prepared, and prepare volumes which are not. - when all volumes are prepared, we can start disk stats collection The problem is we don't know when irs becomes ready, because we do not have any callback or any other way to get that event. When I added the irs.ready property, I suggested to have a callback, which make all of this easy, but people did not like the callback. How this effects the operation of the system? how this issue affect the user experience? When do we start disk stat collection after recovery - when starting a new vm or hot plugging a device? Line 2900: Line 2901: for devType, devClass in self.DeviceMapping: Line 2902: for dev in devices[devType]: Line 2903: self._devices[devType].append(devClass(self.conf, self.log, -- To view, visit http://gerrit.ovirt.org/32406 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd7d32373f5a0a7dd4ef88c5a0ad5a9f3d52938c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
