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