| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-29 | |||
| 14:13:16 | lucasagomes | mriedem, oh, I looked at the wrong column ahah oh right | |
| 14:13:20 | lucasagomes | yeah i think that would be more flexible | |
| 14:13:24 | lucasagomes | if it works | |
| 14:15:47 | lucasagomes | replied to your comment basically agreeing with the task_state + vm_state idea :-) | |
| 14:25:01 | stephenfin | jaypipes: I've a question on https://github.com/openstack/nova/commit/15ac5b688bf6d91ac42ca33860d187d80289d82d | |
| 14:25:20 | jaypipes | stephenfin: yessir? | |
| 14:25:58 | stephenfin | jaypipes: That contains an online data migration. Would I need to run the `nova-manage db online_data_migrations` command for that to take place, or should it happen any time? | |
| 14:26:43 | stephenfin | I'd thought the UUID field would be populated any time I loaded an object from the DB, but then I saw that command | |
| 14:32:21 | jaypipes | stephenfin: there's no nova-manage db stuff in that patch. | |
| 14:32:47 | jaypipes | stephenfin: all data migrations are done upon load/save of the PciDevice object | |
| 14:34:21 | jaypipes | stephenfin: one might make an argument that there *should* have been a corresponding patch that added a nova-manage online data migration (that read in PciDevice objects in batches) | |
| 14:39:52 | mriedem | that's what we did when we added the uuid column to compute_nodes, services and migrations tables | |
| 14:40:05 | mriedem | you could still add one... | |
| 14:42:07 | jaypipes | mriedem: ack | |
| 14:42:34 | jaypipes | mriedem: would need to be done before the PCI device manager is integrated with placement. | |
| 14:42:53 | stephenfin | jaypipes, mriedem: I can do that today | |
| 14:42:57 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: libvirt: Provide VGPU inventory for a single GPU type https://review.openstack.org/514763 | |
| 14:43:12 | stephenfin | jaypipes: In the interim, I think there's a bug with that patch. See here | |
| 14:43:13 | stephenfin | https://bugzilla.redhat.com/show_bug.cgi?id=1516952 | |
| 14:46:54 | mriedem | if key not in ('extra_info', 'uuid'): | |
| 14:46:54 | mriedem | there should be a skip in there | |
| 14:46:54 | mriedem | https://review.openstack.org/#/c/469147/2/nova/objects/pci_device.py@194 | |
| 14:46:54 | mriedem | _from_db_object isn't handling the uuid column properly | |
| 14:46:54 | mriedem | i see the problem | |
| 14:46:54 | openstack | bugzilla.redhat.com bug 1516952 in openstack-nova "Cannot boot vm with sriov port after upgrade OSP11 to OSP12" [Urgent,New] - Assigned to eglynn | |
| 14:48:42 | mriedem | stephenfin: i updated https://bugzilla.redhat.com/show_bug.cgi?id=1516952 - open an lp bug and i'll push a fix | |
| 14:50:29 | dansmith | -2 on that :) | |
| 14:50:29 | dansmith | okay I'm not sure what you're saying, you want to make it null so that a generic loop can set it to none for a few cycles? | |
| 14:50:29 | jaypipes | dansmith: and then when the version of the object goes to the next major version, we change nullable to False and remove the data migration. | |
| 14:50:29 | stephenfin | then we don't need to skip anything | |
| 14:50:29 | stephenfin | Couldn't we just move that non-null setting to the top of the function? | |
| 14:50:29 | jaypipes | dansmith: it gets set to a non-null value right after there.. | |
| 14:50:29 | dansmith | jaypipes: so leave them unset? a nullable field means it can be set to None, and that's extremely wrong for an id, IMHO | |
| 14:50:29 | jaypipes | dansmith: because we can't make them not-nullable until the fields are populated with something. | |
| 14:51:11 | mriedem | fwiw we've only ever bumped the major version on an object before, it doesn't happen really | |
| 14:51:19 | mriedem | Instance is the only one | |
| 14:51:29 | jaypipes | yes, I've complained about that before... | |
| 14:51:43 | dansmith | jaypipes: so do the work to bump.. it's not very fun :) | |
| 14:52:33 | jaypipes | well, sounds like I'm in the minority here, stephenfin, so just add a conditional in that setattr loop to check for whether db_dev['uuid'] is None and if so, don't set obj.uuid to None. | |
| 14:52:55 | mriedem | i've got a patch local | |
| 14:53:03 | mriedem | just need an lp bug | |
| 14:53:07 | jaypipes | dansmith: do we have a good guide to doing that work? | |
| 14:54:06 | openstack | bugzilla.redhat.com bug 1516952 in openstack-nova "Cannot boot vm with sriov port after upgrade OSP11 to OSP12" [Urgent,New] - Assigned to eglynn | |
| 14:54:19 | stephenfin | mriedem: https://bugs.launchpad.net/nova/+bug/1735188 | |
| 14:54:22 | mriedem | https://bugs.launchpad.net/nova/+bug/1735189 | |
| 14:54:24 | mriedem | you beat me | |
| 14:54:29 | stephenfin | Ha! | |
| 14:54:50 | stephenfin | so close | |
| 14:55:14 | dansmith | jaypipes: nope, it also takes work to put that together and nobody else has ever asked to do it. Happy to help with that, but like many other things, I'd value this well below a lot of the other things we've got going on at the moment | |
| 14:55:32 | openstack | Launchpad bug 1735188 in OpenStack Compute (nova) "PciDevice UUID field not populated" [Undecided,New] | |
| 14:55:33 | openstack | Launchpad bug 1735189 in OpenStack Compute (nova) "ValueError: Field `uuid' cannot be None in PciDevice._from_db_object" [Undecided,New] | |
| 14:55:42 | jaypipes | dansmith: ack. maybe a weekend project for me around the holidays. | |
| 14:55:54 | dansmith | jaypipes: you are a sick, sick man | |
| 14:56:03 | jaypipes | heh | |
| 14:59:16 | mdbooth | Oh, dear. I poked something apparently innocuous, and it broke archive_deleted_rows() | |
| 14:59:30 | mdbooth | I don't even know where I am any more | |
| 15:01:39 | mdbooth | Was I supposed to update the shadow table explicitly when I added a column? | |
| 15:02:27 | mdbooth | Ah, ha, it seems yes | |
| 15:02:31 | jaypipes | mdbooth: yep | |
| 15:06:48 | bauzas | jaypipes: reviewing https://review.openstack.org/#/c/377138/62/nova/db/sqlalchemy/api_models.py | |
| 15:07:07 | bauzas | jaypipes: SQLA does ON DELETE CASCADE when you provide a FK ? | |
| 15:07:42 | mriedem | we don't do cascading deletes | |
| 15:09:33 | mriedem | https://github.com/zzzeek/sqlalchemy/blob/rel_1_1_15/lib/sqlalchemy/sql/schema.py#L1514 | |
| 15:09:37 | mriedem | ondelete=None | |
| 15:09:43 | mriedem | https://github.com/zzzeek/sqlalchemy/blob/rel_1_1_15/lib/sqlalchemy/sql/schema.py#L1544 | |
| 15:09:51 | jaypipes | bauzas: no | |
| 15:10:10 | bauzas | jaypipes: so say you delete the root RP | |
| 15:10:23 | bauzas | jaypipes: should we delete the children too ? | |
| 15:10:28 | mriedem | which is part of the reason archive_deleted_rows used to always barf when we'd order the deletes incorrectly | |
| 15:10:29 | bauzas | AFAIK, yes, right? | |
| 15:10:31 | jaypipes | bauzas: we do. | |
| 15:10:47 | bauzas | jaypipes: well, looking at https://review.openstack.org/#/c/377138/62/nova/objects/resource_provider.py@823 and below | |
| 15:11:29 | bauzas | jaypipes: I don't see where we're deleting the children | |
| 15:11:49 | jaypipes | bauzas: we're not. we're *preventing* parents from being deleted if children exist. | |
| 15:12:10 | bauzas | jaypipes: that I saw | |
| 15:12:14 | bauzas | it's the first conditional | |
| 15:12:15 | jaypipes | bauzas: we require that callers delete children before parents. | |
| 15:12:31 | bauzas | wait | |
| 15:12:37 | mriedem | this is a head scratcher https://github.com/openstack/nova/blob/master/nova/objects/pci_device.py#L249-L251 | |
| 15:12:38 | mriedem | if True | |
| 15:13:07 | jaypipes | bauzas: it's the second conditional, too... an IntegrityError will be raised if the DELETE on the parent ends up violating the foreign key constraint on parent_provider_id | |
| 15:13:37 | bauzas | jaypipes: oh right, the first conditional says "if children existing, then return no" | |
| 15:14:37 | bauzas | man, when I looked at the comment, I readed too fast and I saw "do a quick check to see if the provider *has* a parent" in my mind | |
| 15:14:45 | bauzas | grmblbl has != id | |
| 15:14:47 | bauzas | is | |
| 15:18:19 | bauzas | jaypipes: that said, a question, I guess we don't accept to delete a root RP if it has children because... it could be a problem for allocations ? | |
| 15:18:41 | bauzas | or, because it's racy ? | |
| 15:18:49 | bauzas | I mean, why aren't we supporting that ? | |
| 15:18:52 | mriedem | bauzas: dansmith: vladikr: i'm looking at https://review.openstack.org/#/c/249015/ and https://review.openstack.org/#/c/373519/ and trying to figure out why we have https://github.com/openstack/nova/blob/master/nova/objects/pci_device.py#L249-L251 which is always True at this point | |
| 15:19:03 | mriedem | so we are always saving off extra_info regardless of it changing | |
| 15:19:10 | jaypipes | bauzas: no, it just makes our lives easier/safer to say callers need to handle removing children before parents. | |
| 15:19:21 | bauzas | ok | |
| 15:19:30 | bauzas | jaypipes: fair enoguh | |
| 15:20:06 | bauzas | we could have imagined a way to walk on the tree and delete all the allocations/inventories before deleting straight the root RP and cascading the delete, but fine by me | |
| 15:20:25 | bauzas | it's just that's it would mean a synchronous section | |
| 15:20:42 | bauzas | which could be big for a 3-depth RP tree | |
| 15:20:51 | bauzas | so I'm fine with the direction | |
| 15:20:54 | bauzas | mriedem: looking | |
| 15:21:01 | dansmith | mriedem: hmm | |
| 15:21:08 | bauzas | mriedem: I'd call it "tech debt" | |