Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-22
14:13:20 mriedem sdague is the person to ask but he's out for the year, so maybe mtreinish
14:13:27 leakypipes finucannot: we've done that with the ResourceClassField enum.
14:13:56 bauwser finucannot: I guess the problem is more about removing the legacy option
14:13:59 leakypipes finucannot: and I don't believe we've incremented the version for objects that use it...
14:14:07 bauwser say we could just deprecate that option, that's it
14:14:11 leakypipes finucannot: then again, we don't use objects over RPC for placement..
14:14:46 bauwser leakypipes: well, the ResourceClassFieldType is specific because indeed we don't pass it thru RPC
14:15:27 bauwser we just use o.vo to have a coercing
14:17:44 bauwser finucannot: leakypipes: FWIW, I just +W'd that change because I think it's not a problem
14:21:00 cdent leakypipes: have you seen this and the associated bug: https://review.openstack.org/#/c/529397/
14:21:12 cdent there was a lot of associated irc discussion about other options, but that was the simplest
14:21:28 cdent wanted to be sure you saw it before I leave to do some dreaded shopping
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

Earlier   Later