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

abaron at redhat.com abaron at redhat.com
Thu May 2 19:45:22 UTC 2013


Ayal Baron has posted comments on this change.

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


Patch Set 6: (13 inline comments)

....................................................
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'] = []
shouldn't this conf change be under the lock as well? (I didn't have the patience to check who calls this method and if it happens before the vm is fully initialized etc)
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 []
not related, but this line is funky (and racy).  just:
confDrives = self.conf.get('drives', [])
?
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']
shouldn't this conf change be under the lock as well?
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']
and this
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
and here
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'] = ''
and here
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
same.

I'm assuming that you didn't change these since they don't call saveState, but they can change while saveState is running and it assumes that the content of conf doesn't change which would not be true.
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']
...
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'
...
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
...
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"
...
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
...
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
and here
(starting to sound like 'and then' from 'dude where's my car)
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