| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-22 | |||
| 21:28:09 | sean-k-mooney | i think its effectivly part of the contract nova has with cyborg and neutron at this point | |
| 21:28:36 | mriedem | heh, well, it's mutable in the placement api | |
| 21:28:54 | sean-k-mooney | its technically mutable in our config file too | |
| 21:28:57 | mriedem | https://docs.openstack.org/osc-placement/latest/cli/index.html#resource-provider-set | |
| 21:29:14 | sean-k-mooney | well not that but the host paramater | |
| 21:29:14 | mriedem | how so? if you're talking about CONF.host, that's something else | |
| 21:29:22 | sean-k-mooney | ya i was | |
| 21:29:24 | mriedem | hypervisor_hostname comes from the driver | |
| 21:29:25 | mriedem | not the config | |
| 21:29:52 | mriedem | https://github.com/openstack/nova/blob/2e85453879533af0b4d0e1178797d26f026a9423/nova/virt/driver.py#L1622 | |
| 21:30:07 | sean-k-mooney | yep you are right | |
| 21:32:11 | mriedem | anyway, i threw it in the stuck review section for the meeting https://wiki.openstack.org/wiki/Meetings/Nova#Agenda_for_next_meeting | |
| 21:32:24 | mriedem | enumerating options in the ML ahead of the meeting would probably be useful | |
| 22:02:40 | aspiers | efried: around? | |
| 22:02:52 | aspiers | just had another idea for how to provide patch_exists() | |
| 22:02:56 | efried | aspiers: yeaux | |
| 22:03:09 | aspiers | class patch_exists(contextlib.ContextDecorator) | |
| 22:03:36 | aspiers | then it can be used both as a @patch_exists decorator, and via: with patch_exists(...) | |
| 22:04:51 | aspiers | I think there's also a case to be made for providing patch_open in a similar way | |
| 22:05:22 | aspiers | currently there's tons of stuff which does: | |
| 22:05:26 | aspiers | mock_open = mock.mock_open() | |
| 22:05:36 | aspiers | with mock.patch.object(six.moves.builtins, 'open', new=mock_open): | |
| 22:05:43 | aspiers | ... do stuff | |
| 22:05:53 | aspiers | that could be replaced with | |
| 22:06:04 | aspiers | with patch_open(): | |
| 22:06:06 | aspiers | ... do stuff | |
| 22:06:21 | aspiers | and it could support selective patching | |
| 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. | |