| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-20 | |||
| 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 | |
| 13:32:51 | openstackgerrit | Andrey Volkov proposed openstack/nova master: PoC: Select PCI devices with distinct tag values https://review.openstack.org/448008 | |
| 13:33:02 | gibi | bauzas: hm, it might not coming from the claiming | |
| 13:33:51 | gibi | bauzas: ahh I found it, it is a "PUT /placement/allocations/485c7480-939c-4b88-8c00-0f346dc6a924" | |
| 13:33:54 | gibi | bauzas: so yes | |
| 13:34:09 | gibi | bauzas: then I will file a bug for this as well | |
| 13:34:42 | gibi | bauzas: should this log be on info or debug level? | |
| 13:34:58 | bauzas | debug IMHO | |
| 13:35:11 | bauzas | because it just means that's a race condition | |
| 13:35:56 | gibi | bauzas: OK, thanks | |
| 13:37:05 | openstackgerrit | OpenStack Proposal Bot proposed openstack/nova master: Updated from global requirements https://review.openstack.org/485634 | |
| 13:39:30 | openstackgerrit | Alex Szarka proposed openstack/nova master: Transform instance.exists notification https://review.openstack.org/403660 | |
| 13:44:34 | gibi | bauzas: here is the bug report for the error log https://bugs.launchpad.net/nova/+bug/1705487 | |
| 13:44:37 | openstack | Launchpad bug 1705487 in OpenStack Compute (nova) "placement logs an ERROR when PUT /allocation result in an invalid inventory" [Undecided,New] | |
| 13:55:15 | ftersin | mdbooth: hi. If you have a minute, could you look at ScaleIO review (https://review.openstack.org/#/c/407440/)? I addressed your (and mriedem) comments there, and now ready for new ones. | |
| 14:05:50 | openstackgerrit | Radoslav Gerganov proposed openstack/nova master: VMware: serial console log (completed) https://review.openstack.org/450636 | |
| 14:08:06 | mriedem | andreykurilin: melwitt: we need to get https://review.openstack.org/#/c/484152/ in to unblock novaclient tests | |
| 14:08:17 | mriedem | now that the counting instance quotas change merged in nova | |
| 14:08:23 | mriedem | or sdague ^ | |
| 14:09:24 | openstackgerrit | Merged openstack/nova master: [placement] cover deleting standard trait https://review.openstack.org/484153 | |
| 14:09:33 | andreykurilin | mriedem: done | |
| 14:09:43 | mriedem | gibi: just wanted to say thanks for kicking the tires on the scheduler + placement stuff, you're finding some nice hairy bugs | |
| 14:09:45 | mriedem | andreykurilin: thanks | |
| 14:11:46 | openstackgerrit | Alex Szarka proposed openstack/nova master: Transform instance.exists notification https://review.openstack.org/403660 | |