Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-05-20
13:53:03 mriedem https://review.opendev.org/#/c/639396/5//COMMIT_MSG@25
13:54:20 sean-k-mooney mriedem: ya i am aware of why you made the original change
13:54:34 sean-k-mooney which is why im trying to figure out how to wait reliably
13:54:56 sean-k-mooney the patch on top still wait to late in some cases
13:56:14 sean-k-mooney if we take bufferign off the table my next idea is a pre_revert_at_source rpc function what will start the source node waithing for the vif-plugged event before we start the revert
13:56:50 sean-k-mooney that feel heavy wait but it shoudl resolve the issue of missing the event
13:57:05 sean-k-mooney *heavyweight
13:58:22 mriedem umm, isn't the issue just that we're not waiting for the unplugged even on the dest when we unplug vifs before casting to the source?
13:58:58 mriedem i'm kind of in the middle of changing all of my internal domain and email shit b/c of this embargo so can't really dig into this right now, but pretty sure i've gone over this (with dansmith and artom) already in detail once or twice
13:59:32 cdent blargh, that sounds painful
13:59:43 mriedem things are not fun right now no
14:02:02 sean-k-mooney mriedem: ack.
14:05:10 sean-k-mooney as far as i can tell for the logs we are still reciving the event before we start waiting
14:05:57 sean-k-mooney recive the vif-plugged event here http://logs.openstack.org/81/644881/15/check/nova-multinode/628c731/logs/subnode-2/screen-n-cpu.txt.gz#_Apr_29_15_18_02_574298 but starte to wait for it 2 seconds later here http://logs.openstack.org/81/644881/15/check/nova-multinode/628c731/logs/subnode-2/screen-n-cpu.txt.gz#_Apr_29_15_18_04_002370
14:06:11 sean-k-mooney if we fix that i think that will resolve the issue
14:17:14 mriedem sean-k-mooney: i believe i pointed out the issue here https://review.opendev.org/#/c/639396/3/nova/compute/manager.py@4184
14:17:58 mriedem which is what artom's final patch in the series is doing https://review.opendev.org/#/c/644881/15/nova/compute/manager.py
14:18:05 mriedem waiting for the event before doing migrate_instance_finish
14:18:58 sean-k-mooney mriedem thanks im looking at the code now but ill read your comment to make sure im on the same page
14:19:21 mriedem did the bug get punted to you at this point or are you just trying to get caught up and review the series?
14:19:46 sean-k-mooney mriedem: artom is on PTO for a few days so both
14:20:10 sean-k-mooney im trying to get caught up to review and determin if the current fix is correct and if not fix it
14:22:49 sean-k-mooney we think that this issue happens when you are using hybrid_plug e.g. the iptables firewall driver which we use downstream but not upstream so one of the things that i need to test is modifying the gate job temporally to repoduce this upstream without code change to confrim
15:12:32 aarents Hi, If need some more review on change that fix please (only +1 for now) https://review.opendev.org/#/c/659054/
15:12:40 openstackgerrit Stephen Finucane proposed openstack/nova master: Remove 'etc/nova/cells.json' https://review.opendev.org/660146
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

Earlier   Later