Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-06-10
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 :)
20:42:16 artom It's not the generation part that I have issues with, it's that the VIF model suddenly understand the difference between compute manager and virt driver
20:42:26 sean-k-mooney yes but if we get teh info form neutron in the future i can simply change the impliation of that method
20:42:52 artom Anyways, we're ratholing :)
20:43:01 sean-k-mooney and only for the revert resize case too
20:43:32 sean-k-mooney so nova.compute.utils
20:43:52 sean-k-mooney i think it qualifies as a """Compute-related Utilities and helpers."""
20:44:17 sean-k-mooney that has things like is_volume_backed_instance
20:44:46 artom Fair enough
20:45:14 sean-k-mooney you could put it in the virtapi i guess too
20:46:01 sean-k-mooney if you did that then the manager would call the driver to get teh events
20:46:17 artom Eh?
20:46:19 artom How so?
20:46:51 sean-k-mooney well were you thinking of making it a module level function or adding it to the driver interface
20:47:47 sean-k-mooney virtapi is actuly just event stuff...
20:47:49 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/virt/virtapi.py
20:48:33 sean-k-mooney i guess that would be fine too.
20:49:06 artom This *is* event stuff :)
20:49:20 sean-k-mooney ya i would be fine with adding it there
20:50:09 sean-k-mooney i assummed wrongly that the base compute driver api would be defiended there but that is in driver.py
20:51:25 efried aspiers: Reviewed.
20:51:30 aspiers thanks!
20:51:37 sean-k-mooney damn no wonder the libvirt virt driver is 5000 lines lon the base class is almost 2000
21:12:42 openstackgerrit Artom Lifshitz proposed openstack/nova master: WIP: Revert resize: wait for events according to hybrid plug https://review.opendev.org/644881
21:12:43 openstackgerrit Artom Lifshitz proposed openstack/nova master: WIP: Include direct-physical in compute manager events check https://review.opendev.org/664431
21:13:03 artom sean-k-mooney, mriedem, ^^
21:13:10 artom And I think I need to head home at this point.
21:13:22 artom Or potentially to daycare, if wife doesn't make it there in time :/

Earlier   Later