| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-16 | |||
| 12:48:51 | efried | mostly | |
| 12:49:05 | sean-k-mooney | ya that was what i was about to say | |
| 12:49:25 | efried | I think we have a race in select_destinations though | |
| 12:49:38 | efried | been seeing quite a few like this https://storage.gra1.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_778/674895/42/check/openstack-tox-py36/7787cd5/testr_results.html.gz | |
| 12:51:56 | sean-k-mooney | maybe or that test is just not deterministic | |
| 12:52:15 | efried | yeah, either way | |
| 12:52:19 | efried | it's not just that one test | |
| 12:52:32 | efried | I saw several similar failures with that same select_destinations unequal | |
| 12:52:48 | efried | I didn't dig all the way into them (lack of appropriate tooling on my phone) | |
| 12:52:51 | sean-k-mooney | all in the fallback tests | |
| 12:52:53 | efried | but thinking maybe I should at this point. | |
| 12:53:06 | efried | no, I remember seeing one in a pre-existing test (I think) | |
| 12:53:15 | efried | i.e. regression | |
| 12:53:39 | sean-k-mooney | we might not be mocking some global state in the tests? | |
| 12:54:06 | sean-k-mooney | that said the fallback code is very new so there might be something there | |
| 12:56:35 | stephenfin | efried: I'm thinking we should stop rechecking the two few patches for now | |
| 12:56:40 | stephenfin | *top | |
| 12:56:45 | efried | whyzat? | |
| 12:56:58 | efried | oo, I have a local repro | |
| 12:57:04 | efried | running the whole test class | |
| 12:57:10 | efried | nova.tests.unit.scheduler.test_scheduler.SchedulerManagerTestCase | |
| 12:57:15 | efried | sean-k-mooney: ^ | |
| 12:57:39 | sean-k-mooney | ok then it is likely either a shared state issue or a bug in the code | |
| 12:57:42 | stephenfin | On account of having to recheck again if anything lower gets in. Better just get those lower ones in | |
| 12:57:55 | sean-k-mooney | but if that repoduces it locally that makes it eaiser to figure out | |
| 12:58:11 | stephenfin | Hmm, I've seen that as well, actually | |
| 12:58:28 | stephenfin | Okay, that sounds like a bug. That's testing the retry logic | |
| 12:59:05 | efried | stephenfin: if it's not affecting queue times -- which it probably isn't at this point -- then it's better to have the top ones kicked out by merge-failed -2s than to have to wait for them to pass the check pipeline again once the lower ones are in, imho | |
| 12:59:34 | stephenfin | that's a fair point | |
| 12:59:35 | sean-k-mooney | efried: im just about to grab lunch but ill dig into it when i get back if ye have not figured it out by then | |
| 12:59:35 | efried | stephenfin, sean-k-mooney: I definitely remember seeing the failure on a patch lower in the series at some point. | |
| 12:59:48 | efried | I don't remember which one now | |
| 12:59:53 | efried | where was that fallback introduced, again? | |
| 13:00:24 | stephenfin | efried: https://review.opendev.org/#/c/671801/ | |
| 13:00:50 | sean-k-mooney | ya thats the patch that updated those tests too | |
| 13:00:59 | stephenfin | intermittently failing tests are the _best_ | |
| 13:01:09 | sean-k-mooney | brb | |
| 13:05:46 | shilpasd | Eric: finally after so many rechecks isoalte agg patches are merged, tnx for your extended support | |
| 13:06:13 | shilpasd | Gibi: tnx for followup patch, review on isolate agg patches | |
| 13:06:43 | efried | shilpasd: Your work on this feature is really appreciated. Please pass our compliments on to your colleagues as well. | |
| 13:08:25 | shilpasd | dansmith: stephenfin: takashin: thanks for review | |
| 13:08:42 | shilpasd | efried: sure thanks | |
| 13:13:16 | efried | stephenfin: First thing that jumps out is test_select_destination_with_pcpu_fallback_disabled, which is a test, is calling test_select_destination_with_4_3_client, which is also a test. I.e. both are going to run "simultaneously", one being a subset of the other. | |
| 13:13:27 | efried | ...which should be fine, as long as neither is doing anything global | |
| 13:14:43 | stephenfin | they don't _look_ like they're doing anything global | |
| 13:18:01 | kashyap | print domain_caps[arch][machine_type]._os.loader.enums | |
| 13:18:06 | kashyap | Oops | |
| 13:18:38 | kashyap | (Disregard that, please) | |
| 13:21:28 | kashyap | efried: Mired in parsing some XML bits and testing :D | |
| 13:21:38 | efried | suuuure | |
| 13:21:40 | kashyap | Not my day today... | |
| 13:23:02 | efried | stephenfin: I agree. I'm looking through the other tests in that class; the problem could just as easily be caused by one of *them* doing something global. | |
| 13:44:33 | mriedem | gibi: a few comments in this bug fix of yours https://review.opendev.org/#/c/666857/ | |
| 13:44:37 | stephenfin | efried: something is messing with flags | |
| 13:44:59 | efried | stephenfin: Because CONF is global mebbe? | |
| 13:45:08 | stephenfin | quite possibly, yeah | |
| 13:45:45 | stephenfin | 'CONF.workarounds.disable_fallback_pcpu_query' is intermittently 'False' despite me setting it to 'True' in the test | |
| 13:48:32 | efried | stephenfin: I can reproduce reliably locally when I skip test discovery, and reliably *not* reproduce when I let discovery happen (which runs the tests slower). So definitely racy. | |
| 13:54:30 | efried | stephenfin: Yeah, self.flags is just CONF.set_override | |
| 13:54:58 | stephenfin | and CONF is global | |
| 13:55:26 | efried | stephenfin: use oslo_config.fixture.Config | |
| 13:55:30 | stephenfin | This is hardly the first time we've got positive and negative tests for something config opt-driven though, is it? | |
| 13:56:08 | sean-k-mooney | well | |
| 13:56:24 | sean-k-mooney | you wont have two tests running at the same time in the same process | |
| 13:56:37 | sean-k-mooney | so its fine to set things with set flags | |
| 13:56:49 | sean-k-mooney | as long as you alreay set the correct state up in the test | |
| 13:56:51 | efried | sean-k-mooney: same process yes, same thread no, right? | |
| 13:56:58 | sean-k-mooney | no | |
| 13:57:09 | artom | Wait, has oslo_config.fixture.Config finally fixed the global CONF problem? I guess the fixture is not per-test, so no. | |
| 13:57:11 | sean-k-mooney | tox runes tests in multiple processes not threads | |
| 13:57:25 | stephenfin | well, stestr | |
| 13:57:57 | efried | artom: ...no, it looks like it still by default uses global CONF. So much for that idea. | |
| 13:58:10 | sean-k-mooney | yes point is each worker is tis own process that runs its own python interpreter | |
| 13:58:19 | sean-k-mooney | and only 1 test is running in each worker at a time | |
| 13:58:34 | sean-k-mooney | so test only share global state with other test that ran in that worker | |
| 13:58:41 | stephenfin | we're using ConfFixture in our base test class in nova/test.py | |
| 13:58:43 | sean-k-mooney | but only one test is running at a time | |
| 14:00:12 | stephenfin | then wtf is happening here? :-D | |
| 14:00:47 | sean-k-mooney | im just back form lunch so i can start runing it in a debugger | |
| 14:01:02 | sean-k-mooney | ye have narrorwed it down to the config option yes? | |
| 14:01:18 | stephenfin | That's what I'm seeing anyway | |
| 14:01:26 | stephenfin | It should be True but it's False instead | |
| 14:01:53 | sean-k-mooney | and ye are seeing it only when the test class is run | |
| 14:02:00 | sean-k-mooney | rather then the test directly | |
| 14:02:21 | stephenfin | yeah, I'm seeing it intermittently but efried notes that it's consistent if you run without test discovery | |
| 14:02:26 | efried | which gels with the problematic result, which is expecting from GET /a_c {normal results} and is getting {normal results + fallback results} | |
| 14:02:31 | stephenfin | so 'tox -e py27 -- -n nova/tests/unit/scheduler/test_scheduler.py' | |
| 14:03:21 | efried | stephenfin, sean-k-mooney: fwiw I'm running it from within the venv as | |
| 14:03:21 | efried | stestr run -n nova.tests.unit.scheduler.test_scheduler.SchedulerManagerTestCase | |
| 14:03:27 | efried | hitting it every time | |
| 14:03:42 | sean-k-mooney | ya i can run the full file in the debugger too although im not sure if that will show anything specifically | |
| 14:03:55 | efried | ^^ is just the one test class | |
| 14:05:22 | sean-k-mooney | yep it failed for me too | |
| 14:05:52 | mriedem | i haven't been following but is there a new gate failure? | |
| 14:05:59 | mriedem | and if so, do we have a bug and e-r query tracking it? | |
| 14:05:59 | sean-k-mooney | no | |
| 14:06:18 | sean-k-mooney | its a intermitent failure of one of the tests in the PCPU series | |
| 14:10:01 | efried | stephenfin: So I've narrowed it down, by commenting out all the tests in the suite *except* test_select_destination_with_pcpu_fallback_disabled and test_select_destination_with_pcpu_fallback I still get the repro; commenting out either of those and I'm fine. | |
| 14:10:18 | efried | I also factored out the 4_3 thing so the former ^ only runs the one test | |
| 14:10:38 | efried | I also commented out the self.flags in the latter ^ because it's setting the default -- and still get the repro | |