| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-02-08 | |||
| 18:30:40 | bauzas | I mean, if I'm an operator that already sets governor levels, then I should NOT let nova play with my cores | |
| 18:31:13 | sean-k-mooney | so (govoner->state) we can detect that there is a govoner differnce on the cpus, (govoner<-state) we can detect the offline cores | |
| 18:31:28 | bauzas | sean-k-mooney: I don't want to add logic while I can add documentation about supported features | |
| 18:31:56 | sean-k-mooney | i agree on documenting that you cant cahnge this with instances on the host | |
| 18:32:05 | sean-k-mooney | and shoudl reboot if you do change it or something like that | |
| 18:32:18 | sean-k-mooney | but we can detect and error or fix it fi we want too | |
| 18:32:45 | sean-k-mooney | if we have CONF.libvirt.cpu_power_management=true i think that shoudl mean you cannot manage the cpus outside of nova | |
| 18:32:49 | gibi | again, becase we apply the config to the cpus at init_host regardless of any instance on the host you don't need an instance on the host to cause problems | |
| 18:33:11 | sean-k-mooney | gibi: we dont your corect | |
| 18:33:46 | sean-k-mooney | but we can detect it and document that you should not do this without a host reboot | |
| 18:33:53 | bauzas | I'm confused | |
| 18:34:02 | gibi | sean-k-mooney: I agree to document | |
| 18:34:07 | gibi | sean-k-mooney: and detect if possible | |
| 18:34:35 | sean-k-mooney | detect is possibel but i think bauzas would prefer to not require that for the feature to merge and maybe to it later | |
| 18:35:06 | sean-k-mooney | (govoner->state) the concer is we leave some cpus in low perfomacne mode | |
| 18:35:13 | sean-k-mooney | (state-) | |
| 18:35:26 | sean-k-mooney | (state->govoner) the concern is we leave some cores offline | |
| 18:35:44 | sean-k-mooney | we can detect boot and make it a hard error from init_host | |
| 18:36:03 | sean-k-mooney | in either case a host reboot will resolve it or they operator can fix it | |
| 18:37:35 | gibi | yepp | |
| 18:38:47 | sean-k-mooney | bauzas: does ^ make sense | |
| 18:39:06 | sean-k-mooney | the offline core check is triival its just https://review.opendev.org/c/openstack/nova/+/821228/5/nova/virt/libvirt/host.py#755 | |
| 18:39:27 | sean-k-mooney | the inconsitent govoner state is also trivial | |
| 18:39:48 | sean-k-mooney | i dont think we shoudl check if its explictly powersave or anything like that | |
| 18:39:48 | opendevreview | Merged openstack/placement master: Modify the placement API policies defaults and scope_type https://review.opendev.org/c/openstack/placement/+/865618 | |
| 18:40:22 | sean-k-mooney | just in cpu_sate mofe assert the govoner is the same for all cores in cpu_dedicated_set | |
| 18:41:29 | sean-k-mooney | s/mofe/mode/ | |
| 18:41:40 | bauzas | I just want to clarify | |
| 18:42:28 | bauzas | are we OK with doing this in init_host() ? | |
| 18:42:47 | gibi | yes | |
| 18:42:48 | sean-k-mooney | yep | |
| 18:42:58 | bauzas | OK | |
| 18:43:36 | bauzas | and in init_host(), do we agree on detecting that if power strategy is set to governor, all cores should be up ? | |
| 18:43:47 | sean-k-mooney | i would prefer if you put these in a new fucntions that you invoked form there for simpler testing | |
| 18:44:00 | sean-k-mooney | bauzas: yes | |
| 18:44:22 | sean-k-mooney | if its set to govoner the old requiremnt that all core must be up should still apply | |
| 18:44:33 | bauzas | https://review.opendev.org/c/openstack/nova/+/868237/9/nova/virt/libvirt/driver.py#826 | |
| 18:44:47 | bauzas | I already call power_down_all_cores() | |
| 18:45:03 | bauzas | in power_down_all_cores() I can do two checks | |
| 18:45:08 | bauzas | besides the existing one | |
| 18:45:31 | sean-k-mooney | you should not over complicate that function | |
| 18:45:36 | bauzas | 1/ if strategy is set to governor, all cpus need to be online | |
| 18:46:10 | bauzas | 3/ if strategy is set to cpu_state, then all governors should be identical | |
| 18:46:16 | bauzas | s/3/2 | |
| 18:46:32 | sean-k-mooney | i would prefer a new validate_cores() or similr function and only call libvirt_cpu.power_down_all_dedicated_cpus() if the mode is state | |
| 18:47:00 | sean-k-mooney | bauzas: yes those are the two checks that i think you shoudl do | |
| 18:47:08 | sean-k-mooney | but not in libvirt_cpu.power_down_all_dedicated_cpus() | |
| 18:47:36 | sean-k-mooney | do it ins libvirt_cpu.validate_all_cpus() | |
| 18:47:39 | bauzas | I can manage that request | |
| 18:47:40 | gibi | for 2/ if all cores has governor low as nova set it to that before the reconfiguration then we have a problem still | |
| 18:47:49 | sean-k-mooney | gibi: no | |
| 18:47:53 | sean-k-mooney | we cant check for that | |
| 18:48:04 | sean-k-mooney | because the admin might have set it to low intentionally | |
| 18:48:12 | sean-k-mooney | or to any other value | |
| 18:48:36 | bauzas | this is why I think docs is important | |
| 18:48:53 | bauzas | (which is missing, but we can write it before RC1) | |
| 18:48:55 | sean-k-mooney | thats why i stated the requiremnt that they should all be the same if the power state is manged by nova | |
| 18:49:06 | bauzas | I'm OK with this | |
| 18:49:12 | bauzas | nova will try to detect | |
| 18:49:39 | bauzas | if the operator explicitely manages all the governors and wants to set nova to turn off cores, that's his choice | |
| 18:49:46 | gibi | I undrestand that we cannot catch that if the operator changed a governor directly. And I agree that we should not be able to catch that. But if an empty compute was configured with governor strategy, then reconfigured to cpu_state strategy then that compute will have all dediceted cpus in low performance mode for ever | |
| 18:49:55 | bauzas | he'll probably end up with problems, but meh, unsupported | |
| 18:50:11 | sean-k-mooney | gibi: that is fixed by the host reboot | |
| 18:50:28 | sean-k-mooney | that why i think we need to document that if you want to change this you should reboot the host | |
| 18:50:35 | sean-k-mooney | so we start form a clean state | |
| 18:50:35 | gibi | sean-k-mooney: so we will say in the doc that the startegy can only be change via host reboot? | |
| 18:50:52 | sean-k-mooney | that is what im suggestign yes | |
| 18:50:54 | gibi | OK | |
| 18:50:58 | gibi | that will solve it yes | |
| 18:50:59 | bauzas | gibi: the problem is that we can't assume that governor_high is the default value *before* | |
| 18:51:06 | gibi | bauzas: I know :) | |
| 18:51:13 | gibi | host reboot, it is | |
| 18:51:14 | bauzas | ok, so docs | |
| 18:51:40 | sean-k-mooney | docs and the 2 checks you wrote above please | |
| 18:51:45 | sean-k-mooney | docs is the most imporant | |
| 18:51:56 | sean-k-mooney | but i would like to see the check if possibel too | |
| 18:52:05 | gibi | works for me | |
| 18:52:15 | gibi | thanks folks for the discussion | |
| 18:52:19 | bauzas | ++ | |
| 18:52:28 | bauzas | will be working on it on the train tomorrow | |
| 18:53:53 | sean-k-mooney | i will be around for reviews tomorrow and then wednesday | |
| 18:54:05 | sean-k-mooney | so feel free to ping me | |
| 18:54:32 | sean-k-mooney | i would normaly take the week of valentines off but since it FF week im going to be here for wednesday-friday | |
| 18:57:39 | bauzas | don't feel obliged | |
| 18:58:02 | bauzas | we have already promised more than what we can offer | |
| 18:58:08 | bauzas | and we're short in time | |
| 18:58:20 | bauzas | the numbers will be terrible, but I can surely explain those | |
| 20:21:34 | opendevreview | Elod Illes proposed openstack/nova stable/train: DNM: CI test https://review.opendev.org/c/openstack/nova/+/873116 | |
| 21:09:56 | gmann | bauzas: with placement change merged (https://review.opendev.org/c/openstack/placement/+/865618) we can mark this BP as completed https://blueprints.launchpad.net/placement/+spec/policy-defaults-improvement | |
| #openstack-nova - 2023-02-09 | |||
| 03:26:09 | opendevreview | Yusuke Okada proposed openstack/nova master: Fix failed count for anti-affinity check https://review.opendev.org/c/openstack/nova/+/873216 | |
| 03:30:13 | opendevreview | Yusuke Okada proposed openstack/nova master: Fix failed count for anti-affinity check https://review.opendev.org/c/openstack/nova/+/873216 | |
| 06:05:34 | opendevreview | Nobuhiro MIKI proposed openstack/nova master: libvirt: Add 'COMPUTE_ADDRESS_SPACE_*' traits support https://review.opendev.org/c/openstack/nova/+/873221 | |
| 09:14:09 | Uggla | bauzas, gibi o/ | |
| 09:16:39 | Uggla | bauzas, gibi looking at the https://review.opendev.org/c/openstack/nova/+/839401/22 are we all agree to remove the ShareMappingLibvirt* although that is not following the spec and might be more difficult if we want to create a dedicated module (os-share) later ? | |
| 09:17:55 | Uggla | bauzas, gibi, of course doing that will simplify the code but I will have to review lot of stuff. | |
| 09:39:51 | bauzas | Uggla: I agree with you | |
| 09:40:58 | gibi | ...loading context | |
| 09:44:42 | gibi | For me the content of ShareMappingLibvirt is OK, it is just placed to a too public place. | |
| 09:45:48 | gibi | i.e. I'm OK to use inheritance to model that a ShareMapping my need to be mounted by a libvirt or by other virt driver | |
| 09:48:18 | Uggla | gibi, hum I think what bauzas and john expect is the removal of convert and inheritance and move that in the driver itself. | |