| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-11 | |||
| 11:58:27 | gibi | sean-k-mooney: I think you should add your new image meta to the payload | |
| 11:58:34 | gibi | just to follow the pattern | |
| 11:59:00 | sean-k-mooney | but i am the only person that has done that since the notification have been added | |
| 11:59:19 | sean-k-mooney | and we have added lots of image properties since then | |
| 11:59:46 | sean-k-mooney | i also added the 1.1 version bump | |
| 12:00:44 | sean-k-mooney | so i can set and follow a new pattern but if i was to follow the exitising one i would ignore it | |
| 12:00:46 | gibi | I opened the original patch introduced the payload class and at that time it contained everyhing, so the intention is clear to me based on that | |
| 12:01:22 | sean-k-mooney | yep which is why i added the property but we have not been doing that | |
| 12:02:14 | sean-k-mooney | i can file a bug to bring them in sync and work on a patch to do that if that is what we want to do going forward | |
| 12:02:15 | gibi | honestly I would not block you patch on this in either way. It is pretty clear to me know that there are no active notification consumers out there that are willing to give us feedback what they need | |
| 12:02:25 | gibi | s/know/now/ | |
| 12:02:50 | gibi | so I won't be hard with rules | |
| 12:03:30 | sean-k-mooney | well its more efried that was interested since he was addeding image peorperties too and he was wondering if he should be doing the same | |
| 12:04:55 | sean-k-mooney | i was wondering if we should document the policy if there was one somewhere so that we can do the right thing | |
| 12:06:15 | gibi | so we need to 1) decide what is the policy 2) make sure we can somehow enforce the policy | |
| 12:06:41 | gibi | for 1) the original policy was to mirror image meta but then later patches not followed that policy | |
| 12:06:47 | sean-k-mooney | well we can just compare the fields dict of both ovos | |
| 12:07:19 | sean-k-mooney | they shoulds always be in sync right | |
| 12:07:49 | gibi | then we need a separate patch to re-sync the two | |
| 12:08:10 | sean-k-mooney | yes, i commented on the bug that i was not sure why it needed to be a seperate object vs the nova.object.ImageMetaProps | |
| 12:08:18 | sean-k-mooney | *patch | |
| 12:08:44 | sean-k-mooney | was there a reson you made the payload object sperate? | |
| 12:09:41 | gibi | in general we wanted to have separate objects for notification payload so that we are not enforced to push every internal data out in the notification. But for this particular case if we decide that we always want to push every image meta in the notification then sure in this case we don't need a separate payload class | |
| 12:11:41 | sean-k-mooney | well i guess that is the issue right. having a seperate class made the notification update optional since it would not fail tests if you forgot. | |
| 12:12:24 | sean-k-mooney | ok so ill level the notification updates in the patch for now assuming efried is ok with that and ill file a bug for the fact tehy are not in sync. | |
| 12:12:42 | gibi | sean-k-mooney: ack, it works for me | |
| 12:12:51 | gibi | and thanks for taking care of | |
| 12:13:13 | sean-k-mooney | then we can decide if we want to fix that by extending the new object and added the test for the fields or if we want to just use one object in this case sound good? | |
| 13:53:40 | openstackgerrit | Guo Jingyu proposed openstack/nova master: Make scheduling more debuggable https://review.opendev.org/698421 | |
| 14:00:48 | gibi | sean-k-mooney: sorry, I was pulled. Yeah, it sounds like a plan | |
| 14:01:01 | sean-k-mooney | hehe no worries | |
| 14:16:13 | efried | gibi, sean-k-mooney: ack, thanks for the followup. Sounds like we'll want a big patch to sync the two objects. | |
| 14:16:37 | efried | and maybe some kind of clever test that enforces their parity. | |
| 14:16:50 | efried | for the future | |
| 14:17:12 | gibi | efried: hi! I can get behind this plan | |
| 14:17:34 | efried | This ship has probably sailed, but is there no way to use the same object for both purposes? | |
| 14:18:03 | sean-k-mooney | well the clever test is just loop over the fields dict in the nova.object.image_meta.ImageProp object and assert they are in the notificaiton object dict | |
| 14:18:54 | gibi | efried: if we hack the versioning then I guess we can. But I don't know if we really want to heck the versioning | |
| 14:19:06 | gibi | sean-k-mooney: yeah that could work | |
| 14:19:41 | sean-k-mooney | ill write a patch to do that when i finish updating the functional tests | |
| 14:20:21 | efried | sean-k-mooney: and vice versa | |
| 14:20:26 | efried | thanks sean-k-mooney | |
| 14:21:17 | sean-k-mooney | well actully they are dicts so i could jsut assert the keys are equal | |
| 14:21:41 | sean-k-mooney | if we want the typs to match i could jsut assert the dicts are equal to check the keys and values | |
| 14:26:11 | efried | I'd be fine just asserting they have the same keys. | |
| 14:26:32 | efried | it's really just a sniff test to make sure devs didn't miss syncing. | |
| 14:27:06 | sean-k-mooney | yep ill include it in the sync patch | |
| 14:27:24 | sean-k-mooney | also ddt makes updating the func test way simpler | |
| 14:27:42 | efried | sweet. | |
| 14:28:19 | efried | gibi: what's your vacation schedule? | |
| 14:29:33 | gibi | efried: last official day in the office is 16th. If I cannot finish with the qos live migration until then then I will add some extra time to that before 20th | |
| 14:29:58 | gibi | then back in the 6th of Jan | |
| 14:30:35 | efried | Okay. I'll try to get some reviews in on that. I'm also going to ask for another look at the vTPM spec soon if that's okay. I need to do another update, so maybe tomorrow? | |
| 14:31:21 | mriedem | stephenfin: i've got a few questions in this one https://review.opendev.org/#/c/696509/ | |
| 14:32:15 | gibi | efried: sure, lets do it | |
| 14:32:20 | efried | I'm going to make it simpler. Basically, the bits that are really awkward or hard to explain to users, I'm just going to say "behaves like baremetal". | |
| 14:32:29 | gibi | :) | |
| 14:32:33 | mriedem | stephenfin: also, not sure how others feel about this, but a massive rename like this screws up git history https://review.opendev.org/#/c/696745/7 | |
| 14:32:34 | mriedem | and backports | |
| 14:32:40 | mriedem | so i'd rather not do that personally | |
| 14:32:49 | mriedem | dansmith: how are your feels on this ^? | |
| 14:33:05 | efried | I'm more offended by the misused apostrophe in the commit message. | |
| 14:33:39 | stephenfin | mriedem: It really should. git's automerge tooling should detect renames for us | |
| 14:33:42 | mriedem | i guess https://review.opendev.org/#/c/696745/7/nova/network/api.py isn't as terrible as i would expect | |
| 14:33:45 | stephenfin | *it really shouldn't | |
| 14:34:35 | mriedem | maybe i still have ptsd from when ed renamed all of the legacy v2 contrib api modules | |
| 14:34:36 | dansmith | mriedem: yeah, that seems unnecessary to me | |
| 14:34:51 | stephenfin | There will be merge conflicts but that's due to stuff having been moved around. Straight up file renames aren't an issue | |
| 14:35:08 | dansmith | if others really care about it, then whatever, but it seems more trouble than it's worth to me | |
| 14:35:46 | mriedem | backports related to anything touching networking stuff is going to be a nightmare for awhile anyway, but that's expected | |
| 14:36:01 | efried | so might as well shoot the whole hog? | |
| 14:36:10 | stephenfin | dansmith: Well, I really care about it :) | |
| 14:36:24 | dansmith | this definitely makes backports harder, | |
| 14:36:36 | dansmith | I'm not sure why they're hard without it | |
| 14:36:52 | efried | (hm, I've been misusing that idiom for years. It's "go the whole hog".) | |
| 14:37:33 | efried | dansmith: because of all the *other* nova-net removal churn | |
| 14:37:38 | stephenfin | mocks, for one | |
| 14:38:00 | stephenfin | we no longer need to care about mocking stuff that nova-net was doing. If you backport a test, that goes back to not being true | |
| 14:38:05 | dansmith | efried: okay but that's hard like code conflict, not hard like "this file doesn't exist in the old branch" hard | |
| 14:38:41 | efried | I get it. | |
| 14:39:27 | openstackgerrit | sean mooney proposed openstack/nova master: support pci numa affinity policies in flavor and image https://review.opendev.org/674072 | |
| 14:39:40 | mriedem | anywho, i didn't -1 it and it's later in the series, just mentioning it | |
| 14:39:54 | mriedem | someone has to take on the current big ass bottom change that i +2ed already | |
| 14:39:56 | efried | stephenfin: Given that we have nova/image/glance.py, I don't see a problem with having nova/networking/neutron.py | |
| 14:40:02 | sean-k-mooney | efried: i have not added the spy function to check the pci assignment but i think i have made all the other chagnes you suggested | |
| 14:40:08 | stephenfin | dansmith: but it won't - git is smart | |
| 14:40:12 | mriedem | efried: that's a good point | |
| 14:40:16 | mriedem | we also have a cinder.py | |
| 14:40:24 | efried | nova/volume/cinder.py yeah. | |
| 14:40:45 | stephenfin | dansmith: Go make a trivial modification to nova/tests/unit/test_nova_manage.py, commit on master, then backport to stable/stein | |
| 14:40:47 | efried | we should make sure Sundar names it nova/accelerator/cyborg.py and we're golden. | |
| 14:41:00 | stephenfin | it'll happen cleanly, even though I renamed that file in commit nova/tests/unit/test_nova_manage.py | |
| 14:41:08 | stephenfin | whoops, commit 463017b51b8cde48582b2f55ad7a1f2321d03d02 | |
| 14:41:26 | mriedem | actually i did just -1 | |
| 14:42:23 | mriedem | i think having the module having neutron in the name would be less confusing to someone coming to hack on nova and knowing that there was at least once a nova-network thing, and finding nova/network/api.py and wondering if it's nova-net or neutron | |
| 14:42:34 | sean-k-mooney | efried: that would imply that that code is virt driver indepentet. if they cyborg code in that folder was then sure | |
| 14:42:36 | dansmith | stephenfin: aight, well, when we moved tests/ into tests/unit things were not "smart", but whatevs | |
| 14:42:38 | mriedem | since we have explicit glance.py and cinder.py modules those are pretty clear what they are for | |
| 14:42:52 | mriedem | dansmith: oh yeah the tests -> tests/unit was another big one | |
| 14:43:19 | efried | mriedem: oh, I hope we're getting rid of the API shim | |