| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 17:09:47 | bauzas | sean-k-mooney2: well, it's a sysfs call | |
| 17:10:09 | bauzas | I need to open("/sys/class/mdev_bus/<device>/mdev_supported_types/<type>/create", O_WRITE) | |
| 17:10:25 | mdbooth | jaypipes: It's an interesting point, btw. I think everywhere that we use the retry decorator would be broken unless it's guaranteed to be a top-level transaction scope. | |
| 17:10:35 | sean-k-mooney2 | bauzas: oh are you working a lib for that beacues we are planning to write one soon | |
| 17:11:01 | bauzas | sean-k-mooney2: no, I'm implementing the libvirt side for creating a VGPU | |
| 17:11:21 | bauzas | but for that, nova needs to instanciate a mediated device | |
| 17:11:39 | bauzas | then, once that done, it needs to update the guest XML using that UUID it just created | |
| 17:11:42 | sean-k-mooney2 | bauzas: or libvirt could | |
| 17:11:55 | bauzas | sean-k-mooney2: libvirt doesn't do that atm | |
| 17:12:01 | bauzas | call it experimental :) | |
| 17:12:16 | bauzas | sean-k-mooney2 https://libvirt.org/drvnodedev.html#MDEV (bottom page) | |
| 17:12:31 | bauzas | you'll see the lovely sysfs call I need to make | |
| 17:12:32 | sean-k-mooney2 | bauzas: i know that is why we where planning on writing a singel file pytyon wrapper around sysfs to do it | |
| 17:12:33 | jaypipes | mdbooth: certainly something to chat with zzzeek about at some later time. | |
| 17:12:41 | sean-k-mooney2 | and in our case call it for os-vif | |
| 17:12:52 | bauzas | hum | |
| 17:13:08 | bauzas | sean-k-mooney2: okay, so that's definitely a privsep decoy then | |
| 17:13:19 | sean-k-mooney2 | so i would assume that /sys/class/mdev_bus/<device>/mdev_supported_types/<type>/create is owned by root | |
| 17:13:33 | openstackgerrit | Merged openstack/os-vif stable/pike: Update .gitreview for stable/pike https://review.openstack.org/488670 | |
| 17:13:47 | bauzas | sean-k-mooney2: zactly, so I'll just make a public method in the nova.privsep helper module | |
| 17:13:58 | bauzas | call it tech debt if you wish | |
| 17:14:32 | bauzas | if the libvirt API was supporting to do such things like creating a mediated device without asking to use sysfs, then I wouldn't need to do such crap things in nova | |
| 17:14:37 | sean-k-mooney2 | bauzas: yep but the real question is what capablitis do you need? | |
| 17:15:05 | bauzas | sean-k-mooney2: AFAIK, nova privsep uses the global ones | |
| 17:15:30 | ildikov | mriedem: agree, replied, thanks | |
| 17:16:31 | sean-k-mooney2 | bauzas: well my point is there are two parts to this. the mode of the file e.g. is it globally writeabe or do you have to be a specific user/group and second do you need cap_sys_admin for exampel to write to it ignoring the filesystem mode | |
| 17:17:32 | bauzas | sean-k-mooney2: for the first question, I'm supposing to use the uid 0 for opening and writing the file | |
| 17:17:48 | bauzas | so the basic nova.privsep context | |
| 17:18:53 | sean-k-mooney2 | bauzas: so you will reuse this one https://github.com/openstack/nova/blob/2af92486b2dff6eb50c2e1f52e23eeae50d4bf98/nova/privsep/__init__.py#L21 | |
| 17:19:26 | bauzas | sean-k-mooney2 : what I need is actually pretty identical to https://github.com/openstack/nova/blob/master/nova/privsep/libvirt.py#L117 | |
| 17:19:43 | bauzas | sean-k-mooney2 yup that's what I meant | |
| 17:19:49 | sean-k-mooney2 | bauzas: CAP_DAC_OVERRIDE allow you to ignore filesystem permissions | |
| 17:20:19 | bauzas | sean-k-mooney2: nevermind, I'll basically copy/shame the hairpin cut sysfs call | |
| 17:20:50 | bauzas | I need to bail out | |
| 17:20:55 | sean-k-mooney2 | bauzas: yes just reuse @nova.privsep.sys_admin_pctxt.entrypoint | |
| 17:20:59 | bauzas | Christmas tree implementation time | |
| 17:21:06 | sean-k-mooney2 | cool enjoy | |
| 17:21:19 | bauzas | sean-k-mooney2: I knew how to use privsep, I was just wondering whether it was needed | |
| 17:21:31 | bauzas | but yeah, since it's a sysfs call, I need it, so... | |
| 17:21:43 | bauzas | ++ | |
| 17:24:24 | cdent | here’s a question I’ve been wondering about for a long time: How do people decide when a comment is a NOTE and when it is just a comment? | |
| 17:24:34 | sean-k-mooney2 | bauzas: i do feel i might end up duplicating your work so can you add me to the review if you push it up. im working on using mdev's for netwoking currently though it wont be pushed upstream until rocky at the earliset as the qemu changes arent even submitted upstream yet | |
| 17:24:42 | mdbooth | jaypipes: As for creating null uuids in a test, I still think that my 1-liner is better than 22 lines. | |
| 17:25:15 | jaypipes | mdbooth: and adding a testing-only kwarg to a public API? nah... | |
| 17:25:25 | mdbooth | jaypipes: It's not a public api | |
| 17:25:51 | jaypipes | mdbooth: close enough to a public API in my book... | |
| 17:25:53 | mdbooth | And it's clearly called out | |
| 17:26:01 | mdbooth | It's got test_ in the name ;) | |
| 17:26:57 | jaypipes | mdbooth: sorry, I just disagree on that. from a style and a code structure perspective, I think it's better to explicitly create test fixtures and expectations within the test itself. | |
| 17:27:00 | mdbooth | jaypipes: We've also similar in a bunch of places. | |
| 17:27:19 | mdbooth | jaypipes: Meh, ok. | |
| 17:27:21 | jaypipes | mdbooth: can you point me to something similar please? | |
| 17:27:50 | mdbooth | jaypipes: I'm thinking class reset methods | |
| 17:28:01 | mdbooth | functions which only exist so they can be unit tested | |
| 17:28:06 | openstackgerrit | Chhavi Agarwal proposed openstack/nova master: Volume detach should have instance uuid https://review.openstack.org/511804 | |
| 17:28:07 | mdbooth | functions which only exist so that can be mocked | |
| 17:28:28 | jaypipes | mdbooth: that's a separate function. it's not changing a real function just for the purpose of testing. | |
| 17:28:44 | mdbooth | I'm pretty sure I've also seen test only args | |
| 17:28:51 | jaypipes | mdbooth: and we also discourage creating functions that exist only to be mocked. | |
| 17:28:52 | dansmith | I too am interested, | |
| 17:29:01 | mdbooth | It's a wart, but it allows code reuse. | |
| 17:29:02 | dansmith | because I don't think that we do that anywhere I'm familiar with | |
| 17:29:16 | sean-k-mooney2 | mdbooth: test only args kindo of defets the reason for testing | |
| 17:29:25 | dansmith | yeah | |
| 17:29:25 | mdbooth | Otherwise we just have to duplicate code | |
| 17:29:30 | sean-k-mooney2 | mdbooth: you are not testing what runs in production | |
| 17:29:47 | mdbooth | sean-k-mooney2: The use is to create something only for use in testing | |
| 17:29:53 | mdbooth | So no, it doesn't run in production :) | |
| 17:30:13 | mdbooth | sean-k-mooney2: In this case, it's to tweak a function to not default something which was formally not defaulted. | |
| 17:30:57 | mdbooth | Anyway, if it's not going to pass muster I'll change it | |
| 17:32:26 | sean-k-mooney2 | mdbooth: can you send me a link? | |
| 17:32:48 | mdbooth | https://review.openstack.org/#/c/242603/25/nova/db/sqlalchemy/api.py | |
| 17:33:53 | mdbooth | That function is small enough the cut/pasting it into a test isn't entirely evil | |
| 17:35:06 | sean-k-mooney2 | and why do you want to not see the uuid automatically? | |
| 17:35:27 | mdbooth | sean-k-mooney2: So as to test upgrade of legacy objects which don't have it | |
| 17:36:23 | sean-k-mooney2 | and you cant mock the call to uuidutils.generate_uuid() in that case to retrun None? | |
| 17:36:54 | mdbooth | sean-k-mooney2: Cut/paste is less evil than that. | |
| 17:47:51 | mdbooth | dansmith: jaypipes So, I'm going to recut that second patch. No happy faces for https://review.openstack.org/#/c/242602/24, though ? | |
| 17:47:59 | mdbooth | While I'm respinning anyway, that is. | |
| 17:48:17 | mdbooth | It's the patch immediately before. | |
| 17:52:41 | jaypipes | mdbooth: gimme a few | |
| 17:53:00 | mdbooth | jaypipes: Thanks. | |
| 17:53:26 | mdbooth | jaypipes: That patch version is mostly dansmith with some stuff deleted that we don't need any more. | |
| 17:53:48 | jaypipes | mdbooth: in that case, automatic -2 from me | |
| 17:53:58 | mdbooth | jaypipes: Understood. | |
| 17:54:01 | jaypipes | hehe | |
| 17:54:03 | mdbooth | Probably for the best. | |
| 17:54:06 | jaypipes | :) | |
| 18:06:04 | mdbooth | Speaking of anti-patterns, btw: functions which modify their input parameters. I ran across a unit test last week which only passed because the method it called modified the dict it was given as well as its output, so the later assertion that the 2 were equal passed coincidentally. | |
| 18:06:37 | mdbooth | If it had actually mattered, that would be pretty obtuse. | |
| 18:07:30 | mdbooth | Common culprit is popping values from input dicts. | |
| 18:10:40 | sean-k-mooney2 | mdbooth: that depends on the fucntion. if the fucntion returned nothing the modifying the inpu arguments may be correct. like a sort fuction there are other exampels though | |
| 18:10:44 | openstackgerrit | Ed Leafe proposed openstack/nova master: Change RPC for select_destinations() https://review.openstack.org/516707 | |
| 18:10:45 | openstackgerrit | Ed Leafe proposed openstack/nova master: Move the claim_resources method to scheduler utils https://review.openstack.org/511357 | |
| 18:11:00 | edleafe | mriedem: ^^ Got these working. Now starting on your comments on https://review.openstack.org/#/c/511358/ | |
| 18:11:04 | mdbooth | sean-k-mooney2: Right, if the purpose of the function is to modify its input parameters, that's different. | |
| 18:11:13 | sean-k-mooney2 | mdbooth: though i do agreee that in general a fucntion should retrun someting or have sideefect but not both | |
| 18:17:25 | mdbooth | jaypipes: So, I just realised that another reason to use an independent transaction in _create_uuid is that if it were ever called in the future in the context of a read transaction (because the caller is just reading from the db), it's going to be a failure because we can't promote a read transaction to a write transaction. | |
| 18:17:53 | mdbooth | So basically, this only works robustly as long as we continue not using enginefacade | |
| 18:18:28 | jaypipes | mdbooth: all the more reason to use enginefacade, no? :) | |