| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-12-02 | |||
| 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 | |
| 13:57:18 | gibi | sure I will add a reno | |
| 13:57:24 | sean-k-mooney | which passed... | |
| 13:57:26 | gibi | the reproduce was fixed but it is not a func test it is a unit | |
| 13:57:41 | sean-k-mooney | oh ok | |
| 13:57:45 | sean-k-mooney | ya it is | |
| 13:57:59 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/820153/1/nova/tests/unit/objects/test_request_spec.py | |
| 13:58:30 | gibi | creating a pre victoria instance in the func test is far from trivial | |
| 13:58:37 | gibi | so I dropped that direction | |
| 13:59:10 | sean-k-mooney | sorry i normally expect those to be func tests but ya backporting the func test to victoria woudl be non trivail | |
| 13:59:26 | sean-k-mooney | in this specific case i think a unit test is ok | |
| 13:59:40 | gibi | not that what I meant. I mean creating a func test on master that simulates a pre Victoria instance is hard | |
| 14:00:15 | gibi | I would need to dig in to the DB anyhow to backlevel the data to pre-victoria as our object layer will persist the master version | |
| 14:00:34 | sean-k-mooney | yes so you woudl have to boot the vm | |
| 14:00:40 | sean-k-mooney | then update its request spec | |
| 14:00:49 | sean-k-mooney | then migrate it and assert it expodes | |
| 14:01:22 | sean-k-mooney | gibi: i assumed you ment because we willl be missing several fo the helper functions | |
| 14:01:32 | gibi | not that ^^ | |
| 14:01:40 | gibi | but the "then update its request spec" | |
| 14:01:54 | gibi | that would be heavy DB digging in a func test | |
| 14:03:54 | sean-k-mooney | gibi: ya you woudl effectly have to directly execute sql | |
| 14:04:00 | sean-k-mooney | which i think is over kill | |
| 14:04:17 | gibi | and in that sql manipulate a highly nested json dict :D | |
| 14:04:18 | sean-k-mooney | and this is all in a json blob in the db too so its not exactly nice to update via sql either | |
| 14:04:25 | gibi | exactly :D | |
| 14:04:25 | sean-k-mooney | exactly | |
| 14:04:33 | sean-k-mooney | jinks :) | |