Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-05-20
15:12:40 openstackgerrit Stephen Finucane proposed openstack/nova master: Remove conductor_api and _last_host_check from manager.py https://review.opendev.org/651059
15:13:21 openstackgerrit Stephen Finucane proposed openstack/nova master: tests: Stop starting consoleauth in functional tests https://review.opendev.org/652966
15:13:21 openstackgerrit Stephen Finucane proposed openstack/nova master: docs: Remove references to nova-consoleauth https://review.opendev.org/652965
15:13:22 openstackgerrit Stephen Finucane proposed openstack/nova master: nova-status: Remove consoleauth workaround check https://review.opendev.org/652968
15:13:22 openstackgerrit Stephen Finucane proposed openstack/nova master: xvp: Start using consoleauth tokens https://review.opendev.org/652967
15:13:23 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: Remove ConsoleAuthToken.to_dict https://review.opendev.org/652970
15:13:23 openstackgerrit Stephen Finucane proposed openstack/nova master: Remove nova-consoleauth https://review.opendev.org/652969
15:13:24 openstackgerrit Stephen Finucane proposed openstack/nova master: docs: Rework nova console diagram https://review.opendev.org/660147
15:13:48 aarents with less typo: I need some more review on that fix please (only +1 for now) https://review.opendev.org/#/c/659054/ thks !
15:14:52 efried aarents: Looks like you have a merge conflict to resolve.
15:16:19 efried adrianc: are you around to make minor tweaks to https://review.opendev.org/#/c/643024/ if needed?
15:16:27 sean-k-mooney oh passing size when the image is deleted. the logic still seam right but i dont know that code well enough to review it so ill contiue to abstane form that review
15:17:37 adrianc efried: yes
15:18:10 aarents sean-k-mooney: ok no prob, efried: exact, will check that
15:21:13 efried adrianc: commented.
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

Earlier   Later