| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-02-03 | |||
| 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 | bauzas | I don't want to support the old config | |
| 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: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 | |
| 14:29:38 | ralonsoh | right, thanks | |
| 14:30:23 | sean-k-mooney | look like nova api lost access to the db | |
| 14:30:51 | sean-k-mooney | so my guess is mariadb was killed | |
| 14:31:03 | sean-k-mooney | same for keystone it los db connection | |
| 14:31:06 | ralonsoh | that usually happens with a oom | |
| 14:31:23 | ralonsoh | did you try reducing the parallelism in the functional tests? | |
| 14:31:23 | sean-k-mooney | yep so we might need to add a little more swap to that job | |
| 14:31:37 | sean-k-mooney | its not the funcitonl tests its tempest | |
| 14:31:43 | ralonsoh | right, in tempest | |
| 14:33:20 | sean-k-mooney | its currently 4 which should be ok | |
| 14:33:36 | ralonsoh | we reduced some jobs to 3 or 2 | |
| 14:33:41 | ralonsoh | to avoid this issue | |
| 14:33:47 | sean-k-mooney | i think this is using the default swap of 1G we shoudl set it to 8 | |
| 14:34:01 | sean-k-mooney | we could reduce concernace too but i would try swap first | |
| 14:34:07 | ralonsoh | perfect | |
| 14:34:46 | sean-k-mooney | we crrently set concurancy here https://github.com/openstack/os-vif/blob/master/.zuul.yaml#L23 | |
| 14:35:58 | sean-k-mooney | you can add configure_swap_size: 8192 there as well | |
| 14:36:18 | sean-k-mooney | if that does not work then set concurancy to 3 | |
| 14:36:18 | ralonsoh | should I do in a separate patch? | |
| 14:36:26 | sean-k-mooney | ya lets make it a spereate patch | |
| 14:36:32 | ralonsoh | cool, give me 1 min | |
| 14:36:44 | sean-k-mooney | then we can just recheck yours if it passes once its merged | |
| 14:39:37 | opendevreview | Rodolfo Alonso proposed openstack/os-vif master: Increase the swap size to 8GB in tempest jobs https://review.opendev.org/c/openstack/os-vif/+/872655 | |
| 14:40:45 | sean-k-mooney | ok lets leave that run but we should be able to merge both by the end of the day all going well | |