| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-05 | |||
| 19:19:55 | dansmith | they would have to have a service that was deleted before stein, which remains in the database, which they re-started in antelope | |
| 19:20:57 | dansmith | I'll just move the undelete code into this patch | |
| 19:21:04 | dansmith | I thought this would be an easy conversation | |
| 19:21:07 | dansmith | moving it is easier :) | |
| 19:21:15 | melwitt | ok, just trying to think if there's anyway something could break or we lose coverage if we delete it. sorry | |
| 19:21:55 | sean-k-mooney | the coverage we would be losing is assertign that a compute-agent in A can work with a compute service record that does not have a uuid | |
| 19:21:56 | melwitt | like, do we have some other test that makes sure you can delete the service and then restart nova-compute and then migrate and then assert it worked | |
| 19:22:46 | dansmith | it's theoretically losing some coverage I guess, but the only reason the test will pass after the undelete is because the compute node actually does have a uuid that I can undelete from | |
| 19:22:53 | dansmith | it's like, not a thing that could happen in the real world, | |
| 19:22:54 | melwitt | if we do, then we don't need this one | |
| 19:23:10 | dansmith | because their records would actually not have uuids like these fake test ones do | |
| 19:23:44 | melwitt | yeah sorry, I mean without consideration of the uuid. just covering that deleting a service and starting nova-compute again migrate still works | |
| 19:24:24 | melwitt | I agree that the uuid part of it is so old that we need not test for it | |
| 19:25:38 | sean-k-mooney | well the overall functionallity that they were trying to test was InstanceListWithDeletedServicesTestCase | |
| 19:25:52 | melwitt | I wasn't clear on whether the concept in general of deleting a service and then starting it again stuff will still work, if that is covered somewhere | |
| 19:25:52 | dansmith | okay I don't think the bug actually has much to do with migrate, | |
| 19:26:08 | sean-k-mooney | ya i dont think so either | |
| 19:26:10 | dansmith | it's instance list, the test just uses migrate to generate some traffic and records I think | |
| 19:26:26 | sean-k-mooney | right so you could jsut delete the service | |
| 19:26:31 | sean-k-mooney | and then do an instnace list | |
| 19:27:09 | dansmith | melwitt: tbh I think that's probably a risky thing to do right now, not sure if we claim to support it.. it's like we have service delete, we don't have undelete, but if you restart a service with the right name after deleting it, it'll come back from the dead, | |
| 19:27:35 | dansmith | which is actually a problem because of how we recreate compute nodes and potentially can have conflicts with the provider name in placement | |
| 19:27:37 | sean-k-mooney | it will mostly come back form the dead | |
| 19:27:45 | dansmith | because the name will be the same, but the uuid will be different (currently) | |
| 19:27:57 | sean-k-mooney | but not fully | |
| 19:28:18 | sean-k-mooney | right the uuid will be differnt and naythign like pci claims will not be recreated | |
| 19:28:31 | sean-k-mooney | so it will come back in a broken state | |
| 19:29:07 | sean-k-mooney | unfortuntly if our customer have shown us anything its posible to run in that broken state for an extended period of time without noticing | |
| 19:29:14 | melwitt | yeah. I mean like regression coverage that deleting the service and restarting nova-compute with the new undelete will remain working | |
| 19:29:17 | dansmith | heh yeah | |
| 19:29:47 | sean-k-mooney | melwitt: well it will actully work better then it does today | |
| 19:30:00 | melwitt | like is this test the only place we test this or is it covered somewhere else already and this test isn't providing anything new other than uuid checking | |
| 19:30:14 | sean-k-mooney | but that does not mean we technially supprot it today or sould support it going forward | |
| 19:30:37 | dansmith | it sounds like melwitt wants a more generic test to validate that the de-zombification works today, even though it shouldn't be expected to, and that this series will not make it worse | |
| 19:30:50 | dansmith | yeah, that's my only complaint about writing that test, but perhaps I should just do it | |
| 19:31:00 | melwitt | so you're saying we do *not* support deleting a service and restarting nova-compute and having stuff still wowrk? | |
| 19:31:02 | melwitt | *work? | |
| 19:31:30 | sean-k-mooney | melwitt: thats what im saying as an operator you should not expect that to work | |
| 19:31:43 | dansmith | agree, not sure if we're explicit about it though | |
| 19:32:00 | sean-k-mooney | if you do not use any pci/numa stuff or have not vms on it at the time it will work | |
| 19:32:04 | dansmith | also not defending that as a good thing :) | |
| 19:32:08 | melwitt | sean-k-mooney: that seems so unexpected to me. sorry, I just had no idea. I thought they're supposed to be able to do that if the hostname stays the same | |
| 19:32:33 | sean-k-mooney | there is no expection that the compute node uuid would remain the same | |
| 19:32:39 | dansmith | melwitt: the reality is different I think | |
| 19:32:43 | melwitt | so if someone messes up and deletes a service and then says oops that was a mistake, then all those instances are expected not to work? | |
| 19:32:45 | sean-k-mooney | its a uuid4 and not based on the hostname/hypervior_hostname | |
| 19:32:52 | melwitt | dang | |
| 19:32:53 | dansmith | you can't delete a service with instances on it | |
| 19:33:10 | melwitt | ok, so that saves it I guess? ok | |
| 19:33:23 | sean-k-mooney | dansmith: are you sure | |
| 19:33:23 | dansmith | saves it from the single-click-mega-fail, but.. :) | |
| 19:33:27 | dansmith | pretty sure | |
| 19:33:52 | sean-k-mooney | ok cause i know we have code to loop over the allocation in placment and delete them before we delete the placment rp when teh compute serivce is deleted | |
| 19:34:05 | melwitt | just seems so harsh lol (if it were possible to delete the service while instances are on it) | |
| 19:34:07 | dansmith | yup | |
| 19:34:20 | sean-k-mooney | i guess that is just to prevent leaked allocation blocking the placment cleanup | |
| 19:35:37 | sean-k-mooney | ah https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/services.py#L269-L282 | |
| 19:35:43 | sean-k-mooney | we special case the nova-compute | |
| 19:36:02 | sean-k-mooney | so ya you cant delete it if it has instance | |
| 19:36:18 | melwitt | ok, well, if that's the case then I understand why and agree the test can be removed entirely. just seems so harsh, if what I was thinking were possible (and it is not possible bc we don't let you delete a service with instances mapped to it) | |
| 19:36:19 | sean-k-mooney | in which case provide the placment clean up happens properly it does not really matter if the uuid changes in that case | |
| 19:36:28 | sean-k-mooney | or if we undelete | |
| 19:37:07 | sean-k-mooney | https://github.com/openstack/nova/commit/42f62f1ed2ad76829eb9d40a8b9646a523f6381f | |
| 19:37:25 | sean-k-mooney | melwitt: it was only blokced in rocky it looks like | |
| 19:38:03 | sean-k-mooney | https://bugs.launchpad.net/nova/+bug/1763183 | |
| 19:38:13 | melwitt | I think we (maybe I) backported it downstream | |
| 19:38:36 | sean-k-mooney | well it was backported upstream to pike | |
| 19:38:39 | melwitt | I just was not thinking about it or remembering it | |
| 19:38:47 | melwitt | ah ok | |
| 19:39:56 | sean-k-mooney | i rememebr being able to delete compute serivce with instance at one point but i feel like that is just because i mess up my local devstack not because i planed to do it | |
| 19:40:08 | dansmith | melwitt: here are the most service-delete-y tests we have in functional/ https://github.com/openstack/nova/blob/master/nova/tests/functional/wsgi/test_services.py#L119 | |
| 19:40:11 | melwitt | yeah you used to be able to | |
| 19:40:18 | dansmith | none of them ensure we can start an instance on the resurrected service, | |
| 19:40:28 | dansmith | although they do restart the compute to make sure it comes back up | |
| 19:40:43 | dansmith | which is the thing sean-k-mooney and laugh at outside a fake environment :P | |
| 19:41:08 | melwitt | I see, ok. thanks | |
| 19:41:22 | dansmith | melwitt: so your demand is me adding a test that a resurrected compute can fake boot a fake instance un a fake environment, and then I can delete this regression test, right? | |
| 19:41:26 | dansmith | (snarky on purpose, but serious) | |
| 19:41:45 | melwitt | sorry for the longer convo. I was very confused by the test and then I was erroneously thinking of an accidental service delete scenario | |
| 19:42:04 | dansmith | don't apologize | |
| 19:42:20 | melwitt | yeah, I said earlier I understand now and agree the test can be removed without loss of anything | |
| 19:42:24 | dansmith | the stuff I'm having to do in this set to make such a simple thing work is ridiculously incestuous | |
| 19:42:59 | dansmith | melwitt: well, I think adding a "and can boot something" thing to those ^ would make that a defensible position for me :) | |
| 19:43:01 | melwitt | I bet :\ | |
| 19:44:12 | melwitt | thanks for that 😂 | |
| 19:44:12 | melwitt | thanks for that 😂 | |
| 19:49:54 | sean-k-mooney | dansmith: alot of that likel come form how the fixture make restarting compute service work in the past | |
| 19:50:07 | dansmith | yes, I'm well aware | |
| 19:50:33 | melwitt | dansmith: I agree adding a "and can boot something" to those existing tests is a nice thing to cover. but I don't expect it to have to be part of your series, to be clear | |
| 19:51:02 | sean-k-mooney | with the stable uuid serise i am assuming you will have a functional test that start with an empty db and starts a comptue service with the uuid specifed in a file | |
| 19:51:33 | sean-k-mooney | you have a seperte test that delete it form teh db and starts it again if you wanted | |
| 19:52:51 | sean-k-mooney | but ya i think we agreed on nuke the thing and move on with your seriese | |
| 19:53:36 | melwitt | yes | |
| 19:55:05 | dansmith | well, I figure I need to add the other when I drop the regression test | |
| 19:55:21 | dansmith | there's something weird though about not seeing the provider get recreated after restarting the old compute, | |
| 19:55:27 | dansmith | although I see it happen in the logs | |
| 19:56:37 | sean-k-mooney | that happens after teh perodic task runs although it also happens i think in init host | |
| 19:57:06 | dansmith | I see it created before I look for it | |
| 19:57:29 | dansmith | https://pastebin.com/isqJXnfW | |
| 19:57:43 | dansmith | first line is it being created in our db, then placement, then the last one is looking for it, but it's missing | |