| dvo-plv | sean-k-mooney, Hello, are you around today ? | 08:27 |
|---|---|---|
| dvo-plv | melwitt, Hello | 08:48 |
| *** Continuity_ is now known as Continuity | 09:13 | |
| elodilles | if any core wants to do a trivial review for a bot generated patch, then here is a nice candidate: https://review.opendev.org/c/openstack/osc-placement/+/875100 | 09:41 |
| elodilles | o:) (this should have merged months ago) | 09:42 |
| sean-k-mooney | done | 09:51 |
| sean-k-mooney | dvo-plv: o/ | 09:51 |
| dvo-plv | I have a question regarding your comment with implementing functional tests | 09:52 |
| dvo-plv | https://review.opendev.org/c/openstack/nova/+/876075/14/nova/scheduler/request_filter.py#285 | 09:52 |
| sean-k-mooney | sure | 09:53 |
| sean-k-mooney | whats specifically | 09:53 |
| dvo-plv | I had rewrite this method to catch conflict in this way https://paste.opendev.org/show/bzaJaNk53E0OX9otfzzI/ | 09:54 |
| dvo-plv | but during the process of creating http response i caught next error | 09:55 |
| dvo-plv | https://paste.opendev.org/show/bd8oaO0EfYznOP5gkACa/ | 09:55 |
| sean-k-mooney | why | 09:55 |
| sean-k-mooney | oh the dict interfnace for nova/oslo verions objects has been deprecated for years | 09:56 |
| sean-k-mooney | we started revmign all the supprot a while a go | 09:56 |
| dvo-plv | to handle this issue in the test | 09:56 |
| dvo-plv | so my question is, how should i properly solve this issue | 09:57 |
| dvo-plv | implement this to_dict method, or leave it with a comment? | 09:57 |
| sean-k-mooney | neither | 09:57 |
| sean-k-mooney | why is to_dict being called | 09:57 |
| sean-k-mooney | im not sure why you are doing the rewite | 09:58 |
| sean-k-mooney | it should not be required | 09:58 |
| dvo-plv | https://paste.opendev.org/show/blwO5HpbmFyV7UP3Gppk/ | 09:59 |
| dvo-plv | this is how I impolemnted this test | 09:59 |
| dvo-plv | and it fails as expected, so I add this mock @unittest.expectedFailure | 10:00 |
| sean-k-mooney | we dont normaly use that decorator | 10:00 |
| dvo-plv | But folks comfirmed that I should not use it here, because this mock only for bugs | 10:00 |
| sean-k-mooney | we use an expict assert | 10:00 |
| sean-k-mooney | i.e. self.assertRaises(<excption_class>, <function>, *args, **kwargs) | 10:01 |
| dvo-plv | Yes, I already tried this and disscuse it with melwitt | 10:04 |
| dvo-plv | <dvo-plv> I alredy tried this way | 10:04 |
| dvo-plv | <dvo-plv> (Pdb) ex = self.assertRaises(exception.FlavorImageConflict,self._resize_server,server,new_flavor_id) | 10:04 |
| dvo-plv | <dvo-plv> *** AssertionError: Notification instance.resize.end hasn't been received. Received: | 10:04 |
| sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/functional/libvirt/test_numa_servers.py#L643-L713 is an example fo a resize test | 10:05 |
| sean-k-mooney | in your case you need to tell self._resize_server | 10:05 |
| sean-k-mooney | to expect it to fail | 10:05 |
| opendevreview | Merged openstack/osc-placement master: Update master for stable/2023.1 https://review.opendev.org/c/openstack/osc-placement/+/875100 | 10:06 |
| sean-k-mooney | https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/tests/functional/integrated_helpers.py#L527-L531 | 10:06 |
| sean-k-mooney | so the current helpe expect to get to verify reize | 10:07 |
| sean-k-mooney | in this case your expecting to get a api error directly right | 10:07 |
| sean-k-mooney | so you will have to just do the post directly | 10:08 |
| sean-k-mooney | self.api.post_server_action( | 10:08 |
| sean-k-mooney | server['id'], {'resize': {'flavorRef': flavor_id}}) | 10:08 |
| sean-k-mooney | like this https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/tests/functional/test_servers.py#L808-L818 | 10:09 |
| sean-k-mooney | actully def test_resize_server_negative_invalid_state(self): is kind of a good exampel for you to use as a reference in general | 10:10 |
| dvo-plv | This way also was tested | 10:11 |
| dvo-plv | <dvo-plv> (Pdb) self.api.post_server_action(server['id'], {'resize': {'flavorRef': new_flavor_id}}) | 10:11 |
| dvo-plv | <dvo-plv> {} | 10:11 |
| dvo-plv | <dvo-plv> It does not call class Fault(webob.exc.HTTPException) fault names | 10:11 |
| dvo-plv | In this test, which can be reference, it wait for 409 error and verify it here self.assertEqual(409, ex.response.status_code) | 10:12 |
| dvo-plv | So I belive that I also need to extend FlavorImageConflict with error code 409 and catch it in the test | 10:13 |
| sean-k-mooney | no you dont | 10:13 |
| sean-k-mooney | FlavorImageConflict already resultes in a 409 | 10:13 |
| dvo-plv | https://github.com/openstack/nova/blob/master/nova/exception.py#L2341 | 10:14 |
| sean-k-mooney | oh it default to a 500 | 10:14 |
| dvo-plv | I did not found it here, so I thought that I need to extend it | 10:14 |
| sean-k-mooney | but it should be converted elsewhere to a 400 or 409 | 10:14 |
| sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/exception.py#L67 | 10:15 |
| dvo-plv | So, all this changes is not required to raise http conflict ? https://paste.opendev.org/show/bzaJaNk53E0OX9otfzzI/ | 10:16 |
| dvo-plv | It should handle is some other way ? | 10:16 |
| sean-k-mooney | it is part of INVALID_FLAVOR_IMAGE_EXCEPTIONS https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/api/openstack/compute/servers.py#L58 | 10:16 |
| sean-k-mooney | and that gets converted to a 400 bad reuest for resize here https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/api/openstack/compute/servers.py#L1086-L1087 | 10:17 |
| sean-k-mooney | so if you copy test_resize_server_negative_invalid_state and update it with the logic form your test and assert a 400 is retured then that should work | 10:18 |
| sean-k-mooney | so duplicate https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/tests/functional/test_servers.py#L792C9-L821 and update it image/flavor infor required for your test case | 10:19 |
| dvo-plv | Okay, I will try to use it as template now | 10:19 |
| sean-k-mooney | you shoudl get back a client.OpenStackApiException with reponce code 400 | 10:19 |
| dvo-plv | thank you | 10:19 |
| sean-k-mooney | ok i need to step away for a bit and pick up a collegue form there house | 10:20 |
| sean-k-mooney | ill be back later | 10:20 |
| dvo-plv | sean-k-mooney, I have rewrote test in the next way and I still get the same empty response | 11:16 |
| dvo-plv | https://paste.opendev.org/show/bSbVh7CP0oKa1eQdVNUj/ | 11:16 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Refactor CinderFixture https://review.opendev.org/c/openstack/nova/+/885756 | 13:23 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Reproducer for dangling bdms https://review.opendev.org/c/openstack/nova/+/881457 | 13:23 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | 13:23 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Allow to set attachment-id for bdm tests https://review.opendev.org/c/openstack/nova/+/887515 | 13:23 |
| dvo-plv | I never catch this exception | 13:42 |
| dvo-plv | https://paste.opendev.org/show/bBkeTtneX6hnX4kUj79F/ | 13:42 |
| dvo-plv | Maybe I need to wrap my method with some decorator ? | 13:43 |
| sean-k-mooney | it would be similer to look at this if you pushed your change as a seperate commit on top of the curren tone | 13:59 |
| sean-k-mooney | but you should not really be modifying _resize | 14:00 |
| sean-k-mooney | if your not getting to INVALID_FLAVOR_IMAGE_EXCEPTIONS on line 42 | 14:01 |
| sean-k-mooney | that implies you likely are not blocking the resize proeprly | 14:02 |
| sean-k-mooney | i.e. the code path might not be calling the fungiton that does the validation | 14:03 |
| sean-k-mooney | dvo-plv: i belive the validation code is defeintly run for server create yes. | 14:14 |
| sean-k-mooney | i.e. if you create a server with a conflicting image/flavor | 14:14 |
| sean-k-mooney | so perhaps you should start with a funcitonal test to confirm that | 14:15 |
| sean-k-mooney | and then if the that works and the resize one does not we then definetly have a bug | 14:15 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Refactor CinderFixture https://review.opendev.org/c/openstack/nova/+/885756 | 14:29 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Reproducer for dangling bdms https://review.opendev.org/c/openstack/nova/+/881457 | 14:29 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | 14:29 |
| opendevreview | Danylo Vodopianov proposed openstack/nova master: Packed virtqueue support was added. https://review.opendev.org/c/openstack/nova/+/876075 | 14:33 |
| dvo-plv | sean-k-mooney, I just uploaded test | 14:34 |
| dvo-plv | the problem which I get see in the debugger, it does not achive this execption | 14:36 |
| dvo-plv | https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/servers.py#L1086 | 14:36 |
| dvo-plv | but it achive this exception https://review.opendev.org/c/openstack/nova/+/876075/15/nova/virt/hardware.py#1971 | 14:37 |
| dvo-plv | I did not get the vector which I should to move | 14:43 |
| dvo-plv | I can see that _resize method call > /opt/stack/nova/nova/conductor/tasks/migrate.py(345)_schedule() | 14:43 |
| dvo-plv | -> selection_lists = self.query_client.select_destinations( | 14:43 |
| dvo-plv | and this rescheduler calls self.query_client.select_destinations method which call my filter, which call excepetion conflict | 14:44 |
| dansmith | auniyal: do you want to get this fixed up so we can just merge it? https://review.opendev.org/c/openstack/nova/+/885756 | 14:45 |
| dvo-plv | but this exception does not affect on this https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/servers.py#L1086 | 14:45 |
| dansmith | we could do that before you fully fix the dangling bdms thing and then it will be off your plate and out of that stack | 14:45 |
| dvo-plv | it calls rpc server error instead of httconflict | 15:07 |
| dvo-plv | https://paste.opendev.org/show/byeH5cOKuKPovlDt9rGh/ | 15:07 |
| dvo-plv | it sets code 500 by default | 15:11 |
| dvo-plv | https://paste.opendev.org/show/byB3mGcovbJN70D7KL6B/ | 15:11 |
| dvo-plv | sean-k-mooney, conflict exception absent here: https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L294 | 15:28 |
| dvo-plv | and here: https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L147 | 15:35 |
| dvo-plv | but it does not help | 15:35 |
| auniyal | dansmith, yes, MockPatch didn't work for me right now, so I removed WIP. later I might create new patch for same. if its fine we can merge this. | 15:39 |
| dansmith | auniyal: I commented about this, but you were using it as a context manager right? You need to use it as a fixture | 15:40 |
| auniyal | yes, context manager I tried earlier and also mock.patch('nova.volume.cinder.API.attachment_create', side_effect=self.fake_attachment_create).start() | 15:42 |
| auniyal | but after your suggestion on MockPatch I started looking into it, but couldn't figure out it | 15:42 |
| auniyal | this: $ grep 'Fixture.*MockPatch' -r nova/tests | 15:43 |
| dansmith | $ grep 'fixtures.*Mock' -r nova/tests | wc -l ✭reimage-timeout | 15:45 |
| dansmith | 181 | 15:45 |
| auniyal | yes, it was your comment actually, | 15:49 |
| auniyal | the files it shows has example of MockPatch | 15:49 |
| auniyal | so I went through them, and tried to apply on cinder fixture, but it did not work, | 15:50 |
| auniyal | I got the control-flow though | 15:50 |
| dansmith | okay I'm confused | 15:51 |
| sean-k-mooney | dvo-plv: so we seam to be getting to select destination before that runs which is far too late | 15:53 |
| sean-k-mooney | dvo-plv: you should be checkign the image compatibality in _validate_flavor_image_nostatus https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/compute/api.py#L718-L833 | 15:55 |
| sean-k-mooney | for resize its invoked here https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/compute/api.py#L4253-L4272 but its also used in create and rebuild | 15:56 |
| sean-k-mooney | dvo-plv: _validate_flavor_image_nostatus is the common place in the api that is ment to check compaitble before we ever do a api request to placemnt or an rpc to the conductor | 15:57 |
| sean-k-mooney | you will also need to put a call to the check here too https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/compute/api.py#L4268 | 16:00 |
| sean-k-mooney | to validate the bfv path | 16:00 |
| dvo-plv | Maybe it will be better to call this https://review.opendev.org/c/openstack/nova/+/876075/15/nova/virt/hardware.py#1971 before this statement | 16:02 |
| dvo-plv | https://github.com/openstack/nova/blob/4b454febf73cdd7b5be0a2dad272c1d7685fac9e/nova/compute/api.py#L4266 | 16:02 |
| dvo-plv | to call both if and else ? | 16:02 |
| dvo-plv | to catch both if and else | 16:02 |
| dvo-plv | Also I need to handle rebuild somewhere | 16:02 |
| *** LarsErik1 is now known as LarsErikP | 19:07 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!