| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-06 | |||
| 12:05:19 | cdent | But if it makes sense, then sure, if it makes things more clear or clean. | |
| 12:05:40 | bauzas | cdent: the problem is I'm beginning to include all the resource-related methods for inventories and allocations | |
| 12:05:48 | cdent | I think we intentionally chose _not_ to use different objects per resource in the client as that was felt to be overkill given the types of interactions being done | |
| 12:05:54 | bauzas | cdent: and that's an hard work to do so | |
| 12:15:05 | mdbooth | Is anybody working on the gate snafu, btw? | |
| 12:16:09 | mdbooth | nova.tests.unit.test_cinder.CinderV1TestCase.test_cinderclient_unsupported_v1 | |
| 12:17:07 | mdbooth | cdent: You currently have a zuul job failing in ^^^ btw | |
| 12:17:52 | cdent | mdbooth: that was talked about last night | |
| 12:18:11 | cdent | https://bugs.launchpad.net/nova/+bug/1654393 | |
| 12:18:11 | openstack | Launchpad bug 1654393 in OpenStack Compute (nova) ""keystoneauth1.exceptions.catalog.EmptyCatalog: The service catalog is empty." in test_cinderclient_unsupported_v1" [High,In progress] - Assigned to Matt Riedemann (mriedem) | |
| 12:18:22 | cdent | matt's got a fix pending | |
| 12:18:37 | mdbooth | cdent: Impact is nothing can land, right? | |
| 12:18:54 | cdent | I was under the impression it was racy | |
| 12:19:27 | johnthetubaguy | sfinucan: I wasn't sure about this one: https://review.openstack.org/#/c/413658/2 does my comment make sense? | |
| 12:21:30 | sfinucan | johnthetubaguy: Yup, `null` would be the JSON variant | |
| 12:23:58 | sfinucan | johnthetubaguy: In turn, any idea if the comments on this patch correct? I've no idea, I'm afraid :/ https://review.openstack.org/#/c/416959/1/nova/conf/quota.py | |
| 12:25:04 | johnthetubaguy | sfinucan: oh, that is correct I think | |
| 12:25:16 | johnthetubaguy | sfinucan: all the network ones, there is a bit in the code that would confirm that, one sec | |
| 12:26:04 | sdague_ | I'm back now, what's up? | |
| 12:26:17 | sfinucan | johnthetubaguy: Yeah, I've found in rather touch to delineate between nova-net and neutron in 'nova/network' where most of these variables are used | |
| 12:27:25 | sfinucan | sdague: Any idea about the the validity of comments on https://review.openstack.org/#/c/416959/1/nova/conf/quota.py ? | |
| 12:29:34 | sdague | sfinucan: I'm pretty sure Takashi is right, those also only have an effect when nova-network is in play | |
| 12:31:56 | sfinucan | sdague: johnthetubaguy: Cool cool. I'll respin that to drop those so | |
| 13:14:25 | jaypipes | good morning Novas. | |
| 13:36:15 | openstackgerrit | Stephen Finucane proposed openstack/nova: libvirt: Make 'get_domain' private https://review.openstack.org/417378 | |
| 13:54:43 | openstackgerrit | Ed Leafe proposed openstack/nova: Remove the Allocation.create() method. https://review.openstack.org/416751 | |
| 13:54:44 | openstackgerrit | Ed Leafe proposed openstack/nova: Add check for invalid allocation amounts https://review.openstack.org/416752 | |
| 13:55:43 | openstackgerrit | Stephen Finucane proposed openstack/nova: rbd: Remove unnecessary 'encode' calls https://review.openstack.org/412356 | |
| 13:59:40 | openstackgerrit | Stephen Finucane proposed openstack/nova: conf: Deprecate yet more nova-net options https://review.openstack.org/416959 | |
| 14:10:33 | openstackgerrit | melissaml proposed openstack/nova: Fix a typo in documents https://review.openstack.org/417389 | |
| 14:14:25 | cdent | mdbooth: maybe you have some thoughts on this: Do you know to what extent eventlet gets involved when running the unit tests? | |
| 14:15:12 | mdbooth | cdent: I think sdague would be a better authority on this stuff, but to the best of my knowledge: not really. | |
| 14:15:33 | mdbooth | I understand that tox fires up several workers in independent processes, which then run tests serially. | |
| 14:15:49 | cdent | (I was asking in relation to your comment on matt's fix for the gate failure) | |
| 14:15:55 | mdbooth | So 'races' shouldn't really occur. | |
| 14:16:16 | mdbooth | However, you can get ordering artifacts if tests don't clean up after themselves properly. | |
| 14:16:19 | cdent | yeah, that was my thinking too, but if eventlet does getinvolved, then game over | |
| 14:16:31 | cdent | s/over/confused/ | |
| 14:16:42 | mdbooth | However, I'd defer entirely to sdague :) | |
| 14:16:56 | sdague | cdent: what is the eventlet concern when tests are run? | |
| 14:17:10 | cdent | So I've been assuming it doesn't (as that would be bad as it introduces ambiguity). | |
| 14:17:43 | mdbooth | Incidentally, I'm just starting to look at this myself. | |
| 14:17:44 | cdent | sdague: If eventlet is present and monkey patching file io in some sense, you might get an unexpected yield to an unexpected greenthread? | |
| 14:18:11 | cdent | I don't really know, I'm experiening curiosity. Expressing it to see if anyone else knows what's up. | |
| 14:18:20 | sdague | cdent: eventlet is definitely present | |
| 14:20:06 | sdague | but, there are going to need to be spawns to have greenlets running, right? We use that to our advantage on the functional tests with the single process cloud | |
| 14:21:29 | cdent | That's effectively my question: If your first statement/question is true, not an issue. | |
| 14:22:20 | sdague | the parallel running of tests is via testr spawning dedicated processes with subsets of the tests, and running subunit serially in each | |
| 14:23:03 | cdent | yeah, that part I understand (and know too well, dealing with trying to get gabbi to behave properly in that scenario (it does)) | |
| 14:23:07 | sdague | so... as long as tests clean up after themselves, there shouldn't be interference between tests. But cleaning up after themselves is sometimes not super clear, especially if globals are set | |
| 14:38:18 | openstackgerrit | John Garbutt proposed openstack/nova: correct misleading wording https://review.openstack.org/413658 | |
| 14:38:44 | openstackgerrit | John Garbutt proposed openstack/nova: correct misleading wording https://review.openstack.org/413658 | |
| 14:49:54 | mdbooth | cdent: https://bugs.launchpad.net/nova/+bug/1654393/comments/2 | |
| 14:49:54 | openstack | Launchpad bug 1654393 in OpenStack Compute (nova) ""keystoneauth1.exceptions.catalog.EmptyCatalog: The service catalog is empty." in test_cinderclient_unsupported_v1" [High,In progress] - Assigned to Matt Riedemann (mriedem) | |
| 14:50:30 | cdent | go you. mdbooth++ | |
| 14:51:37 | johnthetubaguy | was just looking at matt's patch he has up for that | |
| 14:54:01 | mdbooth | johnthetubaguy: Yeah, I wasn't convinced. | |
| 14:55:02 | mdbooth | So, it looks like it fails because _SESSION already exists | |
| 14:55:48 | johnthetubaguy | yeah, someone already set it up somehow | |
| 14:56:03 | mdbooth | johnthetubaguy: See my comment | |
| 14:56:21 | mdbooth | Looks like it's pretty easy to initialise | |
| 14:56:33 | johnthetubaguy | so if _SESSION is set, the old mock would never get run | |
| 14:56:39 | johnthetubaguy | but thats not true after his patch | |
| 14:56:41 | johnthetubaguy | I think? | |
| 14:58:17 | johnthetubaguy | hang on, I am missreading this | |
| 14:58:36 | mdbooth | johnthetubaguy: I'm going to submit an alt patch which does cinder.reset_globals() instead | |
| 14:58:51 | johnthetubaguy | mdbooth: ah, gotcha | |
| 14:59:06 | mdbooth | Because that's the real issue here. It's not that mock is stamping on things, it's that something previously initialised global state. | |
| 14:59:30 | johnthetubaguy | yeah, its global state already being tweaked | |
| 14:59:52 | mdbooth | Ah... and BaseCinderTestCase also does that | |
| 15:00:33 | openstackgerrit | Stephen Finucane proposed openstack/nova: console: Move proxies to 'console/proxy' https://review.openstack.org/408192 | |
| 15:00:34 | openstackgerrit | Stephen Finucane proposed openstack/nova: conf: Deprecate 'console_driver' https://review.openstack.org/408193 | |
| 15:00:34 | openstackgerrit | Stephen Finucane proposed openstack/nova: tests: Replace use of CONF with monkey patching https://review.openstack.org/408191 | |
| 15:04:14 | cdent | Is it fair to ask "Why do we have global state?"? | |
| 15:04:45 | mriedem | i'm going to be sending off the user survey feedback question today so it's last call for questions | |
| 15:05:46 | johnthetubaguy | cdent: its a cache I believe | |
| 15:06:06 | openstackgerrit | Matthew Booth proposed openstack/nova: Reset global state in CinderV1TestCase https://review.openstack.org/417408 | |
| 15:06:21 | johnthetubaguy | cdent: still a valid question | |
| 15:06:22 | cdent | mriedem: "Do you wish there was an orchestration layer?" | |
| 15:06:23 | mriedem | mdbooth: i've already had a patch up for that since yesterday | |
| 15:06:48 | mdbooth | mriedem: Yeah, I read it earlier. | |
| 15:07:02 | mriedem | mdbooth: my first attempt at it failed, but the one that you -1ed has passed jenkins several times already | |
| 15:07:52 | mdbooth | mriedem: See ^^^. I'm doing reset_globals(). Your patch mocks _SESSION. It has the same effect, but reset_globals() is how the other tests do it. | |
| 15:08:04 | mdbooth | And it's more explicit as to why it's being done. | |
| 15:08:11 | mriedem | mdbooth: the reset_globals() thing seems like a hack to begin with | |
| 15:08:31 | mriedem | mdbooth: we shouldn't have to deal with a global between concurrently running tests | |
| 15:08:34 | mdbooth | You need a hack like this everywhere you have global state. | |
| 15:08:37 | mriedem | which is why i'm mocking out _SESSION altogether | |
| 15:08:45 | mdbooth | We don't have concurrently running tests. | |
| 15:09:02 | mriedem | sure we do, between test classes at least | |
| 15:09:10 | mdbooth | That's why I was -1 on your patch, because I think it obscured the issue. | |
| 15:09:34 | mdbooth | I don't think so. They run serially in multiple processes. | |
| 15:09:46 | mdbooth | The issue here wasn't a race. It was that something else previously initialised the global state. | |
| 15:10:00 | mriedem | mdbooth: then how is it that it was never reproduced locally? | |
| 15:10:04 | mdbooth | It is. | |
| 15:10:05 | mriedem | if it was dirty state? | |
| 15:10:12 | mriedem | i never hit it locally | |
| 15:10:17 | mdbooth | See my comment 2 on the bug. | |
| 15:10:38 | mdbooth | Running those 2 tests in order reproduces the bug deterministically. | |
| 15:11:03 | sdague | mriedem: test grouping and order is an awkward level of deterministic. | |