| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-01-11 | |||
| 14:32:49 | sean-k-mooney | and pull out the id | |
| 14:33:10 | bauzas | ok I can try to fix this | |
| 14:33:38 | bauzas | anyway, good catch | |
| 14:33:50 | sean-k-mooney | ya its still just one call to neutron either gettign the segment by network or if it has an ip geting the segment for the current ip | |
| 14:35:29 | sean-k-mooney | bauzas: fyi the subnet in the port is in port[fixed_ips][0]['subnet_id'] | |
| 14:35:47 | sean-k-mooney | well port['fixed_ips'][0]['subnet_id'] for got the '' | |
| 14:36:18 | bauzas | yeah | |
| 14:36:25 | bauzas | I'm literrally hands-on :) | |
| 14:36:38 | bauzas | did a git stash for the UT work I was doing | |
| 14:36:52 | bauzas | and I'll have to change a few things | |
| 14:37:05 | bauzas | fortunately, you pinged me before I went too far | |
| 14:37:58 | bauzas | behavioural-driven-development, that is. | |
| 14:42:52 | bauzas | sean-k-mooney: can we assume a port always have a fixed-ips list with ip addresses in it ? | |
| 14:43:23 | bauzas | I can be conservative and double-check we have a list of IPs, and if not, fallback to getting the network id | |
| 14:43:32 | bauzas | as we could boot with an unbound port | |
| 14:43:43 | bauzas | right ? | |
| 14:45:10 | sean-k-mooney | bauzas: no i t wont have a fix ip if it was a new boot | |
| 14:45:22 | sean-k-mooney | it will if it has an ip e.g. a move operation or they specified one | |
| 14:45:25 | bauzas | okay, so we need to be conservative | |
| 14:45:38 | sean-k-mooney | yep but if it has no fixed ip | |
| 14:45:45 | bauzas | sure, sec | |
| 14:45:48 | sean-k-mooney | then any segment in the network is valid | |
| 14:46:00 | sean-k-mooney | so you can use the lent of the list to determin how to look it up | |
| 14:46:49 | sean-k-mooney | e.g. if port['fixed_ips']: lookup_by_port_subnet else lookup_by_network | |
| 14:48:35 | bauzas | that's exactly what I wrotre | |
| 14:48:37 | bauzas | pasting it | |
| 14:48:53 | bauzas | hmpf, paste.o.o is super slow | |
| 14:50:08 | sean-k-mooney | ya it is weird | |
| 14:50:18 | bauzas | sean-k-mooney: https://pastebin.com/x0g0anRS | |
| 14:50:41 | bauzas | agreed with the logic ? | |
| 14:50:51 | bauzas | if so, adding the new neutron knob | |
| 14:51:18 | bauzas | (nevermind the pep8 indenting issue, fwiw :) ) | |
| 14:51:53 | sean-k-mooney | ya that works | |
| 14:52:10 | sean-k-mooney | although i kind of feel like lines 17-22 should be under the if on 13 | |
| 14:52:18 | sean-k-mooney | well if/else | |
| 14:52:51 | sean-k-mooney | i mean you dont need to do the if network_id as that will always be true | |
| 14:53:32 | sean-k-mooney | you could keep the if subnet_id but that will only be set if we go to line 14 in which case we know it wont be empty | |
| 14:54:07 | bauzas | yeah, I can improve this | |
| 14:54:14 | sean-k-mooney | so you can put lines 21 and 22 on line 15 and lines 18 and 19 on line 17 | |
| 14:54:29 | sean-k-mooney | but ya the logic looks correct to me | |
| 15:23:22 | openstackgerrit | Balazs Gibizer proposed openstack/nova stable/victoria: Reproduce bug 1907522 in functional test https://review.opendev.org/c/openstack/nova/+/769736 | |
| 15:23:24 | openstack | bug 1907522 in OpenStack Compute (nova) "test_*_with_qos_min_bw_allocation fails in the nova-multi-cell job with: nova.exception.MigrationPreCheckError: Migration pre-check error: Failed to create port bindings for host |
|
| 15:24:46 | openstackgerrit | Balazs Gibizer proposed openstack/nova stable/victoria: Fallback to same-cell resize with qos ports https://review.opendev.org/c/openstack/nova/+/769727 | |
| 16:10:37 | lyarwood | dansmith: https://review.opendev.org/c/openstack/nova-specs/+/769547 - would you mind hitting PS3 today? Should be good to go now. | |
| 16:11:03 | dansmith | roger | |
| 16:11:30 | lyarwood | cheers | |
| 16:15:07 | openstackgerrit | Dan Smith proposed openstack/nova-specs master: libvirt: Update instance machine type stash spec https://review.opendev.org/c/openstack/nova-specs/+/769547 | |
| 16:38:05 | openstackgerrit | Merged openstack/nova-specs master: libvirt: Update instance machine type stash spec https://review.opendev.org/c/openstack/nova-specs/+/769547 | |
| 16:39:57 | gibi | lyarwood: I'm looking at detach_device_with_retry() codepath detach_device_with_retry | |
| 16:40:26 | gibi | lyarwood: and wondering why we have the code set up in a way that we always call an intial detach then after it a detach in a loop | |
| 16:41:41 | gibi | lyarwood: also, I'm wondering what is the way _do_wait_and_retry_detach() normally exit the tool | |
| 16:41:44 | gibi | loop | |
| 16:42:20 | gibi | is it raises DeviceDetachFailed to signal that the detach is done? | |
| 16:42:49 | lyarwood | gibi: just in a call | |
| 16:43:12 | gibi | no worries, I will drop soon, so we can talk about it tomorrow morning | |
| 16:43:27 | lyarwood | gibi: ack if we could that would be great | |
| 16:46:38 | gibi | OK | |
| 16:48:33 | melwitt | gibi: IIRC it's because the initial detach "should" detach it from persistent + live config but the guest can refuse the live detach as it's an ACPI request. so we retry in case that happens (guest refusal by it's busy). under nominal conditions I think the loop detects it's detached in the first iteration and moves on | |
| 16:48:48 | melwitt | *bc it's busy | |
| 16:51:44 | gibi | melwitt: thanks. | |
| 16:52:06 | gibi | melwitt: both the initial and the retry loop calls _try_detach_device() _try_detach_device | |
| 16:52:12 | gibi | https://github.com/openstack/nova/blob/745f835e9d8931261501747130165824334abb4b/nova/virt/libvirt/guest.py#L404 | |
| 16:53:51 | gibi | melwitt: so if the first all to _try_detach_device() finishes without exception then when we call the retry loop it calls _try_detach_device() again that will detach again | |
| 16:54:02 | gibi | * first call | |
| 16:55:02 | melwitt | yeah, IIRC the second call will be a no-op if the guest detached the live config the first call | |
| 16:55:33 | melwitt | if the first call did not result in detach of the live config, it will loop N times to keep trying | |
| 16:55:57 | gibi | melwitt: ohh, is it because we have the condition in the loop config = get_device_conf_func(device) | |
| 16:56:00 | gibi | if config is not None: | |
| 16:56:05 | gibi | if config is not None: | |
| 16:56:05 | gibi | config = get_device_conf_func(device) | |
| 16:56:07 | gibi | sorry | |
| 16:56:10 | gibi | back copy paste | |
| 16:56:19 | gibi | here https://github.com/openstack/nova/blob/745f835e9d8931261501747130165824334abb4b/nova/virt/libvirt/guest.py#L476 | |
| 16:57:10 | melwitt | mmmaybe. I think that only checks whether the persistent config got detached, not the live | |
| 17:00:06 | melwitt | I think that only checks whether the persistent detach succeeded and so it will go ahead and try the live detach and get DeviceNotFound | |
| 17:00:56 | lyarwood | melwitt++ | |
| 17:03:10 | melwitt | and DeviceNotFound gets ignored as a no-op | |
| 17:03:48 | gibi | melwitt: what is the exit condition of _do_wait_and_retry_detach() ? the retry decorator retries on DeviceDetachFailed exception and if _try_detach_device() succeed without exception then _do_wait_and_retry_detach() raises a DeviceNotFound so that also leads to retry | |
| 17:05:38 | gibi | so the exit condition must be that the config become None | |
| 17:08:34 | melwitt | hmm.. yeah I see what you're saying. I hadn't thought the detach from live would be reflected in that get_disk but maybe it is | |
| 17:09:13 | melwitt | gibi: (it seems like it has to be, like you said) | |
| 17:10:42 | gibi | ohh we have two different exceptions in the control flow DeviceDetachFailed and DeviceNotFound, I think I mixed the two | |
| 17:11:12 | gibi | for the initial detach DeviceNotFound means we failed the detaching | |
| 17:11:22 | gibi | in one of the domains | |
| 17:12:13 | gibi | and hence we move to retry | |
| 17:12:17 | gibi | but in the retry loop we only retry on DeviceDetachFailed not on DeviceNotFound | |
| 17:12:56 | melwitt | gibi: ahhhh yes you got it | |
| 17:13:33 | melwitt | that sneaky RetryDecorator | |
| 17:13:33 | gibi | so the retry loop does not care about DeviceNotFound (partial detach failure) but only cares about the config being not None | |
| 17:13:56 | gibi | this code is hard | |
| 17:14:58 | melwitt | yes. this is exactly why lyarwood has plans to refactor it :) | |
| 17:15:08 | gibi | but then if the retry loop sees any exception (other than the self inflicted DeviceDetachFailed) e.g. DeviceNotFound then it just let the exception buble up and not retry. So I don't know how this really retries a partial detach failure in libvirt | |
| 17:15:23 | gibi | melwitt: yeah, and I try to help him, hence my questions :) | |
| 17:16:07 | gibi | (no the exceptions are not inherinting from each other) | |
| 17:16:17 | lyarwood | FWIW with an event based approach we would just ask libvirt to detach once and then wait | |
| 17:16:34 | gibi | lyarwood: and if if sends a failed event, we call detach again? | |
| 17:17:12 | lyarwood | true yeah we could but it should be easier to write and maintain than this mess | |
| 17:18:00 | gibi | lyarwood: or we consider the DeviceRemovalFailedEvent as a permanent failure that we buble up? | |
| 17:18:08 | gibi | s/than/and/ | |
| 17:18:16 | gibi | nvm | |