| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-22 | |||
| 14:25:34 | leakypipes | finucannot: it's not that I don't like your code. I'm just having a hard time with the whole cascading policy loop thing... I agree with you that pools != filtered_pools and that my original complaint about coupling is not valid. however it still feels a bit clever for my taste. I'll remove my -1 and leave it to bauzas and gibi to +W | |
| 14:33:00 | leakypipes | mriedem: morning. | |
| 14:39:08 | openstackgerrit | Matthew Booth proposed openstack/nova master: Add uuid column to BlockDeviceMapping https://review.openstack.org/242602 | |
| 14:39:09 | openstackgerrit | Matthew Booth proposed openstack/nova master: Make BlockDeviceMapping object support uuid https://review.openstack.org/242603 | |
| 14:39:09 | openstackgerrit | Matthew Booth proposed openstack/nova master: Add an online migration for BDM.uuid https://review.openstack.org/525599 | |
| 14:39:10 | openstackgerrit | Matthew Booth proposed openstack/nova master: DriverBlockDevice: make subclasses inherit _proxy_as_attr https://review.openstack.org/524167 | |
| 14:39:10 | openstackgerrit | Matthew Booth proposed openstack/nova master: Expose BDM uuid to drivers https://review.openstack.org/529037 | |
| 14:42:54 | mdbooth | mriedem: Updated ^^^ I haven't completely re-run all tests locally, though. Just the obvious ones. Been bitten by that a couple of times recently, but I'm being optimistic anyway. | |
| 15:01:16 | cfriesen | what do we expect to happen if you do a resize/migrate, and after completion but before confirmation the dest node dies and we do an evacuate? Should the resize/migrate revert, should the evacuation pass without error, or something else? | |
| 15:23:12 | finucannot | leakypipes: Let me take a second shot at it. If it's too clever for you, it's going to be too clever for others too | |
| 15:23:31 | leakypipes | finucannot: maybe. I'm pretty dumb, though. | |
| 15:23:56 | finucannot | Ah, so am I. All the more reason | |
| 15:23:57 | finucannot | leakypipes: I assume you're gone next week? | |
| 15:24:28 | leakypipes | finucannot: nope, I'm working all next week. I got no vacation this year when i joined Verizon... | |
| 15:24:52 | finucannot | Ouch :( | |
| 15:25:16 | finucannot | four days is _more_ than enough for all involved | |
| 15:25:57 | leakypipes | heh | |
| 15:26:23 | finucannot | I had a look at that bug you mentioned too. Totally out of my area of expertise, I'm afraid, so best of luck with that, heh | |
| 15:27:03 | leakypipes | finucannot: yeah, it's a weird one alright... | |
| 15:28:26 | finucannot | and intermittent too. The worst kind of bug :( | |
| 15:38:34 | mriedem | cfriesen: you can't evacuate an instance in VERIFY_RESIZE state can you? | |
| 15:38:43 | mriedem | leakypipes: hi | |
| 15:38:51 | mriedem | leakypipes: did your xmas dinner party turn out ok? | |
| 15:39:10 | mriedem | cfriesen: @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED, | |
| 15:39:11 | mriedem | vm_states.ERROR]) | |
| 15:39:11 | mriedem | def evacuate(self, context, instance, host, on_shared_storage, | |
| 15:39:46 | mriedem | cfriesen: you'd likely want to revert the resize to get the instance back to the source host | |
| 15:41:47 | mriedem | mdbooth: +2 on https://review.openstack.org/#/c/242602/ | |
| 15:41:51 | mriedem | despite that terrible blank space | |
| 15:42:04 | mdbooth | mriedem: Thank you, Sir! | |
| 15:43:16 | mdbooth | Hah! I do that when I want to comment branches separately. Hadn't really thought about it too hard, tbh. | |
| 15:43:45 | mdbooth | I guess the comments can also live inside the block | |
| 15:46:10 | mdbooth | artom: So, that concurrency test | |
| 15:46:33 | mriedem | efried_cya_jan: how does one recheck the powervm CI? it's not on the wiki https://wiki.openstack.org/wiki/ThirdPartySystems/IBM_PowerVM_CI | |
| 15:46:34 | artom | mdbooth, right | |
| 15:47:21 | mriedem | "recheck powervm" i guess | |
| 15:47:45 | mdbooth | It sounded like I hadn't convinced you, yet. I could be persuaded to simplify it, but I'm still thinking about that. | |
| 15:47:46 | artom | mdbooth, so, I grok the Python threads stuff - they don't run concurrently, one will run when another yields to do IO or whatever | |
| 15:48:03 | artom | mdbooth, it's not so much convincing, as explaining :) | |
| 15:48:32 | artom | mdbooth, what's not clear to me, and it may be ignorance of Python's internals on my part, is how your code makes sure there are two threads | |
| 15:48:37 | mdbooth | artom: Well your point is around whether the complexity is worth it, right? | |
| 15:48:56 | mdbooth | I'm confident it's a valid test. I could be persuaded that a simpler test might be better, though. | |
| 15:49:08 | mdbooth | There aren't 2 threads. | |
| 15:49:22 | mdbooth | There's only 1 thread, but it acts like 2 threads. | |
| 15:49:45 | mdbooth | We use mock to intercept a function call in the 'main' thread. | |
| 15:50:05 | mdbooth | When the main thread executes that function call, we interrupt it and do something else first, before continuing. | |
| 15:50:23 | mdbooth | Does that make more sense? | |
| 15:50:55 | mriedem | seems pretty paranoid for something that we've already established a pattern of in several other objects | |
| 15:51:06 | mriedem | or are you actually seeing this race happening with something like the cellsv1 job? | |
| 15:51:40 | artom | mdbooth, let me look at the code again | |
| 15:51:53 | artom | I still can't wrap my head around how purely sequential execution can test a race | |
| 15:51:58 | mdbooth | mriedem: It's more that I can see the bug and I fixed it, and it's not *that* complicated. | |
| 15:52:25 | mdbooth | mriedem: artom is trying to get his head round the test, and I've never yet managed to write a unit test for a race which was easy to read. | |
| 15:52:31 | artom | If it's just about calling _create_uuid twice, surely can do that without the whole flip/race thing | |
| 15:53:28 | mdbooth | artom: That's where I could be persuaded. Except that my test is 1 step up from that. | |
| 15:54:00 | mdbooth | My test asserts that if the race happens whilst reading the bdm object, it will work fine. | |
| 15:54:20 | mdbooth | Your test would be much simpler, and we could possibly agree it's sufficient. | |
| 15:54:24 | artom | But... the race can only happen when writing | |
| 15:54:55 | artom | I guess if you go up one stop from that, it two reads happen at the same time on the same uuid-less BDM, both will attempt to write a UUID | |
| 15:54:59 | artom | *if two | |
| 15:55:02 | mdbooth | No, this is weird. If we *read* a bdm object with no uuid we create one before returning it. | |
| 15:55:28 | mdbooth | So it's a read operation, but we might have to migrate a legacy object during it. | |
| 15:55:32 | artom | Right, but the actual race is the writing part | |
| 15:55:37 | mdbooth | Yes | |
| 15:56:01 | artom | That's what the code, does, right? Read, if there's no UUID, create one and save it, then return | |
| 15:56:02 | mdbooth | My test is at the level of the read operation | |
| 15:56:04 | mdbooth | Your test would just be on the write bit | |
| 15:56:37 | mdbooth | artom: Yep. Notice there's a compare-and-swap in there, which avoids a race. | |
| 15:57:18 | mdbooth | I guess the most important thing is to test the compare-and-swap, which simply executing _create_uuid manually twice would do. | |
| 15:57:24 | mdbooth | The test would also be easier to read. | |
| 15:57:39 | artom | Ah, right, compare and swap is atomic at the DB level, right? | |
| 15:57:45 | mdbooth | Yes | |
| 15:58:02 | mdbooth | There are a couple of db-related hoops in that function which ensure it's atomic. | |
| 15:58:14 | openstackgerrit | Merged openstack/nova stable/ocata: Use instance.project_id when creating request specs for old instances https://review.openstack.org/529387 | |
| 15:58:24 | artom | So if there are two of more getting compare_and_swaps getting to the DB at the same time, only the first one will succeed | |
| 15:58:37 | artom | So, the DB-write race is handled for us | |
| 15:58:42 | artom | I may have been thinking about this wrong | |
| 15:58:43 | mdbooth | Correct. | |
| 15:59:27 | artom | So is there actually a race then? If we try a compare_and_swap and it fails, if we read after that, we're guaranteed to get the correct UUID | |
| 15:59:43 | artom | A read race, I should say | |
| 15:59:51 | mdbooth | There is no read race. | |
| 15:59:59 | mdbooth | Not in this patch, anyway. | |
| 16:00:04 | artom | ... | |
| 16:00:10 | artom | So what are we testing then? | |
| 16:00:16 | mdbooth | However, I think the original one just wrote the uuid to the db. | |
| 16:00:28 | mdbooth | That is a race, and if you executed my test against that version, it would fail. | |
| 16:00:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add PCI NUMA policies https://review.openstack.org/527472 | |
| 16:00:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: trivial: Modify signature of _filter_non_requested_pfs https://review.openstack.org/527473 | |
| 16:01:08 | finucannot | leakypipes, bauwser: Voilà ^^ | |
| 16:01:13 | mdbooth | In fact, I think the original version didn't write it immediately at all, just generated it. | |
| 16:01:16 | leakypipes | mriedem: yeah, dinner was great. dishwasher is leaking, though... | |
| 16:01:44 | mdbooth | So there was a delay of unknown size between generation of the value and writing it to the db without a compare-and-swap. | |
| 16:01:45 | mriedem | leakypipes: "dishwasher" isn't code for one of the pugs is it? | |
| 16:01:47 | leakypipes | mriedem: so after $400 electrician bill I now have another $200 bill for the installers from Lowe's to come day after christmas to fix it. | |
| 16:01:47 | finucannot | leakypipes: I assume the irony of that has already been pointed out multiple times | |
| 16:01:53 | leakypipes | finucannot: yes sir. | |
| 16:02:00 | leakypipes | mriedem: no. | |
| 16:02:12 | artom | mdbooth, oh, so you're not testing that your code in the latest patchset handles a race properly | |
| 16:02:19 | mdbooth | artom: Yes. | |
| 16:02:29 | mdbooth | Erm... | |