| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-06-05 | |||
| 13:59:05 | stephenfin | mriedem: and thanks | |
| 14:00:05 | stephenfin | dansmith: It could also be that we're booting two instances consecutively from different tests | |
| 14:00:17 | dansmith | but they should have different databases and not see each other | |
| 14:00:19 | stephenfin | except, yeah, each test gets its own sqlite DB | |
| 14:00:20 | stephenfin | Hmm | |
| 14:00:52 | mriedem | do the fake hosts/nodes in the tests have the same name? | |
| 14:01:02 | mriedem | because the nova.tests.unit.virt.fake set_nodes or whatever is global | |
| 14:01:15 | mriedem | one of the tests might not be resetting the fake node on cleanup? | |
| 14:01:19 | dansmith | yeah | |
| 14:01:49 | dansmith | I don't see them using fake set_nodes | |
| 14:01:54 | dansmith | unless it's somewhere else | |
| 14:02:00 | mriedem | parent class? | |
| 14:02:42 | dansmith | I don't think so.. they | |
| 14:02:49 | dansmith | are starting their own compute service for some reason | |
| 14:02:58 | dansmith | maybe so they can control the topology | |
| 14:03:07 | stephenfin | dansmith: Yeah, that | |
| 14:04:12 | stephenfin | That's stored in nova.objects.compute_node.ComputeNode.numa_topology, which I think is created on startup for nova-compute | |
| 14:04:51 | dansmith | and they don't do fake node set in the parent class(es) | |
| 14:06:33 | tssurya | mriedem: yea that spec is going crazy | |
| 14:06:49 | dansmith | stephenfin: hmm, just got two failures from that module in one run, so something is definitely wonky there | |
| 14:07:03 | stephenfin | dansmith: Yeah, I'm taking a look at it now | |
| 14:07:15 | dansmith | I really can't think of why my patch would be affecting this kind of thing | |
| 14:09:41 | openstackgerrit | Matt Riedemann proposed openstack/nova-specs master: Add 'numa-aware-vswitches' spec https://review.openstack.org/541290 | |
| 14:09:57 | mriedem | stephenfin: fyi https://review.openstack.org/#/c/541290/17..18/specs/rocky/approved/numa-aware-vswitches.rst | |
| 14:10:33 | mriedem | stephenfin: also, i haven't gone through the code series in detail yet, but it would be best if the api changes for this come at the end of the series | |
| 14:10:37 | mriedem | so build everything from the bottom up | |
| 14:10:51 | stephenfin | mriedem: Which API? | |
| 14:10:55 | stephenfin | The neutron one? | |
| 14:10:58 | mriedem | i.e. create_pci_requests_for_sriov_ports populating InstanceNUMANetworks to set in the RequestSpec comes last | |
| 14:11:01 | stephenfin | gotcha | |
| 14:11:04 | mriedem | yeah | |
| 14:11:12 | mriedem | then nothing gets 'turned on' until the end | |
| 14:11:43 | dansmith | oh, I bet I know something related | |
| 14:11:46 | openstackgerrit | Naichuan Sun proposed openstack/nova master: xenapi(N-R-P): Add API to support vgpu resource provider create https://review.openstack.org/520313 | |
| 14:14:36 | dansmith | stephenfin: I'm calling update_available_resource() now after build success or failure.. is there something that might not be set up that would prevent that from working or something? | |
| 14:16:40 | stephenfin | dansmith: Well, reverting that chunk "fixes" things | |
| 14:16:49 | dansmith | the update? | |
| 14:16:52 | stephenfin | aye | |
| 14:17:01 | stephenfin | at least, locally | |
| 14:17:19 | stephenfin | I've run it a few times and it passed each time with the HEAD~ version of that file | |
| 14:17:38 | dansmith | I see it failing during _delete_server, FWIW, but I wonder if delete just fails because the create left something in a bad state | |
| 14:18:02 | dansmith | stephenfin: so....the update fails because we have a half-created instance with topology that we can't satisfy, yeah? | |
| 14:18:21 | dansmith | seems like we'd hit this in real life if that's the case, if a periodic ran during a failed schedule or something | |
| 14:18:31 | dansmith | because that update call happens all over the place | |
| 14:19:01 | stephenfin | Like you said, it could be a buggy test too | |
| 14:19:55 | dansmith | well, I'm not really sure where to go from here | |
| 14:20:31 | dansmith | this is kinda like the bug we saw with the DiskNotFound, | |
| 14:20:45 | dansmith | where that error for a single instance will block nova from updating the RT for everything on the host | |
| 14:21:00 | stephenfin | Hmm, now I can't get the tests to _fail_ again | |
| 14:21:01 | dansmith | so maybe we should catch this exception somewhere, log it at error and allow the RT to proceed? | |
| 14:21:30 | mriedem | the functional tests shouldn't need to do their own delete server, unless they are intentionally trying to make sure the server is gone - because the test tearDown will wipe the db | |
| 14:21:44 | dansmith | mriedem: yeah | |
| 14:22:09 | dansmith | removing that might make the test failure go away, but it feels kinda dirty | |
| 14:23:46 | dansmith | although the test checks that the instance goes active, so.. | |
| 14:23:54 | openstackgerrit | Naichuan Sun proposed openstack/nova master: xenapi(N-R-P): Add API to support vgpu resource provider create https://review.openstack.org/520313 | |
| 14:24:25 | mriedem | well, the test waits for the server to be != 'BUILD' | |
| 14:24:29 | mriedem | which has always bugged me | |
| 14:24:39 | mriedem | oh update_available_resource_for_node | |
| 14:24:40 | mriedem | oops | |
| 14:24:51 | mriedem | end_status='ACTIVE' i see | |
| 14:25:14 | stephenfin | dansmith: Yeah, I think that's what's happening | |
| 14:25:21 | dansmith | stephenfin: what? | |
| 14:25:26 | mriedem | what is calling update_available_resource_for_node ? | |
| 14:25:43 | dansmith | mriedem: update_available_resource() is, which is what I'm calling from build_success/failure | |
| 14:25:44 | stephenfin | dansmith: you're now calling 'update_available_resource' which iterates through instances to extra their numa_topology | |
| 14:25:58 | stephenfin | and updates utilization based on that | |
| 14:26:07 | dansmith | stephenfin: oh and hitting the incomplete instance or something? | |
| 14:26:17 | stephenfin | yeah, I'd imagine so | |
| 14:26:24 | dansmith | so that's a bug in the actual code then, yeah? | |
| 14:26:25 | mriedem | seems weird that the stacktrace in the test failure doesn't include _build_succeeded | |
| 14:26:38 | dansmith | mriedem: yeah, idk why that is | |
| 14:26:43 | openstackgerrit | Naichuan Sun proposed openstack/nova master: xenapi(N-R-P):Get vgpu info from `allocations` https://review.openstack.org/521717 | |
| 14:27:06 | openstackgerrit | Naichuan Sun proposed openstack/nova master: xenapi(N-R-P): support compute node resource provider update https://review.openstack.org/521041 | |
| 14:27:35 | dansmith | so removing the _delete_server call from that test seems to make it happy, but I think it's still failing to do that update under the covers, we're just not seeing it or whatever | |
| 14:29:03 | dansmith | yeah, so I get a NUMAServersTest failure now in _delete | |
| 14:29:25 | stephenfin | dansmith: This line is probably getting called multiple times https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L948-L951 | |
| 14:30:04 | dansmith | that's multiple lines.. do you mean L949? | |
| 14:30:06 | stephenfin | So we're double freeing, kind of. This is starting to feel familiar but I'm not sure why | |
| 14:30:10 | stephenfin | yup | |
| 14:30:35 | dansmith | okay, but that shouldn't be a problem with repeated calls to update_available_resource()... since it starts fresh when it does that right? | |
| 14:32:16 | stephenfin | Well if compute is thinking the instance is gone, it could be freeing on the first call to that function (the one you added) and then attempting to do so again on our call (the API call to delete) | |
| 14:32:25 | stephenfin | which triggers this, I imagine? https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1127 | |
| 14:32:42 | dansmith | mriedem: you know, unrelated (kinda) to this test failure, we could just increase the counter and depend on the next periodic run of update_available_resources() to update our counter, | |
| 14:33:20 | dansmith | which means we might be failbait for 60s or whatever you have that timer set to, but then we'll update our stuff and get pulled out | |
| 14:33:21 | dansmith | which would save a bunch of cpu and db updates on each build | |
| 14:34:03 | dansmith | stephenfin: not sure that's how that works.. update_available_resource can run at whatever interval it's set to.. it should be idempotent, generating a fresh view of resources all the time, never having partial side effects or anything like that | |
| 14:38:48 | stephenfin | dansmith: Right but...dodgy tests. Perhaps that was never triggered before (the interval would be too short) | |
| 14:39:11 | mriedem | dansmith: yeah....also, _update_available_resource is going to take the big COMPUTE_RESOURCE_SEMAPHORE lock | |
| 14:39:20 | mriedem | when all you really wanted to do was update the compute_node.stats field | |
| 14:39:31 | dansmith | stephenfin: well, I don't see how the tests would cause this to be broken.. I think it has to be broken in the code and we just never poke this (and notice) in tests right? | |
| 14:39:44 | dansmith | mriedem: yeah, the more I think about it, that'd be a better approach | |
| 14:39:52 | mriedem | OR | |
| 14:39:53 | dansmith | mriedem: and would remove a ton of unit test noise I had to add | |
| 14:40:48 | mriedem | RT.abort_instance_claim could do the build failure count | |
| 14:40:58 | mriedem | but, if we fail before we get to the instance claim, then that won't count the build failure | |
| 14:41:09 | mriedem | but abort_instance_claim does update the compute node record | |
| 14:41:20 | dansmith | yeah, that runs before where I'm increasing right? | |
| 14:41:27 | mriedem | yes | |
| 14:41:31 | dansmith | that's why Isaid in my comment just now that the next instance failure would update it for us | |
| 14:41:50 | dansmith | which I think is reasonable lazy update behavior to avoid the synchronous update | |