| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-16 | |||
| 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 | |
| 17:27:52 | tframbo | sean-k-mooney: thank you, I will sleep , good night | |
| 17:28:01 | sean-k-mooney | tframbo: night o/ | |
| 17:28:39 | melwitt | sorry I didn't understand all the previous details but why does the filter require a namespace in order to work? should it be able to work with namespaced and non-namespaced extra specs? | |
| 17:28:51 | melwitt | *shouldn't it | |
| 17:29:35 | sean-k-mooney | melwitt: the filter iterages over all extra specs in the flavor and then asserts they match the metadta if and only if the start with the filters namespace or they have no namespce | |
| 17:29:58 | bauzas | melwitt: that's my thinkings | |
| 17:30:03 | sean-k-mooney | so for all other standard extra specs they are ingore because they have a namespace which is not the filters one | |
| 17:30:06 | bauzas | but apparently we need to namespace now... | |
| 17:30:35 | sean-k-mooney | since this extra spec has no namespace its check by defualt which no other standar extraspec is | |
| 17:30:41 | sean-k-mooney | so there is a behavioral difference | |
| 17:30:50 | bauzas | sean-k-mooney: so | |
| 17:30:52 | bauzas | sean-k-mooney: https://github.com/openstack/nova/blob/46a3bcd80b41e99ec4923c7cf3d0f8dd8505e97c/nova/scheduler/filters/aggregate_instance_extra_specs.py#L55-L58 | |
| 17:30:57 | melwitt | sean-k-mooney: wait but you say "if they have no namespace", doesn't this have no namespace and therefore should be considered? | |
| 17:31:09 | melwitt | gah this is so confusing | |
| 17:31:15 | bauzas | sean-k-mooney: this conditional is here to *PREVENT* other standard keys are ARE prefixed to be read | |
| 17:31:28 | sean-k-mooney | melwitt yes this has no namespace an by the filter logic should be check | |
| 17:31:50 | sean-k-mooney | melwitt: however if you add any other standard extra spec you do not have to update the metadta to boot a vm | |
| 17:31:52 | melwitt | so ... what's the problem? that makes it sound like there's no bug | |
| 17:31:55 | sean-k-mooney | for this extra spec you do | |
| 17:32:02 | bauzas | melwitt: there is NO bug in my mind | |
| 17:32:19 | bauzas | IMHO the bug should be consider Invalid if not Expired | |
| 17:32:44 | sean-k-mooney | bauzas: i strongly dissagre. as i said the bug is not in the filter | |
| 17:32:57 | sean-k-mooney | the bug is that we added a stanard extra spec without a namespace | |
| 17:33:19 | bauzas | and what's the impact then ? | |
| 17:35:04 | sean-k-mooney | by intoducing a flavor extra spec without a namespace, to use the feature enable by that extra spec it addtionally required the operator to update the aggreate metatad and host capabilty if they use the ComputeCapabilitiesFilter or AggregateInstanceExtraSpecsFilter | |
| 17:35:05 | bauzas | oh, the fact that you need to create aggregates in order to use it, let me bet ? | |
| 17:35:13 | sean-k-mooney | yes | |
| 17:35:50 | sean-k-mooney | that is a behavioral differen form every other extra spec that is defined by nova | |
| 17:35:55 | bauzas | holy f..., gotcha | |
| 17:36:38 | sean-k-mooney | melwitt: do you follow too ^ | |
| 17:36:48 | bauzas | okay, so indeed the fix is not about the filter | |
| 17:36:59 | bauzas | it's about where we use this extraspec for booting | |
| 17:37:01 | melwitt | I see, ok. just read through the bug and comments again. yeah, I think so. this is an extra spec meant to be used without having to add a matching metadata key on an aggregate | |
| 17:37:27 | sean-k-mooney | ya so the fix is add a namespaced version of the extra spec and compat code | |
| 17:37:43 | sean-k-mooney | then deprecate teh non namespaced one | |
| 17:37:47 | melwitt | yeah, I think that makes sense. agree that's the way to fix | |
| 17:37:53 | bauzas | sean-k-mooney: easy fix then https://review.opendev.org/#/c/555861/10/nova/virt/libvirt/driver.py | |
| 17:38:25 | bauzas | sean-k-mooney: just add another key there with a prefix and just provide a deprecation warning for the existing one | |
| 17:38:39 | sean-k-mooney | yep | |
| 17:38:45 | bauzas | that's all flavors and aggregates, we don't need to care about the interop | |
| 17:39:11 | sean-k-mooney | im wondering if we should also add a config option for the filter to disable checking unnamesapced extraspecs | |
| 17:39:14 | bauzas | tlrambo dropped but I'll leave a comment in the bug | |
| 17:39:22 | bauzas | sean-k-mooney: oh please don't | |
| 17:39:29 | bauzas | sean-k-mooney: it was a review problme | |
| 17:39:34 | bauzas | not a code problem | |