Change in vdsm[master]: libvirtvm: avoid concurrent VM changes during saveState

fsimonce at redhat.com fsimonce at redhat.com
Fri May 3 15:59:23 UTC 2013


Federico Simoncelli has posted comments on this change.

Change subject: libvirtvm: avoid concurrent VM changes during saveState
......................................................................


Patch Set 6: (17 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1865:                     self.conf['devices'].remove(dev)
Line 1866:                 diskDev = dev
Line 1867:                 break
Line 1868: 
Line 1869:         self.saveState()
This always removes one disk only.

That said, what you're pointing out is valid issue but it's out of the scope of this patch. To solve it we need a two-phase commit, in fact for every action (hotplug/unplug of disks/networks, live snapshots, etc) if vdsm crashes in between the saved state might be out of sync with the libvirt/qemu state.

E.g. in this case if we crash between line 1869 and line 1874 the disk won't be in the configuration anymore (no stats/extensions, etc...) but it's still attached to the vm (in libvirt/qemu).
Line 1870: 
Line 1871:         hooks.before_disk_hotunplug(driveXml, self.conf,
Line 1872:                                     params=params.get('custom', {}))
Line 1873:         try:


Line 1881:                 with self._confLock:
Line 1882:                     self.conf['devices'].append(diskDev)
Line 1883:             if drive:
Line 1884:                 self._devices[vm.DISK_DEVICES].append(drive)
Line 1885:             self.saveState()
Done
Line 1886:             return {
Line 1887:                 'status': {'code': errCode['hotunplugDisk']['status']['code'],
Line 1888:                            'message': e.message}}
Line 1889:         else:


Line 2310:         else:
Line 2311:             raise LookupError("No such drive: '%s'" % srcDrive.name)
Line 2312: 
Line 2313:         srcDrive.diskReplicate = dstDisk
Line 2314:         self.saveState()
Done
Line 2315: 
Line 2316:     def _delDiskReplica(self, srcDrive):
Line 2317:         """
Line 2318:         This utility method is the inverse of _setDiskReplica, look at the


Line 2327:         else:
Line 2328:             raise LookupError("No such drive: '%s'" % srcDrive.name)
Line 2329: 
Line 2330:         del srcDrive.diskReplicate
Line 2331:         self.saveState()
Done
Line 2332: 
Line 2333:     def diskReplicateStart(self, srcDisk, dstDisk):
Line 2334:         try:
Line 2335:             srcDrive = self._findDriveByUUIDs(srcDisk)


....................................................
File vdsm/vm.py
Line 480:         # by existence/absence of the 'devices' key
Line 481:         devices = {}
Line 482:         # Build devices structure
Line 483:         if self.conf.get('devices') is None:
Line 484:             self.conf['devices'] = []
this happens before the vm is initialized... but since we're at it let's not be cheap.
Line 485:             devices[DISK_DEVICES] = self.getConfDrives()
Line 486:             devices[NIC_DEVICES] = self.getConfNetworkInterfaces()
Line 487:             devices[SOUND_DEVICES] = self.getConfSound()
Line 488:             devices[VIDEO_DEVICES] = self.getConfVideo()


Line 614:         """
Line 615:         # FIXME
Line 616:         # Will be better to change the self.conf but this implies an API change
Line 617:         # Remove this when the API parameters will be consistent.
Line 618:         confDrives = self.conf['drives'] if self.conf.get('drives') else []
Done
Line 619:         if not confDrives:
Line 620:             confDrives.extend(self.__legacyDrives())
Line 621:         confDrives.extend(self.__removableDrives())
Line 622: 


Line 703:                 if self._initTimePauseCode == 'ENOSPC':
Line 704:                     self.cont()
Line 705:             else:
Line 706:                 try:
Line 707:                     del self.conf['pauseCode']
Done
Line 708:                 except:
Line 709:                     pass
Line 710: 
Line 711:             if 'recover' in self.conf:


Line 708:                 except:
Line 709:                     pass
Line 710: 
Line 711:             if 'recover' in self.conf:
Line 712:                 del self.conf['recover']
Done
Line 713:             self.saveState()
Line 714:         except Exception as e:
Line 715:             if 'recover' in self.conf:
Line 716:                 self.log.info("Skipping errors on recovery", exc_info=True)


Line 820:         self.user_destroy = True
Line 821: 
Line 822:     def onConnect(self, clientIp=''):
Line 823:         if clientIp:
Line 824:             self.conf['clientIp'] = clientIp
unrelated, it just changes the value of clientIp (dictionary size unchanged)
Line 825: 
Line 826:     def _timedDesktopLock(self):
Line 827:         if not self.conf.get('clientIp', ''):
Line 828:             self.guestAgent.desktopLock()


Line 827:         if not self.conf.get('clientIp', ''):
Line 828:             self.guestAgent.desktopLock()
Line 829: 
Line 830:     def onDisconnect(self, detail=None):
Line 831:         self.conf['clientIp'] = ''
unrelated, it just changes the value of clientIp (dictionary size unchanged)
Line 832:         # This is a hack to mitigate the issue of spice-gtk not respecting the
Line 833:         # configured secure channels. Spice-gtk is always connecting first to
Line 834:         # a non-secure channel and the server tells the client then to connect
Line 835:         # to a secure channel. However as a result of this we're getting events


Line 846:         timer.start()
Line 847: 
Line 848:     def _rtcUpdate(self, timeOffset):
Line 849:         self.log.debug('new rtc offset %s', timeOffset)
Line 850:         self.conf['timeOffset'] = timeOffset
no, these ones didn't stood out because they seemed value changes (but 'timeOffset' might be uninitialized) anyway I mostly focused on flows that were doing length modifications to the dictionary (add/remove devices, etc.). After three weeks when the patch got reviewed I just revised the parts that I was already modifying without doing any extra check.
Line 851: 
Line 852:     def extendDriveVolume(self, vmDrive):
Line 853:         if not vmDrive.blockDev:
Line 854:             return


Line 982:             if hasattr(self, 'updateGuestCpuRunning'):
Line 983:                 self.updateGuestCpuRunning()
Line 984:             self._lastStatus = afterState
Line 985:             try:
Line 986:                 del self.conf['pauseCode']
Done
Line 987:             except:
Line 988:                 pass
Line 989:             return {'status': doneCode, 'output': ['']}
Line 990:         finally:


Line 994:     def pause(self, afterState='Paused', guestCpuLocked=False):
Line 995:         if not guestCpuLocked:
Line 996:             self._acquireCpuLockWithTimeout()
Line 997:         try:
Line 998:             self.conf['pauseCode'] = 'NOERR'
Done
Line 999:             self._underlyingPause()
Line 1000:             if hasattr(self, 'updateGuestCpuRunning'):
Line 1001:                 self.updateGuestCpuRunning()
Line 1002:             self._lastStatus = afterState


Line 1092: 
Line 1093:     def setDownStatus(self, code, reason):
Line 1094:         try:
Line 1095:             self.lastStatus = 'Down'
Line 1096:             self.conf['exitCode'] = code
Done
Line 1097:             if 'restoreState' in self.conf:
Line 1098:                 self.conf['exitMessage'] = "Wake up from hibernation failed"
Line 1099:             else:
Line 1100:                 self.conf['exitMessage'] = reason


Line 1094:         try:
Line 1095:             self.lastStatus = 'Down'
Line 1096:             self.conf['exitCode'] = code
Line 1097:             if 'restoreState' in self.conf:
Line 1098:                 self.conf['exitMessage'] = "Wake up from hibernation failed"
Done
Line 1099:             else:
Line 1100:                 self.conf['exitMessage'] = reason
Line 1101:             self.log.debug("Changed state to Down: " + reason)
Line 1102:         except DoubleDownError:


Line 1096:             self.conf['exitCode'] = code
Line 1097:             if 'restoreState' in self.conf:
Line 1098:                 self.conf['exitMessage'] = "Wake up from hibernation failed"
Line 1099:             else:
Line 1100:                 self.conf['exitMessage'] = reason
Done
Line 1101:             self.log.debug("Changed state to Down: " + reason)
Line 1102:         except DoubleDownError:
Line 1103:             pass
Line 1104:         try:


Line 1112:         self.saveState()
Line 1113: 
Line 1114:     def status(self):
Line 1115:         # used by API.Global.getVMList
Line 1116:         self.conf['status'] = self.lastStatus
I'll watch the movie during the weekend. 'status' is just a value change.
Line 1117:         return self.conf
Line 1118: 
Line 1119:     def getStats(self):
Line 1120:         # used by API.Vm.getStats


--
To view, visit http://gerrit.ovirt.org/13624
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08b4073f5e3f5184baa5f1c7dd3ec5a148ff60b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dafna Ron <dron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list