| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-18 | |||
| 21:36:43 | sean-k-mooney | mriedem: ah your right its in VersionedObjectDictCompat https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/base.py#L773 | |
| 21:36:58 | mriedem | and ImageMeta doesn't use the dict compat mixin | |
| 21:37:02 | mriedem | and RequestSpec.image is an ImageMeta object | |
| 21:37:20 | mriedem | so you can't do request_spec.get('image', {}).get('properties', {}) | |
| 21:37:33 | sean-k-mooney | ya i was actully just assuming it would always be set when we got to the request filetrs | |
| 21:38:04 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add InstanceAction/Event create() method https://review.opendev.org/614036 | |
| 21:38:05 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add TargetDBSetupTask https://review.opendev.org/627892 | |
| 21:38:05 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add Instance.hidden field https://review.opendev.org/631123 | |
| 21:38:06 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Execute TargetDBSetupTask https://review.opendev.org/633853 | |
| 21:38:06 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add CrossCellMigrationTask https://review.opendev.org/631581 | |
| 21:38:07 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add PrepResizeAtDestTask https://review.opendev.org/627890 | |
| 21:38:07 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add prep_snapshot_based_resize_at_dest compute method https://review.opendev.org/633293 | |
| 21:38:08 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add nova.compute.utils.delete_image https://review.opendev.org/637605 | |
| 21:38:08 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add prep_snapshot_based_resize_at_source compute method https://review.opendev.org/634832 | |
| 21:38:09 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add PrepResizeAtSourceTask https://review.opendev.org/627891 | |
| 21:38:13 | mriedem | RequestSpec.image should be set when we get to the request filters yes | |
| 21:38:14 | sean-k-mooney | so i personally was planning on jsut doing "if key in request_spec.image.properties:" | |
| 21:40:00 | aspiers | The problem I'm having is with the properties getter not being there at all, not with testing for the existence of an individual property | |
| 21:40:22 | sean-k-mooney | aspiers: in what code | |
| 21:40:36 | aspiers | the first link I pasted above | |
| 21:40:46 | dansmith | sean-k-mooney: I'm really not sure what you're asking, but I can see the ordering of those needing to be specific | |
| 21:40:53 | aspiers | ||
| 21:41:14 | mriedem | aspiers: lots of tests don't use full objects b/c they can be a pain in the ass to construct if you don't need a full object for what you're testing, | |
| 21:41:23 | mriedem | as you add code that relies on more things being set in the object, you have to adjust the tests as necessary | |
| 21:41:34 | aspiers | mriedem: yes exactly, I was trying to avoid needing full objects which is why I'm running into this problem | |
| 21:41:56 | mriedem | sure, so you need something like image=objects.ImageMeta(properties=objects.ImageMetaProps())) | |
| 21:42:02 | aspiers | if I use "image_meta.get('properties')" as the test, I get AttributeError: 'ImageMeta' object has no attribute 'get'" | |
| 21:42:18 | mriedem | your code could be defensive and do "if 'properties' in image" but that's not really realistic | |
| 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 | |