| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-12-02 | |||
| 09:52:42 | bauzas | gibi: we did some data migrations in the past | |
| 09:52:58 | bauzas | but if you need to update two objects, well, wow | |
| 09:53:04 | gibi | bauzas: yes, and the pcpuset one is broken :) | |
| 09:53:18 | gibi | bauzas: it is one OVO class that is persisted to two db table | |
| 09:53:39 | gibi | InstanceNUMACell is part of instance_extra.numa_topology as well as request_spec.numa_topology | |
| 09:53:43 | bauzas | I guess we don't use a same ovo object for NUMATopology ? | |
| 09:54:01 | gibi | same ovo class for both | |
| 09:54:02 | bauzas | lemme look at your bug above ^ | |
| 09:54:07 | bauzas | hah | |
| 09:54:07 | gibi | sure | |
| 09:54:19 | gibi | I'm hacking on a soluiton atm | |
| 09:59:36 | bauzas | okay, I think I understand the problem | |
| 10:02:10 | bauzas | we updated instance_extra | |
| 10:02:28 | bauzas | when we called the InstanceNumaTopology object | |
| 10:03:15 | bauzas | but if you don't hydrate this object this way, we don't update it | |
| 10:03:39 | gibi | yepp | |
| 10:03:42 | bauzas | I thought we had an db upgrade too before moving to Wallaby then | |
| 10:03:55 | bauzas | at least a nova-upgrade check | |
| 10:04:18 | bauzas | telling that some objects weren't updated | |
| 10:04:33 | gibi | I don't think so as the code still has todos to remove the migration code once we are sure the the objects are loaded once | |
| 10:04:37 | bauzas | at least that's what I'd do if I would write some upgrade change | |
| 10:04:43 | gibi | but even if we had a blocking migration that would miss request spec too | |
| 10:05:09 | bauzas | gibi: because we persist it in the RequestSpec API DB ? | |
| 10:05:16 | gibi | yewpp | |
| 10:05:17 | gibi | yepp | |
| 10:05:29 | bauzas | ok I see the problem | |
| 10:05:42 | bauzas | so in theory the cell db is upgraded | |
| 10:05:46 | gibi | I assume that if we forgot to migrate the request spec then we forgot to add a blocking migration for that too | |
| 10:05:54 | bauzas | but the api db continues to have old values | |
| 10:05:59 | gibi | yes | |
| 10:06:20 | gibi | or more precisely when an instance is loaded it has a proper value, but if a request spec is loaded it has still the old value | |
| 10:06:26 | bauzas | ok, so we need to use this migrate method for the requestspec object then | |
| 10:06:41 | bauzas | gibi: yup, understood | |
| 10:06:47 | gibi | yes, but we need to split the code as in case of request spec we need to persist the updated value to a different table | |
| 10:06:52 | gibi | hence my pain | |
| 10:06:57 | bauzas | hah | |
| 10:06:59 | gibi | it is ugly and non generic | |
| 10:07:03 | bauzas | got it | |
| 10:07:26 | bauzas | well, this should have been done this way in Victoria either way, right? | |
| 10:08:15 | bauzas | problem is, we assume that InstanceNumaTopology object is only persisted by instance_extra db table | |
| 10:08:18 | bauzas | right?N | |
| 10:09:00 | gibi | right | |
| 10:09:03 | bauzas | if so, the object is broken | |
| 10:09:13 | bauzas | I mean the object persistency | |
| 10:09:24 | gibi | the problem is that the InstanceNUMATopology object does not know that it is used in two differnt context | |
| 10:09:26 | bauzas | we need to say we have to persist in two db tables | |
| 10:09:45 | bauzas | gibi: yup, hence my word 'broken' | |
| 10:10:03 | bauzas | in general we have object classes that are backed by a single db table | |
| 10:10:13 | gibi | I can add a generic code to _obj_from_primitives that called in both case and do the data migration genericly, but in that code I cannot decide which table to write the data back | |
| 10:10:17 | bauzas | and we directly map the object fields with the db values | |
| 10:10:35 | bauzas | gibi: agreed with your problem, this is a pain to fix | |
| 10:10:45 | bauzas | the design itself is having flaws | |
| 10:11:29 | bauzas | we somehow need to have ovo objects that know which db table they are related | |
| 10:11:47 | bauzas | but honestly, I somehow feel those necessarly have to be two different objects | |
| 10:12:04 | bauzas | we can nest objects under others | |
| 10:12:34 | bauzas | but given RequestSpecs is at the API DB, we can't just hydrate the value from the cell DB values | |
| 10:13:03 | bauzas | or, the other way to consider that, is that we only use RequestSpecs.InstanceNUMATopology as a non-persisted object | |
| 10:13:45 | bauzas | but we would need to scatter/gather the values from the cell DB before we hydrate such reqspec nested object | |
| 10:14:25 | bauzas | gibi: see ? | |
| 10:14:53 | bauzas | one way to address this would be to gather the instancenumatopology object from the cell DB at the api level before we hydrate the requestspec | |
| 10:14:55 | gibi | I'm not even sure that for a single instnace the InstanceNUMATopology in the instance and in the request spec are the saem | |
| 10:14:58 | gibi | same | |
| 10:15:21 | gibi | i.e. instance multicreate reuses a single request spec object afaik | |
| 10:15:34 | bauzas | this is fine | |
| 10:15:43 | bauzas | this is just for scheduling decisions | |
| 10:15:48 | bauzas | we don't need to persist this | |
| 10:16:19 | bauzas | that's why we always said we were fine with one single reqspec per boot call in case of multicreate | |
| 10:16:34 | bauzas | and in case of a move op, getting the generic reqspec is ok | |
| 10:16:36 | gibi | then I don't get it | |
| 10:16:56 | gibi | the failure happens during the scheduling of a migration of a pre Victoria instance | |
| 10:17:10 | gibi | in the NumaTopology filter | |
| 10:17:42 | bauzas | gibi: ok, so at the API, we need to gather the instance numa topology from the cell DB corresponding to where the instance is stored | |
| 10:18:08 | bauzas | and we will then hydrate the reqspec with the proper values of the instance | |
| 10:18:41 | gibi | no no, I get that. what I don't get is why the NumaTopology filter depends on this value | |
| 10:19:18 | gibi | what does the request_spec.numa_topology describes? | |
| 10:19:22 | gibi | is the what was requested? | |
| 10:19:46 | gibi | and the instance_extra.numa_topology represent what was actually selected | |
| 10:19:58 | bauzas | excellent question | |
| 10:20:09 | gibi | but then in multicreate there is one request but more than one actual selection | |
| 10:20:39 | gibi | and those selections can be have different values, different pcpu ids | |
| 10:21:30 | gibi | anyhow I have to go back hacking on a solution to fix an urgent downstream issue. The fix needs to be backportable so I will do the ugly code path duplication | |
| 10:21:47 | gibi | later on we can think about a proper fix that reimagine the ovo usage | |
| 10:22:13 | bauzas | good luck | |
| 10:22:18 | gibi | thanks | |
| 10:22:22 | gibi | you will see the result :D | |
| 10:48:03 | gibi | bauzas: how could this ever work? https://github.com/openstack/nova/blob/7670303aabe16d1d7c25e411d7bd413aee7fdcf3/nova/objects/request_spec.py#L727-L730 | |
| 10:48:16 | gibi | as far as I see db_spec is a dict | |
| 10:48:29 | gibi | db_spec.save() is a type error | |
| 10:52:51 | bauzas | gibi: no this isn't a dict | |
| 10:53:12 | gibi | is something connected to a db model? | |
| 10:53:42 | bauzas | gibi: IIRC it returns the class object | |
| 10:54:26 | bauzas | oh wait, sec | |
| 10:55:08 | bauzas | it returns some SQLA object | |
| 10:56:03 | gibi | OK I think I see now, thanks | |
| 10:57:07 | gibi | the unit test simulates the db in a wrong way :/ | |
| 10:58:10 | bauzas | https://www.commitstrip.com/wp-content/uploads/2016/01/Strip-Voyage-dans-le-temps-650-finalenglish-2.jpg | |
| 10:58:50 | bauzas | gibi: I guess we need to pdb it | |
| 10:59:11 | bauzas | my object-persistency-fu is a bit rusty | |
| 10:59:24 | gibi | lol | |
| 10:59:44 | gibi | so the test mocks in a dict instaed of a proper object from the db, hence my problem | |