Change in vdsm[master]: vm: enable disk stats collection on recovery

fromani at redhat.com fromani at redhat.com
Thu Sep 4 10:12:41 UTC 2014


Francesco Romani 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()
> But note that in prepare path, we prepare the volumes, and startDiskStatsCo
Good point. I was under the impression something deeper was lurking behind, and turns out is exactly like that.

I was aware of the startDisksStatsCollection being a liar, and about the flag, but definitely not about the irs thing, thanks for the insight.

The impact of the current behaviour is:

* new VMs will enable disk stats independently, so this affects only recovered VMs.
* succesfull live snapshot commands will eventually re-enable stats gathering, so to do a snapshot
  on a recovered VM should incidentally fix this issue
* same for the disk replication (more specifically, diskReplicateFinish).
* the disk stats reporting becomes broken, so engine is fed with stale data or no data.
  The effects depend on how the user consumes this data.
* last, the big one: highWrite, thus the high water checks, becomes a NOP if disk
  stats are stopped. Not good.

I think there is a reasonably clean and correct way forward, which is the following

* since we don't have callbacks, we have to resort to polling. But stats already does
  polling; we want to make this smarter and more efficient, but we're not going to change that.
* since as you said IRS reports when it's ready, it may be sufficent to replace this check

  def _highWrite(self):
      if not self._vm.isDisksStatsCollectionEnabled():
           # Avoid queries from storage during recovery process
           return
      # ...

  with something which takes in account the IRS readyness flag. Something like (pseudocode)

  def _highWrite(self):
      disk_stats_enabled = False
      # local caching to avoid to access irs every time from different threads and avoid
      # contention, will be dropped if this is no issue
      if not disk_stats_enabled:
          disk_stats_enabled = self._vm.isDiskStatsCollectionEnabled() and self._vm.cif.irs.ready
      # ... (no other changes)

This way, all the disk samplings will automatically reenable themselves as soon as possible,
leveraging existing informations.

I believe this could be a good fix not only for the short term. What do you think?
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 <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list