| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-27 | |||
| 21:51:53 | mriedem | because knowing wtf is going on in the 20 methods involved in live migration is not sometihng you can keep in your head | |
| 21:52:12 | mriedem | also https://docs.openstack.org/nova/latest/reference/live-migration.html | |
| 21:52:55 | melwitt | yes. every time I figure out code like that, a few months later I end up wishing I had added a lot of code comments to it, if nothing else | |
| 21:53:30 | mriedem | yup also https://review.openstack.org/#/c/496861/ | |
| 21:53:52 | melwitt | two thumbs up | |
| 21:54:03 | mriedem | thanks ebert | |
| 21:54:18 | melwitt | looking at your change, trying to remember why the instance.create() was split up from the inst mapping update in the first place | |
| 21:54:19 | mriedem | RIP | |
| 21:54:26 | mriedem | melwitt: the quota stuff | |
| 21:54:36 | mriedem | i can find a review comment where we talked about the split | |
| 21:54:41 | melwitt | yeah, trying to re-remember | |
| 21:54:43 | sean-k-mooney | ya i have that bookmarked i just didnt have see we were still in _post_live_migration. that function does a lot | |
| 21:54:57 | mriedem | too much | |
| 21:55:18 | melwitt | I think it was something about, if we failed a quota recheck in the middle of a multi create, and to nix all the instances before creating any mappings | |
| 21:55:25 | melwitt | but we ended up not doing that and putting them in ERROR state | |
| 21:55:45 | sean-k-mooney | part of the issue is ist implementing a state machine and all of that context is mixed in with what its doing | |
| 21:55:58 | melwitt | so that ended up being the wrong thing to do, I think | |
| 21:56:16 | mriedem | melwitt: https://review.openstack.org/#/c/501408/2/nova/conductor/manager.py@1020 | |
| 21:56:54 | mriedem | too bad i didn't link that irc convo in | |
| 21:58:17 | melwitt | yeah, this is coming back to me. there were other things like, at the time I was thinking don't create the BDMs etc until after we know we're good after the quota recheck | |
| 21:58:56 | melwitt | but we discussed on IRC and determined that all had a failure path to clean up anything that was created, and so should have been okay to just do everything normally and check quota at the end | |
| 21:59:05 | melwitt | in one loop instead of two | |
| 21:59:19 | mriedem | http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2017-09-06.log.html#t2017-09-06T20:33:51 | |
| 21:59:44 | mriedem | it was also a refactor we didn't want to backport | |
| 21:59:53 | melwitt | right yeah | |
| 22:00:10 | mriedem | i had a todo to combine back to a single loop on my desk for a long time, b/c i had in mind how to do it, | |
| 22:00:12 | mriedem | but long forgot now | |
| 22:00:26 | sean-k-mooney | mriedem: haha i was just looking at the irc logs to see if i could find it for you. | |
| 22:00:44 | melwitt | I added it to my todo list too so hopefully one of us will do it this time. I had forgotten about it | |
| 22:02:58 | mriedem | "dansmithmriedem: we wouldn't know where to find the instance record to mark it as deleted when they deleted the buildreq, so we'd leave that undeleted but unfindable instance forever" | |
| 22:02:59 | mriedem | heh | |
| 22:03:05 | mriedem | sound familiar? | |
| 22:04:08 | mriedem | "mriedemi shit my pants everytime we touch nova these days" | |
| 22:04:13 | mriedem | ha | |
| 22:04:43 | melwitt | haha, relatable | |
| 22:04:56 | mriedem | mnaser: again, congratulations to you to continue running a business on top of stuff we're still talking about fixing almost 1 year later :) | |
| 22:05:42 | openstackgerrit | karim proposed openstack/nova master: Updated AggregateImagePropertiesIsolation filter illustration https://review.openstack.org/586317 | |
| 22:06:17 | mriedem | i think the tl;dr from the irc convo is just combine the loops and move the quota check to the end | |
| 22:06:37 | mriedem | "locally" deleting the instance will automatically delete the tags and bdms along with the instance from the cell | |
| 22:06:39 | melwitt | I'm trying to think, why didn't we move the instance mapping update earlier last time? | |
| 22:07:01 | melwitt | yeah, that's what I'm getting from it too, merge the loops and check quota at the end | |
| 22:07:52 | mriedem | idk, my guess is tunnel vision on the fix at hand | |
| 22:10:05 | melwitt | wait, that change (last year) *did* move the inst mapping update earlier to right after the instance.create(). looking to see what happened to that | |
| 22:14:36 | mriedem | but only if the quota check failed | |
| 22:14:46 | mriedem | b/c we exit after that | |
| 22:15:07 | mriedem | we don't bury in cell0 if quota check fails because the instances are already created in cells at that point | |
| 22:16:03 | melwitt | I mean this, this is showing an update of the instance mapping right after we create the instance record https://review.openstack.org/#/c/501408/2/nova/conductor/manager.py@1003 | |
| 22:17:01 | mriedem | oh right yewah | |
| 22:17:03 | mriedem | *yeah | |
| 22:17:05 | melwitt | but in the current version of the code, the instance mapping update isn't right after the instance create anymore | |
| 22:17:19 | melwitt | and I can't find how that changed, looking at git blame and failing | |
| 22:17:27 | mriedem | _populate_instance_mapping was only ever used in the cellsv1 path | |
| 22:17:30 | mriedem | the build_instances method | |
| 22:17:33 | mriedem | i'm pretty sure | |
| 22:17:48 | melwitt | but in that old patch, it's in schedule_and_build_instances | |
| 22:19:16 | mriedem | because mnaser was re-using it | |
| 22:19:43 | mriedem | you mean why did we talk him out of that? | |
| 22:20:42 | melwitt | no I mean, as of that patch, the instance mapping update was right after instance create, but the current code has the mapping update much later, and I was wondering why that was moved. I assume it was to fix some other bug or something | |
| 22:21:01 | mriedem | looks like it was changed as a result of the irc convo | |
| 22:21:31 | melwitt | oh gaaaahhh, I didn't realize I was looking at an earlier PS | |
| 22:24:39 | melwitt | okay so the final version only added a mapping update to the cleanup method, like you said earlier I think. so the normal path for updating the mapping was always later on | |
| 22:25:35 | melwitt | ok | |
| 22:27:31 | mriedem | yup. alright gotta run. o/ | |
| 22:27:36 | melwitt | o/ | |
| 23:27:02 | openstackgerrit | Merged openstack/nova master: Use source vifs when unplugging on source during post live migrate https://review.openstack.org/586402 | |
| 23:27:09 | openstackgerrit | Merged openstack/nova master: Pass source vifs to driver.cleanup in _post_live_migration https://review.openstack.org/586568 | |
| 23:27:18 | openstackgerrit | Merged openstack/nova master: Update queued-for-delete from the ComputeAPI during deletion/restoration https://review.openstack.org/566813 | |
| 23:30:18 | melwitt | finally \o/ | |
| 23:49:23 | mnaser | well i know its late | |
| 23:49:29 | mnaser | but now even another whole interesting failure | |
| 23:49:39 | mnaser | no record in nova_api but one in the cell | |
| 23:49:40 | mnaser | lol | |
| 23:54:59 | melwitt | mnaser: no build request or instance mapping? | |
| 23:55:30 | mnaser | melwitt: build request, no instance mapping | |
| 23:55:53 | mnaser | wait sorry | |
| 23:55:57 | mnaser | it doesnt exist in the cell, sorry | |
| 23:56:00 | melwitt | build request, instance in cell, no instance mapping | |
| 23:56:02 | melwitt | build request only? | |
| 23:56:04 | mnaser | yes | |
| 23:56:08 | mnaser | build request only | |
| 23:56:14 | melwitt | that's the exact same thing rdo cloud ran into | |
| 23:56:30 | mnaser | so shows up in list but not deletable etc | |
| 23:56:34 | melwitt | right | |
| 23:56:45 | mnaser | i guess i can just delete the build request and have it disappear? | |
| 23:56:50 | melwitt | do you have several or just a few? like does it happen a lot? | |
| 23:57:01 | melwitt | yes, that's what I told rdo cloud to do too | |
| 23:57:11 | mnaser | i mean after running my fixup script, i still had a few instances that were stuck BUILD/scheduling | |
| 23:57:38 | melwitt | I dug around in the code and didn't see a way it can happen other than nova-api going down at the precise moment between the build_request.create() and the instance_mapping.create() or the instance_mapping.create() somehow failing | |
| 23:57:58 | mnaser | so for context it is possible that rpc and/or db both had issues at the time | |
| 23:58:05 | mnaser | does the build request and instance_mapping get created at the same time or? | |
| 23:58:08 | melwitt | which seems it would be crazy rare ... so maybe we're missing some other way it could happen | |
| 23:58:15 | melwitt | pretty much yeah. let me grab a link | |
| 23:58:49 | melwitt | https://github.com/openstack/nova/blob/3e0b17b1e138615b66293976ca5b55c291957844/nova/compute/api.py#L930-L942 | |
| 23:59:15 | melwitt | yeah, soon you can come fix all these bugs | |
| 23:59:18 | mnaser | ok that's interesting | |
| 23:59:18 | mnaser | haha | |
| 23:59:59 | mnaser | so build request was created, instance mapping was *not* created. unless there was an attempt to delete the instance while it was still in build request | |
| #openstack-nova - 2018-07-28 | |||
| 00:00:05 | melwitt | yeah so, just wonder if there's some bizarre way a delete could miss the build request, similar to what mriedem was saying earlier for the bug we discussed earlier | |
| 00:00:34 | mnaser | i might be like 5 steps behind your discussion so i might just be echoing stuff y'all already discusssed :p | |
| 00:00:48 | melwitt | nah | |