Zhou Zheng Sheng has posted comments on this change.
Change subject: Refactored xmlrpcTests to allow for more complex tests using
running VM
......................................................................
Patch Set 3: Code-Review-1
(5 comments)
Nice patch, you make the generated initramfs cached. -1 for a problems in line
106 and 111.
....................................................
File tests/functional/xmlrpcTests.py
Line 102: global _initramfsPath
Line 103: if _initramfsPath is not None:
Line 104: self._initramfsPath = _initramfsPath
Line 105: else:
Line 106: if self._missingBootImages:
It should call the method.
if self._missingBootImages():
Line 107: _initramfsPath = self._genInitramfs()
Line 108: self._initramfsPath = _initramfsPath
Line 109: else:
Line 110: self._initramfsPath = ("/boot/initramfs-"
Line 107: _initramfsPath = self._genInitramfs()
Line 108: self._initramfsPath = _initramfsPath
Line 109: else:
Line 110: self._initramfsPath = ("/boot/initramfs-"
Line 111: "%s.img" % self._kernelVer)
Actually self._missingBootImages checks if self._initramfsPath is an existing
file, but in the first run self._initramfsPath is not defined at all. I think
this piece should be like this.
self._initramfsPath = ("/boot/initramfs-"
"%s.img" % self._kernelVer)
if self._missingBootImages():
_initramfsPath = self._genInitramfs()
self._initramfsPath = _initramfsPath
Line 112:
Line 113: self._template = {'vmId':
'11111111-abcd-2222-ffff-333333333333',
Line 114: 'vmName': 'vdsmKernelBootVM',
Line 115: 'display': 'vnc',
Line 178:
Line 179: def assertGuestUp(self, vmid):
Line 180: r = self.s.getVmStats(vmid)
Line 181: self.assertVdsOK(r)
Line 182: self.myAssertIn(r['statsList'][0]['status'], self.UPSTATES)
I do not see the difference between assertVmUp() and assertGuestUp().
In the original code, the difference is in the up state. When a VM is started
successfully, it can be in ('Powering up', 'Running'). If the guest agent
report it is started, the VM is marked in 'Up' state. In this case there is no
guest agent, so after 60 seconds, VDSM changes the VM state to 'Up' as well,
and assumes the guest OS is up. I think assertGuestUp() at least can tell us
the VM survives for some time, so I let it check if the VM state is 'Up'.
That's why in original code we have
self.retryAssert(assertVMAndGuestUp, timeout=65)
65 seconds leave 5 second margin for it to detect the 'Up' state.
If you think we wait for too long, I have a suggestion. In assertGuestUp(), we
can examine the VM up time as well as the VM state. assertGuestUp() succeeds
when VM is in self.UPSTATES for 20 seconds.
When a VM is booting, it's in 'Powering Up'. In your code, the "Powering Up" VM
passes assertVmUp() and assertGuestUp() in a flash, then it is tore down. I
think we'd better let the VM run for a while, because there is code for
monitoring VM and VM storage in VDSM.
Line 183:
Line 184: def assertVMAndGuestUp(self, vmid):
Line 185: self.assertVmUp(vmid)
Line 186: self.assertGuestUp(vmid)
Line 236: self._waitForStartup(vm)
Line 237:
Line 238: @skipNoKVM
Line 239: @permutations([['hotplugNic'], ['nic']])
Line 240: def testVmWithDevice(self, devices):
It seem you want devices to be a list/set/touple, because I see code like 'if
nic in devices'. The correct and simple style to do this should be like this.
@permutations([["devA", "devB"], ['devC']])
def testVmWithDevice(self, *devices):
print devices
return
or like this
@permutations([[("devA", "devB")], [('devC',)]])
def testVmWithDevice(self, devices):
print devices
return
You can try it with
./run_tests.sh -s -m 'testVmWithDevice' functional/xmlrpcTests.py
to see what devices is.
From what I can explain, @permutations works like this:
permA = [1, 2] # or (1, 2)
permB = [3, 4] # or (3, 4)
@permutations([permA, permB])
def foo(self, x, y)
print x, y
It prints
XMLRPCTest
foo(x=1, y=2) 1 2
OK
foo(x=3, y=4) 3 4
OK
so if you want devices to be a list, you have to make a list in list in list as
@permutations argument...
Line 241: customization = {'vmId':
'77777777-ffff-3333-bbbb-222222222222',
Line 242: 'vmName': 'testVm', 'devices': []}
Line 243:
Line 244: if 'nic' in devices:
Line 238: @skipNoKVM
Line 239: @permutations([['hotplugNic'], ['nic']])
Line 240: def testVmWithDevice(self, devices):
Line 241: customization = {'vmId':
'77777777-ffff-3333-bbbb-222222222222',
Line 242: 'vmName': 'testVm', 'devices': []}
The device definition dict scatters everywhere in this method. It seems you
have copied the nic dict and pic address. I have an idea that we can have a big
dict to store pre-defined device arguments, as follow.
pciAddress = {'slot':...}
deviceDef = {'virtioNic': {'nicModel': 'virtio', 'address': pciAddress, ...},
'smartcard': {..., 'address': pciAddress, ...},
...}
Then you can do
for deviceName in devices:
customization['devices'].append(deviceDef[deviceName])
with RunningVm(self, customization) as vm:
if 'hotplugNic' in devices:
nic = {'vmId': vm, 'nic': deviceDef['virtioNic']}
# ... hotplugNic(nic) then hotunplugNic(nic)
Line 243:
Line 244: if 'nic' in devices:
Line 245: pciAddress = {'slot': '0x03', 'bus': '0x00', 'domain':
'0x0000',
Line 246: 'function': '0x0', 'type': 'pci'}
--
To view, visit http://gerrit.ovirt.org/17893
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I683c9e056137e7a189815bf6be2eb79ee80994cf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Martin Polednik <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Peter V. Saveliev <[email protected]>
Gerrit-Reviewer: Petr Šebek <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
