| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-30 | |||
| 17:12:56 | openstackgerrit | Merged openstack/nova master: Fix comments in get_all_with_shared() https://review.openstack.org/533195 | |
| 17:13:03 | openstackgerrit | Merged openstack/nova stable/pike: Add --by-service to discover_hosts https://review.openstack.org/554603 | |
| 17:13:14 | openstackgerrit | Merged openstack/nova master: Add AggregateList.get_by_metadata() query method https://review.openstack.org/544728 | |
| 17:16:32 | fried_bunny | claudiub|2, edmondsw: It's specifically this part that breaks us: | |
| 17:16:33 | fried_bunny | When I comment out *just* that fixture, we're fine. | |
| 17:16:33 | fried_bunny | https://review.openstack.org/#/c/447505/44/nova/test.py | |
| 17:16:44 | fried_bunny | Which isn't surprising; that's the only part that could have possibly affected us OOT. | |
| 17:16:50 | fried_bunny | So - what IS that thing? | |
| 17:20:16 | claudiub|2 | you mean what is that fixture? | |
| 17:20:19 | fried_bunny | yeah | |
| 17:20:36 | claudiub|2 | it adds the autospec argument to mock.Mock and mock.MagicMock | |
| 17:21:16 | claudiub|2 | which is even more strange, because I don't see how it could have affected this | |
| 17:22:28 | fried_bunny | claudiub|2: Well, we're inheriting nova.test.TestCase, so that's how we're picking up the change. That much is clear. | |
| 17:22:35 | claudiub|2 | since I don't see autospecs being used anywhere | |
| 17:23:23 | fried_bunny | claudiub|2: By "it adds the autospec argument" do you mean that it just makes it available, not that it actually sets it to anything? | |
| 17:24:28 | claudiub|2 | yep | |
| 17:24:41 | claudiub|2 | if autospec is not specifically given, it is noop | |
| 17:24:51 | claudiub|2 | question, do you split by underscore somewhere? | |
| 17:25:00 | fried_bunny | oh, I'm sure we must | |
| 17:25:15 | claudiub|2 | oh | |
| 17:25:18 | claudiub|2 | that might be it | |
| 17:25:45 | claudiub|2 | yeah, I think I know what's happening | |
| 17:25:46 | fried_bunny | ...not seeing it though. | |
| 17:25:58 | fried_bunny | okay, do tell. Cause I'm stumped. Mystified. Stupefied. | |
| 17:27:48 | claudiub|2 | so, you're using a lot of thing.uuid, or thing.name and so on. so, if that property is not set on the mock, something like this is printed: str(mock_thing.foo) -> <_AutospecMock name='mock.foo' id='139796718268752'> | |
| 17:28:23 | claudiub|2 | the mock class with autospec is called _AutospecMock | |
| 17:28:57 | claudiub|2 | ... and since you're splitting the wrappers by _, it's split in 2 places | |
| 17:29:21 | claudiub|2 | because your wrappers are called wrapper_<_AutospecMock name=...> | |
| 17:29:32 | fried_bunny | claudiub|2: What if I don't see any splits anywhere? | |
| 17:29:47 | fried_bunny | ...by underscore | |
| 17:30:07 | claudiub|2 | you sure? I think I saw one | |
| 17:30:41 | claudiub|2 | yeh | |
| 17:30:43 | claudiub|2 | there is | |
| 17:31:27 | claudiub|2 | pypowervm/utils/transaction.py L 744 | |
| 17:31:37 | claudiub|2 | you're doing a rsplit | |
| 17:31:47 | claudiub|2 | ehm, why rsplit? and not lsplit? | |
| 17:33:02 | claudiub|2 | ehm, rsplit is just split. :)) | |
| 17:33:08 | fried_bunny | oy vay, my search was broken. I see it now. | |
| 17:33:13 | fried_bunny | claudiub|2: I don't think so... | |
| 17:33:51 | fried_bunny | Out[3]: ['one', 'two_three'] | |
| 17:33:51 | fried_bunny | In [3]: 'one_two_three'.split('_', 1) | |
| 17:33:51 | fried_bunny | Out[2]: ['one_two', 'three'] | |
| 17:33:51 | fried_bunny | In [2]: 'one_two_three'.rsplit('_', 1) | |
| 17:33:51 | fried_bunny | claudiub|2: | |
| 17:34:00 | openstackgerrit | Merged openstack/nova master: Add require_tenant_aggregate request filter https://review.openstack.org/545002 | |
| 17:34:05 | fried_bunny | claudiub|2: It matters when limiting the split | |
| 17:34:09 | openstackgerrit | Merged openstack/nova master: Documentation for tenant isolation with placement https://review.openstack.org/557490 | |
| 17:34:42 | claudiub|2 | right, anyways, the thing is, the underscore matters apparently | |
| 17:34:57 | claudiub|2 | I renamed _AutospecMock to AutospecMock and it worked | |
| 17:34:58 | fried_bunny | claudiub|2: And it's because IRL the label part can have underscores, and we want to preserve them. | |
| 17:35:34 | fried_bunny | claudiub|2: ...and the UUID doesn't have underscores. Unless it's a str(mock)ification, apparently :P | |
| 17:36:28 | claudiub|2 | indeed. :) | |
| 17:36:41 | claudiub|2 | well, ok, there are 2 solutions to the problem | |
| 17:37:06 | claudiub|2 | one is a bit of a PINTA, so I'll just submit the easier one, but it'll require a new oslotest release | |
| 17:37:35 | fried_bunny | claudiub|2: The other one is for us to go set our UUIDs in our mocks? | |
| 17:37:42 | fried_bunny | rather than letting them mock themselves | |
| 17:38:10 | claudiub|2 | the easier solution is to rename _AutospecMock and _AutospecMagicMock in oslotest, basically remove that _. I've put that _ there so it would discourage people for using them directly, but meh. | |
| 17:38:23 | claudiub|2 | and the other one is as you say | |
| 17:40:50 | fried_bunny | claudiub|2: Okay, please add me to that review. | |
| 17:40:58 | fried_bunny | ...and I'll go see if I can find those UUIDs... | |
| 17:43:02 | claudiub|2 | fried_bunny: added: https://review.openstack.org/#/c/557974/ | |
| 17:43:23 | claudiub|2 | can you run the UT with this patch applied? | |
| 17:43:49 | fried_bunny | uhm, I think by cloning the project into my tox env... | |
| 17:43:51 | fried_bunny | stand by | |
| 17:44:21 | claudiub|2 | yeah. it seems to work for me. Want to double check. | |
| 17:44:58 | fried_bunny | claudiub|2: Are you running tox on nova-powervm? | |
| 17:45:29 | claudiub|2 | I ran an individual test: python -m unittest nova_powervm.tests.virt.powervm.disk.test_ssp.TestSSPDiskAdapter.test_connect_disk | |
| 17:45:37 | fried_bunny | ight | |
| 17:45:38 | claudiub|2 | this was one of the failures | |
| 17:45:46 | fried_bunny | I'll run the whole suite, sec. | |
| 17:45:53 | claudiub|2 | yeap, please do | |
| 17:46:23 | fried_bunny | claudiub|2: Is this a LP bug thing, or no? | |
| 17:46:28 | openstackgerrit | Merged openstack/nova stable/pike: Re-use existing ComputeNode on ironic rebalance https://review.openstack.org/527423 | |
| 17:46:34 | openstackgerrit | Merged openstack/nova stable/pike: Do not set allocation.id in AllocationList.create_all() https://review.openstack.org/530794 | |
| 17:46:40 | openstackgerrit | Merged openstack/nova stable/pike: Don't launch guestfs in a thread pool if guestfs.debug is enabled https://review.openstack.org/539541 | |
| 17:46:47 | openstackgerrit | Merged openstack/nova stable/pike: unquiesce instance on volume snapshot failure https://review.openstack.org/545966 | |
| 17:46:53 | fried_bunny | claudiub|2, edmondsw: Confirmed, that delta fixes us completely. | |
| 17:46:54 | claudiub|2 | I'll amend the commit message and add a Related-Bug: #the_mock_autospec_LP_bug_id | |
| 17:47:11 | fried_bunny | roger that. | |
| 17:47:20 | claudiub|2 | ok, then i'll amend the commit message | |
| 17:47:58 | claudiub|2 | gonna push for it to merge, and ask for a new oslotest release next week. they don't release at the end of the week. | |
| 17:48:49 | claudiub|2 | sorry for the trouble. :) | |
| 17:49:06 | fried_bunny | claudiub|2: No worries; that was pretty unanticipatable. Thank you very much for the help. | |
| 17:55:55 | claudiub|2 | updated commit message | |
| 17:56:03 | edmondsw | claudiub|2 good catch and thanks | |
| 18:13:53 | fried_bunny | Eric Fried proposed openstack/nova-powervm master: Specify UUIDs in VIOS mocks for FeedTasks https://review.openstack.org/557975 | |
| 18:13:53 | fried_bunny | claudiub|2: Finding those mocked attrs wasn't as hard as I thought. | |
| 18:14:19 | fried_bunny | If you want to pull your patch, I think I'd be okay with that. (edmondsw, thoughts?) | |
| 18:15:39 | edmondsw | might still be a good idea to keep claudiub's patch so that this weirdness doesn't crop up in other places | |
| 18:16:05 | fried_bunny | I'm fine with that too. Weighed against the desire to keep those classes private, though - that's up to claudiub|2 I guess. | |
| 18:16:09 | mriedem | why are nova-powervm repo changes showing up in the nova channel? | |
| 18:16:21 | fried_bunny | mriedem: Because triggered by a nova change | |
| 18:16:34 | mriedem | ? | |
| 18:16:39 | mriedem | https://review.openstack.org/#/c/557975/ isn't nova | |
| 18:16:41 | fried_bunny | mriedem: Oh, that wasn't a bot, that was a copy/paste by me. | |
| 18:16:50 | mriedem | ah | |
| 18:16:51 | fried_bunny | thought you were going crazy | |
| 18:17:40 | fried_bunny | mriedem: We were discussing here because the trigger was https://review.openstack.org/#/c/447505/ | |
| 18:17:55 | mriedem | yar | |
| 18:18:14 | mriedem | i noticed an alarming amount of claudiub|2 in the channel | |