Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-10-03
19:01:44 mnaser yeah, we'd have to talk it out with infra
19:01:56 melwitt dansmith: but sean-k-mooney was thinking checking isinstance was an anti-pattern of some kind
19:02:14 sean-k-mooney melwitt: sorry i should read the scrole back
19:02:30 melwitt sean-k-mooney: we're just talking about the "return exceptions from scatter-gather" thing
19:02:31 dansmith melwitt: overengineering is an anti-pattern :)
19:02:46 tssurya sean-k-mooney: its about this: https://review.openstack.org/#/c/605251/
19:03:05 mriedem nicolasbock: https://review.openstack.org/#/q/Ib10081150e125961cba19cfa821bddfac4614408 is what i'm thinking of
19:03:19 nicolasbock Is it ok that the disk is still on the old host after migration?
19:03:20 melwitt sean-k-mooney: dansmith suggested the same thing I suggested when we first talked about it, just return exception objects instead of the sentinel and check isinstance(thing, Exception) to know whether an error was returned or not
19:03:22 nicolasbock Thanks mriedem
19:03:25 sean-k-mooney dansmith: well i was porting a standard calass form c++ to python. retrun an exception has some weird sidefect in python 2
19:04:16 dansmith sean-k-mooney: I have no idea what weird side effect you mean, other than that re-raising it doesn't keep the exception context properly, but we won't be doing that here
19:04:27 dansmith sys.exc_info I mean
19:04:53 nicolasbock mriedem: gerrit's cherry-pick doesn't seem to know Newton. Is that because Newton is EOL'ed?
19:04:59 sean-k-mooney melwitt: so the exception object in python 2 has a reference to the stack fram form which it was first thrown if the garbage collector cant deallocate it or any locks. sys.exc_info and retruning it is fine
19:05:50 mriedem nicolasbock: correct, newton is eol upstream
19:06:03 mriedem nicolasbock: note that that change is also building on top of two other fixes
19:06:07 mriedem called out in the commit message
19:06:12 sean-k-mooney dansmith: https://www.python.org/dev/peps/pep-0344/#open-issue-garbage-collection
19:06:43 sean-k-mooney dansmith: if we call sys.exc_info() and return the tuple as the sentiel that is fine however
19:06:46 nicolasbock Thanks mriedem , I will apply the fix in our vendor packages only then
19:07:06 nicolasbock Thanks all for the help with the "lost" VM!
19:07:09 dansmith sean-k-mooney: how is returning it any different than encapsulating it in your object here?
19:07:14 dansmith from a GC perspective
19:08:30 sean-k-mooney dansmith: i if you dont raise the exception and catch it it does not have the referecne to the stack frame so retrun VauleError("invalid data") is fine
19:08:43 mriedem nicolasbock: well, you probably should verify that the reason you got into this mess in the first place was due to one of those bugs
19:08:59 mriedem but whatever you want to do downstream is fine with me :)
19:09:01 sean-k-mooney "expect Exception as e: return e" is not
19:09:13 nicolasbock Yes of course :)
19:09:45 sean-k-mooney expect Exception: return sys.exc_info() is also fine
19:09:51 spatel sean-k-mooney: "pci=assign-busses,realloc" was causing issue my server got hung at boot, as soon as i remove that it works.. I have HP DL360p G8
19:09:59 dansmith sean-k-mooney: but this result object of yours is just going to swallow the exception that we get from our handler right?
19:10:11 dansmith sean-k-mooney: so it's still pinning the reference to the stack
19:11:09 sean-k-mooney dansmith: ya i was planning to extend it to do the right thing on each python verion.
19:11:17 sean-k-mooney this is only an issue on python2
19:11:29 sean-k-mooney they fix it in python 3 so you can just return the exception
19:11:34 dansmith we could trivially re-construct the exception since we know it inherits from NovaException and has very specific characteristics
19:12:05 dansmith since it's only py2, and since py3 is the future, and since this is a suuuper corner case that only "may" have issues with some GC being delayed... I would tend to punt on caring about this entirely
19:12:06 dansmith but
19:12:13 sean-k-mooney dansmith: yep but if we are going to do that i taught it would be nice to hide that in a result calss that does the right thing
19:12:17 dansmith just re-creating the exception and returning it would be fine for our purposes
19:13:20 melwitt tssurya: sorry, I'm not really getting the usefulness of the single cell scatter-gather from the commit message. how does it help for down cell?
19:13:24 sean-k-mooney spatel: ya dont add that. it was specically to work around a hardware bug in my old servers
19:13:25 dansmith IMHO we can punt and not worry about this
19:13:42 spatel roger!
19:13:52 spatel i took it out
19:14:08 tssurya melwitt: https://review.openstack.org/#/c/591658/3/nova/compute/api.py@2323
19:14:31 tssurya we could just directly use the scatter_gather_cells(), just that it looked bad
19:14:45 mriedem jaypipes: i have crushed your soul https://review.openstack.org/#/c/607626/
19:15:13 melwitt tssurya: I see, to get the automatic "wait for this amount of time before timing out" part. thanks
19:15:14 tssurya melwitt: we will be using it for Instance.get_by_uuid for the nova show part
19:17:25 sean-k-mooney dansmith: so ya ill likely work on https://review.openstack.org/#/c/605251/2 a little more in my spare time later this week as i want that class as a tool in my tool box for the future but we may not need it for this usecase.
19:17:27 tssurya dansmith, sean-k-mooney: so.. which way are are agreeing on ?
19:17:58 dansmith tssurya: I vote for return the exception and boil the ocean later :)
19:18:03 melwitt so what's all this reconstruction talk? in scatter-gather, when we catch the exception from the cell, are you saying we have to do more than just save the "as exp" part?
19:18:33 tssurya dansmith: ack,
19:18:39 dansmith melwitt: we do not
19:18:45 dansmith melwitt: we could trivially if we want
19:18:48 tssurya melwitt: as far as I understood just returning it should be enough
19:18:51 melwitt mmkay
19:19:07 dansmith just return exp.__class__(exp.args) would be enough
19:19:19 dansmith but it'd need a comment about why
19:19:29 dansmith and if we don't, then.. not
19:19:39 melwitt ok, that's what I meant, we can't just return exp, we have to do that step
19:19:51 dansmith we've just spawned a thread at this point, and most of these things are single DB calls, which means the stack being pinned by the exception is tiny
19:20:22 jaypipes mriedem: awesome.
19:20:22 melwitt ok, yeah. comment if we do that because otherwise I'm not going to remember why
19:20:28 sean-k-mooney dansmith: oh so just return a new exception object not the one we caught
19:20:51 jaypipes mriedem: it's hard enough already for me to give a rat's ass about a stable branch. :)
19:21:08 dansmith melwitt: we don't have to do that step.. we can just return exp. If we want, we can do the reconstruction step (and document it)
19:21:09 dansmith melwitt: I would vote for not reconstructing because I think this is super tiny
19:21:22 tssurya dansmith: ah got it
19:21:34 melwitt ack, thank you
19:21:47 tssurya melwitt, sean-k-mooney, dansmith: thanks
19:22:02 dansmith soooo, back to the meeting,
19:22:08 dansmith I will probably be back, if ya'll want to meet
19:22:10 tssurya dansmith: yea are we having one ?
19:22:35 melwitt I'm neutral about meeting. I don't have anything special to talk about
19:22:49 melwitt mriedem might want to talk about cross-cell stuff? I dunno
19:22:56 dansmith mriedem may want to talk about crossing the streams
19:22:58 dansmith yeah, t hat
19:22:59 tssurya I don't have anything special except some silly bugs
19:23:22 melwitt silly bugs? now I'm curious
19:23:25 sean-k-mooney just one other comment we are not holding any locks or file handels correct wehre we raise the exception in the scater gater case?
19:23:43 tssurya melwitt: https://bugs.launchpad.net/nova/+bug/1794994
19:23:43 openstack Launchpad bug 1794994 in OpenStack Compute (nova) "Update the --max-rows parameter description for nova-manage db archive_deleted_rows" [Low,In progress] - Assigned to Surya Seetharaman (tssurya)
19:23:58 tssurya for now I changed it to a doc fix, but I am skeptical about it
19:24:11 dansmith sean-k-mooney: we would have just gotten a result from a threadpool of db workers, and they would almost definitely have re-raised outside of any locks
19:24:31 tssurya it would be just good to have the API table record removal also in the max-rows
19:24:36 tssurya not sure if people care though
19:25:28 tssurya but yea its not super urgent
19:26:01 tssurya okay then I will head home now and will be lurking around during the meeting time in case we decide to have one
19:26:15 melwitt ok, will read through it. the issue is the command output can be confusing given the treatment of the API records
19:26:16 sean-k-mooney dansmith: ok the the stack frame reference keeps stack locals alive including any file handles or locks so can we add a commet the pep issue if we just return the exception just incase we have issue in the future
19:26:25 tssurya melwitt: exactly
19:26:41 dansmith sean-k-mooney: yep
19:27:14 sean-k-mooney dansmith: i think we will be fine but future me would regret not adding it if we ever have to debug it :)
19:28:07 melwitt tssurya: thanks. this is hard for me to imagine because I can't remember what the archive_deleted_rows output looks like :P will look in the code
19:28:32 openstackgerrit Matt Riedemann proposed openstack/nova stable/queens: stable-only: fix typo in IVS related privsep method https://review.openstack.org/604817
19:30:45 mriedem melwitt: dansmith: i don't really want to talk about cross-cell resize today probably; i suggested to dansmith that i skim my poc with him over a hangout early next week (i'm out tomorrow and friday)

Earlier   Later