| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-10 | |||
| 18:36:28 | sean-k-mooney | spatel: in anycase when the migration eventually complete you can revert it which will be quick as teh data will not have been delete on the souce node. | |
| 18:36:47 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 18:37:02 | aspiers | efried: is this roughly along the right lines? ^^^ | |
| 18:38:01 | spatel | i will revert it without confirm it | |
| 18:38:03 | efried | aspiers: I'm going to have to look a bit later, I'm afraid. | |
| 18:38:27 | aspiers | efried: np. Don't need a full review yet, just confirmation I'm barking up the right tree | |
| 18:55:45 | spatel | sean-k-mooney: revert works! | |
| 18:55:53 | spatel | what a pain :) | |
| 18:56:20 | spatel | sean-k-mooney: thank you again for saving my a... :) | |
| 19:14:01 | openstackgerrit | Matt Riedemann proposed openstack/nova-specs master: Spec to pre-filter disabled computes with placement https://review.opendev.org/657884 | |
| 19:53:07 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: WIP: Revert resize: wait for events according to hybrid plug https://review.opendev.org/644881 | |
| 19:53:08 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Remove finish_revert_migration TypeError try/except https://review.opendev.org/664013 | |
| 19:53:09 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: WIP: Include direct-physical in compute manager events check https://review.opendev.org/664431 | |
| 19:53:48 | artom | sean-k-mooney, mriedem, ^^ did a thing. Still lots of cleanup to do, but maybe check the compute manager/libvirt driver parts to see if the general approach is palatable? | |
| 19:59:44 | sean-k-mooney | that likely wont be backportable. due to the chagne in the signiture of finish_revert_migration or at least its more invasive then it needs to be | |
| 19:59:47 | sean-k-mooney | still reading it | |
| 20:00:48 | artom | finish_revert_migration was changed with the previous approach as well (was adding vifs_already_plugged) | |
| 20:01:19 | sean-k-mooney | yes but we can actully make this work without adding any new parmater to finish_revert_migration | |
| 20:01:57 | sean-k-mooney | if we take the approch i was suggesting. we actully did not need to pass vif_already_plugged | |
| 20:02:16 | artom | Why is adding a parameter bad though? That's not RPC, don't think it'll affect backportability... | |
| 20:02:40 | sean-k-mooney | its chaning the virt dirver api right | |
| 20:02:40 | artom | It's compute manager talking to its virt driver, both are updated at the same time | |
| 20:02:49 | artom | Yeah | |
| 20:02:55 | sean-k-mooney | so it would break out of tree drivers without the try catch | |
| 20:03:10 | artom | Well, yeah | |
| 20:03:13 | sean-k-mooney | * try excpet | |
| 20:04:43 | artom | I don't see a way around passing the virt driver *something* | |
| 20:05:27 | sean-k-mooney | we have all the info we need in the network_info object since that is a list of vifs | |
| 20:05:31 | sean-k-mooney | so we can calualte if we need to wait in the driver or manager form that without passing anything | |
| 20:05:55 | sean-k-mooney | which is part of the reason i suggested adding the function in the vif model class | |
| 20:07:17 | artom | The problem is that finish_revert_migration isn't always called for revert resize... | |
| 20:07:43 | artom | So even if network_info has all it needs in terms of event "types" | |
| 20:07:55 | artom | We still need a way to tell the virt driver when to actually use that information | |
| 20:08:42 | sean-k-mooney | im not following | |
| 20:09:19 | artom | All of this stuff, it's only applicable in the revert resize case, right? | |
| 20:09:36 | sean-k-mooney | and revert migration yes | |
| 20:09:42 | sean-k-mooney | *cold migration | |
| 20:10:17 | artom | Is it? Does the instance remain shutoff on the source with the VIFs still wired while it's cold migrating? | |
| 20:10:25 | sean-k-mooney | yes | |
| 20:11:06 | sean-k-mooney | to both questions | |
| 20:11:08 | artom | So, the only other instance of driver.finish_revert_migration that I've found is https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1001 | |
| 20:11:43 | artom | So maybe you're right, driver.finish_revert_migration is called if and only iff we need to do the compute manager/virt driver events checking thing | |
| 20:11:57 | sean-k-mooney | yes cold migration uses the resize_migrating task state | |
| 20:12:50 | sean-k-mooney | right it only used in two case | |
| 20:13:10 | sean-k-mooney | if we are migrating/resizing instance.task_state == task_states.RESIZE_MIGRATING | |
| 20:13:20 | artom | So in that case yeah, we can stuff the logic inside the VIF model | |
| 20:13:21 | sean-k-mooney | which is that auto rollback if things go wrong | |
| 20:13:45 | artom | Kinda weird for something so specific to go in the model, but I guess it makes sense | |
| 20:13:51 | sean-k-mooney | or if the user revert the migraion/resize form the api after we succeed in the migratio/resize instead of confiming | |
| 20:14:48 | sean-k-mooney | artom: well i had planned to stuff the event info form neutron in that data stcutrue if we implement the spec in anycase | |
| 20:16:00 | sean-k-mooney | artom: we can always add a untility module or make it a free fucntion if you like but i think the encapsulation makes sense | |
| 20:17:04 | artom | Until confirmed (or reverted) | |
| 20:17:12 | sean-k-mooney | yep | |
| 20:17:19 | sean-k-mooney | and we default to auto confirm disabled | |
| 20:18:05 | sean-k-mooney | i more or less had this conversation with stephenfin and mriedem a few hours ago :) | |
| 20:18:20 | artom | (Which is weird UX, btw - because you 'nova migrate', but then you 'nova resize-revert') | |
| 20:18:32 | sean-k-mooney | yep we know :) | |
| 20:18:41 | artom | Yeah, didn't think I was teaching anyone anything | |
| 20:19:54 | sean-k-mooney | hehe its just funny to me because you are reiterating what stephenfin said more or less like 6 hours ago | |
| 20:20:32 | artom | Better late than never, I suppose | |
| 20:20:43 | sean-k-mooney | artom: http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-06-10.log.html#t2019-06-10T14:53:27 :) | |
| 20:21:43 | artom | *snerk* Yeah | |
| 20:24:25 | sean-k-mooney | so are you open to trying to remove the api change to the driver api and filtering the events as i was suggesting ? it shoudl reduce the size of the patch a resonable amount | |
| 20:24:43 | artom | sean-k-mooney, yeah | |
| 20:25:04 | artom | Still opposed to passing 'revert' to _create_domain_and_network though :) | |
| 20:25:42 | artom | But less oot driver pain is indeed better | |
| 20:26:29 | sean-k-mooney | well you might be able to tell form somthing else but why dont you like passing revert to _create_domain_and_network | |
| 20:26:41 | sean-k-mooney | i dont think you replied to the commets did you? | |
| 20:27:19 | artom | It's higher up logic that should live higher up | |
| 20:27:24 | artom | I kinda replied | |
| 20:27:45 | artom | So what I came up with is passing a list of events to wait for | |
| 20:28:25 | artom | I say higher up | |
| 20:28:39 | sean-k-mooney | oh ok. am i guess you could generate that in the driver | |
| 20:28:51 | sean-k-mooney | rather then pass it from teh compute manager | |
| 20:29:24 | sean-k-mooney | that an internal api so it more ok to modify the the driver api as you wont break anything at that point | |
| 20:29:47 | artom | Yeah, so really "higher up" means "more specific" | |
| 20:30:06 | artom | Ie, finish_revert_migration knows we're doing a revert, _cdan has no idea, nor should it | |
| 20:30:29 | sean-k-mooney | artom: dansmith raised a vaild point we need to make sure not to change livemigration behavior | |
| 20:30:54 | sean-k-mooney | artom: oh and i just saw your top level comment regarding revert | |
| 20:31:35 | artom | sean-k-mooney, yeah, which is why I straight up added https://review.opendev.org/#/c/644881/24/nova/compute/manager.py@6499 | |
| 20:31:53 | artom | I can probably just call it revert | |
| 20:32:00 | artom | Because it also applied for reverting cold migrations | |
| 20:32:21 | sean-k-mooney | ya you could rename the other one and add live | |
| 20:32:26 | sean-k-mooney | actully no | |
| 20:32:29 | sean-k-mooney | that is more work | |
| 20:32:32 | sean-k-mooney | and its unrelated | |
| 20:35:23 | artom | sean-k-mooney, where are you in Ireland again, btw? | |
| 20:35:31 | sean-k-mooney | Shannon | |
| 20:35:53 | sean-k-mooney | on the west coast more or less | |
| 20:37:15 | artom | https://i.imgur.com/lqfeJZk.jpg West Coast! | |
| 20:39:56 | sean-k-mooney | i was debating if it could go into the compute untils module | |
| 20:40:09 | sean-k-mooney | so in here somewhere https://github.com/openstack/nova/blob/master/nova/compute/utils.py | |
| 20:40:27 | sean-k-mooney | then you can use it for the driver or the compute manger | |
| 20:40:44 | sean-k-mooney | and not put it in the vif if you dont like that | |
| 20:40:57 | artom | It just feels so alien there | |
| 20:41:07 | artom | It's business logic, after all, right? | |
| 20:41:17 | sean-k-mooney | i was not suggsting puting the event generation in teh VIF by the way | |
| 20:41:17 | artom | Really specific one at that | |
| 20:41:36 | sean-k-mooney | jsut the function that says if it should wait in the manager or driver | |
| 20:41:49 | artom | Well.... "is_manager_vif" or w/e you called it is basically the same logic :) | |