| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-29 | |||
| 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 | mriedem | *online | |
| 15:22:24 | dansmith | mriedem: used to be possible to not have that set, but not anymore? | |
| 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 | |
| 15:42:25 | bauzas | jaypipes: just did +2 on that change but I have an open question that is non blocking the merge https://review.openstack.org/#/c/377138/62/nova/objects/resource_provider.py | |
| 15:42:35 | bauzas | https://review.openstack.org/#/c/377138/62/nova/objects/resource_provider.py@698 rather | |
| 15:42:59 | bauzas | it's about deleting the records without touching how we track the updates | |
| 15:43:27 | bauzas | again, totally unrelated, but I feel we should just say all the fields are good for updates ? | |
| 15:43:49 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix ValueError when loading old pci device record https://review.openstack.org/523914 | |
| 15:43:52 | mriedem | stephenfin: dansmith: lyarwood: ^ | |
| 15:43:57 | bauzas | jaypipes: or a .delete() followed by a .create() would only persist the only touched fields | |
| 15:45:27 | stephenfin | mriedem: Out of curiosity, why not just move the 'if db_dev['uuid'] is None:' block to the top? Seems it would fix the issue in the same manner with less LOC? | |
| 15:46:31 | stephenfin | wait - that would cause a read of None from db_dev. nvm me | |
| 15:46:31 | mriedem | stephenfin: because then you call save() which triggers another round trip through _from_db_object before you load up the other fields | |
| 15:46:50 | stephenfin | Also that | |
| 15:51:12 | mriedem | stephenfin: replied, but no | |
| 15:51:21 | mriedem | we can't 2.0 until we have a way to force the records are migrated, | |
| 15:51:29 | mriedem | and we can't really force that w/o having a cli hook to run the online data migraiton, | |
| 15:51:36 | mriedem | which we don't have, which you pointed out when we started talking about this | |
| 15:52:05 | stephenfin | ++ makes sense to me. Thanks for the info | |
| 15:52:05 | mriedem | if you want to talk web2.0 then i'm game | |
| 15:52:15 | Tengu | (14 | |
| 15:52:18 | Tengu | woops. | |
| 15:54:00 | efried | jaypipes I think we're still going to need a way to return the data about a provider from ProviderTree (xen already asked for a name-to-uuid mapping). Should we simply make ProviderTree.find() return a *copy* of the provider (so updating it locally is a no-op)? | |
| 15:54:08 | openstackgerrit | Claudiu Belu proposed openstack/nova master: POC: tests: autospecs all the mock.patch usages https://review.openstack.org/470775 | |
| 15:54:19 | jaypipes | bauzas: not sure I'm following you on your review comment "shouldn't we reset the changes when we call the deletion ? That makes sense to me given after the deletion, all the fields are now all non-persisted (or all of them needing to be persisted, rather)" | |