| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-12-02 | |||
| 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 | |
| 12:48:25 | sean-k-mooney | and stop persiting it | |
| 12:51:49 | sean-k-mooney | it might slow donw scduling some what as numa_get_constraints is not super cheap but its also not that expensive either | |
| 12:52:34 | sean-k-mooney | its a pure fucntion of its inputs https://github.com/openstack/nova/blob/7670303aabe16d1d7c25e411d7bd413aee7fdcf3/nova/virt/hardware.py#L1858 but we would have to make sure to use the correct falvor on resize | |
| 13:48:26 | gibi | sean-k-mooney: you mean not persist the numa_topology in the RequestSpec at all? Just create it from the image/flavor every time we need it? | |
| 13:50:20 | sean-k-mooney | gibi: yep that is what bauzas is suggesting | |
| 13:50:32 | sean-k-mooney | which we could actully do in the case of the request sepc | |
| 13:50:45 | bauzas | sorry, a bit busy on and off | |
| 13:51:00 | bauzas | but yeah I don't like to persist the same DB table in another DB | |
| 13:51:04 | sean-k-mooney | teh numa constreait fucntion is relitvely cheap in comparison to the actul numa affinity process | |
| 13:51:14 | sean-k-mooney | bauzas: its really two differnt thigns | |
| 13:51:26 | sean-k-mooney | we jsut use the same object | |
| 13:51:36 | sean-k-mooney | one is the numa affinity request object | |
| 13:51:56 | sean-k-mooney | and the other is the final affinty object which stores the assinged cpus and numa nodes | |
| 13:52:07 | sean-k-mooney | we just reused the same object for both | |
| 13:52:12 | gibi | OK. I got it thanks. | |
| 13:53:31 | sean-k-mooney | gibi: if we go with the generate it when needed approch i would prefer not to backport that mainly because i dont want to have to thing do we have that or not when looking at different downstream releases | |
| 13:53:54 | sean-k-mooney | but im not against it for master on | |
| 13:53:56 | gibi | yeah I would not backport that either | |
| 13:54:22 | gibi | Lets see if I can get some time adding that to master | |
| 13:54:34 | gibi | but I will go with the current patch as a backportable thing | |
| 13:54:35 | bauzas | sean-k-mooney: I understand that's two different things | |
| 13:54:43 | bauzas | but using the same object is creating some concerns | |
| 13:54:49 | gibi | we need that to move back til victoria | |
| 13:55:11 | gibi | bauzas: totally agree I think it is worth to remove it from the second table | |
| 13:55:20 | bauzas | ++ | |
| 13:55:34 | sean-k-mooney | gibi: i review that and +1'd by the way the only thing i woudl add is a release note but it looks correct to me | |
| 13:55:53 | sean-k-mooney | well actully | |
| 13:55:59 | sean-k-mooney | did you update the func test | |
| 13:56:16 | sean-k-mooney | i closed the review | |
| 13:56:48 | sean-k-mooney | ok you did not | |
| 13:57:04 | sean-k-mooney | so ya release note and then you need to fix the repoduce func test | |