| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-05 | |||
| 19:04:46 | dansmith | well, they were both added at one point, | |
| 19:04:51 | dansmith | but yes compute node was most recent (although still a long time ago) | |
| 19:05:15 | melwitt | hm, yeah. this is confusing | |
| 19:05:28 | dansmith | the "problem" for me is that the test relies on us creating a new compute node for the resurrected service | |
| 19:05:39 | dansmith | which will get a new auto-generated node uuid | |
| 19:05:43 | sean-k-mooney | compute service uuid was pike https://docs.openstack.org/nova/latest/reference/api-microversion-history.html#maximum-in-pike | |
| 19:05:48 | dansmith | but after I fix that to not happen, it ... doesn't :) | |
| 19:05:58 | dansmith | and fails because the compute node can't be re-created with the same uuid | |
| 19:06:09 | dansmith | I can make it create-or-undelete (and have locally) | |
| 19:06:16 | sean-k-mooney | dansmith: right so creating a new compute node record is wrong | |
| 19:06:25 | melwitt | this is the commit that added the test https://github.com/openstack/nova/commit/81f05f53d357a546c7f9a53cae6ef45b92e28bc1 | |
| 19:06:25 | sean-k-mooney | well | |
| 19:06:28 | dansmith | but that's a bit more change, out of sequence with the rest of the series, etc | |
| 19:06:37 | sean-k-mooney | if we still have a compute node record we shoudl not be creating a new one | |
| 19:06:49 | sean-k-mooney | if its has been deleted then creating one makes sense | |
| 19:06:59 | sean-k-mooney | as its the same as the first time it was created | |
| 19:07:06 | dansmith | well, that's kinda the thing | |
| 19:07:32 | dansmith | we create a new one, but shouldn't, and if we create with the same uuid, the unique contstraint will fail with the deleted one | |
| 19:09:04 | melwitt | sean-k-mooney: the test is deleting the compute node record (implicitly) so that's why it expects to create a new record right? | |
| 19:09:14 | melwitt | afterwards | |
| 19:09:36 | dansmith | it doesn't expect to create a new compute node, it just assumes/relies on it happening | |
| 19:09:37 | sean-k-mooney | melwitt: i think so which is why its checking the hyperviors api to ensure its gone | |
| 19:10:17 | sean-k-mooney | dansmith: its expecting a sidefect of the service delete is that the compute node is removed | |
| 19:10:31 | sean-k-mooney | and the side efffect of the restart is that a new one is created | |
| 19:10:54 | dansmith | my first thought was to make it undelete the compute, then assert the uuid is the same, but then I realized that the test is checking for a thing that can't have happened since stein, so it seems like not worth a bunch of monkeywork to keep asserting this | |
| 19:10:55 | sean-k-mooney | at least that is how im interperting https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/test_bug_1764556.py#L98-L107 | |
| 19:11:30 | melwitt | ok, so what is different with the new change ... if a service and thus compute node are deleted and then nova-compute is started again, will it un-delete the existing compute node record? | |
| 19:11:45 | dansmith | it doesn't currently | |
| 19:11:54 | dansmith | after my series is done then it will | |
| 19:11:55 | melwitt | but your change will make it do that I mean? | |
| 19:11:56 | melwitt | ok | |
| 19:12:03 | sean-k-mooney | do we included deleted in the uniqconstriat for cn table | |
| 19:12:22 | dansmith | sean-k-mooney: no, which is why it conflicts | |
| 19:12:37 | dansmith | sean-k-mooney: we hit that with some of the previous rename customer scenarios too if you recall | |
| 19:12:47 | sean-k-mooney | ack ok so either we undelete or we add it to the uniqcontratint | |
| 19:13:07 | dansmith | yes, and undelete is the right thing IMHO, but that's *after* this point in the series | |
| 19:13:21 | dansmith | and since this test is asserting something that can't be the case anymore, I want to nuke it :) | |
| 19:13:49 | sean-k-mooney | ya so either slap an expect fail on this or nuke it | |
| 19:13:53 | sean-k-mooney | im fine with the latter | |
| 19:14:11 | dansmith | I don't want to xfail it because I don't want to fix it later because I think it's no longer useful | |
| 19:14:23 | dansmith | but if I'm wrong, you (all) need to say so | |
| 19:15:06 | sean-k-mooney | well going forward we dont want to recreate the CN with a differnt uuid | |
| 19:15:14 | melwitt | sorry, I'm going back and trying to understand how that test is relying on a new compute node record | |
| 19:15:28 | melwitt | I know that it is but I can't see why when I look at it | |
| 19:15:50 | dansmith | melwitt: relying on the new compute node or relying on it being recreated in some way? | |
| 19:16:00 | melwitt | dansmith: the recreation | |
| 19:16:09 | dansmith | it relies on there being some compute node because it does a migration, which won't work without it | |
| 19:16:17 | sean-k-mooney | it migrate back to the host that was deleted | |
| 19:16:25 | dansmith | it doesn't care (or know) whether or not it's recreated or undeleted | |
| 19:16:37 | sean-k-mooney | just that it exists | |
| 19:16:54 | melwitt | ok, I guess I don't get why that wouldn't work with your code change | |
| 19:16:55 | dansmith | not even that it exists, just that it can migrate | |
| 19:17:00 | melwitt | if you are going to undelete it | |
| 19:17:17 | dansmith | I'm going to undelete it eventually, but not at patch #3 | |
| 19:17:38 | dansmith | but patch #3 is where we start getting the same uuid for compute nodes, which means it fails to blindly re-create the compute node because of the UC | |
| 19:17:59 | melwitt | ah ok. so this would be a "temporary" failure if we keep the test | |
| 19:18:14 | melwitt | in that it would work again after the undelete patch happens | |
| 19:18:15 | dansmith | it doesn't really matter that it works or doesn't, because I can fix the test or the code.. my point is it's not a case that can exist in real life (since stein) so I think it'd be better not to do that work for no reason | |
| 19:18:26 | dansmith | yeah | |
| 19:18:44 | dansmith | I could xfail it, and then at the end, unxfail it | |
| 19:19:00 | dansmith | but the latter will be "re-enable this test for a thing that can't happen anymore" :) | |
| 19:19:04 | melwitt | yeah I guess I'm thinking does it matter if a customer has deleted service records that are super old that have no uuids? | |
| 19:19:29 | sean-k-mooney | you coudl but we dont intend to support compute service with out a uuid in teh compute agent that is going to be exicurign this code | |
| 19:19:33 | sean-k-mooney | not anymore anyway | |
| 19:19:54 | sean-k-mooney | melwitt: it would have to be pre pike | |
| 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 | |