Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-04
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 mriedem so yes it should
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:42 melwitt is ConcurrentUpdate a base nova exception too?
20:31:47 cdent not any more
20:31:47 mriedem no
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: Delete port bindings in setup_networks_on_host if teardown=True https://review.openstack.org/556333
22:22:58 openstackgerrit Matt Riedemann proposed openstack/nova master: Add "activate_port_binding" neutron API method https://review.openstack.org/555947
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:22:59 openstackgerrit Matt Riedemann proposed openstack/nova master: Implement migrate_instance_start method for neutron https://review.openstack.org/556334
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:23:00 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Port binding based on events during live migration https://review.openstack.org/434870
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: WIP - Add a microversion for consumer generation https://review.openstack.org/558958
23:05:43 openstackgerrit Ed Leafe proposed openstack/nova master: Add handlers for consumer_generation https://review.openstack.org/557959
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
04:35:20 openstackgerrit Tetsuro Nakamura proposed openstack/nova master: Support shared and nested allocation candidates https://review.openstack.org/556514
04:53:45 openstackgerrit Takashi NATSUME proposed openstack/nova master: Remove mox in unit/virt/xenapi/test_vm_utils.py (2) https://review.openstack.org/558993
05:37:18 openstackgerrit Nakanishi Tomotaka proposed openstack/nova master: Test availability zone in multiple cells https://review.openstack.org/558998
05:38:55 openstackgerrit Tetsuro Nakamura proposed openstack/nova master: Return all resources in provider_summaries https://review.openstack.org/558045
07:04:45 openstackgerrit OpenStack Proposal Bot proposed openstack/nova master: Imported Translations from Zanata https://review.openstack.org/548772
07:57:49 openstackgerrit Tetsuro Nakamura proposed openstack/nova master: Check pinning support in NUMATopologyFilter https://review.openstack.org/531049

Earlier   Later