| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-02 | |||
| 16:57:22 | melwitt | mdbooth: question in this unit test patch, what's the mis-named field? I didn't notice anything in this change or the one above it https://review.openstack.org/#/c/502126/3//COMMIT_MSG | |
| 16:59:10 | mdbooth | melwitt: IIRC the error was in the subsequent test | |
| 16:59:27 | mdbooth | But it wasn't picked up because it was just a mock | |
| 17:00:11 | melwitt | mdbooth: "subsequent test" meaning the test in the next patch right? | |
| 17:01:11 | mdbooth | melwitt: IIRC, yes? | |
| 17:01:27 | mdbooth | I've slept many times since writing that. Almost certainly yes. | |
| 17:01:46 | mdbooth | It doesn't really matter, though, because exactly which test it is doesn't matter. | |
| 17:02:06 | mdbooth | The point is that the way it was written allowed a test to assume the existence of a field which doesn't exist. | |
| 17:03:32 | melwitt | mdbooth: yeah, I get the point, this commit message is so vague there's no way to see or understand what the problem was, is all | |
| 17:03:53 | mdbooth | The problem is it's a Mock, which means anything goes. | |
| 17:04:09 | mdbooth | It could have been anything. Exactly what it was doesn't really matter. | |
| 17:04:11 | openstackgerrit | Jay Pipes proposed openstack/nova master: begin refactor AllocCandidates._get_by_filters() https://review.openstack.org/516778 | |
| 17:04:12 | openstackgerrit | Jay Pipes proposed openstack/nova master: build alloc request resources for shared resources https://review.openstack.org/516780 | |
| 17:04:12 | openstackgerrit | Jay Pipes proposed openstack/nova master: build ProviderSummary objects in sep function https://review.openstack.org/516779 | |
| 17:04:13 | openstackgerrit | Jay Pipes proposed openstack/nova master: finish refactor AllocCandidates._get_by_filters() https://review.openstack.org/516782 | |
| 17:04:13 | openstackgerrit | Jay Pipes proposed openstack/nova master: create allocation request for single provider https://review.openstack.org/516781 | |
| 17:04:14 | openstackgerrit | Jay Pipes proposed openstack/nova master: WIP: handle traits with sharing providers https://review.openstack.org/517119 | |
| 17:04:14 | openstackgerrit | Jay Pipes proposed openstack/nova master: required traits for no sharing providers https://review.openstack.org/517027 | |
| 17:04:17 | jaypipes | efried: k, done ^ | |
| 17:04:31 | efried | woot | |
| 17:04:32 | melwitt | mdbooth: yes, I get that. I wanted to see where a "mis-named field" was occuring for my own edification | |
| 17:06:26 | melwitt | I'll pull down the change and revert the bottom patch to see what was going on | |
| 17:07:11 | mdbooth | melwitt: It's really not worth it. | |
| 17:07:26 | mdbooth | melwitt: Couple of reasons: | |
| 17:07:39 | mdbooth | Firstly, it might not even have been that patch as I've reordered them a bunch of times | |
| 17:07:39 | melwitt | well, I want to learn what was going on, so I'm going to do it | |
| 17:07:46 | melwitt | since it doesn't say it in the commit message | |
| 17:07:56 | mdbooth | Secondly, I'll have fixed the issue already, so you won't see a failure regardless | |
| 17:08:24 | melwitt | well, we'll see | |
| 17:08:29 | mdbooth | So in your spelunking you're going to have to look for both the patch affected, and the revision of that patch which contained a bug which was picked up by functional but not unit testing | |
| 17:08:53 | mdbooth | And even when you know, it still doesn't matter, because it's just a general improvement anyway to not use a mock when you can use a real object | |
| 17:09:28 | mdbooth | Absolutely not worth the effort, imho | |
| 17:09:28 | melwitt | no, I'm just going to try reverting the bottom patch and maybe that'll show the problem. if not, then agreed | |
| 17:09:41 | mdbooth | It won't show the problem | |
| 17:09:48 | mdbooth | I'll most definitely have fixed the bug | |
| 17:10:26 | melwitt | that doesn't really make sense, you fixed the bug before changing to a real object? | |
| 17:10:42 | mdbooth | So, I wrote a unit test. | |
| 17:10:54 | mdbooth | In that test I accessed object.fo0 | |
| 17:11:06 | mdbooth | The test passed anyway, because object was a mock | |
| 17:11:16 | mdbooth | But functional testing failed because it didn't use a mock | |
| 17:11:27 | mdbooth | I found this and changed the unit test to also not use a mock | |
| 17:11:31 | mdbooth | But I also fixed the bug | |
| 17:11:34 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: adds REST API for nested providers https://review.openstack.org/384807 | |
| 17:11:35 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: update client to set parent provider https://review.openstack.org/385693 | |
| 17:11:51 | jaypipes | efried: ^ fixed tetsuro's niggle and the tree= param. | |
| 17:12:04 | efried | ack | |
| 17:12:07 | mdbooth | This is a preventative patch to ensure I can't write a unit test with a similar bug in the future | |
| 17:12:31 | mdbooth | We've spent too long on it already, lets just drop it. | |
| 17:12:34 | melwitt | mdbooth: oh, I see | |
| 17:23:55 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Numbered groupings to GET /allocation_candidates https://review.openstack.org/514092 | |
| 17:23:55 | openstackgerrit | Eric Fried proposed openstack/nova master: placement: Parse granular resources & traits https://review.openstack.org/514091 | |
| 17:23:56 | stephenfin | dansmith: This looks like something you care about https://review.openstack.org/#/c/517158/ | |
| 17:40:10 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove cells v2 transition code from update_instance https://review.openstack.org/467382 | |
| 17:40:10 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Cleanup update_instance cell mapping handling https://review.openstack.org/467380 | |
| 18:00:08 | openstackgerrit | Eric Fried proposed openstack/nova master: Parse granular resources/traits from extra_specs https://review.openstack.org/515151 | |
| 18:00:09 | openstackgerrit | Eric Fried proposed openstack/nova master: Granularize resources_from_{flavor|request_spec} https://review.openstack.org/515223 | |
| 18:00:10 | openstackgerrit | Eric Fried proposed openstack/nova master: ResourceRequest.to_querystring() https://review.openstack.org/515811 | |
| 18:00:34 | efried | jaypipes Both series are now fork-based on the common RequestGroup patch ^ ----^ | |
| 18:05:27 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: libvirt: do not remove inst_base when volume-backed during resize https://review.openstack.org/517388 | |
| 18:10:06 | jaypipes | efried: cool. reviewing now. | |
| 18:12:54 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/ocata: libvirt: do not remove inst_base when volume-backed during resize https://review.openstack.org/517390 | |
| 18:15:18 | openstackgerrit | Matt Riedemann proposed openstack/nova master: libvirt: Don't VIR_MIGRATE_NON_SHARED_INC without migrate_disks https://review.openstack.org/507202 | |
| 18:37:27 | mriedem | easy fix https://review.openstack.org/#/c/511459/ | |
| 18:54:42 | melwitt | mriedem: I noticed there are several other places in the file (in log calls) where the exception message isn't converted to unicode. is that because the log statement won't raise in that case? and the more important thing are the ones that are messages in exceptions we'll raise? | |
| 18:58:31 | mriedem | not sure, i thought six.text_type(ex) handled a lot of this for us, | |
| 18:58:37 | mriedem | but might have been a different type of error | |
| 18:58:59 | melwitt | oh, does LOG.x do text_type() underneath? | |
| 18:59:12 | mriedem | no | |
| 18:59:26 | openstackgerrit | Merged openstack/nova master: Implement power_off/power_on for the FakeDriver https://review.openstack.org/509935 | |
| 19:00:27 | mriedem | melwitt: e.g. https://github.com/openstack/nova/commit/73fcf4628089dd784889062e916b80d3fc9988a2#diff-d362bac3c354dd65e061ca91a8d1e14e | |
| 19:01:10 | melwitt | k. just curious | |
| 19:01:12 | mriedem | so might just be a case where we used to cast things using str() | |
| 19:01:26 | mriedem | which makes me nervous about https://github.com/openstack/nova/commit/8fa97d91c005147cd3eba7c6f03136ade84ba46d#diff-d362bac3c354dd65e061ca91a8d1e14e | |
| 19:02:07 | mriedem | which landed in queens, which means no one in asia has probably tried testing with that yet | |
| 19:03:21 | mriedem | heh, i guess that was in the run up to https://review.openstack.org/#/c/507202/ | |
| 19:03:28 | mriedem | but i don't see how it's related | |
| 19:04:36 | melwitt | oh, this part huh https://github.com/openstack/nova/commit/8fa97d91c005147cd3eba7c6f03136ade84ba46d#diff-ec64afccc111bbe81f5c85b27d73b41bR652 should be a to_unicode call? | |
| 19:05:03 | melwitt | er, the comment says should not be unicode. I guess only if it's a name or such | |
| 19:05:49 | mriedem | idk, i don't speak unicode | |
| 19:09:11 | mriedem | edleafe: http status code question | |
| 19:09:59 | mriedem | here is the scenario: | |
| 19:10:10 | mriedem | admin tries to migrate/evacuate an instance with a specified destination host, | |
| 19:10:32 | mriedem | for the dest host machinery to work, we need to put it into the *existing* request spec for the instance and pass that through api->conductor->scheduler | |
| 19:11:13 | mriedem | we have some compat code from newton where if the instance request spec isn't found, we silently create one (without the requested dest host) and pass it to the scheduler, and you might accidentally migrate the instance to what you specified, but it's definitely not intentional | |
| 19:11:45 | mriedem | so, if i were going to change that "if not request_spec: create one and fake it" thing to be a hard error in the API, is that a 400 response? i don't think 409 is correct because the user can't fix it. | |
| 19:20:32 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: config drive https://review.openstack.org/409404 | |
| 19:21:05 | edleafe | mriedem: so it wouldn't matter which host they specified? | |
| 19:21:13 | edleafe | mriedem: if not, then yeah, 400 | |
| 19:22:29 | mriedem | right, request 1 with host: 400: we don't have a request spec to do this, request 2 w/o host: cool, we pick a host for you as before | |
| 19:22:54 | mriedem | the 400 would have to say something about not being able to support migrating with a target host | |
| 19:22:59 | mriedem | as a hint you need to remove the host and retry | |
| 19:23:55 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: config drive https://review.openstack.org/409404 | |
| 19:25:35 | openstackgerrit | Merged openstack/nova master: Add 'done' to migration_get_in_progress_by_host_and_node filter https://review.openstack.org/511351 | |
| 19:25:59 | edleafe | Oh, so it *would* succeed if they modify the request | |
| 19:26:30 | mriedem | yeah, is that a 409 then? | |
| 19:26:34 | edleafe | Then I would go with the 409, with an explanation about not supporting a target host for that instance | |
| 19:26:38 | mriedem | 409: can't migrate with a target host | |
| 19:26:42 | mriedem | ok | |
| 19:27:21 | edleafe | Yeah, 400 usually means that the client request is malformed or otherwise invalid | |
| 19:29:09 | mriedem | bauzas: have we ever consider *not* persistent RequestSpec.requested_destination() | |