| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-27 | |||
| 21:30:19 | melwitt | I don't know | |
| 21:31:23 | mriedem | i'll throw something up quick before i have to head out | |
| 21:32:00 | mnaser | so my audit script helped bring them from 20k down to 308 left which have no build_requests, no cell_id in the mapping | |
| 21:32:26 | mnaser | and not existing in any cells | |
| 21:32:50 | mriedem | mnaser: ok those are likely just instance mappings for deleted and purged instances | |
| 21:32:56 | mriedem | do you archive/purge the cell dbs often/ | |
| 21:32:57 | mriedem | ? | |
| 21:33:25 | mriedem | b/c it wasn't until i think rocky that we added instance mapping and reqspec hard delete to nova-manage db archive_deleted_rows when instances are archive | |
| 21:33:31 | mnaser | select created_at from instances order by id asc limit 1; => 2014-12-14 02:38:53 | |
| 21:33:31 | mriedem | or maybe you run your own archive/purge script? | |
| 21:33:33 | mnaser | ...ha. | |
| 21:34:35 | mnaser | but i think i'm mostly waiting for the rocky archive delete stuff | |
| 21:41:10 | mriedem | do you run your own archive script or nova-manage db archive_deleted_rows? | |
| 21:42:33 | mnaser | mriedem: none of the above, we just have a really really really big database | |
| 21:42:53 | mnaser | mysql indexing seems fast enough that it hasn't really affected us much other than just.. being a big db. | |
| 21:42:54 | sean-k-mooney | mriedem: fyi i left a comment on the review but is the call to self.driver.cleanup in https://review.openstack.org/#/c/586568/1 against the source or dest node? | |
| 21:44:20 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Update instance mapping as soon as instance is created in cell https://review.openstack.org/586713 | |
| 21:44:22 | mriedem | mnaser: melwitt: throwing things at the wall ^ | |
| 21:44:41 | mriedem | sean-k-mooney: source | |
| 21:44:49 | mriedem | _post_live_migration and _rollback_live_migration run on the source host | |
| 21:45:43 | mriedem | sean-k-mooney: replie | |
| 21:45:45 | mriedem | *replied | |
| 21:45:52 | sean-k-mooney | mriedem: oh ok then yes it proably should have the source vif then however i dont think it actully will need them unless we replug the vifs | |
| 21:46:02 | mriedem | that's not what you said last night | |
| 21:46:09 | mriedem | something something ovs hybrid plug cleanup | |
| 21:46:19 | mriedem | but it was 4am and you were maybe loopy | |
| 21:46:20 | sean-k-mooney | mriedem: for the cleanup | |
| 21:47:31 | sean-k-mooney | mriedem: self.driver.post_live_migration_at_source shoudl use the old source vifs so it can unplug correctly | |
| 21:48:25 | mriedem | sean-k-mooney: yes, same thing | |
| 21:48:32 | sean-k-mooney | i dont know what self.driver.cleanup does. if its on the source however it should also proably be using the source vifs | |
| 21:48:50 | mriedem | sean-k-mooney: in the commit message, i pointed out that if post_live_migration_at_source is successful, destroy_vifs=False and the libvirt driver won't try to unplug in cleanup() | |
| 21:49:00 | mriedem | however, not all virt drivers adhere to that destroy_vifs flag | |
| 21:49:04 | mriedem | the hyperv driver doesn't for example | |
| 21:49:22 | sean-k-mooney | ah ok then yes that all looks good then | |
| 21:49:30 | mriedem | it looks...beautiful | |
| 21:50:33 | sean-k-mooney | normally i like shorter fuction names but the at_source and at_destination really helps keep context in this code | |
| 21:51:34 | mriedem | that's why i did https://review.openstack.org/#/c/551371/ | |
| 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/ | |