| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-27 | |||
| 17:15:56 | efried | oh. | |
| 17:16:20 | efried | aspiers: ^ looks like needs rebase. Which ugh probably means need to wait until predecessor finishes merging. | |
| 17:18:00 | melwitt | does anyone know what's the correct way to unit test code that calls placement? I found the PlacementFixture nova/tests/functional/fixtures.py but was thinking that's not appropriate for unit tests | |
| 17:22:33 | efried | melwitt: "unit" and "calls placement" don't really gel. If it's a true unit test, the placement calls ought to be mocked. | |
| 17:23:31 | melwitt | efried: right.. I wondered if there was a specific way to mock them (like a fixture I don't know about) or if it's just ad-hoc | |
| 17:23:48 | efried | melwitt: Do you have a specific example? | |
| 17:23:50 | melwitt | I didn't find any example yet. still looking | |
| 17:23:53 | efried | okay. | |
| 17:24:11 | melwitt | I'm working on quota check code that calls placement, so once I added the placement call, lots of unit tests fail | |
| 17:24:29 | melwitt | and if someone happened to know the recommended way to mock placement, that would help me | |
| 17:24:48 | efried | melwitt: See nova.tests.unit.scheduler.client.test_report | |
| 17:25:17 | efried | melwitt: Those tests mock SchedulerReportClient.<http primitive like get/put/post/delete> | |
| 17:25:18 | melwitt | thank you | |
| 17:26:24 | efried | melwitt: Whereas nova.tests.functional.test_report_client uses the placement fixture to create an interceptor context manager with which to wrap "real" calls to placement. | |
| 17:27:01 | efried | the latter, for functional tests, gets you real placement behavior. The former just lets you pretend you got a certain Response object. | |
| 17:27:17 | melwitt | efried: got it. thanks for the help | |
| 17:27:23 | efried | np, enjoy. | |
| 17:31:27 | efried | melwitt: correction: test_report_client uses PlacementDirect. The difference in usage between that and PlacementFixture is the context manager-ness. | |
| 17:31:55 | efried | but it sounds like you want the unit level stuff in any case. | |
| 17:33:24 | melwitt | right. but that's good info to know anyway. I'll need to use that too for the functional tests I think | |
| 17:34:10 | openstackgerrit | sean mooney proposed openstack/os-vif master: add addtion check and gate jobs for os-vif https://review.openstack.org/639732 | |
| 17:34:32 | efried | melwitt: In case I don't otherwise notice, lmk when you've got code up, would like to review. | |
| 17:35:47 | melwitt | efried: the patch that adds placement calls is here https://review.openstack.org/638073 implementation done, test coverage is what I'm working on now | |
| 17:36:33 | efried | ack | |
| 17:45:32 | efried | melwitt: left a comment. So yeah, if you take that advice, you'll probably want a functional test in test_report_client; and you may decide a unit test isn't necessary. | |
| 17:46:30 | melwitt | efried: sweet, thanks | |
| 17:46:57 | efried | yw | |
| 18:33:38 | openstackgerrit | Merged openstack/nova master: Change LibvirtDriver.capabilities to an instance variable https://review.openstack.org/638677 | |
| 18:34:04 | openstackgerrit | Ghanshyam Mann proposed openstack/python-novaclient master: DNM: Testing legacy jobs on bionic https://review.openstack.org/639768 | |
| 18:37:30 | efried | aspiers: predecessor merged ^ -- The rebase is trivial, I would be happy to do that and address artom's nits at the same time, unless you're already working on it | |
| 18:38:00 | aspiers | efried: already working on it | |
| 18:38:05 | efried | okay | |
| 18:38:06 | aspiers | efried: I've already fixed his nits | |
| 18:38:15 | aspiers | however I think I may have noticed a missing test case | |
| 18:38:38 | openstackgerrit | Ghanshyam Mann proposed openstack/os-vif master: DNM: Testing legacy jobs on bionic https://review.openstack.org/639769 | |
| 18:39:17 | aspiers | efried: if someone uses the placement API to put a capability trait on a compute host, and then at the next periodic the driver declares that cap is unsupported, it needs to be removed, right? | |
| 18:39:35 | efried | yes | |
| 18:39:45 | efried | that's, like, the primary use case here | |
| 18:39:51 | efried | That's not already covered by the tests? | |
| 18:39:59 | aspiers | I think I only tested removal of traits which were put there by the driver | |
| 18:40:04 | aspiers | I'm just checking now | |
| 18:40:30 | efried | okay. It would be one of the test cases with a compute manager reset() in it, if that helps you narrow it down. | |
| 18:42:10 | aspiers | Yeah, it's not there | |
| 18:42:51 | openstackgerrit | Merged openstack/nova master: remove deprecated os_brick import from ScaleIO driver https://review.openstack.org/638592 | |
| 18:43:23 | efried | aspiers: be easy to fold it into test_report_cpu_traits | |
| 18:44:10 | aspiers | efried: I think it needs to go in test_servers.TraitsTrackingTests | |
| 18:44:19 | aspiers | that's where all the others are | |
| 18:44:21 | efried | either way. | |
| 18:44:40 | aspiers | unfortunately I've gotta go but I'll push the rebase as soon as I've covered this test case | |
| 18:45:39 | efried | okay. | |
| 18:54:45 | mriedem | efried: jaypipes: +2 on https://review.openstack.org/#/c/639653/ - bottom of the bw series now, adds report client functionality for removing part of allocations from the whole | |
| 19:06:31 | openstackgerrit | sean mooney proposed openstack/os-vif master: Add "master" parameter to ip.set() API function https://review.openstack.org/639702 | |
| 19:06:32 | openstackgerrit | sean mooney proposed openstack/os-vif master: add addtional check and gate jobs for os-vif https://review.openstack.org/639732 | |
| 19:10:04 | sean-k-mooney | jaypipes: melwitt ^ should fix the os-vif bug and add new gate jobs that will catch them in the future. | |
| 19:10:41 | openstackgerrit | Chris Friesen proposed openstack/nova master: Flavor extra spec and image properties validation https://review.openstack.org/620706 | |
| 19:10:44 | sean-k-mooney | we will need to a do a 1.15.1 release to pick up that change. ill do that after teh ci reports back but i have tested it locally with iptables | |
| 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? | |