Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-27
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
16:01:35 efried that would work too.
16:01:53 gibi it is easier than implementing a weigher, I think
16:01:56 efried trident: So yeah, it's definitely important that we see your test case in "before" and "after" states.
16:02:08 trident Before the fix it doesn't get into error.
16:02:16 efried if you put it all in one patch, we have to pull down the patch and remove your fix manually and run the tests to see that they fail.
16:02:27 gibi I agree ^^
16:02:27 efried cool
16:02:47 efried ocraplookatthetime
16:04:24 trident Great. I'll clean it up a little, get one node disabled and submit something by tomorrow. Thanks everyone for the assistance!
16:05:43 gibi trident: thanks for the effort!
16:14:01 openstackgerrit Merged openstack/nova master: Add doc on VGPU allocs and inventories for nrp https://review.openstack.org/647519
17:12:55 openstackgerrit Balazs Gibizer proposed openstack/nova master: Prepare _heal_allocations_for_instance for nested allocations https://review.openstack.org/637954
17:12:56 openstackgerrit Balazs Gibizer proposed openstack/nova master: cache neutron ports in heal allocation https://review.openstack.org/638207
17:12:56 openstackgerrit Balazs Gibizer proposed openstack/nova master: nova-manage: heal port allocations https://review.openstack.org/637955
17:13:39 melwitt dansmith: do you think there would be any issues with having a module global ComputeAPI instance in console proxy? discussion in this patch of mine https://review.openstack.org/644998
17:14:17 dansmith melwitt: I think the only state computeapi maintains is the compute rpc instance right?
17:14:32 melwitt dansmith: I believe so
17:14:47 dansmith I don't remember how SIGHUP wires into triggering that to re-calculate the pins, so that would be my only concern I think
17:14:58 dansmith but otherwise probably fine
17:15:43 dansmith oh wait
17:15:49 dansmith you mean rpcapi/ComputeAPI yeah?
17:15:52 melwitt dansmith: ok, I'll look into it. thanks
17:15:55 melwitt yeah, I do. sorry
17:16:03 dansmith I was thinking you meant compute_api/TheThingWhatever
17:16:06 melwitt I was just realizing there's two different ones
17:16:08 melwitt yeah
17:16:11 dansmith so confusing that they're ... confusing
17:16:40 dansmith melwitt: yeah so right now that rpcapi gets created each time, which means it's never stale (but also means it's doing a calculation on every load)
17:16:59 dansmith so I think you'd want to make sure to wire up the sighup like the other services use to bump it
17:17:14 dansmith assuming that oslo.service gets fixed of course :)
17:17:19 melwitt I see, ok
17:27:28 openstackgerrit Balazs Gibizer proposed openstack/nova master: Change the TODO to NOTE about instance multi-create https://review.openstack.org/638847
17:39:09 melwitt dansmith: hm, the console proxies don't actually use oslo.service. they are websockify servers, and it looks like websockify currently ignores SIGHUP https://github.com/novnc/websockify/blob/master/other/websocket.c#L768
17:45:20 dansmith melwitt: ah, aight then
17:45:32 dansmith melwitt: it still means it's a change from current, but maybe more meh
17:49:42 melwitt dansmith: yeah. in my patch currently, I have rpcapi/ComputeAPI still as a class variable, but I'm relying on knowing the order of calls internally in websockify. making ComputeAPI global would eliminate that, but then I wouldn't be able to handle SIGHUP
17:50:16 dansmith well, it doesn't matter now because it's instantiated per request right?
17:50:28 dansmith meaning, no need for sighup currently, but after your change...
17:50:40 melwitt per connection yeah
17:51:43 melwitt yeah, if I make it global. I didn't make it global yet. maybe it's better to leave it as it is, per connection. because relying on the websockify call ordering seems less bad
17:57:59 dansmith yeah
18:58:15 openstackgerrit Artom Lifshitz proposed openstack/nova master: [WIP] Revert resize: wait for external events in compute manager https://review.openstack.org/644881
19:00:19 openstackgerrit Artom Lifshitz proposed openstack/nova master: [WIP] Revert resize: wait for external events in compute manager https://review.openstack.org/644881
19:08:43 sean-k-mooney so related to ^ i am currently porposing a change to neutron so that they will tell us when network events are sent so we can do that sanely https://review.openstack.org/#/c/645173/2
19:59:08 efried melwitt: I thought the concern was that we would do a bunch of init on ComputeAPI objects that ended up not being used
19:59:30 efried melwitt: in which case lazy load (but no global/singleton) would work okay, right?
20:19:54 melwitt efried: oh, you mean in new_websocket_client? hm, I guess that should work. from the pre-existing code comment, I was thinking it needed to be instantiated ahead of new_websocket_client, but I can try that and see if the tempest novnc tests pass
20:20:18 melwitt it = ComputeAPI
20:21:00 efried melwitt: I'm saying, instantiate it in the same place, but through a lazy loader
20:21:12 efried so it only actually initializes if you get down into new_websocket_client
20:21:30 efried but in the code paths that never get there, nothing ever happens to it and you don't waste the... whatever
20:21:54 efried but that way you don't have to worry about if like the code path changes from underneath you
20:22:06 melwitt efried: what is a "lazy loader"? I was thinking in __init__ it's set to None, in new_websocket_client, it's instantiated if None
20:22:44 efried We did a thing with this for the scheduler client at some point (we have since removed it)
20:25:13 melwitt that's how lazy load works underneath. won't be instantiated until use. I had thought from the pre-existing comment, it was saying it needed to be instantiated before new_websocket_client, but I don't see why it wouldn't work to instantiate it (once, if None) in new_websocket_client
20:25:48 efried melwitt: https://github.com/openstack/nova/blob/stable/queens/nova/scheduler/client/__init__.py
20:26:20 melwitt yeah, that's what I'm already saying I think
20:26:26 efried melwitt: I'm saying we shouldn't rely on new_websocket_client continuing to be the right place to do that init, in case the code flow changes underneath us.
20:26:50 melwitt efried: I get what you're saying, I'm saying I had thought from the existing code comment that it couldn't be that way. but I can try it and see what happens
20:26:59 efried compute_api = LazyLoader(...)
20:26:59 efried so you keep it in init as
20:27:06 efried oh
20:28:06 melwitt because what LazyLoader will do is what I said, construct a ComputeAPI object in new_websocket_client
20:29:00 melwitt I think it will work fine. I probably took the code comment too literally
20:29:32 efried okay, yeah, the way I read the comment is that new_websocket_client needs to use the ComputeAPI, so it had better be instantiated by the time it needs to use it.
20:29:47 efried but if you make it a lazy load, then it gets instantiated dynamically by the thing that needs to use it

Earlier   Later