| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-16 | |||
| 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 | |
| 17:26:21 | tframbo | https://docs.openstack.org/nova/latest/user/flavors.html | |
| 17:26:27 | sean-k-mooney | so its also the only stanard extrapec the filter uncondtionally checks | |
| 17:26:32 | bauzas | this filter doesn't care a single bit about standard extra specs | |
| 17:27:14 | bauzas | you're confusing with the other filters that do care of those prefixes | |
| 17:27:23 | sean-k-mooney | bauzas: no im not | |
| 17:27:40 | bauzas | honestly, I'm done for the day | |
| 17:27:43 | sean-k-mooney | to be clear the bug was intoduceing hide_hypervisor_id without a namespace | |