| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-10-03 | |||
| 18:59:45 | dansmith | and then you have the exception itself | |
| 18:59:49 | mnaser | we are slowly rolling out nested virt across our entire fleet but that is something to discuss more with the infra team i think | |
| 18:59:52 | mriedem | nicolasbock: but likely not fixed on newton | |
| 19:00:23 | melwitt | dansmith: yeah, that was my other suggestion. I had two ideas: drop the sentinel and check isinstance or keep the sentinel and have tuples | |
| 19:00:26 | tssurya | dansmith: right, that would be simple, is it okay to change the utility's face now ? | |
| 19:00:30 | nicolasbock | ok, do you happen to remember the review this was fixed in mriedem ? Maybe I can backport? | |
| 19:00:38 | mriedem | looking | |
| 19:00:47 | dansmith | melwitt: no reason for the sentinel I don't think | |
| 19:01:01 | nicolasbock | Thanks mriedem | |
| 19:01:04 | dansmith | anything that isinstance(Exception) is... an error, so... | |
| 19:01:15 | nicolasbock | mnaser: it worked! The VM has migrated to a new host | |
| 19:01:27 | melwitt | dansmith: yeah, that's what I was thinking | |
| 19:01:32 | sean-k-mooney | mnaser: for ovs-dpdk and cpu pinning/hugepages we dont need nvme or gpus but we do need nested virt and a vm with multiple numa nodes. it is somthing that i agree i would love to discuss with infra. | |
| 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 | |