| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-12-02 | |||
| 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 | |
| 12:03:07 | sean-k-mooney | gibi: it woudl be automatic if we bumpt the RequestSpec ovo version when the ovo version of one of its contained objects is increased | |
| 12:03:11 | sean-k-mooney | but we dont | |
| 12:03:31 | sean-k-mooney | so the request spec jsut need to detach that the contained ovo is an older veriion then the current one | |
| 12:04:06 | sean-k-mooney | the constoct the new one using from_primitve and then call to_primate to get the updated version and save it back | |
| 12:04:25 | sean-k-mooney | gibi: we shoudl be doing the data migration in the from_primitive function | |
| 12:04:30 | sean-k-mooney | in InstanceNUMATopology | |
| 12:04:46 | gibi | sean-k-mooney: but then you don't know from the client side if re-presisting is needed or not | |
| 12:04:51 | sean-k-mooney | that will make it transparent to the client and keep the encalupation | |
| 12:05:33 | sean-k-mooney | gibi: well you would if the verion changed but i guess that is a catch 22 | |
| 12:05:57 | sean-k-mooney | we might need a way to ask the InstanceNUMATopology ovo if it need migration | |
| 12:06:17 | sean-k-mooney | by passing it the primiave object form the db and having it return true/false | |
| 12:07:36 | stephenfin | gibi: I think you've figured this out already but no, no blocking DB migration yet. We could add one but it hasn't been done | |
| 12:07:52 | stephenfin | reading back through logs. That looks like a hairy bug | |
| 12:08:08 | gibi | stephenfin: thanks. I will not have time to add the blocking migration now, but maybe at some point in the future | |
| 12:11:18 | sean-k-mooney | is this only needed wehn we have mixed cpus | |
| 12:11:23 | gibi | nope | |
| 12:11:36 | gibi | what you need is a pre victoria pinned instance migrated post Victoria | |
| 12:11:38 | sean-k-mooney | ok so its form the PCPU in placment changes | |
| 12:12:15 | opendevreview | Balazs Gibizer proposed openstack/nova master: Migrate RequestSpec.numa_topology to use pcpuset https://review.opendev.org/c/openstack/nova/+/820153 | |
| 12:12:37 | gibi | sean-k-mooney, bauzas, stephenfin: ^^ that is my first stab to fix this | |
| 12:12:44 | sean-k-mooney | gibi: as a quick hack you can just update the numa toplogy object in the filter | |
| 12:13:46 | sean-k-mooney | taht wont actully fix it will it. well i twill work but it wont update the request spec version | |
| 12:14:23 | gibi | yeah that would be an option too If I can assume that only the numa filter depends on this | |
| 12:14:27 | sean-k-mooney | ah you also do it there | |
| 12:14:44 | gibi | I do it now in the RequestSpec loading | |
| 12:15:03 | sean-k-mooney | yep just lookg at it now | |
| 12:15:14 | sean-k-mooney | we use it in the api also | |
| 12:15:25 | sean-k-mooney | wehn validating rebuilds | |
| 12:15:42 | sean-k-mooney | although im not sure we actully depend in the cpu sets directly | |
| 12:15:58 | sean-k-mooney | i think we generate new objects form the old and new image | |
| 12:16:05 | sean-k-mooney | and assert they are the same | |
| 12:16:23 | stephenfin | gibi: that's how I'd have done it also | |
| 12:17:38 | sean-k-mooney | stephenfin: in the filter or gibis patch | |
| 12:17:45 | stephenfin | gibi's patch | |
| 12:18:00 | sean-k-mooney | ya i was assuming we would do it on load | |
| 12:18:19 | sean-k-mooney | to avoid the blocker migration | |
| 12:18:23 | stephenfin | doing it in the filter leaves us having to support that forever. As a temporary fix, sure, but it's not viable long-term | |
| 12:20:17 | sean-k-mooney | gibi im not sure we can assume we can remove this imideatly after yoga | |
| 12:20:43 | stephenfin | sean-k-mooney: when do we ever remove things when we said we would | |
| 12:20:43 | sean-k-mooney | we might want to keep it for a few releases in the event that peoppel do an inplace upgrade | |
| 12:20:53 | gibi | :D | |
| 12:20:59 | sean-k-mooney | lol true | |
| 12:21:08 | gibi | sean-k-mooney: technically if we add a blocking migration then we can remove this in Yoga | |
| 12:21:17 | gibi | probably we wont add that | |
| 12:22:00 | sean-k-mooney | gibi: we could also do it as a nova manage command with a nova status check | |
| 12:22:16 | gibi | yepp | |
| 12:22:25 | bauzas | honestly, my jab is that we shouldn't persist this object in the API DB | |
| 12:22:26 | gibi | if we trust the users to run the upgrade check | |
| 12:22:38 | sean-k-mooney | bauzas: no the curent behavior is correct | |
| 12:22:41 | bauzas | if it's just for asking the scheduler using it | |
| 12:22:54 | sean-k-mooney | bauzas: we need to persit it to persit the numa constraits | |
| 12:23:11 | sean-k-mooney | well technially we can generate it again | |
| 12:23:13 | bauzas | which service is persisting it ? | |
| 12:23:19 | sean-k-mooney | if we have the embeded image and flavor | |
| 12:23:36 | sean-k-mooney | it capture the numa scheduleing requirements | |
| 12:23:50 | bauzas | I need to taxi my daughter now | |
| 12:23:54 | bauzas | but let's discuss this after | |
| 12:23:59 | sean-k-mooney | if we dont persite it we need to regenerate it form the embeded flavor and image | |
| 12:25:19 | bauzas | or from the instance one when moving , nope ? | |
| 12:25:36 | bauzas | anyway, me moving | |
| 12:26:10 | sean-k-mooney | no the instance object is fully populated it might work but its would not work for inital schduling | |
| 12:26:28 | sean-k-mooney | we woudl still need to store it in the request spec to not chagne ethe filter api | |
| 12:44:29 | bauzas | sean-k-mooney: that's something we already do | |
| 12:44:42 | bauzas | sean-k-mooney: we either populate from the image and/or the flavor first | |
| 12:44:55 | bauzas | but then for move ops, we get it from somewhere else | |
| 12:47:26 | sean-k-mooney | where? we currently populate it form the instance or when we build the request spec form its componets | |
| 12:48:20 | sean-k-mooney | bauzas: gibi we coudl make it a propery and constuct it form the image and flavor if we needed too | |