| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-04 | |||
| 20:27:02 | mriedem | you can be a trailblazer here | |
| 20:27:19 | cdent | I was hoping to go to sleep instead | |
| 20:27:30 | mriedem | blaze that treasure trail in the morning | |
| 20:27:44 | mriedem | or lose sleep over it tonight :) | |
| 20:28:04 | cdent | I will lose sleep over trying to generate caring | |
| 20:29:00 | cdent | mriedem: so I can both think about it and not think about it, what is that you're hoping for here? | |
| 20:29:43 | mriedem | i'm -1 without a unit test | |
| 20:29:54 | mriedem | which can be dealt with whenever you feel like it i guess | |
| 20:30:00 | cdent | a test that confirms that the handler raises a 409 when it seens a ConcurrentUpdate, or that a ConcurrentUpdate happens when there is an increment generation failure that casues a ConcurrentUpdate | |
| 20:30:18 | mriedem | i'm fine with the former | |
| 20:30:20 | mriedem | seems easy enough | |
| 20:30:25 | cdent | one of the reasons we haven't done it in the past is because the answer to that ^ is unclear | |
| 20:30:35 | melwitt | +1 on the former | |
| 20:30:49 | mriedem | making sure we don't spew 500 out of the REST API is a simple enough thing to say is a good test | |
| 20:31:03 | cdent | hmmm. Would that even have caught this particular problem? | |
| 20:31:35 | mriedem | the exception moved, | |
| 20:31:37 | cdent | I suppose so, as the unit test itself would have had an import error when it tried to side effet | |
| 20:31:37 | mriedem | so yes it should | |
| 20:31:42 | melwitt | is ConcurrentUpdate a base nova exception too? | |
| 20:31:47 | mriedem | no | |
| 20:31:47 | cdent | not any more | |
| 20:31:48 | mriedem | it moved | |
| 20:31:58 | melwitt | okay, so you'd think it would blow up there | |
| 20:37:11 | melwitt | my concern is just let's patch that test coverage gap since we know it's there. so if that path breaks in the future, we'll catch it. whether that happens now or in a follow up is fine IMHO but I think it's worth doing | |
| 20:43:39 | cdent | I get the concern, my reluctance is mostly because we've done a good job of avoid mock madness in the tests associated with placement | |
| 20:43:59 | openstackgerrit | Merged openstack/nova master: network: add command to configure trusted mode for VFs https://review.openstack.org/458513 | |
| 20:44:06 | cdent | I even added https://review.openstack.org/#/c/557355/ | |
| 20:48:49 | melwitt | yeah, I understand. I do really like the gabbi testing of the placement APIs. I'm just not immediately seeing another way to cover this particular testing gap | |
| 20:49:01 | openstackgerrit | Tyler Blakeslee proposed openstack/nova master: Add __repr__ for NovaException https://review.openstack.org/555812 | |
| 21:01:40 | dansmith | mriedem: melwitt cells meeting? | |
| 21:03:20 | mriedem | oh yeah | |
| 21:09:56 | cdent | efried: on https://review.openstack.org/#/c/548249/ do you remember why you are catching DBDuplicateError around set_aggregates? | |
| 21:10:06 | efried | ... | |
| 21:11:57 | cdent | efried: s/Error/Entry. That exception is handled in set_aggregates but with a pass, so I'm wondering if there's something else. it's not clear | |
| 21:12:00 | efried | cdent: Because Jay via https://review.openstack.org/#/c/548249/6/nova/api/openstack/placement/handlers/aggregate.py@100 pointed me to https://github.com/openstack/nova/blob/master/nova/api/openstack/placement/handlers/inventory.py#L171-L174 which I copy/pasted. | |
| 21:13:33 | cdent | hrmm | |
| 21:14:47 | efried | cdent: Looking through a little bit, it's possible it's not necessary. | |
| 21:14:57 | cdent | will leave a NOTE next to it for the time being | |
| 21:15:04 | efried | ight. | |
| 21:23:38 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add nova-status check for ironic flavor migration https://review.openstack.org/527541 | |
| 21:34:33 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Fix incorrect exception import https://review.openstack.org/558916 | |
| 21:35:06 | cdent | that version adds a unit test, but does it by extracting the problematic code to its own method (as suggested by https://docs.openstack.org/nova/latest/contributor/placement.html#testing ) | |
| 21:42:25 | mriedem | melwitt: i've got a question in https://review.openstack.org/#/c/540258/ | |
| 21:42:33 | mriedem | you and dan might have already covered that months ago though | |
| 21:46:12 | mriedem | so for initial scheduling, it seems necessary to hit all cells since we don't know which one the scheduler is going to pick for the instances in the create request, but for operations on an existing instance, it seems we should only need to care about hosts that are in the same cell as the instance, since we don't support move operations across cells | |
| 22:22:58 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add "activate_port_binding" neutron API method https://review.openstack.org/555947 | |
| 22:22:58 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Delete port bindings in setup_networks_on_host if teardown=True https://review.openstack.org/556333 | |
| 22:22:59 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Implement migrate_instance_start method for neutron https://review.openstack.org/556334 | |
| 22:22:59 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: compute: use port binding extended API during live migration https://review.openstack.org/551371 | |
| 22:23:00 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Port binding based on events during live migration https://review.openstack.org/434870 | |
| 22:23:00 | openstackgerrit | Matt Riedemann proposed openstack/nova master: conductor: use port binding extended API in during live migrate https://review.openstack.org/522537 | |
| 22:24:16 | melwitt | mriedem: thanks, looking. it's true that at present we don't support migrate across cells and we restrict it via request_spec.requested_destination.cell in the conductor task. someday when we do migrate across cells, we'll not want to restrict it | |
| 22:27:32 | melwitt | I'm trying to see if similar could be done with the instance group hosts query, if we can rely on request_spec.requested_destination.cell to limit it and put a NOTE on it that will remind us to remove that along with the others if we get cross-cell migration down the road | |
| 22:28:51 | melwitt | checking if task.execute happens before or after the setup_instance_group call | |
| 22:32:18 | melwitt | oh, it's _in_ execute. so yeah looks like we could read requested_destination.cell to know to limit it | |
| 22:34:05 | melwitt | for live migrate we'd have to move the setup_instance_group call down after the requested cell is set | |
| 22:35:42 | mriedem | efried: edleafe: fyi https://review.openstack.org/#/c/556529/ | |
| 22:36:44 | mriedem | melwitt: so you mean from within the InstanceGroup.get_hosts() call, determine that you have a requested_destination.cell set and use it for the targeted context when doing InstanceList.get_by_filters? | |
| 22:36:51 | mriedem | or whatever the instance query method is, | |
| 22:37:18 | mriedem | you could do that, but you still have setup_instance_group doing a scatter/gather on the cells, so it would likely be redundant | |
| 22:39:45 | melwitt | mriedem: I was thinking in the setup_instance_group method, choose whether to scatter-gather based on whether request_spec.requested_destination.cell is set. (after making sure the setup_instance_group calls are moved until after .cell is set) | |
| 22:42:44 | mriedem | ah | |
| 22:42:46 | mriedem | yeah that might do it | |
| 22:43:38 | mriedem | one problem is InstanceGroup.get_hosts() doesn't take a context | |
| 22:43:51 | mriedem | but, | |
| 22:44:06 | mriedem | you could temporarily mutate it's _context to be the cell-targeted one from the RequestSpec | |
| 22:44:13 | mriedem | well... | |
| 22:44:23 | mriedem | that's not how this is called, it happens via reqspec.instance_group.hosts | |
| 22:45:21 | mriedem | suppose we don't need to mess with the objects at all, just control it in _get_instance_group_hosts_all_cells | |
| 22:46:26 | melwitt | maybe safest to target_cell(context, requested_destination.cell) as cctxt then obj_alternate_context(cctxt) or something | |
| 22:46:41 | mriedem | yeah that's probably cleanest | |
| 22:47:15 | mriedem | anywho, before you go changing anything, would probably also be good to run it by dan in the morrow in case i'm missing something with this | |
| 22:47:28 | melwitt | unless what you were saying earlier is that the context in setup_instance_group(context, request_spec) is already targeted | |
| 22:47:31 | melwitt | k, will do | |
| 22:47:40 | mriedem | i believe it is | |
| 22:47:42 | mriedem | in the move cases | |
| 22:48:08 | mriedem | so the regression i think you're trying to fix is for server create, | |
| 22:48:13 | openstackgerrit | Merged openstack/nova master: Noauth should also use request_id from compute_req_id.py https://review.openstack.org/555266 | |
| 22:48:14 | mriedem | but i'm not sure it's broken for move operations | |
| 22:48:28 | mriedem | kind of hard to tell without functional regression tests | |
| 22:49:06 | melwitt | yeah | |
| 22:49:07 | mriedem | i think all of those move operations use the @targets_cell decorator in conductor manager | |
| 22:49:13 | mriedem | well before we get to setup_instance_group | |
| 22:49:26 | melwitt | I see | |
| 23:05:42 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add columns for generation to consumers https://review.openstack.org/557958 | |
| 23:05:43 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add handlers for consumer_generation https://review.openstack.org/557959 | |
| 23:05:43 | openstackgerrit | Ed Leafe proposed openstack/nova master: WIP - Add a microversion for consumer generation https://review.openstack.org/558958 | |
| 23:22:55 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: List/show all server migration types (1/2) https://review.openstack.org/430608 | |
| 23:23:57 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: List/show all server migration types (2/2) https://review.openstack.org/459483 | |
| 23:25:15 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Fix 500 error while passing 4-byte unicode data https://review.openstack.org/407514 | |
| 23:25:43 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Transform aggregate.update_metadata notification https://review.openstack.org/460625 | |
| 23:26:01 | openstackgerrit | Takashi NATSUME proposed openstack/nova-specs master: Change a validation in creating a server group https://review.openstack.org/546484 | |
| 23:26:18 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Adds view builders for keypairs controller https://review.openstack.org/347289 | |
| 23:28:35 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: api-ref: Parameter verification for servers.inc (2/3) https://review.openstack.org/550648 | |
| 23:28:49 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: api-ref: Parameter verification for servers.inc (3/3) https://review.openstack.org/550659 | |
| 23:29:06 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: api-ref: Fix parameter order in rebuild https://review.openstack.org/529718 | |
| 23:29:34 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: api-ref: Example verification for servers.inc https://review.openstack.org/529520 | |
| #openstack-nova - 2018-04-05 | |||
| 02:29:36 | openstackgerrit | Merged openstack/nova master: Default to py3 for the pep8 tox env because it's stricter https://review.openstack.org/558648 | |
| 03:44:33 | openstackgerrit | Merged openstack/nova master: Fix nits in update_provider_tree series https://review.openstack.org/531260 | |
| 04:35:19 | openstackgerrit | Tetsuro Nakamura proposed openstack/nova master: Consider nested RPs in get_all_with_shared https://review.openstack.org/556450 | |