| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-20 | |||
| 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 | |
| 14:18:45 | openstackgerrit | Kaitlin Farr proposed openstack/nova master: Remove deprecated keymgr code https://review.openstack.org/439855 | |
| 14:19:49 | gibi | mriedem: my pleasure :) | |
| 14:21:02 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: List/show all server migration types (2/2) https://review.openstack.org/459483 | |
| 14:21:56 | sdague | mriedem: lookin | |
| 14:22:09 | sdague | mriedem: ah, andreykurilin already snagged it | |
| 14:22:21 | sdague | mriedem: you got a few moments to think about path forward on request logging? | |
| 14:23:24 | mriedem | sure, although it's over my head | |
| 14:24:17 | sdague | ok, well do you get my concern that we're not logging it through the python subsystem any more? | |
| 14:24:39 | mriedem | yeah it's definitely a problem, and as noted i'm seeing weird formatting | |
| 14:24:48 | mriedem | like parts of the log message are chopped off | |
| 14:25:20 | sdague | mriedem: you see my follow ups? | |
| 14:25:28 | mriedem | reading those now | |
| 14:25:50 | sdague | ok, do that, then we can chat, so I don't repeat myself :) | |
| 14:28:03 | mriedem | sdague: wait, say that again | |
| 14:29:22 | mriedem | sdague: oh man ok so that debug log message starts here then http://logs.openstack.org/65/483565/4/check/gate-tempest-dsvm-py35-ubuntu-xenial/9921636/logs/screen-n-sch.txt.gz#_Jul_19_20_17_18_800467 | |