Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-27
15:25:08 cdent yes, it's not there
15:25:31 efried We do support positive traits from image props, nah?
15:25:42 sean-k-mooney efried: we do
15:25:46 trident efried: Yes.
15:25:53 efried wherezat code?
15:26:13 cdent it's a thing that ought to exist, I think it just didn't happen yet. Perhaps because we couldn't decide precedence? I can't remember
15:26:22 sean-k-mooney in the schduler uitls i think one sec
15:26:32 trident efried: https://github.com/openstack/nova/blob/771e102654b54d257f437f16be2aeeef51fa0783/nova/scheduler/utils.py#L186
15:27:25 sean-k-mooney yep ^ which is called by resources_from_request_spec
15:28:36 sean-k-mooney and i think but i woudl have to confim when we build the request spec we grab the image properties form a voluem in the bfv case so it should also work in that case too
15:29:52 sean-k-mooney there may however be the same issue. the traits are part of the unnumbered request group which is matched agaist the compute node RP but if we did nto requerst any resouces form it that would not work correctly
15:30:32 efried sean-k-mooney: Yup, same issue we've been thrashing in mriedem's multiattach capability patch.
15:30:58 gibi sean-k-mooney: we don't have to solve the empty unnumbered group issue yet. That will happen when NUMA is modelled in placement. And it requires a lot of other changes
15:31:04 efried I think we decided that *right now* we always request resources in the unnumbered group for the root RP.
15:31:10 efried so <sweep under rug>
15:31:15 efried <fingers in ears>
15:31:16 sean-k-mooney gibi: ya that true
15:31:19 efried <problem for future us>
15:31:24 trident So supporting forbidden traits in image props as well seems like a clear case of something that would make sense to implement. But out of scope of what I am working on with this bug. (had it just been a similar bug as with forbidden traits in flavors I might as well just have fixed it while I was at it).
15:31:51 sean-k-mooney efried: easy fix. all compute node have a inventory of instnace which match the max instance per host value that in the nova.conf
15:32:12 sean-k-mooney and all vms in the futre requerst one allocation of an instance resouce class
15:32:15 efried trident: Right. I agree it's something that makes sense to implement, though we would probably want a clear use case / demand before prioritizing the work.
15:32:31 sean-k-mooney efried: we could allso kill one of the filter if we did that
15:33:06 sean-k-mooney specifically we could kill this filter https://github.com/openstack/nova/blob/master/nova/scheduler/filters/num_instances_filter.py
15:33:09 efried sean-k-mooney: that would require us to tack on that "resource" to every request
15:33:19 sean-k-mooney efried: yes
15:33:22 efried because we can't expect it to be in flavors.
15:33:32 sean-k-mooney but i think that would be fine
15:33:42 sean-k-mooney it requires not change to placement
15:33:55 sean-k-mooney and this is really a nova specific issue with a nova specific fix
15:33:57 efried trident: So given that it's *not* supported currently, do you actually have any other way of introducing a forbidden trait that doesn't get picked up by the scheduler?
15:34:13 efried trident: but still conforms to the rules ("you can't have a request group with no resources")?
15:35:07 efried sean-k-mooney: Isn't there a way to set max instances per host to unlimited?
15:35:55 sean-k-mooney yes but i think max int is close enough
15:36:13 gibi famous last words :)
15:36:26 trident efried: Yes. My initial issue and what the bug report / review is about is when introducing it from flavors. So, that still stands.
15:36:44 efried sean-k-mooney: Could work. Seems like a pretty heavy solution, not sure if it's worth it. But we could talk about it at the PTG if you're into it.
15:36:46 sean-k-mooney efried: if you think there may be some merrit in it i can write up a short spec proposing how it would work
15:36:46 trident efried: And that is supposed to be supported.
15:37:16 efried trident: Okay, so the very best thing you could do is what gibi suggested: come up with a functional test case that demonstrates it being broken.
15:37:23 sean-k-mooney we could proably make it opt in via a config and a prefilter.
15:37:35 efried sean-k-mooney: I have no opinion. It seems like a messy solution to something that's not really a problem.
15:37:42 efried (I guess that's an opinion)
15:38:05 efried sean-k-mooney: I just think we have more important things to focus on for train.
15:38:20 efried it would be nice if we had a future/wishlist bucket we could put such things into...
15:38:21 sean-k-mooney sure it was just a taught
15:38:38 trident efried: That, I have done. Looking at unit tests at the moment as well.
15:39:05 efried trident: Sweet. We'll look for that patch. Thanks for working on this!
15:39:10 sean-k-mooney i might just write it up as a way to migrate eh numInstacnefilter to placemetn and we can decide to do it if/when it makes sense
15:39:17 efried sean-k-mooney: ++
15:40:59 gibi trident: hit me with the patch adding the functional test and I can review it quickly
15:42:58 trident I have two questions related to the functional tests and the review flow. First, Looking at the test on https://github.com/openstack/nova/blob/771e102654b54d257f437f16be2aeeef51fa0783/nova/tests/functional/test_servers.py#L4524 , wouldn't that one potentially pass around 50% of the times even if the required flavor didn't work? When it ends up on the expected node out of chance?
15:44:31 gibi trident: looking
15:45:14 sean-k-mooney trident yes and no
15:45:22 sean-k-mooney the use of VMX is unfortuete
15:45:36 sean-k-mooney the upsteam ci does not provide nested vert
15:45:54 gibi sean-k-mooney: this is functional test so no virt layer is involved
15:46:06 sean-k-mooney ya just realised that
15:46:14 sean-k-mooney as i was typeing
15:46:15 efried trident: The point of that test is that the flavor (not the image) contains the required trait
15:46:26 gibi trident: I think this test can pass half of the time
15:46:27 trident The second one, how should I submit the functional tests for review? In the same commit or some other way to get it in as a "related change"? I imagine we would like to first see it fail and then not fail with the patch.
15:46:31 efried trident: So it should always land on the host that exposes that trait.
15:47:02 efried trident: A good way to do it is to propose a patch with the test code that demonstrates the broken behavior with a FIXME or TODO comment describing what's supposed to happen, and Related-Bug: #XXXXX in the commit message
15:47:14 gibi trident: I agree with efried ^^
15:47:34 sean-k-mooney well it depend on if you are fixing a bug or writing a new feature
15:47:42 efried trident: Then propose the fix as a separate commit on top of that one (in a series) which removes the FIXME and demonstrates the correct behavior. This one includes Closes-Bug: #XXXXX in the commit message.
15:47:48 efried sean-k-mooney: We're talking about a bug
15:48:07 sean-k-mooney ah then i think we have a doc for this
15:48:13 efried sean-k-mooney: bug 1821824
15:48:14 openstack bug 1821824 in OpenStack Compute (nova) "Forbidden traits in flavor properties don't work" [Undecided,In progress] https://launchpad.net/bugs/1821824 - Assigned to Magnus Bergman (magnusbe)
15:48:48 trident efried: Exactly, and it verifies that it ends up on the correct host. But, if traits are not passed it will end up on any one of the two nodes, so the verification will pass when it by chance end up on the host expected by the test.
15:49:09 sean-k-mooney in that case yes two patch one that intoduces the fuctional test that intentonlly assert the broken behavior or has the skip expect fail decortor and the a follow up that fixes it is the best way to go
15:49:20 efried trident: Correct. But we don't (or shouldn't) have a test case that behaven that way.
15:49:22 efried behaves*
15:52:21 trident efried: So, I should remove the fix from my branch, add the test cases and re submit for review. And then add the fix and submit again. Right?
15:53:10 trident efried: Yeah, so I am right in my conclusion that the current test that I pointed to is buggy and should be fixed as well?
15:53:54 gibi trident: yes, I think so
15:55:39 efried gibi, trident: I don't see the bug in the test
15:56:51 gibi efried: there are two hosts and only compute1 has the required trait
15:57:04 efried gibi: Right. That's the whole point of the test.
15:57:12 efried the flavor is requiring that trait
15:57:17 efried so we should always land on the host with that trait.
15:57:20 gibi efried: so if the required trait handling is broken then this test only pass by chance
15:57:29 efried oic
15:57:31 efried right
15:57:46 efried but trident wasn't saying the *required* trait handling was broken.
15:57:54 efried He was saying the *forbidden* trait handling was broken.
15:58:01 efried (I thought)
15:58:17 efried so duplicating that test case with forbidden traits would be a good start to the test case.
15:58:46 efried you could implement a weigher that would normally prefer the "wrong" host so you could get consistent results.
15:59:17 efried like this: https://review.openstack.org/#/c/647811/1/nova/tests/functional/test_json_filter.py
15:59:39 openstackgerrit Dan Smith proposed openstack/os-traits master: Add COMPUTE_IMAGE_TYPE_SUPPORT_* traits https://review.openstack.org/648147
15:59:41 trident efried: That's my point. If duplicating that test, I'll get a test for forbidden traits that also will pass half of the times even if the forbidden trait is not passed.
15:59:59 efried trident: see above, implement a weigher to make the scheduler behavior deterministic
16:00:49 trident efried: Ah, nice. That sounds like a good solution. I have already implemented a test that sets the forbidden trait on both hosts and verifies that it gets into error.
16:01:00 efried sweet
16:01:19 efried ...gets into error only after your fix is in place, though?
16:01:25 trident Yes.
16:01:27 gibi efried, trident: I would suggest to disable the host that has the trait and asser that the boot fails

Earlier   Later