| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 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? :) | |
| 18:18:40 | mdbooth | jaypipes: No, the exact opposite | |
| 18:19:01 | mdbooth | If we were using enginefacade the way it was intended, i.e. to create larger-scoped transactionss | |
| 18:19:08 | mdbooth | We'd be hitting bugs here | |
| 18:19:28 | mdbooth | We're only relatively safe because we don't do that | |
| 18:20:04 | mdbooth | Ideally something which might read a few bdms would have its own read transaction | |
| 18:20:12 | mdbooth | Which covered all of them | |
| 18:20:25 | mdbooth | Then suddenly one of them doesn't have a uuid and we need to do a write | |
| 18:20:36 | mdbooth | This would fail, because we're in a read transaction. | |
| 18:21:00 | mdbooth | If we always do the write in an independent transaction, that doesn't matter | |
| 18:21:32 | mdbooth | Actually, I wonder if we'd hit that in practise... | |
| 18:24:51 | mdbooth | No, we currently still have separate micro-transactions everywhere | |
| 18:26:31 | _ix | Hello again. I was curious about how to best segment my deployment. I'd like some hypervisors running off of rados block devices, and others running off of local ssd. Are availabiltiy zones what I'm looking for? | |
| 18:27:09 | mdbooth | But if, for eg, we annotated BlockDeviceMapping.get_by_volume_id with @reader so that it fetched the joined instance in the same transaction as the bdm, that would be a bug. It would be pretty obtuse, though, because it's not at all obvious that _from_db_object can write | |
| 18:30:11 | _ix | I must be looking for host aggregates. | |
| 18:31:33 | mdbooth | jaypipes: Nah, the comments are longer than the code. | |
| 18:31:58 | mdbooth | s/code/code to fix the problems requiring comments/ | |
| 18:33:50 | sean-k-mooney2 | every time i read bdms i keep wanting to change it to dbms as i assume i reversed the letter because of my dyslexia | |
| 18:38:45 | jeblair | hi, we're looking at having nodepool ask nova for its quota periodically to make it more responsive to changes. does nova cache limit information for users (cores, ram, instances)? if so, what's a typical cache period? | |
| 18:40:46 | melwitt | jeblair: I don't think limit info is cached | |
| 18:42:45 | sean-k-mooney2 | melwitt: is limit info still stored in nova or is that contolled by keystone now for cores,ram and insances? | |
| 18:42:58 | jaypipes | mdbooth: actually, decorating the _from_db_object() with the @writer context is actually a very *explicit* way of saying the expectations of that method. https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L902 | |
| 18:43:38 | jaypipes | mdbooth: and functional tests of the data online migration will blow up nicely if it's set to @reader instead of @writer. | |
| 18:43:49 | jaypipes | mdbooth: and blowing up is a good thing in this case... | |
| 18:43:50 | mdbooth | jaypipes: Absolutely, but it's unnecessary except in this really weird edge case. | |
| 18:43:56 | jaypipes | mdbooth: agreed | |