| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-11 | |||
| 22:07:33 | mriedem | the vifs migration was also new in stein | |
| 22:09:45 | melwitt | oh. nevermind me | |
| 22:10:20 | mriedem | imacdonn_: w/o looking at the code i think the migration is creating a marker instance record | |
| 22:10:29 | mriedem | which is why it's using an empty admin context with no project, | |
| 22:10:34 | mriedem | it should be using a sentinel for the project_id probably | |
| 22:10:59 | mriedem | should be fairly easy to reproduce that by just modifying an existing test to run the command twice | |
| 22:10:59 | melwitt | it uses a sentinel of all zeros uuid | |
| 22:11:45 | melwitt | https://github.com/openstack/nova/blob/master/nova/objects/virtual_interface.py#L303 | |
| 22:11:50 | mriedem | melwitt: but the context doesn't have that | |
| 22:11:54 | mriedem | and thta's what the db api is looking for i think | |
| 22:12:33 | melwitt | oh, other direction | |
| 22:12:38 | mriedem | http://git.openstack.org/cgit/openstack/nova/tree/nova/db/sqlalchemy/api.py#n4037 | |
| 22:13:03 | melwitt | I see, ok | |
| 22:13:18 | openstackgerrit | Dustin Cowles proposed openstack/nova master: WIP/PoC: Introduces the openstacksdk to nova https://review.openstack.org/643664 | |
| 22:13:26 | mriedem | imacdonn_: report a bug | |
| 22:13:53 | mriedem | i'm glad we didn't backport that data migration yet...i was worried about just backporting it before anyone was using it (besides ovh) since it's pretty complicated | |
| 22:14:03 | mriedem | maciejjozefczyk: ^ | |
| 22:14:10 | imacdonn_ | OK. Are we still using launchpad? I've been a bit out of the loop | |
| 22:14:20 | mriedem | ceryx: i've got this --dry-run patch coming, just building docs and running tests locally first | |
| 22:14:25 | mriedem | imacdonn_: of course | |
| 22:14:30 | mriedem | only crazy projects move to SB :) | |
| 22:14:35 | imacdonn_ | heh ok | |
| 22:14:37 | melwitt | imacdonn_: yes launchpad for nova. it's placement that has moved to storyboard | |
| 22:16:19 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add --dry-run option to heal_allocations CLI https://review.openstack.org/651932 | |
| 22:16:20 | mriedem | ceryx: eandersson: ^ should be backportable to rocky i think, that code hasn't changed much | |
| 22:16:26 | mriedem | or run it in a container or something | |
| 22:18:52 | imacdonn_ | https://bugs.launchpad.net/nova/+bug/1824435 | |
| 22:18:54 | openstack | Launchpad bug 1824435 in OpenStack Compute (nova) "fill_virtual_interface_list migration fails on second attempt" [Undecided,New] | |
| 22:24:31 | mriedem | imacdonn_: thanks triaged - are you working a fix? | |
| 22:25:13 | imacdonn_ | mriedem, negative .. I don't think I understand the problem well though (yet?) | |
| 22:29:03 | efried | mriedem: I still don't get how the duplicate row is getting created. | |
| 22:29:14 | efried | Shouldn't https://github.com/openstack/nova/blob/03322bb517925a9f5a04ebdb41c3fd31e7962440/nova/db/sqlalchemy/api.py#L4037 only happen the first time? | |
| 22:32:19 | openstackgerrit | Dustin Cowles proposed openstack/nova master: WIP/PoC: Use SDK instead of ironicclient for node.get https://review.openstack.org/642899 | |
| 22:33:06 | mriedem | hmm yeah i'm not sure how https://github.com/openstack/nova/blob/03322bb517925a9f5a04ebdb41c3fd31e7962440/nova/db/sqlalchemy/api.py#L3874 can't either return at least 1 or raise | |
| 22:35:45 | efried | oh, that part is because there's two rows in the database with the "right" project_id (NULL) | |
| 22:36:03 | efried | mriedem: So the initial check (==) fails because there's *more* db rows than expected | |
| 22:36:16 | efried | and then the for loop doesn't hit because all the names match (because they're the same) | |
| 22:36:21 | mriedem | ah yup | |
| 22:36:33 | mriedem | i don't know why the unique constraint doesn't blow up - because NULL isn't considered unique? | |
| 22:36:37 | efried | so the problem is that we're somehow creating two rows, and I don't know how that .... | |
| 22:36:40 | mriedem | 'name0deleted'), | |
| 22:36:40 | mriedem | name='uniq_security_groups0project_id0' | |
| 22:36:40 | mriedem | schema.UniqueConstraint('project_id', 'name', 'deleted', | |
| 22:36:52 | efried | calling all zzzeek? | |
| 22:37:01 | mriedem | imacdonn_: what db are you using? oracle? | |
| 22:37:10 | mriedem | or mysql? | |
| 22:37:27 | imacdonn_ | mysql | |
| 22:38:24 | mriedem | https://stackoverflow.com/questions/3712222/does-mysql-ignore-null-values-on-unique-constraints/16541686 | |
| 22:38:33 | mriedem | i remember something about this when i added db2 support to nova way back when | |
| 22:38:42 | mriedem | db2 was very strict about null values in a constraint but mysql isn't | |
| 22:39:02 | mriedem | our tests don't fail b/c we're using sqlite | |
| 22:39:09 | efried | butbutbut | |
| 22:39:12 | efried | unique constraint or no | |
| 22:39:27 | efried | what, are we hitting that NotFound from multiple threads, reliably, at the same time?? | |
| 22:44:16 | mriedem | i'm able to recreate the same thing in the db in my devstack http://paste.openstack.org/show/749218/ | |
| 22:44:22 | mriedem | but i'm not hitting errors running the online data migration | |
| 22:44:34 | mriedem | i created a server and ran the migrations a few times | |
| 22:45:50 | mriedem | imacdonn_: are you running the CLI concurrently or something? or able to recreate manually? | |
| 22:46:39 | imacdonn_ | mriedem, you mean like two instances of nova-manage at the same time? no.... I can run it manually and it fails consistently | |
| 22:46:58 | mriedem | on a fresh install? | |
| 22:47:54 | imacdonn_ | One of these was a fresh install, but I did create an instance at some point | |
| 22:51:36 | mriedem | ok i'm not sure how to recreate it then | |
| 22:52:12 | mriedem | ceryx: eandersson: it's getting late here for the work day but please follow up with me on whatever you figure out with this allocations thing | |
| 22:52:45 | melwitt | dansmith: would another option other than trying to lock for discover_hosts be to try-except around the host_mapping.create() and catch and ignore DBDuplicateEntry? | |
| 22:54:01 | melwitt | https://github.com/openstack/nova/blob/master/nova/objects/host_mapping.py#L193 | |
| 22:54:33 | mriedem | melwitt: here as well https://github.com/openstack/nova/blob/master/nova/objects/host_mapping.py#L211 | |
| 22:54:42 | mriedem | that seems reasonable though | |
| 22:54:49 | melwitt | yes, two places | |
| 22:55:37 | ceryx | mriedem: Thanks, will do. eandersson is building a new container now with that patch to test. | |
| 22:55:57 | dansmith | melwitt: obviously that will avoid the trace, yeah. I'd still abort and say "looks like you're doing naughty things, so I'm stopping" | |
| 22:56:20 | mriedem | ceryx: unfortunately there isn't a way to just run heal_allocations on a specific instance yet, but maybe you won't get too much output | |
| 22:56:27 | melwitt | dansmith: hm, ok | |
| 22:56:48 | mriedem | the discover hosts periodic is in the scheduler process right? | |
| 22:57:07 | dansmith | mriedem: yup | |
| 22:57:07 | mriedem | so you could have multiple schedulers running discover_hosts at the same time | |
| 22:57:19 | dansmith | yup | |
| 22:57:29 | mriedem | so...probably not a terrible idea to just handle the duplicate and move on | |
| 22:57:34 | melwitt | I guess I'm not sure why it's so bad because the database will synchronize everything anyway. if there's a collision, just ignore it | |
| 22:57:42 | dansmith | that periodic was really just for the ironic case where you'd have a small control plane | |
| 22:58:03 | mriedem | sure, but people are going to use knobs if we give them | |
| 22:58:25 | mriedem | i'm not saying that's what osa / chef are hitting here, but it's another possibility | |
| 22:58:29 | eandersson | Unfortunately a lot of changes between master and rocky in that script | |
| 22:58:46 | mriedem | eandersson: oh - probably the report client refactoring stuff... | |
| 22:58:53 | dansmith | melwitt: it's not so bad, it just seems like a bad idea to act like that's okay or expected.. we're looking for un-mapped records and adding host mappings, then marking those service records as mapped | |
| 22:59:05 | mriedem | eandersson: if we had a bug or something we could think about backporting that change upstream | |
| 22:59:38 | mriedem | heh ibm loves the email | |
| 22:59:38 | dansmith | melwitt: if you make sure none of that gets skipped (like one gets set mapped without a mapping being created, etc) then it's okay I guess | |
| 23:00:01 | efried | pretty sure they're bouncing 'em, cause I got a snail mail letter from another thing where I hadn't changed it. | |
| 23:00:24 | dansmith | melwitt: and the scheduler periodic is a case where we're kinda inviting you to run multiple in parallel, so there's that | |
| 23:00:34 | melwitt | dansmith: yeah... I could see that too. at the same time, I could see two discover_hosts happening to overlap. and yeah would have to be done with care | |
| 23:01:08 | dansmith | melwitt: but if we just skip that one and keep scanning, then you could have multiples of those just hammering your database when you bring a bunch of nodes online, all but one of them losing on every attempt | |
| 23:01:22 | melwitt | I was just thinking about the lock thing and wondered about ignoring dupes | |
| 23:01:31 | dansmith | if they all backoff and stop, then the one that won will proceed | |
| 23:01:37 | dansmith | the lock thing is just a hack to handle the sloppy ansible case | |
| 23:01:42 | dansmith | or puppet or whatever | |
| 23:01:44 | melwitt | yeah | |
| 23:02:08 | dansmith | since the have the same case for other things in nova-manage, | |
| 23:02:31 | dansmith | like db_sync, online_data_migrations, etc, it seems like we should just prescribe that those are to be run singly | |
| 23:02:44 | dansmith | and if not, we just need to fix it all, otherwise we're inviting confusion | |
| 23:02:46 | melwitt | that's true | |