| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-08 | |||
| 22:17:27 | dansmith | mriedem: okay got N-3 | |
| 22:18:09 | dansmith | the actual microversion patch is like half the size now, which is oh so much better | |
| 22:19:09 | mriedem | yeah | |
| 22:19:31 | mriedem | and most of it is samples | |
| 22:20:01 | dansmith | yeah | |
| 22:20:32 | mriedem | welp it's 4:20, too late to start think friday now | |
| 22:37:58 | fried_rice | o ovo wizards, remind me what the idiom is for getting extra specs out of a flavor obj | |
| 22:38:16 | fried_rice | if 'extra_specs' in flavor: | |
| 22:38:16 | fried_rice | extra_specs = flavor.extra_specs | |
| 22:38:16 | fried_rice | ? | |
| 22:38:36 | mriedem | anyone want to drop the nova-multiattach job from nova changes? https://review.openstack.org/#/c/606981/ | |
| 22:38:50 | mriedem | fried_rice: yes, but i think it's always there | |
| 22:39:11 | mriedem | return cls._from_db_object(context, cls(context), db_flavor, | |
| 22:39:11 | mriedem | expected_attrs=['extra_specs']) | |
| 22:39:12 | fried_rice | apparently not, at least in test: http://logs.openstack.org/43/631243/8/check/openstack-tox-py27/e61ba3b/testr_results.html.gz | |
| 22:39:28 | mriedem | unit tests not withstanding | |
| 22:39:30 | fried_rice | maybe the test is just borked. | |
| 22:39:45 | mriedem | yeah in real runtime code the flavor would have the extra specs loaded on get from the db | |
| 22:40:02 | fried_rice | can it be None? | |
| 22:40:16 | mriedem | is the field marked as nullable=True? | |
| 22:40:19 | fried_rice | ... | |
| 22:40:27 | mriedem | 631243 | |
| 22:40:28 | mriedem | oops | |
| 22:40:33 | mriedem | 'extra_specs': fields.DictOfStringsField(), | |
| 22:40:33 | mriedem | no | |
| 22:40:35 | mriedem | so it's not nullable | |
| 22:40:43 | mriedem | if it's set, it's not None | |
| 22:40:49 | mriedem | and cannot be set to None | |
| 22:41:02 | fried_rice | (Whether it is or not, I've learned not to trust my intuition when it comes to db objs and ovos) | |
| 22:41:18 | mriedem | you're usually pretty safe unless it's RequestSpec | |
| 22:41:23 | mriedem | then you're in for a world of pain | |
| 22:41:27 | fried_rice | okay, so it's always legal to say flavor.extra_specs.get('some_key') and not have to dork with checking whether extra_specs is a) present, b) a dict? | |
| 22:41:38 | mriedem | it's alwasy a dict | |
| 22:41:45 | fried_rice | ight. | |
| 22:41:47 | mriedem | in runtime code it should be present | |
| 22:43:00 | mriedem | yeah this is wonky https://review.openstack.org/#/c/631243/8/nova/objects/flavor.py@231 | |
| 22:43:14 | mriedem | that could just be self.extra_specs.get('accel:device_profile') | |
| 22:43:16 | fried_rice | yeah, that's what I'm ripping on. | |
| 22:43:21 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add detection of SEV support from QEMU/AMD-SP/libvirt on AMD hosts https://review.openstack.org/633855 | |
| 22:44:35 | aspiers | fried_rice: thanks for the recheck - is the dsvm-nova gate broken, or is my code? ... or both? X-D | |
| 22:44:51 | aspiers | I can't get through to the NSX CI logs | |
| 22:44:58 | fried_rice | aspiers: hanging on by one rusty hinge | |
| 22:45:02 | aspiers | hehe | |
| 22:45:21 | fried_rice | it's not your code | |
| 22:45:46 | fried_rice | if your code were a problem, the whole CI would be red | |
| 22:45:53 | openstackgerrit | Matt Riedemann proposed openstack/nova master: neutron: make migrate_instance_finish call setup_instance_network_on_host https://review.openstack.org/635343 | |
| 22:45:53 | fried_rice | (for that patch) | |
| 22:46:01 | aspiers | yeah I guess | |
| 22:46:04 | openstackgerrit | Matt Riedemann proposed openstack/nova master: neutron: make migrate_instance_finish call setup_instance_network_on_host https://review.openstack.org/635343 | |
| 22:47:36 | fried_rice | mriedem: Um, you said something about RequestSpec being problematic? | |
| 22:48:08 | fried_rice | You mean if I'm trying to do this on RequestSpec fields themselves, or if I'm trying to pull RequestSpec.flavor.extra_specs.get('something') ? | |
| 22:48:11 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Follow up (#2) for the bw resource provider series https://review.openstack.org/634767 | |
| 22:48:46 | mriedem | fried_rice: i'm just be facetious | |
| 22:48:56 | mriedem | see my request spec rant this morning | |
| 22:49:03 | fried_rice | emphasis on feces. | |
| 22:49:09 | mriedem | HI-O! | |
| 22:49:34 | fried_rice | so this is just using fake_spec_obj, which exists previously https://review.openstack.org/#/c/631243/8/nova/tests/unit/scheduler/test_device_profile.py | |
| 22:50:12 | fried_rice | but apparently trying to pull extra specs out of that with spec_obj.flavor.get('extra_specs') is blowing up. | |
| 22:50:15 | fried_rice | (in test.) | |
| 22:51:16 | mriedem | req_obj.flavor = fake_flavor.fake_flavor_obj(ctxt) | |
| 22:52:09 | mriedem | my guess is there is a problem in how that sets the extra_specs on that fake Flavor object | |
| 22:52:47 | mriedem | yup | |
| 22:52:57 | mriedem | 'extra_specs' needs to be in expected_attrs | |
| 22:53:25 | mriedem | https://github.com/openstack/nova/blob/master/nova/objects/flavor.py#L258 | |
| 22:53:40 | mriedem | extra_specs is a joined column so that fake_flavor_obj code is a bit misleading | |
| 22:54:09 | fried_rice | and that always happens in real life, so that test code ought to be updated accordingly? | |
| 22:54:15 | mriedem | yes | |
| 22:54:15 | mriedem | https://github.com/openstack/nova/blob/master/nova/tests/unit/fake_request_spec.py#L78 | |
| 22:54:24 | mriedem | should pass expected_attrs=['extra_specs'] | |
| 22:54:46 | mriedem | or fake_flavor_obj should just default to extra_specs if expected_attrs is empty | |
| 22:55:21 | mriedem | b/c that's what the get from db methods in the object do https://github.com/openstack/nova/blob/master/nova/objects/flavor.py#L378 | |
| 22:55:36 | fried_rice | yeah, this fixes: | |
| 22:55:36 | fried_rice | --- a/nova/tests/unit/fake_flavor.py | |
| 22:55:36 | fried_rice | +++ b/nova/tests/unit/fake_flavor.py | |
| 22:55:36 | fried_rice | @@ -50,7 +50,7 @@ def fake_db_flavor(**updates): | |
| 22:55:36 | fried_rice | ||
| 22:55:37 | fried_rice | ||
| 22:55:37 | fried_rice | def fake_flavor_obj(context, **updates): | |
| 22:55:38 | fried_rice | - expected_attrs = updates.pop('expected_attrs', None) | |
| 22:55:38 | fried_rice | + expected_attrs = updates.pop('expected_attrs', ['extra_specs']) | |
| 22:55:39 | fried_rice | return objects.Flavor._from_db_object(context, | |
| 22:55:39 | fried_rice | objects.Flavor(), fake_db_flavor(**updates), | |
| 22:55:40 | fried_rice | expected_attrs=expected_attrs) | |
| 22:55:46 | mriedem | ++ | |
| 23:06:48 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Share snapshot image membership with instance owner https://review.openstack.org/630769 | |
| 23:18:32 | mriedem | jroll: just a reminder that when https://review.openstack.org/#/c/635006/ is ready to go for wider review to throw it in the runways queue https://etherpad.openstack.org/p/nova-runways-stein | |
| 23:20:36 | aspiers | fried_rice, mriedem: was trying to understand the discussion around https://review.openstack.org/#/c/538498/ and it seems one point of debate was the corner case where a trait might need to be removed from a node, right? | |
| 23:20:51 | aspiers | fried_rice, mriedem: FWIW, that is not too unlikely with SEV | |
| 23:21:31 | fried_rice | aspiers: The basic idea is that there are some traits that are "owned" by compute, and any others are not. | |
| 23:21:36 | aspiers | It could be triggered by a downgrade of the kernel or QEMU or libvirt, or by disabling the sev option in the kvm_amd kernel module | |
| 23:21:50 | aspiers | fried_rice: yeah I got that, and I presume that the SEV trait would be owned by compute | |
| 23:22:01 | fried_rice | If a trait is owned by compute, you can't (un)set it as an operator or whatever. Next periodic will restore it to its "proper" (according to compute) value. | |
| 23:22:09 | aspiers | sure | |
| 23:22:24 | fried_rice | so if your trait is owned by compute, all you have to worry about is whether it needs to be set or not, and do that. | |
| 23:22:52 | fried_rice | If you need to be able to do that *without* compute service restarts, then I recommend doing it in update_provider_tree, not via a driver capability. | |
| 23:23:06 | aspiers | My point is that SEV requires support all the way up the stack, and if one link in the chain vanishes, then the trait would have to be removed. So that code under review does have to correctly handle that case, as you were saying in the discussion | |
| 23:23:28 | aspiers | I don't think it needs to happen without nova-compute restart | |
| 23:23:37 | aspiers | It's not gonna be a regular occurrence at all | |
| 23:23:51 | fried_rice | oh, you're worried about whether I'm right to -1 mriedem's patch?? Of *course* I am! | |
| 23:24:08 | fried_rice | I've *never* -1'd a mriedem patch and been *wrong* about it! | |