| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-08 | |||
| 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! | |
| 23:24:45 | fried_rice | aspiers: Are you looking to take over that patch and make it go? That would be appreciated, I think, by me & mriedem and others. | |
| 23:24:59 | aspiers | haha | |
| 23:25:38 | fried_rice | you're referring to https://review.openstack.org/#/c/538498/7/nova/compute/resource_tracker.py@962 ? | |
| 23:25:38 | aspiers | fried_rice: I like the idea in principal, but the reality is that I'm *hopelessly* new to all of this code so I worry it would take me far too long to figure out | |
| 23:26:38 | aspiers | yeah that was part of the discussion I was referring to | |
| 23:26:39 | fried_rice | aspiers: you're demonstrating an understanding that's certainly adequate to where you'd be capable of making this patch go. Matt and I would be available to advise if you got stuck. | |
| 23:26:56 | aspiers | fried_rice: maybe I'm just a good bluffer ;-) | |
| 23:27:19 | fried_rice | So yeah, I'm +2 on this patch if that condition is removed. (And also rebase, and also test fixups, I think) | |
| 23:27:34 | mriedem | the trick on that thing is really just functional testing the scenarios | |
| 23:27:54 | mriedem | 1. compute reports traits via driver capabilities, user removes them externally, run periodic and they come back | |
| 23:28:03 | mriedem | 2. user externally sets traits and compute periodic doesn't remove them | |
| 23:28:16 | mriedem | 3. driver reports traits in t0, then at t1 doesn't report the trait, so it's removedd | |
| 23:28:19 | mriedem | something like that | |
| 23:28:23 | fried_rice | ++ | |
| 23:28:51 | fried_rice | the patch already has a start on that stuff. | |
| 23:28:53 | mriedem | once the functional test scenarios are in place then the code should be pretty straight-forward | |
| 23:28:58 | aspiers | I see | |
| 23:28:59 | fried_rice | https://review.openstack.org/#/c/538498/7/nova/tests/functional/test_servers.py | |
| 23:29:05 | aspiers | yeah, looking at that | |
| 23:29:20 | mriedem | with that it's 5:30 and i need to shove off | |
| 23:29:30 | fried_rice | o/ | |
| 23:29:31 | mriedem | i hope y'all will do the same | |
| 23:29:42 | aspiers | it's 11:30pm here :-/ | |
| 23:29:44 | fried_rice | I'm going to rip apart Sundar's PoC a little more. | |
| 23:30:04 | fried_rice | Where in the build flow do neutron ports get bound? | |
| 23:30:20 | aspiers | it looks like it doesn't yet test the behaviour of a driver-reported trait being removed, right? | |
| 23:32:36 | fried_rice | aspiers: I think it only really tests that capabilities are converted to traits, stuffed onto the compute rp, and new custom ones are vivified in placement. | |
| 23:32:45 | fried_rice | that would be scenario 0 in mriedem's list above. | |
| 23:32:51 | aspiers | ok | |
| 23:32:59 | fried_rice | So 1, 2, and 3 all still need to be implemented. | |
| 23:33:24 | aspiers | got it | |
| 23:33:38 | fried_rice | I guess the existing code is mriedem's scenario 1 before the first comma. | |
| 23:36:35 | aspiers | I see | |
| #openstack-nova - 2019-02-09 | |||
| 03:00:54 | openstackgerrit | Merged openstack/nova stable/queens: Null out instance.availability_zone on shelve offload https://review.openstack.org/606155 | |
| 03:52:58 | openstackgerrit | Merged openstack/nova stable/queens: Remove stale pip-missing-reqs tox test https://review.openstack.org/628128 | |
| 03:57:55 | openstackgerrit | Merged openstack/nova stable/queens: Removing pip-missing-reqs from default tox jobs https://review.openstack.org/628956 | |
| 04:13:31 | openstackgerrit | Merged openstack/nova stable/queens: Don't call begin_detaching when detaching volume from shelved vm https://review.openstack.org/633962 | |
| 04:13:37 | openstackgerrit | Merged openstack/nova stable/queens: Send context.global_id on neutron calls https://review.openstack.org/627604 | |
| 04:13:44 | openstackgerrit | Merged openstack/nova stable/queens: Skip checking of target_dev for vhostuser https://review.openstack.org/634236 | |
| 06:56:19 | openstackgerrit | Merged openstack/nova master: Modify InstanceMappingList.get_not_deleted_by_cell_and_project() https://review.openstack.org/635120 | |