| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-22 | |||
| 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 | (1292, "Incorrect datetime value: '2017-08-10 16:50:06+00:00' for column 'created_at' at row 1") | |
| 20:04:11 | kbringard | and fails with | |
| 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 | |
| 20:13:32 | cburgess | So I wonder if the issue here is the version of oslo_versionedobjects.. along the lines of what dansmith said. | |
| 20:14:02 | mriedem | that's been around forever https://github.com/openstack/oslo.versionedobjects/commit/b6e71f4524c0fece34e2f7e2a8d3176538538912 | |
| 20:14:06 | kbringard | ^^ | |
| 20:14:08 | cburgess | kbringard What version of versionedobjects do we have? | |
| 20:14:20 | kbringard | when we're running this it's stable/ocata | |
| 20:14:50 | cburgess | We are checking that out from git and building it in a venv or using something RedHat provided? | |
| 20:15:03 | kbringard | python-oslo-versionedobjects-lang-1.21.0-1.el7ost.noarch | |
| 20:15:03 | kbringard | python-oslo-versionedobjects-1.21.0-1.el7ost.noarch | |
| 20:15:14 | kbringard | vanilla Rh package | |
| 20:15:24 | dansmith | it's likely a timeutils thing not an o.vo thing right? | |
| 20:15:53 | cburgess | Good point | |
| 20:15:58 | cburgess | We use oslo.timeutiles to do the conversion | |
| 20:16:21 | cburgess | kbringard so what version of oslo_utils do we have? | |
| 20:16:52 | kbringard | python-oslo-utils-3.22.0-1.el7ost.noarch | |
| 20:16:52 | kbringard | python-oslo-utils-lang-3.22.0-1.el7ost.noarch | |
| 20:17:49 | kbringard | my initial thought was if we maybe don't have enough test coverage? | |
| 20:17:58 | cburgess | kbringard netwon was constrained to 3.16.1 | |
| 20:18:01 | kbringard | like, the default flavors we used to create had NULL for all those guys | |
| 20:18:07 | kbringard | and that all works | |
| 20:18:42 | kbringard | so I'm wondering if this has just been a bug that we'd not caught before, and if no one was doing these migrations (or said anything when they did) then maybe we just never discovered it before? | |
| 20:18:51 | cfriesen_ | mriedem: did you want me to do a fix for the live migration cell restriction on top of your fix at https://review.openstack.org/#/c/49603 ? | |
| 20:19:02 | cfriesen_ | mriedem: or were you going to propose a patch for that? | |
| 20:22:32 | mriedem | cfriesen_: wrong link? i've got a patch though | |
| 20:22:36 | mriedem | just finishing unit tests | |
| 20:22:57 | cfriesen_ | mriedem: gah, I meant https://review.openstack.org/#/c/496031 | |
| 20:23:11 | cfriesen_ | anyways, cool | |
| 20:23:23 | mriedem | and yes it's on top of that series | |
| 20:23:27 | mriedem | to avoid merge conflicts | |
| 20:27:29 | cfriesen_ | mriedem: looking at that commit, in the future if we pass "skip-filters" to the scheduler wouldn't we also want to skip the initial placement checks? (since they're essentially what used to be filters) Presumably the "claim resources on destination" step would fail though. | |
| 20:28:58 | mriedem | cfriesen_: tbd | |
| 20:29:09 | mriedem | cfriesen_: conductor already essentially does the RamFilter | |