| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-22 | |||
| 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 | |
| 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-1.21.0-1.el7ost.noarch | |
| 20:15:03 | kbringard | python-oslo-versionedobjects-lang-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-lang-3.22.0-1.el7ost.noarch | |
| 20:16:52 | kbringard | python-oslo-utils-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 | |
| 20:29:15 | mriedem | which is replaced by Placement's check on MEMORY_MB | |
| 20:29:30 | mriedem | so the thing that would be different would be filtering, via placement, on VCPU and DISK_GB | |
| 20:29:37 | cfriesen_ | mriedem: I think that should be pulled out since it was probably legacy code from before we ran the scheduler filters | |
| 20:29:49 | mriedem | ? | |
| 20:30:02 | cfriesen_ | having conductor do the memory check, I mean | |
| 20:30:09 | mriedem | pretty sure that was added in 2.30 | |
| 20:30:25 | cfriesen_ | guess I didn't review that patch | |
| 20:30:30 | mriedem | because that happens in the force host flow | |
| 20:30:36 | mriedem | which doesn't call select_destinations | |
| 20:30:50 | cfriesen_ | ah, sure. in the force flow it makes sense | |
| 20:31:27 | mriedem | actually looks like you're right | |
| 20:31:32 | mriedem | https://review.openstack.org/#/c/29077/ | |
| 20:31:35 | mriedem | that's way old | |
| 20:31:55 | cfriesen_ | I was just thinking that in the force case you could skip the placement checks and filters in the scheduler and just jump right to the resource claim. | |