| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-20 | |||
| 15:21:48 | sean-k-mooney | aarents: for what its worth if mdbooth thinks its ok then that is a +1.5 when it comes to the image cache so once you rebase against master it shoudl be easier to get review form the cores | |
| 15:21:52 | adrianc | efried: thx, will address shortly (meeting ...) | |
| 15:22:05 | efried | adrianc: A little background: First off, if you're reraising the original exception, you want to just say | |
| 15:22:06 | efried | because the former preserves the original stack trace and exception context. | |
| 15:22:06 | efried | raise e | |
| 15:22:06 | efried | rather than | |
| 15:22:06 | efried | raise | |
| 15:22:34 | efried | But it's also theoretically possible for intervening calls (like LOG.*) to mess with the exception context | |
| 15:22:54 | sean-k-mooney | efried: i like your deffintion of spelling :) | |
| 15:23:07 | efried | adrianc: so oslo provides a save_and_reraise_exception context manager that saves off the exception context while you do whatever, then makes sure to restore it properly before reraising. | |
| 15:23:32 | efried | sean-k-mooney: I have no words for you. | |
| 15:24:33 | sean-k-mooney | efried: i was refering to to your comment on adrianc patch | |
| 15:24:36 | efried | and misspelled | |
| 15:24:41 | efried | yeah, I know :) | |
| 15:24:48 | tssurya | mriedem, dansmith: could you both circle back to https://review.opendev.org/#/c/636132/ whenever you get some time ? thanks in advance! | |
| 15:24:55 | sean-k-mooney | ya i avoid that word for irony reasons | |
| 15:25:06 | dansmith | tssurya: yeah, sorry | |
| 15:25:13 | efried | I was trying to come up with some way to respond to sean-k-mooney commenting on anything to do with spelling, and I just came up blank. Totally speechless. | |
| 15:25:43 | tssurya | dansmith: no need to be sorry :) I understand you must be busy with lots of other stuff | |
| 15:26:19 | tssurya | this spec is a little too specific after all | |
| 15:33:42 | aspiers | efried: I've switched the SEV capability patch we discussed on Friday to instead set an instance variable and then provide the trait in u_p_t(). I'm now wondering where I should put the functional tests: in nova.tests.functional.test_servers.TraitsTrackingTests, or nova.tests.functional.test_report_cpu_traits.LibvirtReportTraitsTests | |
| 15:34:06 | aspiers | The latter seems more appropriate, I guess | |
| 15:34:24 | efried | aspiers: looking... | |
| 15:34:41 | aspiers | but the former already has tests closer to what I need (I wrote them, in fact) | |
| 15:38:14 | efried | aspiers: Is FakeLibvirtDriver in the way here? | |
| 15:38:56 | efried | I still don't fully understand how that thing works. | |
| 15:39:11 | efried | Obviously we want your tests to run through the real libvirt driver's upt | |
| 15:39:25 | efried | so as long as that's happening, it doesn't matter from a code point of view. | |
| 15:39:26 | efried | but | |
| 15:39:33 | efried | test_servers is way too effin big already. | |
| 15:39:34 | aspiers | efried: Ideally it should test not just that a) it provides the SEV trait on detection, but also b) it replaces it if the admin removes it, and that it removes it if c) the host isn't SEV-capable but an admin provides it or d) if the host *becomes* SEV-incapable | |
| 15:39:56 | aspiers | i.e. the same scenarios I tested when doing the cap-to-trait thing before | |
| 15:40:19 | aspiers | but this time with the trait coming from a different bit of code | |
| 15:40:23 | efried | ...And these are testing that libvirt reports traits, so LibvirtReportTraitsTests seems appropriate just based on the name. | |
| 15:40:37 | aspiers | Yeah, name-wise that wins for sure | |
| 15:40:52 | efried | aspiers: I understand. I wouldn't be averse to a patch that moves those tests :) | |
| 15:41:09 | aspiers | but yeah, you're right this needs to test the real libvirt driver | |
| 15:41:23 | efried | any patch that refactors test_servers.whatever into more-appropriately-named-smaller-module would be a win imo. | |
| 15:41:26 | aspiers | Moves which tests? | |
| 15:41:35 | efried | aspiers: The ones you did about capability traits | |
| 15:41:46 | aspiers | To where? They're not libvirt-specific | |
| 15:41:50 | efried | ahem, I guess those should... right | |
| 15:41:57 | efried | ...be in a module about drivers setting traits. | |
| 15:42:22 | efried | bah | |
| 15:42:34 | aspiers | How about this: | |
| 15:42:52 | efried | ++ (/me predicts future) | |
| 15:43:46 | aspiers | I find some co-conspirators from Red Hat, and stage a coup where the PTL gets overthrown. Oh good, +1 from you already | |
| 15:44:10 | efried | yeah, I walked into that | |
| 15:44:22 | aspiers | What I was actually gonna suggest is, I'll stick them in LibvirtReportTraitsTests, but if that means duplicating code already in test_servers.py, then I'll extract it out into a shared utils library | |
| 15:44:32 | aspiers | which might already exist | |
| 15:44:37 | aspiers | or might not | |
| 15:44:37 | efried | ++ | |
| 15:45:02 | efried | Doesn't have to be shared utils per se, just a separate module that tests trait-y things but not libvirt-specific. | |
| 15:45:08 | efried | Then the libvirty one can inherit from that. | |
| 15:45:08 | aspiers | right | |
| 15:45:29 | efried | Do the refactor in a new patch underneath, if you please. | |
| 15:45:49 | efried | If it's straightforward enough, I might be able to ninja approve it. | |
| 15:46:37 | aspiers | OK | |
| 15:47:30 | efried | yeah, test_servers is nearly 7kloc. That's too much | |
| 15:47:35 | aspiers | agreed | |
| 15:48:32 | aspiers | efried: should I first just extract TraitsTrackingTests to a separate file? | |
| 15:49:00 | efried | yeah, and TraitsBasedSchedulingTest with it | |
| 15:49:01 | aspiers | or that and ProviderTreeTests? | |
| 15:49:20 | aspiers | something like test_server_traits.py? | |
| 15:49:33 | bnemec | efried: Worth noting that the save_and_reraise context was to work around a bug in eventlet that I believe has since been fixed. | |
| 15:49:37 | aspiers | or test_server_provider_tree.py | |
| 15:49:47 | bnemec | I'm not sure you still have to worry about log calls and such messing with the exception context. | |
| 15:50:06 | efried | bnemec: oh, I didn't know that. It makes sense to me that intervening code could still muck with the exception context. | |
| 15:50:40 | efried | bnemec: or is well-behaved code supposed to not do that | |
| 15:51:16 | bnemec | It was _probably_ eventlet doing something naughty. | |
| 15:51:55 | bnemec | I'm sure it could still happen, but it's probably not common anymore. | |
| 15:53:14 | bnemec | (my overuse of probably is pretty representative of my confidence in this ;-) | |
| 15:53:36 | efried | adrianc: in any case, 'raise' rather than 'raise e' is appropriate | |
| 15:53:36 | efried | bnemec: So you're not yet recommending that we go through and rip out all our uses of that context manager? | |
| 15:53:38 | bnemec | I guess I'm not aware of anyone running into a bug that required save_and_reraise since the eventlet fix though. | |
| 15:54:15 | bnemec | efried: No, I don't think it hurts anything to have it there. | |
| 15:54:17 | efried | aspiers: test_servers_provider_tree seems like a winner. | |
| 15:54:24 | aspiers | ok | |
| 15:54:31 | bnemec | And +1 to the raise vs. raise e point. | |
| 15:58:13 | efried | Recent guidance is just to include the license. But legally, since you're porting code from A to B, you should preserve A's copyright and license header. Sorry. | |
| 15:59:43 | efried | It is stupid. I've actually spent hours with lawyers on this. | |
| 16:00:21 | aspiers | Haha my sympathies | |
| 16:03:41 | cdent | FIGHT THE POWER | |
| 16:10:12 | aspiers | :) | |
| 16:10:20 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract provider tree functional tests into new file. https://review.opendev.org/660157 | |
| 16:11:00 | aspiers | efried: ^^^ | |
| 16:11:04 | efried | on it | |
| 16:13:37 | aspiers | Why the hell did I finish the commit title with a period | |
| 16:13:56 | aspiers | I never do that | |
| 16:16:08 | efried | arght, /me retracts +2! | |
| 16:16:18 | efried | whatever | |
| 16:16:39 | aspiers | ;) | |
| 16:16:43 | efried | fix it or not. It's only your reputation at stake, permanently and indelibly etched into git | |
| 16:16:50 | aspiers | ROFL | |
| 16:17:22 | aspiers | I see plenty of other trailing '.' in the commit history, thankfully | |
| 16:17:38 | aspiers | I'll just be joining a distinguished group of disgraced engineers | |
| 16:17:56 | aspiers | Hmm, "Disgraced Engineer" sounds like a job title | |
| 16:18:05 | aspiers | Maybe that's in my career track | |
| 16:18:58 | aspiers | Crap, now I have no way to re-submit the SEV trait change without rebasing all the other SEV stuff on top of this | |
| 16:19:10 | aspiers | or without rebasing this on top of the other SEV stuff | |