| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-12-02 | |||
| 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 | |
| 11:00:48 | gibi | moving on | |
| 11:14:09 | gibi | stephenfin: do you remember if we have a blocking db migration for the cpuset -> pcpuset data migration? | |
| 11:29:43 | sean-k-mooney | yay another libosinfo bug that can break live migration this time. | |
| 11:29:49 | sean-k-mooney | gibi: i did not think we needed one | |
| 11:30:33 | gibi | sean-k-mooney: if want to remove the data migration code from InstanceNUMATopology ever then we need a blocking migration to see that all the objects is loaded at least once | |
| 11:31:39 | sean-k-mooney | gibi right but i tought we were not goign to drop that | |
| 11:32:17 | sean-k-mooney | i mean we can but we do it on load to avoid a data migration | |
| 11:32:38 | sean-k-mooney | gibi: back to your instance numa toploty question | |
| 11:32:49 | sean-k-mooney | the one in the request spec is not the same as the one in the instnace | |
| 11:33:05 | sean-k-mooney | the request spec version will not have actul cpus ectra assigned | |
| 11:33:14 | sean-k-mooney | it just has the constriants | |
| 11:33:21 | sean-k-mooney | e.g. 2 numandoes with pinned cpus | |
| 11:33:32 | sean-k-mooney | the one in the instance actully will be fully populated | |
| 11:33:46 | sean-k-mooney | once it has landed on the compute node | |
| 11:33:59 | sean-k-mooney | and been assinged cores and save back to the db | |
| 11:34:58 | gibi | sean-k-mooney: yeah, so we cannot use the data in instance_extra.numa_topology to populate request_spec.numa_topology due to multicreate | |
| 11:35:15 | gibi | we need to independently handle request_spec.numa_topology and migrate it | |
| 11:35:37 | sean-k-mooney | well we shoudl not be populating the request spec version and persiting it | |
| 11:38:01 | sean-k-mooney | gibi: we clone the request spec to avoid it modifying it https://github.com/openstack/nova/blob/7670303aabe16d1d7c25e411d7bd413aee7fdcf3/nova/scheduler/filters/numa_topology_filter.py#L64-L71 | |
| 11:38:18 | sean-k-mooney | so that multi create works | |
| 11:38:49 | sean-k-mooney | stephen fixed that in https://github.com/openstack/nova/commit/bff2030ecea8a1d21e03c61a7ece02f40dc25c5d | |
| 11:39:12 | sean-k-mooney | https://bugs.launchpad.net/nova/+bug/1655979 | |
| 11:39:37 | gibi | sean-k-mooney: so there will be a request spec for each multicreated instance during scheduling but only the first request spec is persisted the rest is just temporary | |
| 11:39:57 | gibi | or we call create() on the clones as well? | |
| 11:40:31 | gibi | ohh | |
| 11:40:41 | sean-k-mooney | no only one will be persited | |
| 11:40:53 | sean-k-mooney | we make a copy and use that in the filter | |
| 11:40:55 | gibi | and that one has numa_topology ? | |
| 11:41:23 | sean-k-mooney | the initial request spec has a partly populated numa_toplogy object | |
| 11:41:28 | sean-k-mooney | which is intentional | |
| 11:42:27 | sean-k-mooney | its create by numa_get_constraits https://github.com/openstack/nova/blob/8d9785b965657d42f20e1ad7234f570077a387d7/nova/virt/hardware.py#L1858 | |
| 11:42:45 | sean-k-mooney | in the api | |
| 11:43:11 | sean-k-mooney | https://github.com/openstack/nova/blob/ff4b396abff80dea5a54dfe830a7db3a97a7360c/nova/compute/api.py#L1068 | |
| 11:43:19 | sean-k-mooney | as part of _validate_and_build_base_options | |
| 11:43:27 | sean-k-mooney | that is what is stored in the request_spec | |
| 11:44:45 | gibi | so that create numa cell objects with cpuset values | |
| 11:45:02 | sean-k-mooney | no the cpuset values will not be populated | |
| 11:45:13 | gibi | https://github.com/openstack/nova/blob/8d9785b965657d42f20e1ad7234f570077a387d7/nova/virt/hardware.py#L1553 | |
| 11:45:48 | gibi | https://github.com/openstack/nova/blob/8d9785b965657d42f20e1ad7234f570077a387d7/nova/virt/hardware.py#L2041 | |
| 11:45:55 | gibi | based on this they will | |
| 11:46:10 | sean-k-mooney | well ok they will https://github.com/openstack/nova/blob/a027b45e46fb3a63166b8b86ef7f99b0b04bcec8/nova/virt/hardware.py#L1552-L1553 | |
| 11:46:51 | sean-k-mooney | from https://github.com/openstack/nova/blob/a027b45e46fb3a63166b8b86ef7f99b0b04bcec8/nova/virt/hardware.py#L2040-L2051 | |
| 11:46:56 | gibi | yepp | |
| 11:47:14 | sean-k-mooney | they wont be mapped to host cores at this point | |
| 11:47:19 | gibi | true | |
| 11:47:38 | sean-k-mooney | but we will know how many floatign or pinned cores are required | |
| 11:48:41 | sean-k-mooney | gibi: what is your current issue by the way | |
| 11:48:48 | sean-k-mooney | i only read back part of the irc log | |
| 11:48:55 | gibi | OK. So i) we need to do the cpuset -> pcpuset data migration in request_spec.numa_topology that was missed originally in Victoria. And we cannot avoid that by populating request_spec.numa_topology from instance_extra.numa_topology | |
| 11:49:29 | gibi | sean-k-mooney: the cpuset -> pcpuset data migration was only done in instance_extra.numa_topology and not in request_spec.numa_topology | |
| 11:49:51 | gibi | so when a pre Victoria pinned instance is migrated after upgrade to Victoria the scheduler blows | |
| 11:50:03 | gibi | as it triest to read out pcpuset but it is not populated | |
| 11:50:33 | gibi | see https://review.opendev.org/c/openstack/nova/+/820121 | |
| 11:51:23 | gibi | so I'm working on a bugfix for it, but it will be ugly as the InstanceNUMATopology object is persisted in two tables but the OVO itself does not know about that fact | |
| 11:51:32 | gibi | so I cannot add a generic solution | |
| 11:51:49 | gibi | I have to keep the upgrading of the two table in separate paths | |
| 11:52:03 | sean-k-mooney | gibi well we can just do the migration in the ovo no? | |
| 11:52:14 | sean-k-mooney | rather then in the db | |
| 11:52:15 | gibi | de ovo does not know which table to update | |
| 11:52:33 | gibi | the current migration persists the data after the data migration | |
| 11:52:39 | gibi | but only in instance_extra | |
| 11:52:48 | sean-k-mooney | well it only gets saved indretly via eithe a request_spec.save or instance.save | |
| 11:52:58 | gibi | nope | |
| 11:53:17 | sean-k-mooney | wehre do we save the toplogy diretly | |
| 11:53:25 | gibi | sec... | |
| 11:53:52 | gibi | https://github.com/openstack/nova/blob/7670303aabe16d1d7c25e411d7bd413aee7fdcf3/nova/objects/instance_numa.py#L203 | |
| 11:53:55 | gibi | here | |
| 11:54:16 | sean-k-mooney | right so that is in the data migration code | |
| 11:54:27 | gibi | so when an instance is loaded that needed a migration the instance_extra is changed in the DB during the load | |
| 11:54:28 | sean-k-mooney | but normally we do not do that | |
| 11:54:43 | sean-k-mooney | ya only for old instances | |
| 11:54:51 | sean-k-mooney | we can do the same in the request spec | |
| 11:55:08 | sean-k-mooney | wehn its loaded if it has an old instance object it can also just save it | |
| 11:55:53 | gibi | yes, that is what I'm doing but that is ugly | |
| 11:56:10 | gibi | not generic enough to handle the case when object will be persisted in a 3rd | |
| 11:56:10 | gibi | i) ugly ii) does | |
| 11:56:13 | gibi | table iii) create a bad precedence for future OVO data migrations | |
| 11:56:31 | sean-k-mooney | the https://github.com/openstack/nova/blob/7670303aabe16d1d7c25e411d7bd413aee7fdcf3/nova/objects/request_spec.py#L245-L262 | |
| 11:56:36 | gibi | I will do it as I have to provide a fix today downstream that is backportable, but I hate i | |
| 11:56:59 | sean-k-mooney | gibi: well you should not be updating multiple tables really | |
| 11:57:03 | gibi | sean-k-mooney: those are only called if the request spec is created from an instance object | |
| 11:57:18 | sean-k-mooney | you are either udpateing the instance copy or the request spec copy | |
| 11:57:25 | sean-k-mooney | they are not ment to have the same data in them | |
| 11:57:30 | gibi | yes, but from the OVO perspective it does not know whihc | |
| 11:58:07 | gibi | so I cannot have a generic _obj_from_primitive call in InstanceNUMATopology that does the migration as that call has no info where to persist the result | |
| 11:58:09 | sean-k-mooney | the instance request spec object wont | |
| 11:58:17 | sean-k-mooney | but the request spec will know it the request spec | |
| 11:58:39 | sean-k-mooney | you shoudl be fixint this in the request psec object not the InstanceNUMATopology | |
| 11:59:00 | gibi | so now the RequestSpec object has to know that there is an InstanceNUMATopology data migration that needs to be run, that is baaad encapsulation | |
| 11:59:17 | sean-k-mooney | not really | |
| 11:59:36 | sean-k-mooney | the instance numa toplogy object is a child object of the request spec | |
| 11:59:36 | gibi | the data format change should be encapsulated in the InstanceNUMATopology in my eyes | |
| 12:00:09 | sean-k-mooney | gibi: well it can be encaplusted there but there | |
| 12:00:47 | sean-k-mooney | but the request spec shoudl call InstanceNUMATopology to parse the data and also to gereate teh updated serialised version | |
| 12:01:06 | sean-k-mooney | then the the request spec shoudl save that as part of iteslf | |
| 12:01:25 | gibi | but that is bad encapsulation | |
| 12:01:45 | gibi | the data migration should be automatic and opaque from the client of the object | |
| 12:01:59 | sean-k-mooney | gibi: how its just create the InstanceNUMATopology form a json blob and then saving it as a json blob | |