| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-29 | |||
| 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 | |
| 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 | 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:31 | stephenfin | wait - that would cause a read of None from db_dev. nvm me | |
| 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 | mriedem | if you want to talk web2.0 then i'm game | |
| 15:52:05 | stephenfin | ++ makes sense to me. Thanks for the info | |
| 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)" | |
| 15:54:42 | efried | jaypipes The alternative is to have ProviderTree.name_for(uuid), .parent_for(name_or_uuid), .inventory_for(name_or_uuid) etc. | |
| 15:55:33 | cdent | I see you baby, rebasing that code, rebasing that code. | |
| 15:55:36 | jaypipes | efried: I'd have to see where the xen folks are trying to do things. | |
| 15:55:42 | mriedem | this isn't quite right either https://github.com/openstack/nova/blob/master/nova/objects/pci_device.py#L247 | |
| 15:56:04 | mriedem | setting self.uuid in save() does'nt get it into the updates dict wihch is passed to db.pci_device_update | |
| 15:56:14 | bauzas | jaypipes: I have to bail out, but I can explain later | |
| 15:56:23 | jaypipes | mriedem: sure it does... | |
| 15:56:24 | mriedem | so save() will call _from_db_object which will do the uuid migration and call save() again | |
| 15:56:28 | mriedem | jaypipes: how? | |
| 15:56:36 | mriedem | oh obj_get_changes | |
| 15:56:37 | mriedem | nvm | |
| 15:56:39 | jaypipes | mriedem: obj_get_changes() will pick up that uuid.. | |
| 15:56:51 | jaypipes | bauzas: k | |
| 15:57:02 | bauzas | jaypipes: basically, I imagine a case where I create the object, save the object in DB, modify a field, deletes the record in DB, and then recreate it in the DB | |
| 15:57:19 | jaypipes | bauzas: I don't imagine that case :) | |