Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-05-22
22:26:45 efried aspiers: I just reviewed one this morning in fact.
22:26:51 aspiers sean-k-mooney: except the approach you linked there doesn't work if the code path under test makes additional calls to open or exists
22:27:18 aspiers sean-k-mooney: sometimes that can happen (Especially in functional tests) and then you don't want to hardcode assumptions that those calls happen in a particular order
22:27:42 aspiers you just want to patch when it is called with a certain param, otherwise pass through transparently
22:28:52 sean-k-mooney aspiers: you can pass an iterable of callable as the return value and have it return differnet things but yes that is really just for mocking things in unit tests
22:29:32 efried aspiers: nova/tests/unit/test_versions.py:45
22:30:00 sean-k-mooney aspiers: in unit test you actully do generally want to encode the order. or rather we generall do in openstack
22:31:54 sean-k-mooney efried: that is not really generic either
22:32:19 aspiers efried: thanks
22:32:30 efried sean-k-mooney: that's the point. aspiers is wanting to write a thing to do that pattern, so it could be used in places like that in a more readable and generic way.
22:32:40 aspiers right
22:32:55 sean-k-mooney well in the unit test we never want it to call real_open
22:33:06 sean-k-mooney and in the functional test im not sure that is vaild either
22:33:36 aspiers in the functional test I'm writing, placement needs to read placement-policy.yaml from the testenv
22:33:50 aspiers there's no way I'm hardcoding that path into my test
22:34:03 aspiers nor any assumption about *when* it needs to read that file
22:34:17 efried mriedem: Do you have a take on whether nova should talk to cyborg with an admin auth or with the user's auth?
22:34:20 sean-k-mooney why does it need to read a file instead of embeding it in a sting
22:34:44 aspiers because it's not mocking or stubbing placement
22:34:47 efried sean-k-mooney: It was just an example. Point is that there are valid and viable reasons unit tests should invoke real open().
22:34:50 aspiers it's not even testing placement
22:35:01 aspiers exactly
22:35:04 efried so we want to be able to mock it conditionally.
22:35:23 efried I was just dorking with one this morning where libvirt tests set up a TempDir fixture to read and write fake images
22:35:26 aspiers this makes sense when checking files outside the testenv
22:35:34 efried sucker used open()s and other os.path stuff all over the place.
22:35:37 sean-k-mooney well the only valid case would be if they created the file they are opening in which case they would not mock it
22:35:39 aspiers like /etc/nova/release, or /sys/module/kvm_amd/parameters/sev
22:36:12 sean-k-mooney aspiers: well that is the thing we shoudl not be checking files outside the test env
22:36:27 sean-k-mooney the unit test and function test shoudl work on a host without install nova
22:36:28 aspiers sean-k-mooney: ... and that's exactly why I need to selectively patch
22:36:36 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove PlacementAPIConnectFailure handling from AggregateAPI https://review.opendev.org/660852
22:36:40 sean-k-mooney e.g. you shoudl be able to git clone and then run it
22:36:55 sean-k-mooney *run tox -e py36
22:36:55 mriedem efried: dansmith: ^ for tomorrow, could use some thoughts on how to handle failures in the latter case noted in there
22:36:57 aspiers sean-k-mooney: yes, that is what I am aiming for
22:37:10 mriedem efried: re admin auth it depends on what we're doing i guess and what the cyborg api policy is
22:37:20 aspiers sean-k-mooney: I need to mock the presence and contents of /sys/module/kvm_amd/parameters/sev
22:37:22 sean-k-mooney right so im not seeing why you would ever fall back to real_open
22:37:35 aspiers because of other things like placement-policy.yaml which live inside the testenv
22:37:46 efried mriedem: It sounds like Sundar has thought it through and has been assuming the operations should be done on behalf of the user so that proper policy and quota can be taken into account. Do you see any problem with that approach?
22:37:50 aspiers or temp files created by the test framework like efried said
22:38:09 efried mriedem: johnthetubaguy and I have advised him on enabling service_user, but otherwise, should be okay yes?
22:38:16 sean-k-mooney if they are withing the tox venv i gues its fine
22:38:23 mriedem efried: i think for most things with external-to-nova resources we try to use the user auth, for things with volumes/images/ports
22:38:41 sean-k-mooney just so long as the files are not form the host system
22:38:57 mriedem but there are certain APIs on those resources that we use admin creds, like port binding is admin-only since it's host-level info
22:39:12 efried mriedem: k, so ironic is the outlier. And then there's a little bit of neutron that does admin, not sure what that's about. And then there's a little edge case in cinder that uses admin as well.
22:39:18 efried yeah
22:40:11 mriedem nova didn't even have config to do admin level stuff with cinder until a few releases ago
22:40:25 mriedem to forcefully detach a volume when we didn't have a token
22:41:23 mriedem so if there is host-level stuff we need to do i'd expect those apis to be admin-only by policy in cyborg
22:41:35 mriedem i don't know enough about their api though
22:41:51 mriedem like, you as a user can create an fpga resource and provide that to nova on server create to wire it up right?
22:42:06 sean-k-mooney mriedem: that was one of the thing in the spec we called out
22:42:35 sean-k-mooney e.g. that cyborgs api whoudl be admin by default
22:42:44 mriedem so ironic
22:43:08 sean-k-mooney since its manaing host level resouces that are potentially damaging if used incorrectly
22:43:23 mriedem there are also nova/cinder interactions we probably should have done differently from the start, because some cinder apis that nova uses leak host level connection information and aren't admin apis
22:43:51 mriedem weee https://bugs.launchpad.net/cinder/+bug/1740950
22:43:52 openstack Launchpad bug 1740950 in Cinder "Volume details shows attached compute host for non-admins" [Undecided,New]
22:44:04 sean-k-mooney lick the conection context or whatever its called that has the iscsi info
22:44:15 sean-k-mooney attachment?
22:44:17 mriedem heh "If it lingers past Queens release day, we can revisit the advisory task."
22:44:34 mriedem yes the attachment has the connection_info dict
22:44:45 sean-k-mooney ya that has come up a few times
22:46:43 sean-k-mooney in the cyborg case that is one of the open items in the spec here https://review.opendev.org/#/c/603955/12/specs/train/approved/nova-cyborg-interaction.rst@301
22:49:51 sean-k-mooney :( https://github.com/openstack/cyborg/blob/master/cyborg/common/policy.py#L76-L122
22:50:23 sean-k-mooney why is the program endpoint using allow ...
22:53:15 efried sean-k-mooney: That (cyborg APIs being admin only) was something you brought up, but it sounds like that's not how they're planning on doing it.
22:54:28 mriedem someone call lbragstad
22:54:29 sean-k-mooney yes well currently anyway can update any fpga recored with there default policy so how they were planning on doing it and what they proably should do are likely not the same thing
22:55:04 efried mriedem: Just this morning someone was asking about a rare failure in the logs where cinder complained of not having admin creds.
22:55:09 mriedem admin-only by default seems smartest and makes the nova interaction strict from the get-go
22:55:29 efried http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-05-22.log.html#t2019-05-22T12:46:19
22:55:41 mriedem but idk, that might make it hard to allow non-admin stuff later
22:55:43 sean-k-mooney the deployable create/update/delete are adming only
22:56:22 efried mriedem: seems to me like that's worse, because it means we *require* admin creds in the conf, and all operations would be allowable.
22:56:29 sean-k-mooney update and delete on acclertors are admin or own so for those it proably fine
22:57:14 efried cyborg sounds like they have a plan for granular policy (is that the right use of that term?) and if so it makes sense to do user auth from nova.
22:57:37 mriedem efried: without knowing where that users was getting the error i can't really say on that cinder thing,
22:58:09 mriedem it was added b/c there is a periodic in the compute which will shelve offload instances after a period of time, and cleanup soft-deleted instances after a period of time, which means doing things on volumes w/o a user token
22:58:10 sean-k-mooney efried: for some things that makes sense but i think there fpga update should ast least be admin_or_owner
22:58:36 efried mriedem: Well, I wound up concluding that it was probably the thing you just mentioned, the parts of cinder where we need the admin creds for those weird periodic edge cases.
22:59:15 efried they didn't have any admin creds in their conf
22:59:19 mriedem ok on the cyborg policy thing, idk, if only we had like a technical committee in openstack or something with security guidelines
22:59:23 mriedem or an auth/identity team...
22:59:41 mriedem but it's 6pm and i'm not sean-k-mooney so i need to drop off
22:59:46 efried o/
22:59:53 sean-k-mooney o/
23:00:22 sean-k-mooney hay look its thursday ...
23:00:41 sean-k-mooney i should proably go to sleep soon.
23:01:06 sean-k-mooney efried: i didnt get to rework the vidoe-model stuff but i shoudl get that done tomorrow
23:01:11 efried no worries
23:03:08 sean-k-mooney Sundar: is there a reason that anyone can update any fpga recored even if they dont own it or are not admins https://github.com/openstack/cyborg/blob/master/cyborg/common/policy.py#L119-L121
23:03:16 sean-k-mooney becaue ^ seams like a bug
23:04:35 Sundar sean-k-mooney: We got some holdovers from previous releases. We need to scrub all these policies for sure.
23:04:43 sean-k-mooney the same issue is there for the deployable program endpoint.
23:05:06 sean-k-mooney Sundar: ok as long as you are aware but that is something that shoudl be backported
23:05:30 sean-k-mooney normally you would not backport that kind of change but in this case i think its needed

Earlier   Later