Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-11-29
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)"
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 :)
15:57:27 bauzas when I recreate in DB, the updates would only be that field
15:57:35 bauzas so we would only persist that field
15:57:59 bauzas again, it's unrelated to your change, so not a big deal for now
15:58:04 openstackgerrit Eric Berglund proposed openstack/nova master: WIP: PowerVM Driver: SEA https://review.openstack.org/523216
16:00:20 mriedem onfig like changes isolated_images), so it's just an efficiency thing
16:00:20 mriedem dansmith: on that cve errata, i'm thinking we maybe don't need to hold things up for https://review.openstack.org/#/c/521391/ which was the lesser regression; w/o that fix, we'll always run through the scheduler for rebuild of a volume-backed instance even if the image isn't changing, but with your change for RUN_ON_REBUILD, we shouldn't actually fail in the scheduler if the image is the same (unless the admin does something
16:00:31 mriedem still needs to be fixed, but i'm not sure that tristanC should hold up on the errata for it
16:01:07 dansmith mriedem: makes sense
16:01:57 mriedem since we're going to release your RUN_ON_REBUILD fix for newton before eol, we should still probably try to get that fixed and backported as well, since the regression was backported to newton also
16:04:06 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove vestigial extra_info update in PciDevice.save() https://review.openstack.org/523919
16:04:08 dansmith mriedem: why aren't you using is_volume_backed from compute utils?
16:04:21 mriedem dansmith: because we already have the stuff in scope for doing the same thing
16:04:26 mriedem the root_bdm
16:05:02 dansmith mriedem: yeah, just seems wrong to not use the util in case there's another detail we add later
16:05:08 dansmith but whatever
16:07:06 openstackgerrit Ed Leafe proposed openstack/nova master: placement: update client to set parent provider https://review.openstack.org/385693
16:07:48 openstackgerrit Ed Leafe proposed openstack/nova master: placement: adds REST API for nested providers https://review.openstack.org/384807
16:08:16 edleafe jaypipes: efried: ^^ Just fixed commit messages
16:08:29 jaypipes efried: ^^
16:08:36 jaypipes oops, sorry
16:08:38 efried ack
16:08:45 efried jaypipes ack
16:15:11 mriedem oomichi_afk: i've got some issues with the functional tests in https://review.openstack.org/#/c/408964/ which can be cleaned up separately, but +2 on that one if you want to review it again
16:20:32 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Add cache headers to placement api requests https://review.openstack.org/521640
16:20:32 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Object changes to support last-modified headers https://review.openstack.org/521639
16:20:38 cdent ^ rebase wars
16:24:47 openstackgerrit Matt Riedemann proposed openstack/nova master: Enable cold migration with target host(2/2) https://review.openstack.org/408964
16:53:02 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Enable limiting GET /allocation_candidates https://review.openstack.org/513526
16:56:52 cdent sigh, at microversin wars with myself
17:01:24 openstackgerrit Eric Fried proposed openstack/nova master: Make _Provider really private https://review.openstack.org/523932
17:01:29 efried jaypipes ^
17:01:52 efried jaypipes I didn't try to anticipate getters of _Provider attributes; we can add those as demand arises.
17:21:29 openstackgerrit Eric Fried proposed openstack/nova master: ProviderTree.uuid_set() https://review.openstack.org/520243
17:21:29 openstackgerrit Eric Fried proposed openstack/nova master: Make _Provider really private https://review.openstack.org/523932
17:21:55 efried jaypipes And there's uuid_set peeled out and modified as you suggested ^
17:22:02 efried (plus a silly UT fix on the base)
17:22:18 artom_ mriedem, is the topic set right for this series? https://review.openstack.org/#/c/521200/
17:22:46 artom_ I feel like bug/1732947 is the most obvious one
17:23:09 artom_ But I'd like to have related/1664931 somewhere, to keep track of all the changes that are happening because of that initial CVE fix

Earlier   Later