Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-09-26
16:35:32 mriedem b/c clearly we weren't testing this before which is why it's a bug
16:35:41 imacdonn mriedem: yeah, I figured that'd come up ... but there aren't any tests at all for this stuff currently, or I failed to find them
16:36:28 imacdonn Trying to verify the contents of a PrettyTable didn't seem appealing :/
16:36:39 mriedem https://github.com/openstack/nova/blob/41ac87f812f239e38ad3b72c2f2108523ad3444d/nova/tests/unit/test_nova_manage.py#L749
16:37:08 mriedem there are tests for each individual migration method but you're right that they aren't tested together in aggregate
16:37:09 imacdonn oh. Why isn't that under "cmd" ?
16:37:16 mriedem b/c history
16:37:19 imacdonn mmkay
16:37:28 mriedem no reason we couldn't move it
16:37:35 mriedem except it makes backports a pain in the ass
16:37:37 melwitt mgariepy: oh, hm. ok, we have had online data migrations before that skip deleted records, so that sounds legit but I need to look and double check. it's surprising though that we haven't had this reported before, I'd think it would be common
16:38:10 imacdonn k, I'll look into why the test didn't catch the bug
16:38:40 imacdonn oh, I guess because it didn't have more than 50 of any migration type
16:40:44 melwitt tssurya, mriedem: I've got a call at the top of the hour, so I won't be able to participate much in a cells meeting if we have one
16:43:21 mgariepy melwitt, the cloud i'm upgarding lives from some times. first install was kilo and upgraded
16:43:43 mgariepy but depending on the when it's upgraded, it can cause inconsistency some times..
16:46:07 melwitt mgariepy: yeah, so I'm looking at this and the migration is that it's moving keypairs from the "main" database to the api database. and it's counting only the non-deleted InstanceExtra. InstanceExtra are marked as deleted when instances are deleted through the nova API https://github.com/openstack/nova/blob/stable/queens/nova/objects/keypair.py#L245
16:50:20 melwitt I don't yet understand how this works, I see the migration creates a keypair in the api db, deletes the keypair from the cell db, but I'm not yet seeing the connection with instance_extra
16:51:48 melwitt it looks like the instance_extra part is a separate migration that should have happened before this cell db => api db one that I'm looking at
16:52:43 melwitt ok yeah, instance_obj.migrate_instance_keypairs
16:52:49 melwitt from newton
16:53:21 mgariepy i'm currently using : nova_git_install_branch: d7864fbb9c2c558c409559e1d5989f84c7403832 # HEAD of "stable/queens" as of 23.07.2018
16:53:54 mgariepy melwitt, can you point me to the commit ?
16:54:51 melwitt mgariepy: here's the code https://github.com/openstack/nova/blob/stable/ocata/nova/objects/instance.py#L1398 that should have run when you upgraded to newton, when you ran the nova-manage db online_data_migrations. did you do that for newton?
16:55:53 melwitt the code no longer exists in queens, so if you missed running that online data migration in newton, you can't run it with the queens code
16:56:16 mgariepy i ran it for sure.
16:58:33 melwitt and there were no errors? because while this is only migrating non-deleted records, the check that is blocking you in queens is also only considering non-deleted records, so I don't see how this could be happening unless the online migration from newton was missed or had errors
16:58:53 openstackgerrit Merged openstack/nova master: placement: Always reset conf.CONF when starting the wsgi app https://review.openstack.org/604693
17:00:24 melwitt as for getting out of it, if the unmigrated instance_extra records are associated with non-deleted instances, I think you're going to have to do something like use the old newton code to migrate the records. if they unmigrated instance_extra records are associated with deleted instances, you're going to have to delete them manually
17:02:27 mgariepy the Ocata code i was runnig wasn't the latest
17:03:16 mgariepy anyway seems to be fixed now.
17:04:34 mgariepy is there a script to validate the database ?
17:07:17 mgariepy i run this cloud since kilo
17:08:01 mgariepy maybe some stuff didn't migrate well, it's just not fun to find out the day you upgrade it
17:11:20 melwitt yeah, so there's this tool called the upgrade status check
17:12:21 melwitt https://docs.openstack.org/nova/pike/cli/nova-status.html
17:12:29 cfriesen mriedem: re: weighing based on patching, the main usecase currently is for things that fundamentally require taking the node out of service (kernel and kernel modules, for example). But it's coupled with our proprietary patching orchestrator, so it's not really upstreamable.
17:30:32 mgariepy anyway, thanks for you help melwitt,
17:33:27 melwitt mgariepy: yeah, take a look at the nova-status command, that should help you out during each upgrade to make sure you've done all the steps needed for the upgrade to succeed
17:44:44 mriedem melwitt: i assumed the cells meeting was not happening since tssurya isn't around and dan is out
17:45:04 cfriesen melwitt: for the "image properties and flavor extra-spec validation", can we use the fact that rebuild/resize/create can all already return a 400 error to avoid the need for a new microversion?
17:46:14 cfriesen mriedem: maybe you could answer this too. ^
17:46:24 melwitt yeah, I was thinking mriedem would be better for answering that
17:48:11 mriedem cfriesen: uh, no
17:48:51 mriedem because on cloud A i can do something which i can't on cloud B
17:49:32 cfriesen mriedem: what about the argument that it silently failed before (you have to check the instance state to see it go to ERROR) but now we'll return an error?
17:50:06 mriedem i could maybe buy that, but this is a big enough change that i think it's hard to justify
17:50:43 mriedem tbc, i don't think a spec is warranted just b/c of a microversion, but also about how this is going to work and which specs will be validated and how
17:51:56 cfriesen mriedem: okay, new microversion it is. I'm not going to be the one writing this spec, I'm just establishing the parameters for someone else to write it up.
17:52:07 cfriesen (someone else on our side, I mean)
17:52:54 mriedem i'm also only one person
17:52:59 mriedem others might have other opinions
17:53:50 mriedem like alex_xu and gmann
17:54:27 cfriesen we can write it up assuming a new microversion and call it out in the review. easy enough to remove if not needed
17:55:18 mriedem yeah just use the alternatives sectoin
17:55:19 mriedem *section
18:07:57 openstackgerrit iain MacDonnell proposed openstack/nova master: nova-manage - fix online_data_migrations counts https://review.openstack.org/605329
18:46:56 openstackgerrit Jonte Watford proposed openstack/nova master: Add __str__ method to NUMACell related objects https://review.openstack.org/600269
18:49:24 openstackgerrit Jonte Watford proposed openstack/nova master: Add __str__ method to NUMACell related objects https://review.openstack.org/600269
19:18:28 openstackgerrit Jonte Watford proposed openstack/nova master: Add __str__ method to NUMACell related objects https://review.openstack.org/600269
19:37:51 efried imacdonn: The test will hopefully help me understand it - or at least prove to myself that you've fixed something.
19:38:48 imacdonn efried: I pasted example output of the failure case in the bug report
19:39:29 mriedem i could use a hashable ovo right about now
19:40:30 efried imacdonn: Yeah, I saw that and it helped me understand the problem, but looking at the code I couldn't see a) how the problem was happening, or b) how you fixed it. It's been a few hours now, maybe a second look will do the trick.
19:40:37 mriedem i've got a sorted list of things, but some of those things could be duplicates, and i need to filter out the duplicates, but ovos are'nt hashable as far as i can tell
19:40:50 efried mriedem: set()?
19:41:01 efried oh, you have a sorted list of OVOs.
19:41:05 mriedem right
19:41:08 efried got it
19:41:15 efried are the __eq__ able?
19:41:19 imacdonn efried: heh. the problem was that each invocation of the inner method was returning a dict with counts from that run, and it was being used to update() a dict in the outer functions .. that replaced the values in the outer one, instead of adding to them
19:41:19 mriedem specifically, 2 of the Migration objects in the same list have the same uuid
19:41:34 mriedem but their updated_at values are different and i just need to remove the older one
19:41:41 efried mriedem: So you sorted by UUID?
19:41:48 mriedem not necessarily
19:42:16 mriedem default sort keys are: sort_keys=['created_at', 'id']
19:43:16 mriedem http://paste.openstack.org/show/730960/
19:43:25 efried mriedem: You could use filter() to bastardize set() logic
19:45:19 efried but, yeah, one way or another you're not getting out of this without a loop.
19:45:27 mriedem right i've got a loop
19:45:32 mriedem but it's fugly
19:45:47 efried You could monkey-patch a __hash__ function onto the objects...
19:45:49 mriedem plus, i have to maintain the sort order of the list
19:46:43 efried this is nova.objects.Migration?
19:46:48 mriedem yeah
19:46:52 mriedem ovo actually has a ComparableVersionedObject mixin
19:47:01 mriedem we only use it in one place in nova's objects, the Selection object
19:47:41 efried but you only want to use the UUID to compare the lists?
19:54:04 mriedem i think i might have it
19:55:16 mriedem yup got it
19:55:41 mriedem ffs, you know how sometimes you come across a programming problem which seems like something you dealt with on a daily basis when you were a junior in college?
19:56:00 mriedem and now, getting paid high professional tech person salary, you feel dumb as a brick
19:56:38 efried college is way harder than real life
19:57:41 efried imacdonn: Okay, I'm there. The bug is the .update, which is patently wrong.
19:57:51 efried imacdonn: I convinced myself by solving it a different way: http://paste.openstack.org/show/730962/
19:58:00 efried which isn't better than what you've done, just different.
19:59:04 imacdonn efried: I tried to do it your way, but I got a complaint about tuples ... oh, you changed it to a list
19:59:12 efried yeah
19:59:19 efried no reason for it to be a tuple afaics
19:59:41 efried anyway, I'm approving your patch. Good catch, good fix.

Earlier   Later