Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-04-16
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 bauzas I just feel it's a configuration problem
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: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 sean-k-mooney its a stanard one and its the only standard extra spec without a namesapce
17:26:08 bauzas (if you use this filter)
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?

Earlier   Later