| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-18 | |||
| 20:25:40 | artom | (Whoever implements "diff of diffs" for Gerrit will have songs written about them) | |
| 20:26:37 | mriedem | mostly easy stuff | |
| 20:27:17 | mriedem | but you've missed some of the 35 conditionals from the existing libvirt driver _create_domain_and_network logic to determine if we should wait for events | |
| 20:28:11 | artom | Oh only 35? | |
| 21:17:47 | sean-k-mooney | dansmith: im pretty sure i know the answer to this but the order in which https://github.com/openstack/nova/blob/master/nova/scheduler/request_filter.py#L157-L161 is defined is not ment to change the behavior of the prefilters right? | |
| 21:18:59 | sean-k-mooney | because if i reorder that alphabetically then it breaks unit test by changing behvior | |
| 21:19:02 | sean-k-mooney | ALL_REQUEST_FILTERS = [ | |
| 21:19:03 | sean-k-mooney | require_tenant_aggregate, | |
| 21:19:05 | sean-k-mooney | map_az_to_placement_aggregate, | |
| 21:19:07 | sean-k-mooney | require_image_type_support, | |
| 21:19:09 | sean-k-mooney | ] | |
| 21:20:06 | sean-k-mooney | works but | |
| 21:20:09 | sean-k-mooney | LL_REQUEST_FILTERS = [ | |
| 21:20:11 | sean-k-mooney | map_az_to_placement_aggregate, | |
| 21:20:13 | sean-k-mooney | require_image_type_support, | |
| 21:20:15 | sean-k-mooney | require_tenant_aggregate, | |
| 21:20:17 | sean-k-mooney | ] | |
| 21:20:21 | sean-k-mooney | breaks | |
| 21:20:54 | aspiers | ooh, interesting | |
| 21:21:23 | sean-k-mooney | yes .... that is totally the word i would use | |
| 21:21:26 | aspiers | :) | |
| 21:21:27 | sean-k-mooney | :0 | |
| 21:22:18 | sean-k-mooney | i was trying to figure out how i broke the teant az fileter with https://review.opendev.org/#/c/665775/1/nova/scheduler/request_filter.py@195 | |
| 21:22:34 | sean-k-mooney | but then i just reorderd them on master and it also broke | |
| 21:27:34 | aspiers | sean-k-mooney: would you be able to advise me on an OVO problem? | |
| 21:27:51 | sean-k-mooney | aspiers: dansmith in case ye are wondering this aggragte https://github.com/openstack/nova/blob/master/nova/tests/unit/scheduler/test_request_filter.py#L163-L165 gets returned here https://github.com/openstack/nova/blob/master/nova/tests/unit/scheduler/test_request_filter.py#L177 and it should not | |
| 21:28:07 | sean-k-mooney | aspiers: i can try | |
| 21:29:08 | aspiers | sean-k-mooney: so neither of these approaches work https://review.opendev.org/#/c/664420/8..9/nova/virt/hardware.py | |
| 21:29:41 | aspiers | sean-k-mooney: and when I changed to hasattr I got a NotImplemented error from the ovo base class because image_meta.py doesn't implement obj_load_attr() | |
| 21:30:30 | aspiers | I tried copying an implementation from one of the other object classes which just does setattr(self, attrname, None), but that errors because None is not a valid value for image properties | |
| 21:31:12 | aspiers | I think the problem is maybe that while outside test environments, ImageMeta objects always have the "properties" property defined, inside test environments they don't | |
| 21:31:46 | sean-k-mooney | i just did "if key in request_spec.image.properties:" | |
| 21:32:07 | sean-k-mooney | but i could be safe and use .get() | |
| 21:32:15 | aspiers | no, I need to test "if properties in image" | |
| 21:32:25 | aspiers | not for some image property | |
| 21:32:36 | aspiers | see https://review.opendev.org/#/c/664420/9/nova/tests/unit/virt/test_hardware.py@3650 where ImageMeta is constructed without properties | |
| 21:32:43 | aspiers | this seems common in unit tests | |
| 21:32:48 | aspiers | they don't bother to populate all the fields | |
| 21:32:57 | sean-k-mooney | e.g. request_spec.get("image",{}).get("properties",{}).get(key) | |
| 21:33:24 | aspiers | I already tried that, it didn't work | |
| 21:33:42 | aspiers | well, I think it might have worked when starting with a request_spec | |
| 21:33:46 | sean-k-mooney | request_spec.get("image",{}).get("properties") will return none if its not set | |
| 21:33:55 | aspiers | but not when starting with objects.ImageMeta() | |
| 21:34:04 | sean-k-mooney | yes it will | |
| 21:34:05 | aspiers | in test_hardware.py there are no request_specs | |
| 21:34:16 | sean-k-mooney | the base ovo implmentes get for all fileds | |
| 21:34:35 | mriedem | wrong | |
| 21:34:45 | mriedem | only if using the DictCompatMixin | |
| 21:35:01 | aspiers | I will try .get() again but I'm pretty sure I tried that already | |
| 21:35:10 | mriedem | *nova/tests/unit/compute/test_compute_api.py | |
| 21:35:12 | mriedem | oops | |
| 21:35:15 | mriedem | NovaObjectDictCompat | |
| 21:35:20 | mriedem | not everything uses that | |
| 21:35:29 | mriedem | and we actually want to ween things off relying on it | |
| 21:35:36 | aspiers | Ahh interesting | |
| 21:36:05 | aspiers | Yeah, the "object or dict" paradigm kinda seemed weird to me | |
| 21:36:31 | mriedem | ImageMetaProps.get is it's own special thing | |
| 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 | |