WIP test_host_evacuate: check for IP in xenstore first #182
Conversation
| @@ -34,6 +34,7 @@ def _host_evacuate_test(source_host, dest_host, network_uuid, vm, expect_error=F | |||
| source_host.xe('host-evacuate', args) | |||
| wait_for(lambda: vm.all_vdis_on_host(dest_host), "Wait for all VDIs on destination host") | |||
| wait_for(lambda: vm.is_running_on_host(dest_host), "Wait for VM to be running on destination host") | |||
| vm.host = dest_host | |||
There was a problem hiding this comment.
it is not related to wait for xenstore VM IP right? It looks like a bug fix when evacuation is done on the destination host currently self.host is not updated no? If it is the case I think that it is a fix that should be done in its own PR.
There was a problem hiding this comment.
It is indeed required so the upcoming check for VM IP runs xenstore commands on the destination host, although it is a workaround that can likely be used for other things. Could be moved in a separate commit for more clarity.
There was a problem hiding this comment.
perhaps a vm.resident_on would be better.
There was a problem hiding this comment.
Agreed with @benjamreis here: as I said earlier, vm.host really should be vm.pool and will probably be in the future. It is not where the VM is running, it's the pool master of the pool to which the VM belongs, and is badly named.
You need to use the VM.get_residence_host method in VM.try_get_ip_xenstore.
There was a problem hiding this comment.
so we're indeed evacuating the host into another pool?
There was a problem hiding this comment.
Agreed with @benjamreis here: as I said earlier, vm.host really should be vm.pool and will probably be in the future. It is not where the VM is running, it's the pool master of the pool to which the VM belongs, and is badly named.
And in the test we are sure that we migrate a VM from a slave to the master (or another slave) during the evacuate? Because if the evacuation is done from the master than we need to update vm.host in this case. Or we are sure that it cannot happens.
There was a problem hiding this comment.
Oh wait, in fact no because evacuate from master doesn't mean that we need to change the master... maybe.
There was a problem hiding this comment.
What I mean is VM.host is not where the VM is currently executed. There is no guarantee about this, and it usually just is the master host, regardless of where the VM is (note also that the concept of residence for a VM on a host doesn't mean anything for a VM which is turned off and is on a shared SR. When you start it from XAPI, any host of the pool can become temporarily where it resides). And thus we shouldn't change the value in the test, and should use VM.get_residence_host in order to find which host's xenstore we need to check.
so we're indeed evacuating the host into another pool?
No, host evacuate is intra-pool and doesn't involve storage migration (that's why the VM disks being stored on a shared SR is a prerequisite).
lib/vm.py
Outdated
| if result.returncode != 0 or result.stdout.startswith('169.254.'): | ||
| return False | ||
| else: | ||
| logging.info("Xenstore VM IP: %s" % result.stdout) |
There was a problem hiding this comment.
stdout must be stripped, because it ends with a newline character.
ydirson
force-pushed
the
evac-check-xenstore
branch
from
3a753c9
to
e9c120b
Compare
This can show when after migration an IP address has been published to Xenstore, but XAPI misses it nevertheless.
ydirson
force-pushed
the
evac-check-xenstore
branch
from
e9c120b
to
5895768
Compare
|
I see there were pushes on this PR. Do you want a re-review? |
IPv6 can be found in xenstore under |
This can show when after migration an IP address has been published to Xenstore, but XAPI misses it nevertheless.