| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-20 | |||
| 17:42:11 | mriedem | awesome | |
| 17:43:17 | edleafe | cdent: if the allocation had an id, then it already exists, no? | |
| 17:44:07 | cdent | no, it's being assigned the .id, but then db deadlock happens and the transaction is reverted and started again, but the list of allocs now contains that alloc with an id | |
| 17:44:25 | mdbooth | artom: Do I recall you being involved in device tagging? | |
| 17:44:56 | edleafe | cdent: ah, so it should change the object, but it's missing the object revert? | |
| 17:46:17 | cdent | edleafe: that's one way to look at it. The other way is that we should be side effecty on on method parameters that are list or dict when we have an option to re-enter. So I'm trying to figure out how to change that in the best way | |
| 17:46:29 | cdent | dammit, should _not_ be side effecty | |
| 17:47:15 | mriedem | cdent: right if you copy the allocs list, then retry, you'll likely then hit a duplicate entry error | |
| 17:47:23 | edleafe | ok, I see what you're saying. It's modifying a mutable param | |
| 17:48:32 | cdent | i need to poke at what the db retry handler is actually doing to be sure | |
| 17:50:32 | mriedem | maybe we need to move the insert portion to another method and that method has the retry on deadlock handler | |
| 17:50:47 | mriedem | apart from the _set_allocations code that's checking the object id | |
| 17:51:03 | mriedem | but, | |
| 17:51:19 | mriedem | we could still then hit duplicate entry errors i'd think | |
| 17:51:26 | mriedem | any time you're retrying to insert over a list... | |
| 17:52:18 | mriedem | so loop over the objects, then insert separately per object in another method with the deadlock retry on that method? would that work? | |
| 17:54:21 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Use instance.project_id when creating request specs for old instances https://review.openstack.org/529384 | |
| 17:54:22 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Workaround missing RequestSpec.project_id when moving an instance https://review.openstack.org/529385 | |
| 17:55:11 | cdent | mriedem: unclear. still trying to get all the pieces to line up. one thing that's breaking my brain is that the retry wrapper is not explicitly saying anything about the current transaction. will keep looking | |
| 17:56:58 | cdent | mriedem: i think you're right though, extracting a method for the individual insert seems right | |
| 17:57:49 | mriedem | seems easiest anyway | |
| 18:03:45 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/ocata: Use instance.project_id when creating request specs for old instances https://review.openstack.org/529387 | |
| 18:08:21 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/newton: Use instance.project_id when creating request specs for old instances https://review.openstack.org/529389 | |
| 18:08:48 | cdent | mriedem: hmmm, jay's comment from when he added the retry is potentially derailing, he's got a diferent theory of what could be causing the issues: https://github.com/openstack/nova/commit/e001b0538a9d413f864 | |
| 18:10:06 | mriedem | that likely makes more sense, the inventory thing | |
| 18:10:43 | mriedem | but in that case, the deadlock isn't on the insert, it's on the generation update yeah? https://github.com/openstack/nova/commit/e001b0538a9d413f864#diff-a040eef16b56cc7862fb2a334c757deaR1855 | |
| 18:12:02 | cdent | right, which I'm not sure we have any way of knowing | |
| 18:12:59 | cdent | the easiest fix is just simply not doing alloc.id = lastrowid | |
| 18:13:05 | mriedem | http://logs.openstack.org/84/529184/2/check/legacy-tempest-dsvm-py35/888d647/logs/screen-placement-api.txt.gz#_Dec_20_15_15_19_616413 | |
| 18:13:13 | mriedem | Dec 20 15:15:19.616413 ubuntu-xenial-citycloud-lon1-0001533915 devstack@placement-api.service[15195]: DEBUG oslo_db.api [None req-d117a878-c8e0-4c3b-a073-848d73575036 service placement] Performing DB retry for function nova.objects.resource_provider.AllocationList._set_allocations {{(pid=15198) wrapper /usr/local/lib/python3.5/dist-packages/oslo_db/api.py:149}} | |
| 18:13:20 | mriedem | at least it gets logged :) | |
| 18:13:49 | cdent | oh, hmmm | |
| 18:13:59 | cdent | that's saying it is on the insert | |
| 18:14:17 | mriedem | where does it say that? | |
| 18:14:23 | cdent | previous line? | |
| 18:14:30 | mdbooth | artom: Hey, I just added a patch which makes it meaningful to tag a local disk | |
| 18:14:43 | mdbooth | As local disks can now have serial numbers | |
| 18:15:11 | artom | mdbooth, https://review.openstack.org/#/c/529380/? | |
| 18:15:18 | mdbooth | The way disk tagging works, though, is that when deleting disk metadata it 'matches' based on device.serial, which it assumes to be volume_id | |
| 18:15:22 | mdbooth | Although.......... | |
| 18:15:32 | mdbooth | Wait a sec, I might be trying to solve a problem which doesn't exist | |
| 18:15:40 | mdbooth | This is only relevant in detach_volume | |
| 18:15:46 | mdbooth | And as you can't detach a local disk.... | |
| 18:15:49 | mdbooth | There's no problem here | |
| 18:16:26 | mdbooth | artom: That's the one, yeah. | |
| 18:16:36 | mdbooth | artom: Ok, stand down. I don't think this is a thing. | |
| 18:16:43 | mdbooth | However, if you want to review that series ;) | |
| 18:16:53 | artom | Also, if I read your patch correctly, it still uses volume UUID as the serial for volumes | |
| 18:16:56 | artom | So nothing changes there | |
| 18:17:05 | mdbooth | artom: Right, that's correct. | |
| 18:17:12 | mdbooth | So this will continue to work for detach_volume. | |
| 18:17:12 | artom | Then we should be fine | |
| 18:17:23 | artom | Let me check whether I included this in the tempest tests | |
| 18:17:23 | mdbooth | But it wouldn't work for detach_ephemeral | |
| 18:17:29 | mdbooth | ...which doesn't exist. | |
| 18:17:30 | artom | ... | |
| 18:17:34 | artom | which haven't merged yet | |
| 18:17:40 | mdbooth | orly? | |
| 18:17:47 | mdbooth | I need to write tempest tests for my stuff. | |
| 18:17:56 | mdbooth | Can you ping me the reviews? | |
| 18:17:58 | artom | mdbooth, they'll merge in time for brexit | |
| 18:18:20 | artom | mdbooth, https://review.openstack.org/#/c/391947/ | |
| 18:18:44 | mdbooth | artom: You mean they're a complete trainwreck with inexplicable momentum marching inevitably towards their doom? | |
| 18:18:48 | artom | Yeah, they only test the attach case | |
| 18:18:51 | artom | I should add the detach case | |
| 18:19:03 | artom | mdbooth, isn't that all of openstack? ;) | |
| 18:19:19 | mdbooth | Ooh, harsh! :) | |
| 18:19:32 | artom | I'm an arse because I care? | |
| 18:21:14 | mdbooth | artom: Ok, looks like that needs a rebase. | |
| 18:21:47 | mdbooth | I'll take a look at that tomorrow, as I also need to write a test which boots with a tagged root/ephemeral/swap disk. | |
| 18:23:58 | openstackgerrit | rahul bardia proposed openstack/python-novaclient master: nova limits ERROR (Exception): Field names must be unique https://review.openstack.org/527922 | |
| 18:29:13 | artom | mdbooth, sure, thanks :) | |
| 18:29:22 | artom | I'll try to get to yours as well | |
| 18:29:24 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add nova-status check for ironic flavor migration https://review.openstack.org/527541 | |
| 18:45:34 | rybridges | Hey guys I have a quick question. Is there a way to list all instances for a particular user across all projects? | |
| 18:46:24 | rybridges | Is that what openstack server list --user <username> does? | |
| 18:49:21 | openstackgerrit | Chris Dent proposed openstack/nova master: Do not set allocation.id in AllocationList.create_all() https://review.openstack.org/529397 | |
| 18:49:41 | cdent | mriedem: there's ^ a first stab, I went for the simplest thing, for reasons listed in the commit message, gonna dine now. | |
| 18:52:17 | melwitt | rybridges: it appears so, it might require user uuid. and you might also need to pass --all-projects | |
| 18:53:21 | rybridges | ok | |
| 18:53:27 | rybridges | Thanks i will give it a shot! | |
| 18:55:38 | melwitt | let me know if it doesn't work. not that familiar with openstackclient but a glance at the code looks like it should do what you want | |
| 18:56:59 | rybridges | looks like this works: openstack server list --user rybridges --all-projects | |
| 18:57:09 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Pass RequestSpec to ConductorTaskAPI.build_instances https://review.openstack.org/515495 | |
| 18:57:10 | rybridges | but this does not: openstack server list --user rybridges | |
| 18:57:16 | rybridges | so ya you need --all-projects | |
| 18:58:27 | melwitt | rybridges: okay, thanks for confirming. without --all-projects it will default to filtering that user under the project making the request | |
| 18:58:47 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Pass RequestSpec to ConductorTaskAPI.build_instances https://review.openstack.org/515495 | |
| 18:59:30 | rybridges | that is correct melwitt | |
| 19:04:48 | edleafe | cdent: huh, that was way simpler than I thought it would be. | |
| 19:25:01 | cfriesen | maybe odd python style question, but why does nova code use super(ClassName, self).func() rather than super(type(self), self).func()? | |
| 19:25:43 | melwitt | I dunno. is the latter a more common pattern elsewhere? | |
| 19:26:58 | cfriesen | melwitt: well, in python 3 you can just use super(). the nice thing about the second one is that you don't need to update it if you subclass it and copy/paste a snippet of code. | |
| 19:29:03 | cfriesen | melwitt: heh...some additional googling answered my question. turns out in python2 it needs to be explicit to allow for multiple levels of inheritance. | |
| 19:29:39 | melwitt | ah, okay. now I know | |
| 19:39:22 | cdent | edleafe: I could have made it way more complex, but figured, what's the point? | |
| 19:45:59 | openstackgerrit | Chris Dent proposed openstack/nova master: Do not set allocation.id in AllocationList.create_all() https://review.openstack.org/529397 | |
| 19:46:33 | edleafe | cdent: I'm not disagreeing | |
| 19:47:56 | cdent | I know | |
| 19:50:56 | mriedem | edleafe: i think we're pretty close on https://review.openstack.org/#/c/511358/ but i don't know why you had to modify the FilterScheduler in there | |