| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-29 | |||
| 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 | jaypipes | dansmith: because we can't make them not-nullable until the fields are populated with something. | |
| 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: it gets set to a non-null value right after there.. | |
| 14:50:29 | stephenfin | Couldn't we just move that non-null setting to the top of the function? | |
| 14:50:29 | stephenfin | then we don't need to skip anything | |
| 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 | 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 | dansmith | -2 on that :) | |
| 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" | |
| 15:21:21 | bauzas | probably one adding the conditional and the other doing the dict amendment | |
| 15:21:45 | efried | jaypipes I'm thinking to do a refactor of ProviderTree to hide everything within _Provider as you mentioned a couple weeks ago. Then I'll put the uuid_set() change on top of that. | |
| 15:21:54 | mriedem | already did the git blame, those are the two changes | |
| 15:21:59 | bauzas | oh heh https://github.com/openstack/nova/blame/master/nova/objects/pci_device.py#L249-L251 | |
| 15:22:08 | dansmith | mriedem: looks like just refactoring right? | |
| 15:22:20 | mriedem | yes the latter change dropped some oneline migration code for extra_info | |
| 15:22:22 | efried | jaypipes At a glance, there's nothing pending in the nrp series that touches ProviderTree before uuid_set(), so I was just gonna pell that outta there and start a new series on master tip. | |
| 15:22:24 | dansmith | mriedem: used to be possible to not have that set, but not anymore? | |
| 15:22:24 | mriedem | *online | |
| 15:22:28 | efried | jaypipes That work for you? | |
| 15:22:41 | efried | s/pell/peel/ | |
| 15:22:46 | dansmith | mriedem: yeah | |
| 15:23:20 | bauzas | yeah, when looking at the change, we had some conditionals that we removed | |
| 15:23:32 | bauzas | so probably just a nit | |
| 15:23:44 | bauzas | now we always have that key | |
| 15:27:34 | jaypipes | efried: yep, exactly what I was thinking. | |
| 15:28:04 | efried | jaypipes k, on it | |
| 15:28:44 | mriedem | bauzas: dansmith: so i think we can remove https://github.com/openstack/nova/blob/master/nova/objects/pci_device.py#L249 | |
| 15:28:46 | mriedem | yes? | |
| 15:29:16 | dansmith | mriedem: if True: return "yes" | |
| 15:29:59 | bauzas | mriedem: I think it's harmless indeed | |
| 15:30:10 | bauzas | but you know, I'm not a libvirt expert :D | |
| 15:30:54 | mriedem | what could possibly go wrong with upgrading nova with existing pci device records | |
| 15:34:57 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Save updated libvirt domain XML after swapping volume https://review.openstack.org/523910 | |
| 15:35:09 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/ocata: Save updated libvirt domain XML after swapping volume https://review.openstack.org/523911 | |