| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-19 | |||
| 14:30:54 | kashyap | (Don't know the root cause of it yet; could be QEMU, could be libvirt. There are 2 other bugs filed for them - https://bugzilla.redhat.com/show_bug.cgi?id=1367620) | |
| 14:31:24 | mriedem | ^ is libvirt 1.3.1 and qemu 2.5 | |
| 14:31:29 | mriedem | we're way newer than that on queens | |
| 14:31:37 | kashyap | Yeah, saw the versions earlier in the day | |
| 14:31:55 | kashyap | Is it worth it to use UCA in that case? Maybe not, for these rare one-off cases | |
| 14:32:13 | mriedem | not at this point for newton | |
| 14:32:31 | kashyap | Yep, noted. | |
| 14:32:47 | mriedem | bauzas: this is a regression introduced in newton https://review.openstack.org/#/c/528835/ - would be good to get your review on that | |
| 14:33:12 | openstackgerrit | Bernhard M. Wiedemann proposed openstack/nova master: Fix 4 doc typos https://review.openstack.org/529084 | |
| 14:35:41 | bauzas | mriedem: ack, looking | |
| 14:36:29 | bauzas | mriedem: ah, good call | |
| 14:37:22 | bauzas | I remember we had a shit number of races for the BuildRequest object | |
| 14:38:48 | edleafe | mriedem: about the func test failure: this line should de-allocate against the instance: https://review.openstack.org/#/c/511358/43/nova/compute/manager.py@1778 | |
| 14:38:57 | mriedem | right so what was added there in newton was just for novalidhost on the initial create | |
| 14:39:01 | mriedem | but didn't take into account reschedules | |
| 14:39:09 | mdbooth | mriedem: Any chance you could have another look at the BDM uuid patches? https://review.openstack.org/#/c/242602/25 and the following 2 are the ones which do the db modification. I addressed your review comments. | |
| 14:39:46 | bauzas | mriedem: the point is that we were not having cell conductors yet | |
| 14:39:50 | mriedem | edleafe: ah, well, that's a race :) | |
| 14:40:00 | mriedem | edleafe: we cast to build_instances *before* compute cleans up the allocations | |
| 14:40:32 | bauzas | mriedem: now that we reschedule per cell conductors, yes it's a problem | |
| 14:40:38 | mriedem | bauzas: you could still run newton in split MQ mode | |
| 14:40:43 | mriedem | and split db | |
| 14:40:45 | edleafe | mriedem: so it's only locked for build, not claim | |
| 14:40:45 | mriedem | i think anyway | |
| 14:41:01 | bauzas | mriedem: sure | |
| 14:41:21 | mriedem | edleafe: the lock in compute doesn't matter | |
| 14:41:30 | mriedem | compute rpc casts to conductor build_instances | |
| 14:41:36 | mriedem | and then goes to delete the allocation for the instance | |
| 14:42:11 | edleafe | mriedem: that's my point - it's only locking builds for that host | |
| 14:42:34 | mriedem | in fact, this could overwrite what conductor claims on the alternate if the timing window hits it just right | |
| 14:42:35 | edleafe | I'll move the allocation cleanup so it is run before the cast | |
| 14:42:56 | mriedem | edleafe: you can't just move it, | |
| 14:43:03 | mriedem | it's there for reschedules and any other kind of failure | |
| 14:43:44 | mriedem | build_results.RESCHEDULED) | |
| 14:43:44 | mriedem | fails = (build_results.FAILED, | |
| 14:43:44 | mriedem | edleafe: i think this: | |
| 14:43:45 | edleafe | mriedem: all of the other cleanups are in _do_build_and_run_instance() | |
| 14:43:50 | mriedem | becomes just build_results.FAILED | |
| 14:44:55 | mriedem | but if we change that then self._build_failed() won't get called... | |
| 14:45:00 | bauzas | mriedem: looking at http://www.voidspace.org.uk/python/mock/magicmock.html#mock.NonCallableMagicMock | |
| 14:45:16 | bauzas | mriedem: it means that we call it, then we would have an exception ? | |
| 14:45:20 | mriedem | bauzas: yes | |
| 14:45:34 | bauzas | interesting | |
| 14:45:41 | bauzas | I wasn't knowing it | |
| 14:45:50 | mriedem | edleafe: so if you're going to leave the cleanup in the compute, then i think we can only call https://review.openstack.org/#/c/511358/43/nova/compute/manager.py@1778 if result == build_results.FAILED in that block | |
| 14:45:58 | mriedem | because we still need to call self._build_failed() | |
| 14:46:19 | mriedem | and then *add* rt.reportclient.delete_allocation_for_instance(instance.uuid) right before we cast to build_instances | |
| 14:46:21 | mriedem | yeah? | |
| 14:46:32 | edleafe | mriedem: I can split the code running under that conditional so that the deallocation only runs for FAILED, but the rest runs for both | |
| 14:46:36 | bauzas | mriedem: any reason why you're not just using http://www.voidspace.org.uk/python/mock/mock.html#mock.Mock.called ? | |
| 14:46:57 | mriedem | bauzas: one less thing to do | |
| 14:47:01 | edleafe | yeah, that's where I was going to move it to. I'll just copy the call. | |
| 14:47:14 | mriedem | bauzas: NonCallableMock just does the thing i already want | |
| 14:47:17 | bauzas | I see | |
| 14:47:38 | bauzas | anway, I don't want to discuss about the pattern | |
| 14:47:58 | bauzas | my point is just that when reviewing the change, we need to understand that noncallablemock already supports that | |
| 14:48:12 | bauzas | without needing to verify the call count | |
| 14:48:30 | bauzas | less explicit, but interesting tho | |
| 14:48:33 | mriedem | we = you? | |
| 14:48:39 | mriedem | now you know :) | |
| 14:48:55 | mriedem | i expect to see it in all of your new tests now | |
| 14:49:25 | bauzas | heh | |
| 14:50:49 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.59 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 15:00:54 | openstackgerrit | Merged openstack/nova stable/newton: Make request_spec.spec MediumText https://review.openstack.org/528338 | |
| 15:02:20 | mriedem | huh https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:stable/newton | |
| 15:02:25 | mriedem | to eol or not to eol | |
| 15:02:26 | lyarwood | \o/ | |
| 15:02:53 | mriedem | i worry about not having https://review.openstack.org/#/c/528835/ in newton | |
| 15:03:24 | mriedem | but i also don't know how many people running newton are going to have conductor split out yet and not have the cell conductor configured to hit the api db | |
| 15:03:35 | mriedem | as bauzas noted, probably not a real worry | |
| 15:06:17 | mriedem | unrelated, i also started thinking about https://review.openstack.org/#/q/topic:fix-bfv-boot-resources+(status:open+OR+status:merged) again... | |
| 15:06:42 | mriedem | and whether or not we should just take on the debt since shared provider modeling is who knows how far off yet | |
| 15:13:25 | mriedem | no shame intended | |
| 15:13:57 | mriedem | it's that we put that off for a few releases because we were saying placement would fix the problem, and we haven't yet, and people (ops) ask for it at least once per cycle | |
| 15:14:02 | jaypipes | cdent shaming is indeed the best kind of shaming. second only to pug shaming. | |
| 15:14:56 | openstackgerrit | Merged openstack/nova master: [placement] Add x-openstack-request-id in API ref https://review.openstack.org/523007 | |
| 15:15:09 | cdent | mriedem: don't worry, I'll feel shame, even for things entirely outside my control and/or the result of perfectly reasonable decision making processes | |
| 15:15:19 | cdent | I may be part pug | |
| 15:29:30 | maciejjozefczyk | jaypipes: Hey :) I responded to your comment https://review.openstack.org/#/c/520024/ Could you please check it? Is it possible to discuss it when you'll check it? Maybe on Thursdays meeting? Thanks :) | |
| 15:30:09 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.59 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 15:38:03 | jaypipes | maciejjozefczyk: I should be able to get to that patch today, yes. | |
| 15:39:24 | maciejjozefczyk | jaypipes: thanks a lot :) | |
| 16:07:04 | mriedem | lyarwood: artom: bauzas: did we or did we not say that we needed a minor version bump on stable for the release with the schema migration? | |
| 16:07:54 | lyarwood | mriedem: we don't \need\ it for anything but I think we agreed it would be nice to have a minor version bump for this, yes. | |
| 16:11:14 | mriedem | ok here is ocata https://review.openstack.org/529100 | |
| 16:14:04 | mriedem | and newton: https://review.openstack.org/529102 | |
| 16:27:12 | openstackgerrit | Merged openstack/nova master: Updated from global requirements https://review.openstack.org/528881 | |
| 16:32:11 | mriedem | jaypipes: on maciejjozefczyk's patch, i'm assuming the shutdown instances thing is a problem because of _update_usage_from_instance which is called between the initial compute node update and the final one, | |
| 16:32:19 | mriedem | and _update_usage_from_instance calls self.stats.update_stats_for_instance(instance, is_removed_instance) | |
| 16:32:24 | mriedem | which looks at things like vm_sate | |
| 16:32:25 | mriedem | *state | |
| 16:32:57 | jaypipes | yeah | |
| 16:33:04 | mriedem | and calls _update_usage | |
| 16:33:33 | mriedem | i'm not sure wth cn.current_workload = self.stats.calculate_workload() is for | |
| 16:34:06 | mriedem | no filters use that, it's just for reporting out of the API i guess | |
| 16:34:12 | jaypipes | mriedem: switched my vote on it. | |
| 16:39:04 | openstackgerrit | Merged openstack/python-novaclient master: Updated from global requirements https://review.openstack.org/528911 | |
| 16:39:14 | mriedem | jaypipes: i think he still has changes to make | |
| 16:39:17 | mriedem | per my earlier review | |
| 16:39:44 | mriedem | in _check_for_nodes_rebalance | |