| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-02-03 | |||
| 11:52:33 | bauzas | I was thinking of this | |
| 11:52:47 | sean-k-mooney | we have done similar things in the past. dansmith is doing somethign similar in the stable uuid series | |
| 11:52:49 | bauzas | but I was afraid that when we call migrate_instance() it wouldn't work | |
| 11:53:00 | sean-k-mooney | well there is one way to find out | |
| 11:53:32 | sean-k-mooney | try it and see | |
| 11:53:35 | bauzas | I could use a context manager | |
| 11:53:44 | bauzas | yeah, I'll try | |
| 11:54:25 | sean-k-mooney | if its a proably the we can do it sleightly differnt by doing mock.patch.object on the speficic compute instnaces | |
| 11:54:36 | sean-k-mooney | *compute objects | |
| 11:54:47 | sean-k-mooney | and ensure each one will use the correct one | |
| 11:56:26 | bauzas | I'll test it | |
| 11:56:36 | bauzas | I'm pretty done with my series | |
| 11:56:41 | bauzas | I'm just adding more tests | |
| 11:57:01 | sean-k-mooney | as in "ready to give up" or its good for re review | |
| 11:57:06 | sean-k-mooney | im hoping the latter :) | |
| 11:57:52 | sean-k-mooney | main feedback at a glance is you have no docs and no release note so in addtion to tests please think about those | |
| 11:58:21 | sean-k-mooney | ping me when you would like a full review and/or manual testing of this | |
| 11:58:53 | bauzas | sean-k-mooney: yup, you can manually test if you want | |
| 11:59:23 | bauzas | sean-k-mooney: the WIP is +W because of the tests and indeed the documents | |
| 11:59:32 | sean-k-mooney | you probaly have not tried using mixed cpus (a vm with pinned and unpined cores) and im not sure if you have check what happens when you use vcpu_pin_set vs cpu_dedicated_set | |
| 11:59:43 | bauzas | s/+W/-W of course | |
| 11:59:58 | bauzas | sean-k-mooney: good point, I can try | |
| 12:00:46 | sean-k-mooney | ill test those manully but when you have some basic test functional test to cover the commen cases pushed in an non wip it would be nice to have functional tests for those | |
| 12:02:15 | sean-k-mooney | we should also test the other funky numa cases i really dont expect it to matter but asymetic numa nodes and multiple numa. all of those advanced cases however can be added later | |
| 12:02:22 | sean-k-mooney | i dont belive your code will care | |
| 12:03:27 | sean-k-mooney | instance.numa_topology.cpu_pinning should be ever PCPU the vm is pinned too regardless of numa | |
| 12:04:20 | sean-k-mooney | including the extra one not used by vm cores when you use hw:emulator_thread_policy=isolate | |
| 12:06:04 | bauzas | not sure I fully understand your point, sorry | |
| 12:06:40 | sean-k-mooney | if you tried to parse the pinned cpus form the xml there are a bunch of edgecases that you would have had to handel | |
| 12:06:51 | sean-k-mooney | like also lookign at the emulator thread | |
| 12:07:19 | sean-k-mooney | but since your using instance.numa_topology.cpu_pinning you can mostly ignore that complexity | |
| 12:07:56 | bauzas | yup changed it based on your point | |
| 12:08:32 | sean-k-mooney | so while it would still be nice to test some fo the more complex numa toplogies and edgecases the code should not genrally have to care about them | |
| 12:08:54 | bauzas | yup, that's why I want to have migration tests + some numa ones | |
| 12:09:17 | sean-k-mooney | bauzas: we are expictly only supporting this when you use cpu_dedicated_set right and not vcpu_pin_set | |
| 12:09:29 | bauzas | correct | |
| 12:09:48 | bauzas | well, checking my code | |
| 12:10:01 | sean-k-mooney | do we want to raise a config error like dansmith is doing in the stable uuid series | |
| 12:10:15 | sean-k-mooney | i.e. if you enable this but also have vcpu_pin_set defiend | |
| 12:10:39 | sean-k-mooney | or dont have cpu_dedicated_set defiend | |
| 12:10:58 | bauzas | good question, I need to think about it | |
| 12:11:14 | sean-k-mooney | there are extra edgecases that only come into play if you use vcpu_pin_set | |
| 12:11:18 | sean-k-mooney | which is why im asking | |
| 12:11:34 | bauzas | https://review.opendev.org/c/openstack/nova/+/868237/6/nova/virt/libvirt/cpu/api.py | |
| 12:11:37 | sean-k-mooney | specificaly related to cpu_thread_policy=isolate on a host with hyperthreading | |
| 12:11:55 | bauzas | here, we only check get_decicated_set() | |
| 12:12:15 | bauzas | but we try to look the numa_topo blob from the instance | |
| 12:12:47 | sean-k-mooney | ok so that is not going to block it | |
| 12:12:49 | bauzas | if the instance is not having a numa_topo blob, we could have an exception, shit | |
| 12:13:02 | sean-k-mooney | well we shoudl not have an expction | |
| 12:13:06 | bauzas | as I directly call the subfield from numa_topo | |
| 12:13:07 | sean-k-mooney | we should do nothing | |
| 12:13:12 | bauzas | what's the default ? | |
| 12:13:22 | bauzas | i need to check the object default values | |
| 12:13:38 | sean-k-mooney | oh you ment it will currently raise | |
| 12:13:39 | bauzas | for the standard instance typo | |
| 12:13:40 | sean-k-mooney | yes it will | |
| 12:13:45 | sean-k-mooney | the default is None | |
| 12:13:52 | bauzas | yeah | |
| 12:14:03 | bauzas | my code is wrong for standard non-numa instances | |
| 12:14:05 | sean-k-mooney | so right now this will casue an atribute error if there is no numa toplogy | |
| 12:14:10 | sean-k-mooney | yep | |
| 12:14:16 | bauzas | correct, I need to fix it | |
| 12:15:01 | sean-k-mooney | but also we shoudl add a check in init host in the driver to see if CONF.libvirt.cpu_power_management is enabled and cpu_dedicated_set is not defined | |
| 12:15:32 | sean-k-mooney | and raise an InvalidConfiguration exception | |
| 12:16:08 | bauzas | we haven't said it in the spec but that looks ok to me | |
| 12:16:37 | sean-k-mooney | well the old way of doing cpu pinning is deprecated for removal | |
| 12:17:24 | sean-k-mooney | and the old way ( if the vm has cpu_thread_policy=isolate and the host has hyperthread) claims 2 host cpus per guest cpu | |
| 12:17:39 | sean-k-mooney | bauzas: i think your code will handel that but if we want to allow that we need to test it | |
| 12:17:58 | sean-k-mooney | so either raise an error or we need to test that in a fucntional test later in the series | |
| 12:17:58 | bauzas | I don't want to support the old config | |
| 12:18:08 | sean-k-mooney | works for me | |
| 12:18:34 | bauzas | so I'd rather prefer to hardstop at startup if power management is set with legacy pinning config | |
| 12:18:53 | bauzas | hence me saying about the non-discussed in the spec but I'm OK | |
| 12:18:57 | sean-k-mooney | i would like to remove the old config and pining logic next cycle if we can find time to do it | |
| 12:19:28 | sean-k-mooney | bauzas: yep we didnt dicuss it on the spec since we generally for get about the legacy pinning | |
| 12:19:39 | bauzas | cool | |
| 12:19:41 | bauzas | no worries | |
| 12:19:43 | sean-k-mooney | it was ment to be remvoed 2 or 3 releases ago | |
| 12:20:51 | bauzas | np | |
| 12:23:34 | bauzas | sean-k-mooney: one last question, can we both have cpu_dedicated_set and vcpu_pin_set defined ? | |
| 12:23:54 | bauzas | or are they mutually exclusive ? | |
| 12:25:19 | sean-k-mooney | cpu_dedicated_set and vcpu_pin_set are mutally exclsive | |
| 12:25:25 | bauzas | ++ | |
| 12:25:38 | sean-k-mooney | although that is checked later in the code not in the config option definition | |
| 12:25:53 | sean-k-mooney | ones in compute and the others in default | |
| 12:27:02 | sean-k-mooney | bauzas: you can simpley check "if CONF.compute.cpu_dedicated_set is None and CONF.libvirt.cpu_power_management: raise" | |
| 12:30:03 | bauzas | that was my guess | |
| 12:32:10 | sean-k-mooney | im going to swap to downstream stuff for a while unless pinged | |
| 12:32:31 | sean-k-mooney | bauzas: can we try and land dansmith's stable uuid seriese again today | |
| 12:32:50 | bauzas | sean-k-mooney: yup, I can try to take a look again | |
| 12:33:25 | sean-k-mooney | cool im +2 all the way up. dan did some rebases and fix a minior bug so they lost the +2s they had before | |
| 14:23:45 | ralonsoh | sean-k-mooney, hi! Do you know if this is a known error? | |
| 14:23:46 | ralonsoh | https://zuul.opendev.org/t/openstack/build/0d9af8c4e906422a9e4a27b1b849f315 | |
| 14:23:55 | ralonsoh | This is the second recheck with this error | |
| 14:25:34 | sean-k-mooney | no thats not a know error | |
| 14:25:42 | sean-k-mooney | although the volume tests can be flaky | |
| 14:26:02 | sean-k-mooney | this looks unrealted | |
| 14:29:09 | sean-k-mooney | i think there may have been an OOM issue | |
| 14:29:28 | sean-k-mooney | https://zuul.opendev.org/t/openstack/build/0d9af8c4e906422a9e4a27b1b849f315/log/controller/logs/screen-memory_tracker.txt#4768 | |