| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-22 | |||
| 19:00:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove source node allocation after live migration completes https://review.openstack.org/496032 | |
| 19:02:44 | mriedem | cfriesen_: were you working on this? https://bugs.launchpad.net/nova/+bug/1712210 | |
| 19:02:45 | openstack | Launchpad bug 1712210 in OpenStack Compute (nova) "Live migration does not restrict to the original cell" [Medium,Triaged] - Assigned to Chris Friesen (cbf123) | |
| 19:02:54 | mriedem | if not i'll fix it quick | |
| 19:05:31 | mriedem | dansmith: something we likely don't want to think about, but if we allocate resources in placement for the dest host during live migration but then something fails and the instance never makes it to the dest node, we aren't cleaning up those allocations anywhere | |
| 19:06:21 | mriedem | the periodic task would have, but that's no longer doing it once everything is upgraded | |
| 19:07:29 | mriedem | i reckon once have the allocations counted against a migration uuid, we could put a periodic in compute that checks for failed migrations where dest_compute == CONF.host and removes any leftover allocations | |
| 19:12:21 | openstackgerrit | Eric Berglund proposed openstack/nova master: WIP: PowerVM Driver: config drive https://review.openstack.org/409404 | |
| 19:12:31 | openstackgerrit | Eric Berglund proposed openstack/nova master: WIP: PowerVM Driver: config drive https://review.openstack.org/409404 | |
| 19:14:54 | cdent | mriedem: migration uuid will fix everything! | |
| 19:15:10 | mriedem | i hope so | |
| 19:15:16 | cdent | _everything_ | |
| 19:16:35 | mriedem | https://bugs.launchpad.net/nova/+bug/1712411 for tracking | |
| 19:16:36 | openstack | Launchpad bug 1712411 in OpenStack Compute (nova) "Allocations may not be removed from dest node during failed migrations" [Undecided,New] | |
| 19:17:15 | mriedem | i'm not sure we'd even need the allocation tracked against the migration uuid | |
| 19:17:55 | mriedem | the migration record has a status (error), dest_compute, and instance_uuid in it, that's basically all we'd need for a periodic task in the compute to look for failed migrations targeted themselves and remove the allocations for the dest node and the instance involved in the migration | |
| 19:18:25 | mriedem | using scheduler report client remove_provider_from_instance_allocation | |
| 19:34:51 | cdent | mriedem: your attention to detail is appreciated | |
| 19:38:34 | dansmith | mriedem: the allocation for the migration_uuid can be cleanup-able by whoever doesn't have the instance after the failure, | |
| 19:38:59 | dansmith | and we could have a nova-manage command (or something) that would look at all the active migrations, see which are stalled/failed, and make sure to clean up the allocations for them or something | |
| 19:39:01 | dansmith | but yeah | |
| 19:39:20 | mriedem | sure it doesn't really matter who does it | |
| 19:43:31 | mriedem | things also get weird when you have the ability to cancel an in-progress migration | |
| 19:45:36 | mriedem | if only we had, like, a periodic task in the compute service that would, like, automatically heal the allocations.... | |
| 19:45:40 | mriedem | o-) | |
| 19:51:38 | cburgess | @dansmit Do I recall correctly something about an issue with flavor migration on upgrade to N or O around the created_at, deleted_at, and updated_at columns? I feel like you mentioned that to me at a summit or PTG or something? | |
| 19:51:45 | cburgess | @dansmith even ^^^^ | |
| 19:52:02 | dansmith | um | |
| 19:52:29 | dansmith | cburgess: which flavor migration are you talking about? the more recent one with moving them from the nova to api database? | |
| 19:52:46 | cburgess | Yeah the code in nova/objects/flavor in Ocata. | |
| 19:52:59 | cburgess | dansmith Though it looks like Newton does the same thing. | |
| 19:53:02 | cburgess | Maybe.. | |
| 19:53:20 | dansmith | maybe that there is no soft delete in the api database? | |
| 19:53:29 | mriedem | there would be no deleted_at column either | |
| 19:53:40 | mriedem | because of what dan said | |
| 19:54:25 | mriedem | cburgess: you see it in newton b/c it was added in newton | |
| 19:54:49 | mriedem | https://github.com/openstack/nova/commit/e05acd2005d22556918a60a7621e463b593c34e6 | |
| 19:55:03 | cburgess | Right ok.. | |
| 19:55:54 | cburgess | So we are getting a mysql exception when we try and do the online migration. Its due to the data time formant. The DB has it stored as ISO formant but the migration is expecting it in mysql datetime it looks like. Does that ring a bell? | |
| 19:55:58 | cburgess | Like we missed a step? | |
| 19:56:45 | cburgess | Feels like we have to be missing something because this is too obvious a bug. | |
| 19:57:00 | mriedem | seems like someone was in here talking about something similar a couple of weeks ago | |
| 19:57:35 | cburgess | Maybe @kbringard who is hitting it on our side? | |
| 19:57:49 | kbringard | http://i.imgur.com/SMI1LKj.gif | |
| 19:59:24 | kbringard | @cburgess what now? | |
| 19:59:29 | cburgess | @kbringard well seems like @mriedem and @dansmith don't recall anything like this so... file a bug and propose your patch. Assuming we are right and it merges it would be worth of a backport to the stable branches. | |
| 20:00:08 | cburgess | I need to stop @ | |
| 20:00:09 | cburgess | Stupid twitter habbit. | |
| 20:01:19 | dansmith | cburgess: kbringard yeah I don't know anything about a date format issue | |
| 20:01:33 | dansmith | kbringard: sorry I missed your ping in -dev earlier .. I was out for a bit | |
| 20:01:52 | kbringard | no worries | |
| 20:02:36 | kbringard | the crux of it looks like, the migrate code gets the data from created_at in nova.instance_types and stores it as a datetime.datetime object with TimeZone (like it's supposed to) | |
| 20:02:40 | kbringard | but it's ISO compliant | |
| 20:02:56 | kbringard | so it looks like so: 2017-08-18 16:45:33+00:00 | |
| 20:02:57 | kbringard | as a string | |
| 20:03:14 | kbringard | or datetime.datetime(2017, 8, 10, 16, 50, 6, tzinfo=<iso8601.Utc> as the object | |
| 20:03:43 | kbringard | then, when it calles flavor_flavor_create the it generates an insert query like so: | |
| 20:03:57 | kbringard | INSERT INTO flavors (created_at, updated_at, id, name, memory_mb, vcpus, root_gb, ephemeral_gb, flavorid, swap, rxtx_factor, vcpu_weight, disabled, is_public) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s)'] [parameters: (datetime.datetime(2017, 8, 10, 16, 50, 6, tzinfo=<iso8601.Utc>), None, 18, 'VCO_cisco_metapod_validation_flavor', 2048, 1, 10, 0, 'db6d7ce0-1d85-49ee-8a73-13f6f64bd46e', 0, 1.0, 0, 0, 1)] | |
| 20:04:11 | kbringard | and fails with | |
| 20:04:11 | kbringard | (1292, "Incorrect datetime value: '2017-08-10 16:50:06+00:00' for column 'created_at' at row 1") | |
| 20:04:26 | kbringard | because ISO compliant datetime != mysql compliant datetime | |
| 20:04:53 | kbringard | simply running it through nova.utils.strtime(), which changes the string to 2017-08-18T16:45:33.000000, fixes it | |
| 20:05:46 | kbringard | so I did a dumb hack like this: http://paste.openstack.org/show/619091/ | |
| 20:05:57 | kbringard | which probably isn't the right way to do it | |
| 20:06:19 | kbringard | but it verifies that if you simply change the object to be mysql datetime compliant the problem goes away and it migrates fine | |
| 20:06:42 | kbringard | so I guess the question I'd have for you, dansmith, since it looks like you did a lot of the base object code, specifically the DateTime objects | |
| 20:06:53 | cburgess | @kbringard Did we look at the earlier schema migrates to see if one of them changes the table schema and we some how misses that one? | |
| 20:06:55 | kbringard | is there some other, earlier, place we should modifying the object | |
| 20:06:56 | mriedem | flavor_values['deleted_at'] = utils.strtime(flavor_values['deleted_at']) is wrong | |
| 20:07:06 | mriedem | there is no deleted_at column in the nova_api.flavors table | |
| 20:07:07 | kbringard | cburgess: yea, I looked at it, they're both datetime | |
| 20:07:22 | cburgess | kbringard k... ok well file a bit and propose the patch I guess... lets see what they say. | |
| 20:07:29 | mriedem | maybe "if 'deleted_at' in flavor_values" is just always False so you never hit that | |
| 20:07:31 | kbringard | mriedem: sure, but it exists in the original nova one | |
| 20:07:40 | mriedem | right, but we don't migrate that column | |
| 20:07:47 | kbringard | so if they're all NULL then it works fine | |
| 20:07:58 | kbringard | if created_at or updated_at isn't NULL then it fails | |
| 20:08:11 | kbringard | and like you said it just drops deleted_at | |
| 20:08:17 | kbringard | I only added it in the patch for sanity | |
| 20:08:26 | kbringard | and I mean, I don't think this is necessarily a good patch | |
| 20:08:28 | dansmith | I'm confused how do you think this is working upstream if the code is wrong for the column data type? | |
| 20:08:37 | kbringard | well, I don't know | |
| 20:08:40 | kbringard | that's what we're trying to figure out | |
| 20:08:44 | dansmith | there have been oslo changes around timeutils, maybe you've got something crossed there? | |
| 20:10:03 | kbringard | yea, I dunno, that's what I'm trying to get some insight into… I don't know this code very well (or really at all) | |
| 20:10:27 | mriedem | besides the deleted and deleted_at columns, is the table schema the same between nova.instance_types and nova_api.flavors? | |
| 20:10:42 | kbringard | https://github.com/openstack/nova/blob/stable/ocata/nova/objects/base.py#L154-L164 | |
| 20:10:45 | kbringard | this is the base object | |
| 20:11:13 | kbringard | https://github.com/openstack/nova/blob/stable/ocata/nova/objects/flavor.py#L733-L758 | |
| 20:11:18 | kbringard | this is the migration object code | |
| 20:11:34 | kbringard | specifically when it does this: https://github.com/openstack/nova/blob/stable/ocata/nova/objects/flavor.py#L744-L745 | |
| 20:11:51 | kbringard | the field: it creates has the TZ as +00:00 | |
| 20:12:13 | kbringard | so when you dump the JSON you can see the string is "wrong" | |
| 20:12:43 | mriedem | yeah the objects aren't tz-aware | |
| 20:12:52 | cburgess | Actually.. | |
| 20:12:54 | kbringard | of course that's the print stuff doing the strings, so you know | |
| 20:12:55 | cburgess | @mriedem @kbringard https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/fields.py#L459-L460 | |
| 20:13:08 | cburgess | Thats in master. | |
| 20:13:11 | kbringard | right, so there's the other thing I was looking at cburgess | |
| 20:13:30 | kbringard | that same code has existed for awhile, iirc | |