| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-18 | |||
| 21:42:23 | aspiers | Ah, so I always have to construct an ImageMetaProps inside? | |
| 21:42:31 | aspiers | I thought I saw existing test code which got away without doing that | |
| 21:42:43 | aspiers | but maybe it wasn't doing property lookups like my new code does | |
| 21:42:49 | mriedem | if the code didn't care about accessing properties in the ImageMeta object then it didn't need to | |
| 21:42:54 | sean-k-mooney | dansmith: i guess i can add a comment | |
| 21:43:03 | aspiers | mriedem: OK thanks, I'll try that approach | |
| 21:43:27 | sean-k-mooney | dansmith: basically i was hoping we had not wrote the filter to requrie to be run in a specifc order without documenting it | |
| 21:43:37 | sean-k-mooney | dansmith: espcially when the beahvior changes | |
| 21:44:16 | sean-k-mooney | dansmith: my real question is should i try to make it order independent | |
| 21:44:33 | dansmith | sean-k-mooney: okay I'm not sure there are any such inter-dependencies at the moment, but I can very much imagine that there could be. it's an internal list, not a configured sequence or anything... | |
| 21:44:33 | sean-k-mooney | or should i comment that the order matters and just append my filter to the end | |
| 21:44:52 | mriedem | since those request filters can be disabled i'm not sure why order should matter - i assumed that order wouldn't matter | |
| 21:44:58 | sean-k-mooney | dansmith: ther is a depenc currently | |
| 21:45:10 | dansmith | mriedem: some of them cascade a bit, like the tenant aggregate one | |
| 21:45:12 | mriedem | but is that a side effect of how the tests are written | |
| 21:45:29 | sean-k-mooney | mriedem: no | |
| 21:45:37 | dansmith | I don't understand what the big deal is, I don't think there's any promise that they are or aren't dependent, | |
| 21:45:39 | sean-k-mooney | its actully broken if reorderd | |
| 21:45:49 | dansmith | so unless it breaks if you enable one and not the other and it's not documented, then I don't see the problem | |
| 21:46:12 | dansmith | sean-k-mooney: isn't that like saying you expect you can reorder any code or method calls in other places and be surprised that it breaks? | |
| 21:46:17 | dansmith | no promises means no promises | |
| 21:46:29 | dansmith | if we want to have such promises, then we should document, but if not, why are you surprised? | |
| 21:47:06 | sean-k-mooney | im surprised because i understood that the filter were additive | |
| 21:47:22 | dansmith | s/understood/assumed/ ? | |
| 21:47:48 | sean-k-mooney | quickly read the code and that what it appeared to be reorded and the unit test broke | |
| 21:48:04 | dansmith | the sequence isn't mutable in a config file, so I don't see why we would make that code more complicated to handle the potential reordering of them, when it's in code | |
| 21:48:28 | sean-k-mooney | the reason we would is so that its easy to extend | |
| 21:48:47 | dansmith | sean-k-mooney: right, but if I quickly read some code and decide I should be able to reorder the code in the live migration routine, it's highly likely that tests will break right? | |
| 21:49:02 | sean-k-mooney | its not the same thing | |
| 21:49:09 | dansmith | why not? | |
| 21:49:23 | dansmith | seems like exactly the same thing to me, except that you made some assumption that it shouldn't be :) | |
| 21:49:32 | sean-k-mooney | mecasue these are filters that extend the request spec | |
| 21:49:40 | dansmith | no, | |
| 21:49:41 | sean-k-mooney | with addtion constratits | |
| 21:49:44 | dansmith | they *mutate* the request spec | |
| 21:49:51 | sean-k-mooney | yes | |
| 21:49:56 | sean-k-mooney | additivly | |
| 21:50:08 | dansmith | so it's entirely expected, IMHO, that changing the order of a set of mutations might end up in a different result | |
| 21:50:30 | dansmith | not additively, that's your implicit assumption :) | |
| 21:50:48 | sean-k-mooney | is it expected that one that adds a constratin on an az an annothe r on a tenatn would allow an aggate that has neither the tenatn or the az set to work | |
| 21:51:28 | sean-k-mooney | that https://github.com/openstack/nova/blob/master/nova/tests/unit/scheduler/test_request_filter.py#L154 is the test that fails | |
| 21:51:43 | sean-k-mooney | aggreate 3 | |
| 21:51:46 | sean-k-mooney | objects.Aggregate( | |
| 21:51:48 | sean-k-mooney | uuid=uuids.agg3, | |
| 21:51:50 | sean-k-mooney | metadata={'other_key': 'owner'})], | |
| 21:52:10 | sean-k-mooney | get retruned in reqspec.requested_destination.aggregates[0] | |
| 21:52:17 | dansmith | this is precisely my point | |
| 21:52:35 | sean-k-mooney | when the order chages but we are requireing the project id and teh ax in the request spec | |
| 21:52:36 | dansmith | pretty much all our unit tests assume that the code runs in a specific ordering | |
| 21:52:56 | dansmith | you think you should be able to reorder those method calls and have the tests not notice, but I have no idea why | |
| 21:53:08 | dansmith | I refer back to my previous example about any random other sequence of code | |
| 21:53:38 | sean-k-mooney | what i think is broke is i think its not correctly comparing project_id='owner' to filter_tenat_id | |
| 21:53:50 | sean-k-mooney | but maybe im not understanding how that works | |
| 21:55:07 | sean-k-mooney | anyway if this is the expected behavior ill revert the ordering change and add a comment to document that the ordering is important | |
| 21:55:51 | sean-k-mooney | i have not actully read throuhg how those filters actully work line by line | |
| 21:56:38 | sean-k-mooney | so i was just comparing what i changed to determin what could have broken it and then noticed i changed the ordering and asked if it was imporant | |
| 22:05:27 | sean-k-mooney | anyway ill see if i can repoduce the same issue without reordering the filters as a sperate test. | |
| 22:06:54 | sean-k-mooney | the test code and filters seam to be working slightly differently then i thought from first reading | |
| 22:36:15 | openstackgerrit | melanie witt proposed openstack/nova-specs master: Propose showing server status UNKNOWN when host status UNKNOWN https://review.opendev.org/666181 | |
| 22:58:32 | openstackgerrit | Sundar Nadathur proposed openstack/nova-specs master: Nova Cyborg interaction specification. https://review.opendev.org/603955 | |
| 23:06:31 | sean-k-mooney | dansmith: on i figured out what was confusing me | |
| 23:06:44 | sean-k-mooney | the fitler will work in any order | |
| 23:07:12 | sean-k-mooney | the mock data we are generating in the unit test is not order indepent | |
| 23:09:49 | sean-k-mooney | i missed the fact that if you set the side_effect to an iterable then each time the mocked fucntion is called it will return the next item form that iterable so the results i was seing make sense again. | |
| 23:10:32 | dansmith | this is why I said I'm not aware of any dependencies currently, and that the test expecting that the code runs in a specific order is, you know, pretty common and not a problem to me | |
| 23:10:34 | dansmith | anyway, I gotta run | |
| 23:21:32 | openstackgerrit | Merged openstack/nova master: Validate requested host/node during servers create https://review.opendev.org/661237 | |
| 23:21:44 | openstackgerrit | Merged openstack/nova master: docs: Remove references to nova-consoleauth https://review.opendev.org/652965 | |
| 23:21:53 | openstackgerrit | Merged openstack/nova master: tests: Stop starting consoleauth in functional tests https://review.opendev.org/652966 | |
| #openstack-nova - 2019-06-19 | |||
| 00:43:24 | openstackgerrit | Brin Zhang proposed openstack/python-novaclient master: Microversion 2.74: Support Specifying AZ to unshelve https://review.opendev.org/665136 | |
| 01:45:35 | openstackgerrit | Merged openstack/nova master: Log quota legacy method warning only if counting from placement https://review.opendev.org/665765 | |
| 01:52:36 | openstackgerrit | Fan Zhang proposed openstack/nova master: Log disk transfer stats in live migration monitor. https://review.opendev.org/619395 | |
| 02:04:25 | openstackgerrit | Fan Zhang proposed openstack/nova master: Retry after hitting libvirt error VIR_ERR_OPERATION_INVALID in live migration. https://review.opendev.org/612272 | |
| 02:35:21 | openstackgerrit | Takashi NATSUME proposed openstack/python-novaclient master: Fix duplicate object description error https://review.opendev.org/666203 | |
| 02:52:50 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Add a live migration regression test https://review.opendev.org/641200 | |
| 03:21:26 | openstackgerrit | Merged openstack/nova master: Clean up NumInstancesFilter related docs https://review.opendev.org/665768 | |
| 04:47:18 | openstackgerrit | Takashi NATSUME proposed openstack/python-novaclient master: Add irrelevant files in dsvm job https://review.opendev.org/666217 | |
| 05:16:30 | openstackgerrit | Madhuri Kumari proposed openstack/nova master: Replace deprecated with_lockmode with with_for_update https://review.opendev.org/666221 | |
| 05:55:19 | openstackgerrit | melanie witt proposed openstack/nova-specs master: Propose showing server status UNKNOWN when host status UNKNOWN https://review.opendev.org/666181 | |
| 06:30:10 | amotoki | gmann: hi | |
| 06:30:21 | gmann | amotoki: hi | |
| 06:30:34 | amotoki | gmann: there is a discussion on exceptions from novaclient in a horizon review https://review.opendev.org/#/c/661526/ | |
| 06:30:59 | amotoki | gmann: we are struggling on how to catch a specific exception from nova API and/or novaclient. | |
| 06:31:19 | amotoki | gmann: some suggestion would be appreciated. | |
| 06:32:14 | amotoki | gmann: in case of neutron, neutron returns an exception type in a response body and neutronclient decodes it, so consumers of neutronclient python binding can catch a specific error. | |
| 06:34:27 | gmann | checking | |
| 06:34:47 | amotoki | gmann: thanks. no need to rush :) | |
| 06:49:29 | gmann | amotoki: BadRequest is not generic error. I mean nova API convert the various related exception to HTTP Exception and then raise | |
| 06:49:32 | gmann | like - https://opendev.org/openstack/nova/src/branch/master/nova/api/openstack/compute/volumes.py#L354 | |
| 06:50:23 | gmann | HTTPBadRequest can occur due to multiple reason and their details are in error message | |
| 06:52:54 | gmann | amotoki: Horizon showing the error message which include the error details for example: "invalid volume id is request" not enough ? or you want to prepare some addition helpful error msg ? | |
| 06:54:00 | gmann | novaclient also decode them into ClientException - https://github.com/openstack/python-novaclient/blob/003ac57d9af74aa4658a7bf6cc6b6b3bafa58c11/novaclient/exceptions.py#L249 | |
| 06:56:44 | amotoki | gmann: thanks for checking. The horizon patch I mentioned above is to try to show a message from nova API as-is instead of showing a generic message "Unable to attach volume". | |
| 06:57:29 | openstackgerrit | Takashi NATSUME proposed openstack/python-novaclient master: Add irrelevant files in dsvm job https://review.opendev.org/666217 | |
| 06:57:52 | amotoki | gmann: it is generally a thing avoided in horizon (as messages cannot be translated and sometimes they are not friendly to GUI users) but we don't have a good idea for this case. | |
| 06:58:19 | gmann | amotoki: i see your point. | |
| 06:58:24 | amotoki | gmann: my initial question is whether we can assume BadRequest only for that case. | |
| 06:58:55 | amotoki | gmann: for example, I wonder BadRequest can be raised for input validation or something others. | |
| 06:59:23 | gmann | amotoki: it is hard to say, there can be various other error can be raised by Nova which is API specific and many times we do add/improve the exception | |
| 07:00:13 | gmann | amotoki: how about putting else only in case of 500 error code. ex.htttp_status == 500 | |