| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-16 | |||
| 15:57:27 | dansmith | sean-k-mooney: yes | |
| 15:57:38 | bauzas | dansmith: found why | |
| 15:57:40 | sean-k-mooney | ya it should get the exising ones and reuse them | |
| 15:57:45 | bauzas | dansmith: sec, giving you the link | |
| 15:59:03 | gibi | nova meeting starts in 2 minutes on #openstack-meeting-3 | |
| 16:15:02 | melwitt | lbragstad: hey, would you be able to take a gander at the proposed doc patch for policy scopes https://review.opendev.org/#/c/720129/4/doc/source/configuration/policy-new-defaults.rst we want to have it clearly explain what are the action items for operators to be able to use scope types. i.e. will it involve creating new keystone roles and stuff like that | |
| 16:15:34 | lbragstad | melwitt sure - i can take a look | |
| 16:15:50 | melwitt | thanks | |
| 16:21:14 | gmann | melwitt: quota class policy is ready, fixed the johnthetubaguy comment of removing the non-working 'quota_class' from check_str - https://review.opendev.org/#/c/720042/1 | |
| 16:21:18 | gmann | and its base pacth | |
| 16:21:56 | melwitt | gmann: ok, I wanted johnthetubaguy to look at it first and make sure he's ok with it since he found issue earlier | |
| 16:22:09 | gmann | melwitt: ok | |
| 16:32:39 | gmann | melwitt: thanks for nice feedback on doc patch, I will wait for other comments and update it together. | |
| 16:33:22 | melwitt | gmann: cool sounds good | |
| 16:54:38 | bauzas | dansmith: hah, found why it wasn't leaving a VGPU allocated when you revert https://github.com/openstack/nova/blob/e1359567e4985e9a671359d4c0d53404a8ba64ab/nova/virt/libvirt/driver.py#L6359 | |
| 16:54:50 | bauzas | dansmith: we redefine the existing domain here ^ | |
| 16:56:44 | dansmith | I don't think that answers my question | |
| 16:57:49 | dansmith | when we delete a domain, do we unallocate the mdev, or do we just leave it so the next spawn that needs one will find it? | |
| 17:00:17 | bauzas | dansmith: when we undefine the domain, the mdev is consider being freed | |
| 17:00:41 | dansmith | bauzas: "considered free" because it's not assigned to any other domain, but still allocated in sysfs yeah? | |
| 17:00:45 | sean-k-mooney | tframbo: so first the namespace should be hw not os | |
| 17:00:52 | bauzas | dansmith: yup, you're correct | |
| 17:01:22 | sean-k-mooney | that extra specs as you know alter the cpu feature flags reported to the guest and hide the fact its a vm form guest os | |
| 17:01:24 | tframbo | so it is add compat for "hw:hide_hypervisor_id” | |
| 17:01:25 | tframbo | ? | |
| 17:01:51 | sean-k-mooney | yes so we woudl deprecate hide_hypervisor_id and add "hw:hide_hypervisor_id” | |
| 17:02:18 | sean-k-mooney | make the diver accept both and then we could special case hide_hypervior_id so it is ignored | |
| 17:02:45 | bauzas | sean-k-mooney: wait, like I said, the bug is about a scheduling decision | |
| 17:02:51 | bauzas | sean-k-mooney: not a boot problem | |
| 17:02:58 | sean-k-mooney | you would use aggregate_instance_extra_specs:hide_hypervior_id in the flavor if you wanted to match on it in that filter | |
| 17:03:04 | bauzas | OVH and a couple of other folks already use this knob | |
| 17:03:13 | bauzas | which was merged a while ago | |
| 17:03:36 | sean-k-mooney | bauzas: yes i know but the current filter check all unamesapced extra specs and assert they are in the metadta | |
| 17:03:40 | bauzas | but here, what's wanted is to trick the scheduler to automatically accept hosts on such things | |
| 17:04:13 | bauzas | sean-k-mooney: I explained this can be achieved without amending with this filter | |
| 17:04:36 | sean-k-mooney | how | |
| 17:04:46 | bauzas | tbc : create a flavor with a random keypair | |
| 17:05:07 | bauzas | create aggregates with hosts having the same keypair in their metadata | |
| 17:05:11 | bauzas | and you're done | |
| 17:05:26 | bauzas | you don't need to use this specific property | |
| 17:05:44 | sean-k-mooney | your missing the point | |
| 17:06:00 | bauzas | which is ? | |
| 17:06:19 | sean-k-mooney | we cant remove hide_hypervisor_id form the current flavor without changing the behavior | |
| 17:06:44 | sean-k-mooney | and if it si set in the falvor then the AggregateInstanceExtraSpecsFilter will check it against the aggreate metadata | |
| 17:07:19 | dansmith | bauzas: so my point is, when we revert, we will allocate a second mdev, and then abandon the original one when we redefine.. but I guess like delete, that one then will get used the next time we go to spawn something that needs one | |
| 17:07:24 | sean-k-mooney | we will return false here https://github.com/openstack/nova/blob/46a3bcd80b41e99ec4923c7cf3d0f8dd8505e97c/nova/scheduler/filters/aggregate_instance_extra_specs.py#L62-L68 | |
| 17:07:58 | sean-k-mooney | tframbo: the other workaround is to add the metadata key to the aggreate | |
| 17:08:22 | bauzas | dansmith: you're probably correct, we could have a window in time where we would allocate a new mdev and then releasing the other | |
| 17:08:51 | bauzas | which could lead to a capacity issue if you're out of available mdevs | |
| 17:08:59 | sean-k-mooney | tframbo: basically create two two new agggreats that conatin all hosts, then add hide_hypervisor_id=true to one and hide_hypervisor_id=false to the other | |
| 17:09:31 | bauzas | dansmith: s/probably/absolutely after re-thinking of it | |
| 17:09:35 | dansmith | bauzas: right | |
| 17:09:39 | sean-k-mooney | tframbo: if you do that we will get to here https://github.com/openstack/nova/blob/46a3bcd80b41e99ec4923c7cf3d0f8dd8505e97c/nova/scheduler/filters/aggregate_instance_extra_specs.py#L69-L71 and break then return ture and pass the host | |
| 17:10:07 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Add new default roles in remaining servers policies https://review.opendev.org/720116 | |
| 17:10:10 | openstackgerrit | Merged openstack/nova master: Fix servers policy for admin_or_owner https://review.opendev.org/718501 | |
| 17:10:11 | bauzas | dansmith: this being said, I don't exactly know how to fix this, since this is the whole revert workflow | |
| 17:10:26 | openstackgerrit | Merged openstack/nova master: Add test coverage of existing server policies https://review.opendev.org/718348 | |
| 17:10:32 | bauzas | and I guess the problem remains for all other resources | |
| 17:10:38 | openstackgerrit | Merged openstack/nova master: Introduce scope_types in servers Policies https://review.opendev.org/662968 | |
| 17:10:48 | openstackgerrit | Merged openstack/nova master: Add new default roles in servers policies https://review.opendev.org/662971 | |
| 17:10:55 | openstackgerrit | Merged openstack/nova master: Add test coverage of existing server attributes policies https://review.opendev.org/719726 | |
| 17:11:04 | openstackgerrit | Merged openstack/nova stable/train: Reset the cell cache for database access in Service https://review.opendev.org/718934 | |
| 17:11:16 | openstackgerrit | Merged openstack/nova stable/stein: Use placement stable version for functional job https://review.opendev.org/719121 | |
| 17:11:51 | bauzas | dansmith: we could consider to special case the revert and find the existing mdev so we would pass it again instead of calling _allocate_mdevs() tho | |
| 17:12:05 | dansmith | bauzas: seems like that would be better | |
| 17:12:11 | dansmith | also avoids needing to grab the allocations right? | |
| 17:12:22 | bauzas | indeed | |
| 17:12:25 | bauzas | dansmith: <3 with love | |
| 17:12:44 | bauzas | that's what I call a top-notch review | |
| 17:15:33 | sean-k-mooney | tframbo: does my responce on the bug or above make sense too you | |
| 17:15:42 | tframbo | sean-k-mooney: yeah, we could also add the aggregate metadata "hide_hypervisor_id=true" to solve the bug. | |
| 17:16:02 | sean-k-mooney | yes | |
| 17:16:13 | tframbo | thank you sean-k-mooney and bauzas | |
| 17:16:23 | sean-k-mooney | do you also use the compute capability filter by the way | |
| 17:16:33 | sean-k-mooney | if not be aware it has the same behavior | |
| 17:16:52 | tframbo | I think the hide_hypervisor_id is bool before , sorry | |
| 17:17:57 | sean-k-mooney | tframbo: its is a bool that default to false if unset | |
| 17:18:11 | sean-k-mooney | so there isnt really a reason to ever set it to false explcitly | |
| 17:18:16 | bauzas | tframbo: sorry I dropped focus | |
| 17:18:21 | sean-k-mooney | unless you want to prevent images form settin git too true | |
| 17:20:58 | bauzas | sean-k-mooney: tframbo: so, again, the intent is to have the aggregate metadata key to be set | |
| 17:21:18 | tframbo | sean-k-mooney: yes ,we alse use the compute capability filter by the way | |
| 17:21:36 | tframbo | bauzas: yes | |
| 17:22:19 | bauzas | I honestly don't get the problem | |
| 17:22:20 | sean-k-mooney | tframbo: ok so the point i wanted to make in that case was that if you use both then you are required to not use unnamespaced keys or they can conflict | |
| 17:23:10 | bauzas | wait | |
| 17:23:26 | bauzas | the namespace is only for specifying that the key is for this filter | |
| 17:23:43 | sean-k-mooney | bauzas: the problem is the AggregateInstanceExtraSpecsFilter require that an aggreate assocaited with a host must have all extra specs in the aggregate_instance_extra_specs as well as all non namespaced extra specs in the metadta | |
| 17:23:43 | bauzas | I just feel it's a configuration problem | |
| 17:24:25 | bauzas | or prefixed by aggregate_instance_extra_specs https://github.com/openstack/nova/blob/46a3bcd80b41e99ec4923c7cf3d0f8dd8505e97c/nova/scheduler/filters/aggregate_instance_extra_specs.py#L54 | |
| 17:24:29 | bauzas | and this is not a bug | |
| 17:24:30 | sean-k-mooney | bauzas: the filter also looks at non namespced extra specs and we only have 1 standard extra specs that falls into that catagory | |
| 17:25:09 | bauzas | this *works* with any random key | |
| 17:25:17 | bauzas | I don't get the point again | |
| 17:25:36 | bauzas | if you're using this flavor key for passing QEMU, fine | |
| 17:25:47 | bauzas | that's how it's intended to be used | |
| 17:25:53 | sean-k-mooney | hide_hypervisor_id is not a random custom key | |
| 17:25:59 | bauzas | but then, indeed, you need to have aggregates matching it | |
| 17:26:08 | bauzas | (if you use this filter) | |
| 17:26:08 | sean-k-mooney | its a stanard one and its the only standard extra spec without a namesapce | |