| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-06-05 | |||
| 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 | |
| 14:41:53 | dansmith | plus, 60s by default | |
| 14:44:46 | mriedem | yeah that makes sense | |
| 14:45:14 | dansmith | aight, well, that likely removes this from my path, | |
| 14:45:26 | dansmith | although I still am concerned that something ain't right in that numa accounting path | |
| 14:46:00 | dansmith | although it will make my testing a little harder to make deterministic | |
| 14:46:23 | dansmith | you know, I had this thing for functional tests that let me trigger periodics reliably that got shot down with fire....... | |
| 14:47:54 | mriedem | i'll assume i wasn't on the firing end of that | |
| 14:49:20 | dansmith | nope | |
| 14:53:31 | efried | melwitt: We got a dashboard or anything for spec reviewing? | |
| 14:58:47 | stephenfin | efried: I use https://goo.gl/EjSxKw | |
| 14:58:57 | efried | stephenfin: Thanks! | |
| 14:59:08 | stephenfin | efried: The tail end | |
| 14:59:32 | efried | is that all the specs we got? That ain't so bad :) | |
| 14:59:45 | stephenfin | It's limited at 20, so no :) | |
| 15:00:11 | stephenfin | there's the full one https://review.openstack.org/#/q/project:openstack/nova-specs+status:open | |
| 15:08:33 | openstackgerrit | Surya Seetharaman proposed openstack/nova-specs master: Handling a down cell https://review.openstack.org/557369 | |
| 15:19:22 | openstackgerrit | Theodoros Tsioutsias proposed openstack/nova-specs master: Add PENDING vm state https://review.openstack.org/554212 | |
| 15:25:14 | mriedem | this is my specs review dashboard http://tiny.cc/2g1sty | |
| 15:36:45 | stephenfin | Can someone with +2 for nova-specs put this in? The guy has been refreshing it pretty much continuously since March and it's valid IMO https://review.openstack.org/#/c/551802/ | |
| 15:59:23 | mdbooth | mriedem: If I've read it correctly, AttachVolumeMultiAttachTest.test_volume_swap_with_multiattach seems to test something which I'd expect us to forbid, because it can't work. | |
| 15:59:54 | mdbooth | It looks like we attach a volume to 2 servers | |
| 16:00:23 | mdbooth | Then do a swap volume to a new volume on one of the servers | |
| 16:01:03 | mdbooth | The problem is, IIUC that's a block migration, and there's no way for the block migration on server 1 to know about writes on server 2 | |
| 16:01:37 | mdbooth | So the resulting data on volume2 attached to server1 is going to be inconsistent, and there's nothing you can do about it in either guest | |
| 16:02:51 | mriedem | stephenfin: done | |
| 16:02:53 | mdbooth | forbade? | |
| 16:03:11 | stephenfin | mriedem: cheers | |
| 16:03:15 | mdbooth | forbode? No, definitely not. Perhaps appropriate, though. | |
| 16:03:19 | mriedem | mdbooth: cinder forbids a retype to/from multiattach capability for an in-use volume | |
| 16:03:36 | mriedem | also on a call atm | |