| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-06-05 | |||
| 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 | |
| 16:03:42 | mriedem | THE call | |
| 16:03:46 | mdbooth | mriedem: np | |
| 16:13:29 | openstackgerrit | Merged openstack/nova-specs master: Follow the new PTI for document build https://review.openstack.org/551802 | |
| 16:29:46 | melwitt | efried: yeah, no specific dashboard. just whatever you normally like to use for specs | |
| 16:35:54 | openstackgerrit | Merged openstack/nova master: Fix bug for hypervisors https://review.openstack.org/572063 | |
| 16:36:01 | openstackgerrit | Merged openstack/nova master: Fix bug to doc https://review.openstack.org/572268 | |
| 16:40:59 | mriedem | dansmith: i know i say this every 3 days, but i think stephenfin's numa aware vswitches spec is ready to go https://review.openstack.org/#/c/541290/ | |
| 16:41:56 | efried | We should really have a spec review day so we can merge that thing. | |
| 16:42:21 | dansmith | mriedem: can I skip this one and wait three days until it's ready again? | |
| 16:42:30 | mriedem | no | |
| 16:45:27 | dansmith | damn | |
| 16:48:02 | dansmith | I feel like the proof is in the code anyway and I'm out of steam on the spec | |
| 16:48:08 | dansmith | so, damn the torpedoes | |
| 16:49:02 | mriedem | yeah i already went through the code series this morning and made notes on the patches about what needs to change | |
| 16:49:04 | mriedem | at a high level | |
| 16:52:55 | openstackgerrit | Merged openstack/nova stable/queens: Allow cinderv2 endpoints within the request context catalog https://review.openstack.org/572213 | |
| 16:56:54 | openstackgerrit | Merged openstack/nova-specs master: Add 'numa-aware-vswitches' spec https://review.openstack.org/541290 | |
| 17:10:24 | efried | bhagyashri_s: Greate work on https://review.openstack.org/#/c/560459/ -- just a couple of minor things and I think it's ready to go. I'm going to have to let other cores (probably bauzas and jaypipes) approve it, since I contributed to the fix. | |
| 17:31:13 | openstackgerrit | Chris Friesen proposed openstack/nova-specs master: Add support for emulated virtual TPM https://review.openstack.org/571111 | |
| 17:45:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: api-ref: mention that you can't re-parent a resource provider https://review.openstack.org/572501 | |
| 18:18:04 | openstackgerrit | Dan Smith proposed openstack/nova master: Change consecutive build failure limit to a weigher https://review.openstack.org/572195 | |
| 18:55:50 | mriedem | dansmith: melwitt: tssurya: on the handling a down cell spec, https://review.openstack.org/#/c/557369/ - i'm thinking that if the user is filtering or paging while listing instances, we need to ignore down cells | |
| 18:55:57 | mriedem | because we can't provide accurate results in that case | |
| 18:56:23 | mriedem | not that we're providing much in the way of results with just a flat 'nova list', but at least it's a dump of all instances (under the default limit) with minimal fields specified | |
| 18:56:56 | dansmith | you mean sorting? | |
| 18:56:56 | melwitt | yeah, either that or do the minimal shell of an instance thing for those in down cells | |
| 18:57:02 | tssurya | mriedem: yes makes sense, | |
| 18:57:08 | mriedem | dansmith: no, filtering | |
| 18:57:19 | dansmith | if they're sorting we can't really show the shells, | |
| 18:57:20 | dansmith | since we don't know what the sort key value is | |
| 18:57:21 | mriedem | sorting kind of goes out the window as well | |
| 18:57:21 | dansmith | same for filtering I guess | |
| 18:57:40 | mriedem | melwitt: the minimal shell is the problem when filtering | |
| 18:57:46 | tssurya | if its the default sorting, I think it uses created_at, | |
| 18:57:55 | mriedem | if i'm doing 'nova list --status PAUSED', don't show me 4 PAUSED instances and 100 UNKNOWN instances | |
| 18:57:55 | melwitt | oh | |
| 18:58:00 | melwitt | yeah | |
| 18:58:02 | dansmith | right | |
| 18:58:15 | mriedem | if i do: nova list --all-tenants | |
| 18:58:19 | mriedem | then sure, give me the dump | |