| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-03 | |||
| 17:25:44 | efried | ...once we're checking gens on aggs, which is what this patch introduces. | |
| 17:25:54 | dansmith | okay so just a lot of bumping the agg generation yeah? | |
| 17:26:45 | efried | dansmith: update_from_provider_tree is smart enough to invalidate the cache entry for the provider that 409s, so the next time around it'll re-GET and have the new generation. But up to this point, we're not doing that retry until the next periodic hits. | |
| 17:27:10 | efried | So from the pov of periodic, we're fine, it heals itself. But if the 409 happens in the instance_claim path, it can cause a resched/fail. | |
| 17:27:27 | dansmith | okay but the failure that breaks a test is the agg mirroring in the api | |
| 17:27:35 | dansmith | which is presumably because it doesn't retry on 409 | |
| 17:27:39 | dansmith | what I'm wondering is, | |
| 17:27:40 | efried | right | |
| 17:27:53 | dansmith | why is the compute causing any update churn on the agg generation | |
| 17:27:57 | dansmith | ? | |
| 17:28:15 | efried | Don't we bump the RP gen whenever we allocate? | |
| 17:28:29 | efried | The agg doesn't have a generation. The RP does. | |
| 17:29:19 | dansmith | okay, so the 409 that the api is getting is that the RP it's trying to add to the agg has changed generations (and it doesn't retry)? | |
| 17:29:57 | efried | right | |
| 17:30:07 | dansmith | so what extra churn on the RP is the patch causing? | |
| 17:30:28 | efried | It's not causing extra churn. Before this patch, we weren't checking generations on agg updates - we were just blasting them in. | |
| 17:30:40 | efried | So we never got 409 - just whoever went second won. | |
| 17:30:50 | dansmith | right, but that's my point, | |
| 17:30:52 | efried | which is a bug | |
| 17:30:56 | efried | but it would be hidden | |
| 17:31:02 | dansmith | we're doing the same number of RP updates as before this patch? | |
| 17:31:03 | efried | checking gens exposes the bug. | |
| 17:31:05 | efried | yes | |
| 17:31:22 | dansmith | ...so what did it change that causes it to race and conflict with the api mirroring? | |
| 17:31:42 | efried | The change that caused the race was the introduction of host agg sync. | |
| 17:31:42 | dansmith | or are we potentially doing one more update if we got a 409 on the compute node and do a retry? | |
| 17:31:51 | efried | But the race stayed hidden because we weren't checking rp gen on agg updates. | |
| 17:31:55 | efried | Until this patch. | |
| 17:32:08 | efried | Nothing does a retry (yet) | |
| 17:32:12 | dansmith | no, that doesn't make sense | |
| 17:32:15 | dansmith | you checking aggs on the compute node doesn't make something else get a 409 | |
| 17:32:19 | efried | I'm suggesting that as the fix. | |
| 17:32:36 | efried | Oh, the generation checking is in the method that's common to both the compute side and the host agg sync. | |
| 17:32:52 | efried | sorry that wasn't clear | |
| 17:33:08 | dansmith | oh set_aggregates_for_provider() | |
| 17:33:09 | dansmith | ? | |
| 17:33:37 | efried | https://review.openstack.org/#/c/556669/11/nova/scheduler/client/report.py@1250 yeah | |
| 17:33:41 | dansmith | api is using that for the mirroring, which never would fail before because it didn't check, but now is failing because it does, in the patch | |
| 17:33:57 | dansmith | okay, I got it.. I thought you were saying lots of things other than this | |
| 17:34:14 | efried | :) | |
| 17:34:23 | efried | You were too late. I already killed your hit man. | |
| 17:34:36 | dansmith | I requested NFO | |
| 17:34:42 | dansmith | so that must have been jaypipes' hit man | |
| 17:34:42 | efried | while typing with the other hand | |
| 17:35:46 | efried | dansmith: So do you agree adding a retry on both sides of this thing would be a viable solution? | |
| 17:36:14 | dansmith | well, I agree that (a) mirroring should be checking generation and (b) that it should retry if generation conflicts, so .. yes? | |
| 17:36:25 | efried | dig | |
| 17:38:28 | efried | I'll retract what I said earlier about there not being a bug, mriedem. There is, we were just hiding it before. I'll open it and *try* to explain it. Then fix it via ^ | |
| 17:38:40 | dansmith | agree | |
| 17:39:21 | efried | Does the Big Lock in the rt mutex the _update periodic with the instance_claim path? | |
| 17:39:39 | efried | I guess it would have to. | |
| 17:41:04 | dansmith | that's the point of it I think | |
| 17:41:55 | efried | confirmed. | |
| 18:11:50 | openstackgerrit | Merged openstack/nova master: Merge server create schema for multiple create extension https://review.openstack.org/578338 | |
| 18:11:57 | openstackgerrit | Merged openstack/nova master: Merge server create schema for scheduler hint extension https://review.openstack.org/578343 | |
| 18:42:49 | karimull | Eric: I have one issue with a functional test case in nova on shared RP, can you help me understand where the issue is? here is the link | |
| 18:43:07 | karimull | Eric: http://paste.openstack.org/show/i9a6MRtJ1IUxF9xlI8to/ | |
| 18:45:49 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add action initiator attribute to the instance payload https://review.openstack.org/536243 | |
| 18:52:11 | karimull | efried: oops! pasted with wrong handle | |
| 18:52:31 | efried | karimull: looking... | |
| 18:52:48 | karimull | efried:thank you.. | |
| 18:53:05 | efried | karimull: What patch are you working on this test for? | |
| 18:57:01 | karimull | efried:https://review.openstack.org/#/c/569498/9 | |
| 18:57:56 | efried | karimull: Is the rebuild succeeding | |
| 18:57:57 | efried | ? | |
| 18:58:21 | efried | karimull: This is trying to rebuild to the same host? | |
| 18:59:27 | openstackgerrit | Merged openstack/nova master: conf: Correct documentation for '[pci] passthrough_whitelist' https://review.openstack.org/552874 | |
| 18:59:33 | openstackgerrit | Merged openstack/nova master: [doc] enhance admin/configuration/api.rst https://review.openstack.org/566008 | |
| 18:59:40 | openstackgerrit | Merged openstack/nova master: Remove mox in sec group test and functional tests https://review.openstack.org/576751 | |
| 19:00:21 | karimull | Efried:server state is Active after rebuild, but the DISK_GB usage is not accounted? | |
| 19:00:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Time how long pre_live_migration() takes https://review.openstack.org/574481 | |
| 19:00:50 | efried | karimull: or it got the disk from the compute node. | |
| 19:00:56 | efried | karimull: Where is the flavor defined? | |
| 19:02:53 | openstackgerrit | Merged openstack/nova master: conf: Resolve Sphinx errors https://review.openstack.org/579545 | |
| 19:02:55 | karimull | efried:I commented out "self.assertEqual({'DISK_GB': 1}, shared_rp_usages)" and the test passed | |
| 19:03:02 | karimull | efried: one min | |
| 19:03:39 | efried | karimull: I'm not completely sure how rebuild works, but I can't see where you're checking that the rebuild happened at all. | |
| 19:04:34 | efried | karimull: The other thing I'm confused about is how the rebuild could be succeeding - assuming the flavor asks for MEMORY_MB - because afaict you've *replaced* the inventory with just VCPU:1 (L92 of your paste). That's why I want to see the flavor... | |
| 19:04:50 | efried | oh, I guess it doesn't reallocate. | |
| 19:05:04 | efried | okay, I think I understand what's going on. | |
| 19:06:45 | efried | When you initially create the VM, it's getting its DISK_GB from the compute node. If you want to force it to get the DISK_GB from the sharing provider, you'll have to remove the DISK_GB inventory from the compute node. I think the libvirt driver does this for you at this point; try copying L76 to after L80 and see if it blows up. | |
| 19:07:18 | karimull | efried:ok..let me check | |
| 19:08:25 | efried | I think the other thing to know here is that I kinda doubt we're reallocating properly on rebuild with sharing providers. I don't think we've gone through and scrubbed the code for this. | |
| 19:15:24 | cdent | efried: there's a fixme at that part of the code for any kind of migration-like thing, if I remember right | |
| 19:16:21 | efried | cdent: Yes, I believe you are correct. | |
| 19:16:56 | jmlowe | I've been chipping away at getting erasure coding support for ceph, patches in review for glance and cinder. I hit a wall with nova. | |
| 19:17:50 | efried | cdent, karimull: It's not clear where we're messing up in this flow, though. Is the initial build allocating from the sharing provider or the compute node provider? Is the rebuild changing allocations at all? No combination of answers fully makes sense with what karimull is seeing, though. | |
| 19:18:27 | openstackgerrit | Andreas Karis proposed openstack/nova master: Add documentation for emulator threads policy https://review.openstack.org/579956 | |
| 19:18:34 | efried | cdent: Do I tag a bug `placement` if it's a bug in nova's use of placement? | |
| 19:19:05 | karimull | efried: failing at the same place...looks like libvirt (mock?) is not setup correctly for this test case... compared to other test cases similar to this one .. | |
| 19:19:15 | jmlowe | Basically all you need to do is give the name of an erasure coded pool when you do create or clone operations. I saw some soft of inferred pool name for cloning back into glance and I have no idea how to infer the data pool option. | |
| 19:19:33 | mriedem | dansmith: see what you think about what yikun is doing here during this refactor https://review.openstack.org/#/c/579113/ | |
| 19:19:55 | jmlowe | Any ideas or am I just talking nonsense here? | |
| 19:20:04 | efried | karimull: Oh, yeah, I missed that, you're using SmallFakeDriver, you should be using FakeLibvirtDriver. | |
| 19:20:06 | cdent | efried: for now, yes | |
| 19:20:07 | mriedem | i understand why he's removing the ability to save changes to InstanceGroup.policies (it saves him some work for a thing that isn't used today and isn't supported anyway), but still | |
| 19:20:38 | efried | mriedem, dansmith: FYI https://bugs.launchpad.net/nova/+bug/1779931 | |
| 19:20:38 | openstack | Launchpad bug 1779931 in OpenStack Compute (nova) "Provider update race between host aggregate sync and resource tracker" [Undecided,New] | |
| 19:21:42 | karimull | efried:good catch..let me try with FakeLibvirtDriver..thanks .. | |
| 19:22:26 | mriedem | jmlowe: no idea, jbernard or mnaser or cfriesen might have ideas | |