| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 16:56:21 | mdbooth | jaypipes: So I'm also entirely sure we must have an enginefacade transaction scope there | |
| 16:56:34 | mdbooth | Which we normally do at function scope using a decorator | |
| 16:56:47 | mdbooth | I'm actually being super-defensive there, I think | |
| 16:57:01 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Add regression test for bug 1735407 https://review.openstack.org/526095 | |
| 16:57:02 | openstack | bug 1735407 in OpenStack Compute (nova) "[Nova] Evacuation doesn't respect anti-affinity rules" [Medium,In progress] https://launchpad.net/bugs/1735407 - Assigned to Balazs Gibizer (balazs-gibizer) | |
| 16:57:43 | mdbooth | If the transaction fails on commit, it'll trigger a retry | |
| 16:57:56 | mdbooth | When we retry, we want a new transaction | |
| 16:57:57 | mdbooth | So we need to create one somehow | |
| 16:58:05 | mdbooth | I'm explicitly using an independent transaction | |
| 16:58:16 | mdbooth | So I'm explicitly *not* using any enclosing transaction scope | |
| 16:58:23 | jaypipes | mdbooth: ahhhh... sorry, yeah, I didn't realize there was no enginefacade decorator being used on any of these object entrypoints (BlockDeviceMapping.create()/save(), etc) | |
| 16:58:57 | mdbooth | So it definitely looks weird because I'm being super-defensive. | |
| 16:59:00 | mriedem | ildikov: replied in https://review.openstack.org/#/c/525787/ | |
| 16:59:07 | jaypipes | mdbooth: my presumption was that there was an enginefacade trx context decorator on one of the BDM object methods. :( | |
| 16:59:08 | mriedem | ildikov: if you agree, i'll start on those changes after my next meeting | |
| 16:59:18 | jaypipes | mdbooth: and I see now there isn't... | |
| 16:59:27 | mdbooth | Actually, when I originally wrote that I was less defensive and jsut wrote a comment about how this might fail on multi-master galera | |
| 16:59:36 | mdbooth | But we don't care, because it almost definitely won't | |
| 16:59:57 | mdbooth | And that version looked more normal | |
| 17:00:12 | jaypipes | mdbooth: instead of using that independent context using() thing, I believe you can instead just decorate the _create_uuid() method with the @writer enginefacade decorator, in addition to the @wrap_db_retry decorator. | |
| 17:00:16 | melwitt | mriedem: replied on the review about power_on | |
| 17:00:18 | mdbooth | (but it failed safely) | |
| 17:00:43 | mdbooth | jaypipes: Note the 'independent' in there | |
| 17:01:10 | mdbooth | That's so a race there wouldn't cause a failure of the encompassing transaction | |
| 17:01:58 | mdbooth | jaypipes: Again, I could be convinced to roll back to my big comment explaining why this might fail sometimes. | |
| 17:02:16 | jaypipes | mdbooth: *what* encompassing trx though? I had originally thought there was an encompassing transaction managed by an enginefacade transaction context manager, but I see now there isn't any enginefacade decorator being used on any of the BDM object methods like create(), save(), etc | |
| 17:02:38 | mdbooth | jaypipes: Not today, no. | |
| 17:03:01 | jaypipes | mdbooth: you're implying there will be? :) | |
| 17:03:22 | mdbooth | jaypipes: The design of the enginefacade encourages it, and we have no way of knowing. | |
| 17:03:52 | mdbooth | e.g. if the thing *calling* save had its own transaction | |
| 17:03:59 | mdbooth | we'd be in an encompassing scope | |
| 17:04:36 | mdbooth | Do we ever call save() within a transaction scope? | |
| 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. | |