Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-10-26
14:10:41 fried_rice ack
14:25:57 finucannot mriedem: This is another one of those changes that I thought would require a microversion but apparently does not. The output is wrong but it doesn't raise a HTTP 5xx https://review.openstack.org/#/c/608685/
14:26:48 finucannot What distinguishes a fix from this from, say, modifying the output of os-hypervisor to respect (cpu|ram)_allocation_ratio. Both are currently giving "wrong" output
14:29:03 mriedem i don't have a good answer for that except the os-hypervisors stuff has been around forever and i don't know if including allocation ratios in that calculation would be considered by all to be "right"
14:30:33 finucannot Yeah, probably not the best example
14:30:51 finucannot mriedem: Would it be fair to ask for 'bug'-type reno for these kind of changes, in that case?
14:30:59 finucannot I'm thinking of the case where different clouds would give different results for what's supposed to be the same version
14:31:33 mriedem i wouldn't say it's unfair, although this simple tenant usage thing lpetrut is fixing is admin-only by default
14:31:39 mriedem non-admins can't list usage across all tenants
14:31:51 mriedem so that's not really an interop concern
14:32:11 finucannot Ohh, I didn't know that. In that case, not an issue here
14:32:55 mriedem well,
14:33:05 mriedem his change isn't specific to list (admin), it's also show (non-admin)
14:33:17 mriedem so idk, if you want a "fixes" release note then ask for one
14:33:26 mriedem "paging was broken, now it's fixed"
14:34:00 finucannot Yeah, I think I will. Probably makes sense assuming we want to backport this too (and to alert folks who'd want this downstream)
14:35:10 mriedem in general, we don't have a great formula for when a microversion is required. obviously major behavior changes that people need to opt into. and request/response schema changes. but there are so many other behavior-ish and bug fix type things that could be argued require a microversion we'd never do anything
14:35:17 mriedem or we'd have a thousand microversions by now
14:35:32 mriedem e.g. the trusted VFs thing from sahid technically could have required a microversion
14:36:19 mriedem and your requested networks physnet stuff
14:37:16 finucannot All good points
14:37:38 finucannot Clearly my "must raise HTTP 5xx" rule is too rigid
14:37:47 openstackgerrit Chris Dent proposed openstack/nova master: WIP: Integration tests using gabbi-tempest https://review.openstack.org/613386
14:45:37 openstackgerrit Lucian Petrut proposed openstack/nova master: Fix os-simple-tenant-usage result order https://review.openstack.org/608685
14:47:55 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix os-simple-tenant-usage result order https://review.openstack.org/608685
14:50:39 mriedem i can't figure out for the life of me why i can't send a RequestSpec object to prep_resize over rpc https://gist.github.com/mriedem/9fce3c68336f8abb5d4753c156d8f96f but we pass RequestSpec objects over rpc in several other places, like build_and_run_instance and select_destinations
14:51:14 mriedem there is an old oslo.messaging bug for the fake impl rpc driver https://bugs.launchpad.net/oslo.messaging/+bug/1529084 but how does that not break on those other calls?
14:51:15 openstack Launchpad bug 1529084 in oslo.messaging "RPC fake driver should accept datetime items for data" [Undecided,Invalid]
15:22:44 openstackgerrit Eric Fried proposed openstack/nova master: Kill @safe_connect in _get_provider_traits https://review.openstack.org/613613
15:22:59 fried_rice edmondsw: ^ (and leakypipes and other haters of @safe_connect)
15:24:29 edmondsw fried_rice thanks
15:24:47 cdent fried_rice: where does the exception go, now?
15:25:11 fried_rice cdent: up
15:25:13 fried_rice as it should
15:25:16 cdent indeed
15:25:19 cdent but then what?
15:25:32 fried_rice well, it will go the same place as the AttributeError did.
15:25:37 fried_rice http://184.172.12.213/04/602804/8/check/nova-in-tree-pvm/9a0e925/logs/n-cpu.txt.gz#_Oct_26_02_24_17_490196
15:26:02 cdent okay, but surely that's not enough
15:26:34 cdent either the attributeerror of the keystone error needs to be handled in some approrpriate fashion
15:26:48 cdent s/of/or/
15:27:17 fried_rice It's really just so we can know what actually happened rather than having to rely on the tribal knowledge that when a placement helper method returns None, it's probably because comm failed.
15:27:49 cdent (re that link: my eyes!!!!)
15:27:49 fried_rice Getting rid of @safe_connect is mostly about ^ that, not about doing something more appropriate with the now-not-as-mysterious exception. That would be a separate thing.
15:28:54 fried_rice cdent: hold on, we've got some of my log coloring experimental code on that server, I think there's a secret option to change themes...
15:29:42 cdent sure, but the reason safe_connect existed in the first place was to do some kind of "right thing" when comm's failed. what's wrong here is that safe_connect is broken, not that it exists
15:30:02 cdent safe_connect was supposed provided away for interactions with the placement api to be correctly retried (or logged) when things went awry
15:30:18 cdent somewhere along the way it lost that and became simply a bad thing that returned None when it shouldn't
15:30:51 cdent There's no doubt (at all) that it is a bad thing, now. But the lost of the intention is also a bad thing.
15:31:26 cdent If we're cool with the idea of removing it (because its masks real things that need to be fixed) will to stuff actually being fixed, cool
15:31:26 fried_rice in this case, the next spin through the periodic will hopefully work.
15:31:48 cdent that seems an odd approach to take, to me
15:31:57 fried_rice That's "retry"
15:32:04 fried_rice or if this call happened as the result of an actual operation, we've got retries built in, IIRC. Sec...
15:32:06 cdent because the next periodic could be many minutes away if the deployer has chosen to do so
15:32:22 fried_rice Yeah, and if an operation isn't happening, who cares
15:32:35 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Pass request_spec from compute to cell conductor on reschedule https://review.openstack.org/582417
15:33:06 cdent if the failed request was e.g., updating inventory, scheduling will go wrong during that window
15:33:55 cdent What I'm trying to get at here is that I suspect (actually, am confident that) the system is more complex than "just let the expceptions rise" can bear
15:33:57 fried_rice So this is all happening under the umbrella of _update, which has a retry around it for conflicts
15:34:19 cdent but what about for a network partition?
15:35:33 cdent (or things that look like a network partition (such as the load balance being reconfigured))
15:36:16 fried_rice Look, I'm not disputing that we could do a better job of handling placement comm errors in this whole flow (i.e. the resource tracker). But first we have to know we're having a placement comm error.
15:36:36 fried_rice If we want to put in retries at some level based on ClientException, I'm all behind that.
15:36:42 fried_rice But that's not what this patch is for.
15:38:41 cdent Okay, but "handling placement comm errors" is why safe_connect was created, so simply killing it is probably not the right approach. Fixing it probably is.
15:39:12 cdent I'm harping on it because it is clear that the reasson for its existence has now been lost to the sands of time (because somewhere along the way it borked)
15:39:41 cdent But we seem to want to say "it sucks, let's just kill it" (at least jokingly) and that's not really sufficient in a distributed system.
15:41:00 cdent These issues will come back around again, after safe_connect has gone, and we'll realize "you know, we need to create this thing that allows us to safely connect"
15:41:08 fried_rice We have made several assays toward fixing it. It can't be done. Because it's at too low a level in the call stack.
15:41:12 SteelyDan cdent: actually safe_connect was really to handle the "have they set up placement or not" situation
15:41:33 SteelyDan which is why we've been talking about removing it entirely since placement became required
15:41:50 cdent SteelyDan: that was _one_ of the reasons, and yes, the very first reason, but very quickly it took on "deal with errors from keystoneclient"
15:42:31 SteelyDan yes, it has evolved
15:42:42 openstackgerrit Eric Fried proposed openstack/nova master: Kill @safe_connect in _get_provider_traits https://review.openstack.org/613613
15:42:46 cdent we had: is it in the service catalog, then can I reach it
15:42:52 fried_rice cdent: added words clarifying intent --^
15:43:32 cdent thanks
15:44:31 openstackgerrit sean mooney proposed openstack/nova master: harden placement init under wsgi https://review.openstack.org/610034
15:44:34 cdent I'm trying really hard to not to think to much about the resource tracker lately but sometimes I get sucked in.
15:45:34 sean-k-mooney cdent: ^ uses the run_once decorator
15:45:38 fried_rice cdent: Anything we would do to @safe_connect, beyond making it essentially a no-op shell (which isn't better than removing it IMO), would also entail a thorough audit of everywhere it is used, which is a nearly impossible task due to how many different ways and in how many layers things get called.
15:45:38 fried_rice For example, if we decided to make it retry, we would have to restrict the decorator to *only* single primitive API calls. We would have to remove it from all the methods that do more complex things e.g. involving multiple API calls.
15:46:02 cdent sean-k-mooney: yeah, thanks, reading
15:46:25 sean-k-mooney cdent: ill propose the placement version shortly if people are fine with it
15:47:07 cdent fried_rice: yes, I know, thus why I'm trying really hard not to think about the resource tracker lately. It was already really complicated before placement, and we somehow managed to make it worse, not better.
15:47:28 fried_rice I have made a couple of attempts, and I think SteelyDan has as well, to do something to @safe_connect itself to improve it. At some point in a review leakypipes said we should just target one usage at a time and get rid of it. Which so far has been the only approach that has gotten us to actually merge code around this.
15:47:45 SteelyDan yup
15:47:54 fried_rice cdent: I still hope that it will be better in the long run, once we've purged all the legacy stuff from this painful transitional period.
15:48:09 fried_rice like @safe_connect :P
15:48:34 fried_rice and stuff gibi has been doing lately has cleaned up some of that legacy gorp too.
15:48:37 fried_rice So we're getting there.
15:48:49 cdent well, just to cover my ass here: If I had my druthers we never would have had safe_connect
15:49:09 cdent but we gots we gots
15:49:12 cdent i'll cope
15:49:28 fried_rice My dad used to say, "If frogs had wings, they wouldn't bump their asses so much."
15:49:45 fried_rice (I made that sound like he's dead or something. He would still say it, given the slightest opportunity.)
15:49:50 cdent heh
15:55:50 cdent sean-k-mooney: have you run the functional tests on that change? I think you're going to run into trouble with the databse fixture
15:55:58 spatel sean-k-mooney: is there a way to find real instance hostname using virsh list command?

Earlier   Later