GNOME Bugzilla – Bug 744098
test updates for latest libvirt
Last modified: 2016-03-31 13:22:07 UTC
container for patches
Created attachment 296277 [details] [review] tests: Load IP from virsh directly Default.leases file is not present anymore on F22 so we got IPs from output of virsh net-dhcp-leases.
Review of attachment 296277 [details] [review]: Good apart from some nitpicks * F22 -> Fedora 22 (you are upstream now :)) but I think mentioning specific OS is not needed here. Its latest libvirt that has dropped this file. * Lets mention the actual filename with full path: /var/lib/libvirt/dnsmasq/default.leases. * Lets enclose commandlines with spaces in `` or ''. i-e 'virsh net-dhcp-leases'. ::: tests/steps/general.py @@ +121,3 @@ + if t > time: + time = t + ips = re.findall( r'[0-9]+(?:\.[0-9]+){3}', line) redundant space before 'r' above. @@ +124,3 @@ + + if ips: + ip = ips[0] We are assuming first IP is the target one. Since this issue is not introduced by this patch, lets fix that after this patch.
Created attachment 296564 [details] [review] tests: Load IP from virsh directly Libvirt project removed /var/lib/libvirt/dnsmasq/default.leases file so we need to read IP from output of 'virsh net-dhcp-leases' command.
(In reply to Zeeshan Ali (Khattak) from comment #2) > Review of attachment 296277 [details] [review] [review]: > > Good apart from some nitpicks > > * F22 -> Fedora 22 (you are upstream now :)) but I think mentioning specific > OS is not needed here. Its latest libvirt that has dropped this file. > * Lets mention the actual filename with full path: > /var/lib/libvirt/dnsmasq/default.leases. > * Lets enclose commandlines with spaces in `` or ''. i-e 'virsh > net-dhcp-leases'. > altered > ::: tests/steps/general.py > @@ +121,3 @@ > + if t > time: > + time = t > + ips = re.findall( r'[0-9]+(?:\.[0-9]+){3}', line) > > redundant space before 'r' above. > removed > @@ +124,3 @@ > + > + if ips: > + ip = ips[0] > > We are assuming first IP is the target one. Since this issue is not > introduced by this patch, lets fix that after this patch. Not taking the first IP. I am taking first IP occurrence (as there is the only one) from the newest line of virsh output (time compared). This has some logic around dealing with previous machines, etc. Working more reliably then the top one before.
(In reply to vladimir benes from comment #4) Reminder: please use 'Review' link for replying to inline comments. > > @@ +124,3 @@ > > + > > + if ips: > > + ip = ips[0] > > > > We are assuming first IP is the target one. Since this issue is not > > introduced by this patch, lets fix that after this patch. > > Not taking the first IP. I am taking first IP occurrence (as there is the > only one) from the newest line of virsh output (time compared). This has > some logic around dealing with previous machines, etc. Working more reliably > then the top one before. But its still not checking if the IP is of the right machine or not. It works reliably now but without this check, its likely to break at some point with different test cases.
Review of attachment 296564 [details] [review]: Looks good.
Attachment 296564 [details] pushed as e55a1f7 - tests: Load IP from virsh directly
(In reply to Zeeshan Ali (Khattak) from comment #5) > But its still not checking if the IP is of the right machine or not. It > works reliably now but without this check, its likely to break at some point > with different test cases. It finds the latest and tries to ping it. If the latest is already stored (some machine was installed before in the same test) or is unpingable it waits for new one. Finally times out when machine not reachable. Could you tell me what test case can you think about this wouldn't work? I can rewrite it of course a bit. Is it possible to match via mac from the same line and machine's xml dump?
(In reply to vladimir benes from comment #8) > (In reply to Zeeshan Ali (Khattak) from comment #5) > > > But its still not checking if the IP is of the right machine or not. It > > works reliably now but without this check, its likely to break at some point > > with different test cases. > > It finds the latest and tries to ping it. If the latest is already stored > (some machine was installed before in the same test) or is unpingable it > waits for new one. Finally times out when machine not reachable. Could you > tell me what test case can you think about this wouldn't work? I can rewrite > it of course a bit. Its less about the tests but more about us assuming how libvirt handles this internally (The timing of it). We have test cases already where we have multiple machines running so being the first pingable IP doesn't necessarily mean its of the machine we are interested in. > Is it possible to match via mac from the same line and machine's xml dump? Yeah, thats what I had in mind. Get mac from config XML and get the IP against it.