| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-02-08 | |||
| 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 | opendevreview | Merged openstack/placement master: Modify the placement API policies defaults and scope_type https://review.opendev.org/c/openstack/placement/+/865618 | |
| 18:39:48 | sean-k-mooney | i dont think we shoudl check if its explictly powersave or anything like that | |
| 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 | gibi | sean-k-mooney: so we will say in the doc that the startegy can only be change via host reboot? | |
| 18:50:35 | sean-k-mooney | so we start form a clean state | |
| 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. | |
| 09:48:44 | gibi | sure tha is also an option | |
| 09:49:21 | Uggla | gibi, if not in the object where do you put the act code ? | |
| 09:50:30 | gibi | the libvirt specific classes can be placed to nova.virt.libvirt afaik. But as you said bauzas and johnthetubaguy might want not to have them as ovos even if they are moved under nova.virt.libvirt | |
| 09:51:13 | gibi | so to expedite things I think it would be easier to simply remove the driver specific ovo | |
| 09:51:34 | gibi | and create functions in the driver that takes a generic ShareMapping and do the driver specific bits with it | |
| 09:52:20 | gibi | I guess that what bauzas and johnthetubaguy suggests | |
| 09:54:23 | Uggla | gibi, yes sounds like it is ok for you to go in this way as well ? | |
| 09:54:46 | gibi | yeah, OK with me | |
| 09:56:10 | Uggla | ok so I'll change the code in this way. | |
| 09:57:54 | bauzas | gibi: tbc, I w | |
| 09:58:10 | bauzas | I don't want to have a ovo inheritance for this object | |
| 09:58:18 | bauzas | Uggla: ^ | |
| 09:58:46 | bauzas | since this object is not about providing fields or persisting them | |
| 09:58:53 | bauzas | by RPC calls | |
| 09:59:15 | bauzas | but yeah, we can have a specific python object for it in libvirt if Uggla wants | |
| 09:59:28 | Uggla | bauzas, yep I will keep only the ShareMapping / ShareMappingList objects | |
| 10:00:36 | Uggla | bauzas, not sure it is worth creating an object in the libvirt part, I'll see. | |
| 10:05:37 | gibi | bauzas: we have examples where we use ovo not just to persist data. We even have examples to driver specific data in ovo https://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py | |
| 10:06:19 | gibi | or driver specific ovo | |
| 10:09:27 | bauzas | gibi: sure, but those objects are used for being passing between services | |
| 10:10:07 | bauzas | and those are only for fields | |