| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-20 | |||
| 12:13:26 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: placement: init alloc_reqs earlier https://review.openstack.org/485585 | |
| 12:16:05 | openstackgerrit | Alex Szarka proposed openstack/nova master: Add method for verify multiple versioned notifications https://review.openstack.org/465526 | |
| 12:20:50 | gibi | cdent: thanks for the comment on the patch. Let's wait for the others to wake up to discuss if this is the good fix for the problem | |
| 12:21:18 | cdent | yeah, that’s pretty much what I was trying to say in my comment, in too many words | |
| 12:21:29 | gibi | cdent: then I got your message :) | |
| 12:22:16 | openstackgerrit | Chris Dent proposed openstack/nova master: Add functional test for local delete allocations https://review.openstack.org/470578 | |
| 12:22:32 | openstackgerrit | Alex Szarka proposed openstack/nova master: Raise Exception instead of Exception method call https://review.openstack.org/482200 | |
| 12:26:14 | openstackgerrit | Merged openstack/nova master: Fix indentation in policy doc https://review.openstack.org/484646 | |
| 12:26:21 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: placement: init alloc_reqs earlier https://review.openstack.org/485585 | |
| 12:30:12 | openstackgerrit | Merged openstack/nova master: Only setup iptables for metadata if using nova-net https://review.openstack.org/480765 | |
| 12:31:11 | openstackgerrit | Merged openstack/nova master: Add log info in scheduler to mark start of scheduling https://review.openstack.org/481340 | |
| 12:32:01 | openstackgerrit | Merged openstack/nova master: Updated from global requirements https://review.openstack.org/485410 | |
| 12:32:50 | openstackgerrit | Merged openstack/nova master: VStorage: changed default log path https://review.openstack.org/458557 | |
| 12:33:51 | openstackgerrit | Chris Dent proposed openstack/nova master: Optional separate database for placement API https://review.openstack.org/362766 | |
| 12:49:04 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: List/show all server migration types (2/2) https://review.openstack.org/459483 | |
| 12:50:20 | openstackgerrit | Matthew Booth proposed openstack/nova master: Allow wrapping of closures https://review.openstack.org/479801 | |
| 12:50:21 | openstackgerrit | Matthew Booth proposed openstack/nova master: Use _error_out_instance_on_exception in finish_resize https://review.openstack.org/485601 | |
| 12:51:37 | openstackgerrit | Sean Dague proposed openstack/nova master: WIP: request_log addition for running under uwsgi https://review.openstack.org/485602 | |
| 13:01:04 | cdent | sdague: I wrote a couple of alternative ideas (to avoid paste changes) on ^. If it seems useful and you haven’t got the time I can probably look into tomorrow. | |
| 13:08:13 | mdbooth | dansmith: https://review.openstack.org/#/c/479802/ | |
| 13:08:56 | mdbooth | You wanted me to break it up, but as I put in a comment I don't think that works. | |
| 13:10:44 | cdent | gibi: if you learn something while I’m away, let me know | |
| 13:11:39 | mdbooth | dansmith: Anyway, working on it now so I'd really like to make sure I've understood you. | |
| 13:11:58 | openstackgerrit | iswarya vakati proposed openstack/nova master: Fixed wrap from taking negative values https://review.openstack.org/481465 | |
| 13:13:42 | dansmith | mdavidson: what doesn't work? | |
| 13:14:09 | dansmith | heh, mdbooth ^ | |
| 13:14:14 | bauzas | gibi: so, could you please tl:dr the problem about https://bugs.launchpad.net/nova/+bug/1705446 ? | |
| 13:14:15 | openstack | Launchpad bug 1705446 in OpenStack Compute (nova) "filter scheduler raises TypeError: argument of type 'NoneType' is not iterable when placement returns no allocation candidates" [High,In progress] - Assigned to Balazs Gibizer (balazs-gibizer) | |
| 13:16:49 | openstackgerrit | Alex Szarka proposed openstack/nova master: Transform the transformed notifications functional tests https://review.openstack.org/483448 | |
| 13:16:49 | gibi | bauzas: hi! so when placement returns no allocation candidates because there is no resource left, the scheduler/manager calls the scheduler drivers with None in alloc_reqs_by_rp_uuid | |
| 13:17:02 | gibi | bauzas: then the FilterScheduler tries to iterate on alloc_reqs_by_rp_uuid and blows up | |
| 13:17:29 | gibi | bauzas: see also https://review.openstack.org/#/c/485585/3/nova/scheduler/manager.py | |
| 13:18:17 | gibi | bauzas: alloc_reqs_by_rp_uuid onyl properly initialized in the else branch at line 147 | |
| 13:18:31 | mdbooth | dansmith: I think it's an atomic change | |
| 13:18:41 | openstackgerrit | Alex Szarka proposed openstack/nova master: Transform instance.exists notification https://review.openstack.org/403660 | |
| 13:18:54 | mdbooth | dansmith: I appreciate it's obtuse | |
| 13:19:14 | gibi | bauzas: as soon as I moved the initialization up to the top the FilterScheduler was happy again | |
| 13:19:21 | mdbooth | But being obtuse is the problem | |
| 13:20:03 | mdbooth | We've got this 1 thing which is there for no immediately discernable reason. After a mountaintop retreat, I have communed with the spirits and believe I have discerned a reason. | |
| 13:20:37 | mdbooth | If I remove it, I have to simultaneously replace its function, right? | |
| 13:21:19 | mdbooth | It looks like it's all over the place, but only because the code was previously all over the place. | |
| 13:21:43 | mdbooth | ...unless the spirits were lying to me, and I haven't correctly discerned its purpose. | |
| 13:21:54 | dansmith | the refactor of the decorator into a context manager does not have to be atomic with the rest of it (although I think I'd rather not even have that refactor, tbh) | |
| 13:22:17 | mdbooth | I could do that, but there would be no user of the context manager without the rest of it. | |
| 13:22:24 | dansmith | but that refactor has nothing to do with the removal of the two state exclusion right? | |
| 13:22:43 | mdbooth | Yeah, it does. The only reason I added a context manager is so I could reduce the scope. | |
| 13:23:18 | mdbooth | And the only reason I want to do that is to replace (my guest guess of) that exclusion block's purpose. | |
| 13:23:47 | dansmith | you can refactor it to a context manager first and then remove the state exclusion | |
| 13:23:50 | bauzas | gibi: sure, the point I had for the change adding that was about having different values | |
| 13:24:01 | mdbooth | dansmith: Yep, but the context manager wouldn't have a user. | |
| 13:24:08 | bauzas | gibi: if placement wasn't supported, returning None | |
| 13:24:20 | mdbooth | I guess I'm ok with that, but we just normally don't do it. | |
| 13:24:24 | dansmith | mdbooth: sure it would, the decorator | |
| 13:24:27 | dansmith | mdbooth: we do it constantly | |
| 13:24:33 | bauzas | gibi: if placement was supported but returning no candidates, then an empty dict | |
| 13:24:36 | mdbooth | Oh, ok | |
| 13:24:47 | dansmith | we do transitions as setup, migration, cleanup all the time | |
| 13:24:57 | bauzas | gibi: FWIW, your change is saying that if we don't have placement, it would return a None | |
| 13:24:59 | gibi | bauzas: sure that makes sense | |
| 13:25:00 | bauzas | oops | |
| 13:25:06 | bauzas | it would return {} | |
| 13:25:11 | mdbooth | Yeah, sure I can do that. Not sure it's worth it for a relatively small patch, but it's no great hassle. | |
| 13:25:13 | bauzas | while I was wanting a None | |
| 13:25:18 | gibi | bauzas: so you want to make a bit more finegrained fix | |
| 13:25:26 | dansmith | after the refactor, | |
| 13:25:36 | bauzas | gibi: https://review.openstack.org/#/c/485585/3/nova/tests/unit/scheduler/test_scheduler.py@135 tbc | |
| 13:25:39 | gibi | bauzas: where {} is returned only when we got empty allocations back | |
| 13:25:53 | dansmith | mdbooth: you can apply it to a smaller scope to avoid the race with the rpc call/notification stuff, | |
| 13:25:58 | mdbooth | dansmith: I thought you wanted me to split out the individual function changes. | |
| 13:26:02 | dansmith | and after that you can remove the state exclusion rght? | |
| 13:26:22 | bauzas | gibi: yup, that | |
| 13:26:24 | dansmith | mdbooth: well, I do, if possible | |
| 13:26:34 | gibi | bauzas: OK, I will rework the patch | |
| 13:26:36 | gibi | bauzas: thanks | |
| 13:26:37 | mdbooth | Those 2 really are the same issue. | |
| 13:26:50 | openstackgerrit | Alex Szarka proposed openstack/nova master: Transform instance.exists notification https://review.openstack.org/403660 | |
| 13:26:56 | dansmith | mdbooth: how? | |
| 13:27:00 | bauzas | gibi: thanks for working on it :) | |
| 13:27:22 | mdbooth | Because the exclusion is the current mechanism by which we reduce the scope of errors_out_migration | |
| 13:28:18 | mdbooth | If you do it lexically, the exclusion no longer has a user. | |
| 13:28:23 | dansmith | mdbooth: but your commit message says there's no reason for that code at all right? | |
| 13:28:35 | mdbooth | The exclusion code? | |
| 13:28:35 | gibi | bauzas: I have another placement question if you have a minute | |
| 13:28:57 | dansmith | mdbooth: the state exclusion yeah | |
| 13:29:00 | mdbooth | Its purpose is undocumented, and I've only guessed what it is | |
| 13:29:07 | bauzas | gibi: sure | |
| 13:29:15 | mdbooth | But I'm assuming that the person who put it there did it deliberately | |
| 13:29:39 | gibi | bauzas: when the scheduler claims the resources it is possible that there is a conflict | |
| 13:29:46 | mdbooth | It appears to be to prevent the migration being put in an error state after certain points in the task | |
| 13:29:55 | gibi | bauzas: I see that placement return http 409 | |
| 13:30:00 | gibi | bauzas: and that seems correct to me | |
| 13:30:15 | gibi | bauzas: I mean when there is really a conflict | |
| 13:30:22 | bauzas | gibi: yup, correct | |
| 13:30:24 | dansmith | mdbooth: right but I don't see how removing that (which you think needs to happen) is atomically related to the other scope change | |
| 13:30:36 | dansmith | mdbooth: I gotta join this terrible 6:30am call now, so do whatever you think is right I guess | |
| 13:30:47 | gibi | bauzas: but at the same time placement logs a ERROR log about the invalid inventory and that seems not that nice as it suggest a software error for me | |
| 13:31:07 | mdbooth | dansmith: Enjoy :) | |
| 13:31:22 | bauzas | gibi: when PUT /allocation ? | |
| 13:31:39 | bauzas | gibi: if so, yup, it seems weirdo to pass a error level log | |
| 13:32:18 | gibi | bauzas: here is the stack trace https://pastebin.com/cJP97xQr | |