Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-05-22
22:06:38 efried and custom side effects
22:06:44 aspiers right
22:06:51 aspiers with patch_open() as mock_open:
22:06:56 aspiers mock_open.side_effect = ...
22:07:13 efried whereupon you might as well just say
22:07:13 efried with mock.patch.object(six.moves.builtins, 'open', new=mock.mock_open()) as mock_open:
22:07:39 efried but sure, patch_open would be a neat little helper.
22:08:09 aspiers hrm
22:08:12 aspiers in two minds about this
22:08:24 aspiers there's a danger of trying to reinvent a bunch of the mock API
22:08:41 aspiers with patch_open(side_effect=...) as mock_open:
22:08:43 aspiers etc.
22:09:02 aspiers the bit I'm really missing easy selective patching
22:09:25 aspiers as in "patch opening path X, but pass anything else through to builtins.open"
22:11:48 aspiers efried: I can't find any existing implementing of selective patching of open(), although I thought you said you had done that in the past. Maybe outside nova?
22:14:05 sean-k-mooney i think we have an example of this in os-vif
22:14:25 sean-k-mooney by selectivly patching you mean mock right
22:14:48 sean-k-mooney you do it like this https://github.com/openstack/os-vif/blob/6f08a3b4f8be77b2b5eae653c05db6e5770b562d/vif_plug_ovs/tests/unit/test_linux_net.py#L65
22:14:58 sean-k-mooney @mock.patch('six.moves.builtins.open')
22:15:17 sean-k-mooney or this @mock.patch.object(builtins, 'open')
22:17:49 sean-k-mooney aspiers: oh you were sugging addign a wrapper to make it simpler
22:18:19 sean-k-mooney ya that soudn like a good idea becuse tis a bit of a pain to get right
22:24:56 openstackgerrit Eric Fried proposed openstack/nova master: Remove nova.compute.*API() shims https://review.opendev.org/660527
22:26:19 aspiers sean-k-mooney: yeah exactly
22:26:20 efried aspiers: Sorry, hold on, lemme find
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

Earlier   Later