| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-27 | |||
| 19:11:28 | efried | mriedem: I can't remember why we decided to use retrying instead of the home-grown @retries in https://review.openstack.org/#/c/556669/, can you? | |
| 19:11:55 | efried | because I'm inclined to push for same in gibi's | |
| 19:12:30 | mriedem | https://review.openstack.org/#/c/556669/17/nova/utils.py@1301 | |
| 19:12:59 | mriedem | i guess that's not what you're looking for | |
| 19:13:45 | sean-k-mooney | im going to grab dinner and kick off a devstack build of a linux bridge env and validated locally too. | |
| 19:14:02 | mriedem | efried: do you mean, why does SchedulerReportClient use our own retries decorator rather than use retrying the library? | |
| 19:14:24 | mriedem | or the RetryDecorator from oslo? | |
| 19:14:37 | jaypipes | mriedem: ack | |
| 19:14:56 | mriedem | b/c that was added in https://review.openstack.org/#/c/516708/ | |
| 19:15:20 | cfriesen | mdbooth: any chance you could take a look at https://review.openstack.org/#/c/616692/ ? | |
| 19:15:32 | efried | mriedem: Well, report client uses home-grown @retries for a few things and @retrying.retry for others. The latter appear to be my handiwork, as linked above; but I also remember having a conversation with you about the relative merits of using retrying lib vs oslo vs this home-grown thing (which is gross btw). | |
| 19:15:46 | mriedem | i asked https://review.openstack.org/#/c/516708/1/nova/scheduler/client/report.py@97 | |
| 19:15:49 | efried | jaypipes: Don't send that one to the gate too quick. I have a gripe about the retries. | |
| 19:15:50 | mriedem | but no reply | |
| 19:16:41 | efried | mriedem: Mm, yeah. IMO we should get rid of that. | |
| 19:16:51 | efried | and at least for now stop adding more usages of it. | |
| 19:17:14 | mriedem | let me phone a friend: dansmith | |
| 19:17:26 | mriedem | i'm assuming he just added his own for simplicity | |
| 19:17:54 | mriedem | and i'm assuming gibi is re-using it because the new code borrows heavily from the claim_resources stuff | |
| 19:18:22 | dansmith | what now? | |
| 19:18:31 | mriedem | why did you write your own https://review.openstack.org/#/c/516708/1/nova/scheduler/client/report.py@97 | |
| 19:18:59 | mriedem | because now gibi is copying it and efried doesn't like it | |
| 19:19:22 | dansmith | oh gah, I dunno.. because I'm not good at remembering all the crap in libraries | |
| 19:19:59 | artom | efried, aspiers, actually, on that capabilities patch, why are we not also adding the driver-owned CUSTOM_ stuff to os_traits? | |
| 19:20:23 | artom | Seems like it would clear stuff up, at the expense of an extra patch that I image won't be difficult to hack up and merge | |
| 19:20:28 | jaypipes | sean-k-mooney: +2 from me on both. | |
| 19:20:36 | artom | *I imagine | |
| 19:20:40 | jaypipes | efried: k | |
| 19:21:07 | jaypipes | mriedem: https://review.openstack.org/#/c/538498/ is showing it needs a rebase anyway. | |
| 19:21:39 | efried | artom: Anything that's a capability-based trait should indeed go into os-traits. Is that what you're saying? | |
| 19:21:45 | artom | efried, yeah | |
| 19:21:52 | efried | random driver-owned CUSTOM_ stuff notsomuch. | |
| 19:21:56 | mriedem | jaypipes: i didn't ask you about that | |
| 19:22:00 | artom | efried, exactl | |
| 19:22:01 | artom | y | |
| 19:22:16 | artom | It's a weird message we're sending to admins | |
| 19:22:24 | efried | jaypipes: that was me. aspiers is working on the rebase (and adding a test case). | |
| 19:22:36 | artom | "The driver owns this stuff, but also this other differently-named stuff that shares a namespace with the stuff you can set" | |
| 19:23:01 | efried | artom: Well, the namespace is COMPUTE_, not CUSTOM_ | |
| 19:23:12 | jaypipes | mriedem: sorry, meant efried :) | |
| 19:23:27 | efried | CUSTOM_ just means it's not in os-traits (yet) | |
| 19:24:03 | efried | artom: Where are you seeing a driver capability-based trait that's not in os-traits? | |
| 19:24:12 | efried | Or are you just concerned about the fact that we're testing that scenario? | |
| 19:24:28 | artom | efried, IMAGECACHE? | |
| 19:25:30 | efried | mm. mriedem said "optionally" at https://review.openstack.org/#/c/538498/19/nova/virt/driver.py@143 -- whyzat? | |
| 19:26:36 | efried | Trying to think of a scenario (heterogeneous levels of nova-compute / os-traits in a cloud, upgrade, something) where we would *need* to support a custom version of a driver capability trait. | |
| 19:26:48 | mriedem | let me remember something from over a year ago... | |
| 19:27:19 | efried | iow could we just make L1033-5 an exception? | |
| 19:27:40 | mriedem | not all capabilities would have traits, | |
| 19:27:43 | mriedem | like has_imagecache | |
| 19:27:53 | efried | okay, but why does has_imagecache not have a trait? | |
| 19:27:57 | mriedem | but your question is if it's in CAPABILITY_TRAITS_MAP should it also be a standard trait | |
| 19:28:02 | efried | right | |
| 19:28:04 | efried | always | |
| 19:28:07 | mriedem | we don't schedule based on has_imagecache | |
| 19:28:32 | efried | then why are we bothering to add it as a trait? | |
| 19:28:45 | efried | and, who says we wouldn't schedule based on has_imagecache? | |
| 19:28:56 | mriedem | i am | |
| 19:29:00 | mriedem | i am saying that right now | |
| 19:29:10 | efried | Maybe I have a flavor that favors nodes with imagecache | |
| 19:29:16 | mriedem | the day we schedule based on has_imagecache is the day i go work at walgreens | |
| 19:29:34 | efried | then wy are we bothering to add it as a trait? | |
| 19:29:38 | efried | on the compute provider | |
| 19:30:04 | mriedem | you're talking about L1034 right? | |
| 19:30:11 | efried | yes | |
| 19:30:17 | mriedem | gdi, did i write that code? | |
| 19:31:20 | artom | That should be, like, our motto. | |
| 19:31:21 | efried | yes, it is present in PS7 (though with normalize_name done manually, tsk) | |
| 19:32:08 | efried | as well as the corresponding comment on L1023 | |
| 19:32:08 | mriedem | looking at https://review.openstack.org/#/c/538498/7/nova/virt/driver.py | |
| 19:32:26 | mriedem | i was probably doing it because supports_trusted_certs wasn't in os-traits at the time | |
| 19:32:33 | mriedem | and didn't think about how that would generate CUSTOM_HAS_IMAGECACHE | |
| 19:32:43 | mriedem | and i think https://review.openstack.org/#/c/538498/7/nova/virt/driver.py@145 | |
| 19:32:53 | mriedem | relates to something i asked jaypipes when he started adding these to os-traits | |
| 19:33:02 | mriedem | so let me dig that up and i can redirect you to jaypipes | |
| 19:33:16 | efried | buck about to be passed | |
| 19:33:51 | mriedem | https://review.openstack.org/#/c/546713/ | |
| 19:33:51 | jaypipes | damn | |
| 19:33:53 | efried | I'm not really concerned about whodunit; what's the right thing here? Should we make it a rule that compute driver capability traits must be standard, and anything that's not will be ignored? | |
| 19:33:56 | mriedem | "I don't seehas_imagecachesupports_recreatesupports_migrate_to_same_host" | |
| 19:34:01 | mriedem | "None of those things are capabilities that a flavor or image would require." | |
| 19:34:33 | efried | that conversation sounds familiar | |
| 19:34:43 | efried | I was apparently convinced enough to +2 the thing. | |
| 19:34:52 | mriedem | "Eric Fried Mar 29, 2018 Patch Set 1: Code-Review+1Okay, I'm convinced we don't need the missing capabilities as traits. Thanks for your patience, all." | |
| 19:35:01 | efried | artom signed off on it too. | |
| 19:35:42 | efried | so, fine, we decided we didn't need them to be standard, and the reason was because we didn't think they would need to be scheduled to. So the question remains: do we need to put them onto the compute node as traits? (Given ^ they would be CUSTOM_ fo sho) | |
| 19:35:42 | mriedem | i think i would agree that if a compute driver capability is going to be reported as a trait, it should probably be a standard trait | |
| 19:35:49 | artom | efried, yeah, but that's like saying a kid ate some candy ;) | |
| 19:36:07 | artom | And in my defense, 'twas me who started this whole kerfuffle we're in now | |
| 19:36:27 | efried | yup, so it's only fair that the blame ended up back atcha. | |
| 19:36:54 | artom | But... was it a *useful* kerfuffle? | |
| 19:36:56 | mriedem | especially because if we added a new driver capability, like supports_trusted_certs, and before it's standard we report CUSTOM_SUPPORTS_TRUSTED_CERTS and then we have a standard trait and it becomes COMPUTE_TRUSTED_CERTS, that's going to be a weird situation for anyone building flavors requiring the former | |
| 19:37:07 | artom | Or did we just re-hash a past discussion to arrive at the same conclusion? | |
| 19:37:22 | efried | artom: No, this is a slightly new discussion and a decision that needs to be made. | |
| 19:37:29 | efried | but we did have to rehash the old discussion as a prerequisite :) | |
| 19:37:34 | artom | \o/ I'm useful! | |
| 19:37:38 | mriedem | i kind of also wish we weren't discussing the design of this ~1 week from feature freeze | |
| 19:37:49 | artom | Sorta | |
| 19:37:51 | efried | mriedem: Yes, there should be no reason we need to introduce a standard trait but wait to use it in nova. | |
| 19:37:54 | mriedem | because i'm going to say "sure whatever you want idk anymore" | |