Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-06
17:04:51 mdbooth We never used to, because they were confined to db.api()
17:05:01 mdbooth However, they've been moving out of there for a while.
17:06:10 mdbooth jaypipes: Anyway, this is very defensive. I wrote it such that it shouldn't fail however you call it. That makes it look different, so it's a tossup whether it's worth it.
17:06:15 openstackgerrit Theodoros Tsioutsias proposed openstack/python-novaclient master: CommandError is raised for invalid server fields https://review.openstack.org/525110
17:06:17 jaypipes mdbooth: commented on the patch again...
17:06:31 jaypipes mdbooth: lemme know if that code makes more sense.
17:07:01 jaypipes mdbooth: sorry, I originally had missed the fact that no enginefacade trx context decorators were in use for BDM object
17:07:09 bauzas quick question for privsep specialists
17:07:41 openstackgerrit Theodoros Tsioutsias proposed openstack/python-novaclient master: CommandError is raised for invalid server fields https://review.openstack.org/525110
17:07:43 mdbooth jaypipes: Yeah, I think the only reason I didn't do that was to have an independent transaction
17:07:57 bauzas given I need to open a file for write attributes, do I need to use a privsep method doing os.open() or can I just call it directly?
17:08:04 mdbooth jaypipes: i.e. a transaction which is explicitly divorced from any encompassing scope
17:08:29 mdbooth Because if that was called from within an encompassing scope, it would still fail on retry, as well as failing the encompassing scope
17:08:51 mdbooth But as I said, this is super-unlikely.
17:08:55 bauzas AFAICT, I can see in the libvirt driver some os.open calls that write directly
17:09:08 mdbooth So a comment might suffice.
17:09:16 sean-k-mooney2 bauzas: if you dont need elevated privlages to open the file in the mode you need then you dont need privsep
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 mdbooth Otherwise we just have to duplicate code
17:29:25 dansmith yeah
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.

Earlier   Later