| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-11-15 | |||
| 16:10:01 | bauzas | but that's a good call, lemme check | |
| 16:10:08 | mriedem | this is why i said you don't really need unit testing on these types of commands, it should mostly, if not all, be functional test driven | |
| 16:10:30 | bauzas | that's what happens when you do three things at same time... | |
| 16:10:42 | bauzas | meeting, patching, discussing | |
| 16:11:14 | bauzas | gibi: I just run the functest now | |
| 16:11:28 | bauzas | hopefully it will get caught | |
| 16:12:00 | bauzas | oh, and FWIW, I was rushing on updating a new revision, I haven't used yet the COMPUTE_NODE trait or did the split | |
| 16:13:19 | openstackgerrit | Merged openstack/nova stable/train: Use admin neutron client to query ports for binding https://review.opendev.org/694013 | |
| 16:13:38 | bauzas | mriedem: I'm not opposed to have most of the logic being checked by functional tests, I just added a few unit tests for sanity :) | |
| 16:14:24 | gibi | bauzas: nova.tests.functional.test_nova_manage.TestNovaManagePlacementAudit.test_audit_orphaned_allocations_from_deleted_compute_evacuate failed for me locally now but with a different error than what I saw in the devstack | |
| 16:14:27 | mriedem | that's a slippery slope and bad habit to get into with commands like this | |
| 16:14:30 | mriedem | so just, don't do it | |
| 16:14:51 | gibi | bauzas: File "nova/cmd/manage.py", line 2927, in audit | |
| 16:14:51 | gibi | ctxt, placement, output, provider, delete) | |
| 16:14:51 | gibi | File "nova/cmd/manage.py", line 2781, in _check_orphaned_allocations_for_provider | |
| 16:14:55 | gibi | inst_uuids, mig_uuids = result | |
| 16:14:56 | bauzas | gibi: yeah, I just saw it | |
| 16:14:57 | gibi | TypeError: 'bool' object is not iterable | |
| 16:15:02 | bauzas | I'm on it | |
| 16:15:05 | gibi | bauzas: cool | |
| 16:15:35 | gibi | mriedem: do you mean sanity is slippery slope with nova manage cli ? :) | |
| 16:15:58 | bauzas | gibi: but for *some* reason, the functests should have caught the argument issue | |
| 16:16:04 | bauzas | (for the microversion) | |
| 16:16:11 | bauzas | I mean, the other issue is unrelateed | |
| 16:16:19 | bauzas | or a side effect | |
| 16:16:30 | bauzas | so, mriedem's point is legit | |
| 16:16:41 | bauzas | I'm leaking coverage here | |
| 16:17:30 | mriedem | gibi: heh, no, i just don't really trust unit tests in nova anymore for anything that involves more than one service or even more than a couple of methods interacting within the same service | |
| 16:17:38 | mriedem | it's too easy to mock things that become false positives | |
| 16:20:52 | bauzas | so the bug is fixed, but the argument issue isn't shown | |
| 16:26:27 | gibi | mriedem: I share your view regarding the unit tests in nova | |
| 16:27:16 | bauzas | 2019-11-15 17:26:12,996 INFO [placement.requestlog] 127.0.0.1 "GET /placement/resource_providers" status: 200 len: 840 microversion: 1.0 | |
| 16:27:21 | bauzas | magnifico ^ | |
| 16:27:39 | bauzas | the report client is blindly accepting the 'microversion' keyword | |
| 16:27:48 | bauzas | but just says "meh" to it | |
| 16:27:57 | bauzas | gibi: mriedem: ^ | |
| 16:28:40 | bauzas | I don't see how to catch this unless to mock and assert the call | |
| 16:28:50 | bauzas | which is nonsense | |
| 16:29:21 | gibi | bauzas: then something is wrong with the placement fixture we use in the functional test | |
| 16:29:45 | bauzas | and, with using the 'version' keyword : | |
| 16:29:52 | bauzas | 2019-11-15 17:29:16,087 INFO [placement.requestlog] 127.0.0.1 "GET /placement/resource_providers" status: 200 len: 1684 microversion: 1.14 | |
| 16:30:05 | bauzas | we probably kwargs | |
| 16:30:07 | mriedem | def get(self, url, **kwargs): is your problem | |
| 16:30:08 | bauzas | gibi: ^ | |
| 16:30:13 | bauzas | heh, this | |
| 16:30:13 | mriedem | like gibi said it's a bug in the fixture | |
| 16:30:41 | mriedem | SchedulerReportClient.get won't let you pass microversion= | |
| 16:30:42 | mriedem | def get(self, url, version=None, global_request_id=None): | |
| 16:30:43 | gibi | then it is a good time to fix the fixture | |
| 16:30:51 | gibi | :) | |
| 16:31:42 | bauzas | aaaaand I love to see a bug about some code with a few TODO(sbauza) around it... | |
| 16:33:02 | openstackgerrit | Stephen Finucane proposed openstack/nova master: functional: Change order of two classes https://review.opendev.org/689178 | |
| 16:33:03 | openstackgerrit | Stephen Finucane proposed openstack/nova master: functional: Rework '_delete_server' https://review.opendev.org/689179 | |
| 16:33:03 | openstackgerrit | Stephen Finucane proposed openstack/nova master: functional: Make '_wait_for_state_change' behave consistently https://review.opendev.org/689180 | |
| 16:33:04 | openstackgerrit | Stephen Finucane proposed openstack/nova master: functional: Unify '_wait_until_deleted' implementations https://review.opendev.org/689181 | |
| 16:33:04 | openstackgerrit | Stephen Finucane proposed openstack/nova master: functional: Make 'ServerTestBase' subclass 'InstanceHelperMixin' https://review.opendev.org/689182 | |
| 16:33:36 | mriedem | should just remove those todos in the placement fixture about passing a token, that's never going to happen if it hasn't happened by now | |
| 16:33:48 | mriedem | we should like, put a 3 year timer on todos in the code | |
| 16:34:05 | mriedem | "TODO(vishy): do this in grizzly" | |
| 16:34:28 | gibi | :) | |
| 16:36:06 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: Add a placement audit command https://review.opendev.org/670112 | |
| 16:38:06 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Remove 'adv-config', 'system-admin' subdocs https://review.opendev.org/684402 | |
| 16:38:06 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Replacing underscores with dashes https://review.opendev.org/685929 | |
| 16:38:07 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Strip '.rst' suffix https://review.opendev.org/687264 | |
| 16:42:43 | stephenfin | melwitt: If you have time before the end of the day, think you could hit this quotas doc patch? https://review.opendev.org/#/c/667165/ | |
| 16:42:53 | stephenfin | It's been around for a few months. Would be nice to close it out | |
| 16:43:09 | KeithMnemonic1 | hello all any chance to get some more reviews and a possible WF+1 on this one ? https://review.opendev.org/#/c/683008/ | |
| 16:43:30 | melwitt | stephenfin: that's a biggun. I'll try to get to it | |
| 16:44:06 | stephenfin | melwitt: Sure thing. tbh, I'd just forget whatever was there previously and read it like a new doc. If nothing stands out as totally wrong, it's gonna be better than what we have | |
| 16:44:09 | stephenfin | IMO, obv | |
| 16:44:51 | sean-k-mooney | KeithMnemonic1: i think lyarwood should proably review that | |
| 16:45:07 | melwitt | stephenfin: ok, I'll approach it with that in mind | |
| 16:47:45 | stephenfin | ta | |
| 16:48:12 | stephenfin | Have a good weekend, all o/ | |
| 16:49:02 | melwitt | happy weekend | |
| 16:51:26 | gibi | bauzas: another bug in https://review.opendev.org/#/c/670112/9/nova/cmd/manage.py@2695 this is also not caught by any functional test as that is green for me on PS9 | |
| 16:52:10 | bauzas | gibi: thanks | |
| 16:52:34 | gibi | bauzas: don't be confused by the line numbers in my stack trace. I had to cherry-pick your patch top of the nova master in my devstack | |
| 16:52:36 | melwitt | gibi: I dunno if you saw this but I attempted to do a TODO you mentioned in the NeutronFixture for fun https://review.opendev.org/693453 | |
| 16:52:47 | bauzas | gibi: ack no worries | |
| 16:53:29 | gibi | melwitt: yeah I think I saw it during the PTG but then I got distracted. Looking at it now. Thanks for picking that TODO up | |
| 16:53:53 | melwitt | gibi: don't thank me yet haha, I hope I didn't misunderstand | |
| 16:56:25 | bauzas | gibi: and I get why you catched the error and not my tests | |
| 16:56:53 | bauzas | gibi: that's because when I use the cache, I forget to use the cell mapping | |
| 16:57:13 | bauzas | so, unless you have more than one instance for the same compute, you don't have the problem | |
| 16:57:29 | bauzas | (and I only verify one instance... :) ) | |
| 16:59:00 | KeithMnemonic1 | thanks sean-k-mooney, lyarwood would you have time in the next few business days to look at it? | |
| 17:00:11 | KeithMnemonic1 | can someone please continue the reviews on mriedem patches for the "openstack list marker hang" https://review.opendev.org/#/c/690721/4 | |
| 17:01:46 | sean-k-mooney | KeithMnemonic1: lyarwood for what its worth im +0.5 on it. i dont know htat part of the code well enought to tell if all of the change form queens make sense but the conflcits are called out in the commit and it looks more or less correct | |
| 17:03:35 | gibi | bauzas: I have one instance in shutoff state on the compute | |
| 17:03:51 | bauzas | no worries, I'm just fixing the bug | |
| 17:03:55 | gibi | bauzas: sure | |
| 17:04:02 | bauzas | and thanks, I should actually test it | |
| 17:04:35 | bauzas | it's 6.04pm tho, so I'll just fix the bug and add a TODO in the commit msg for saying I need to add a new func test for it :) | |
| 17:04:39 | bauzas | ie. two instances :-) | |
| 17:04:49 | bauzas | s/add/modify | |
| 17:05:01 | gibi | bauzas: yeah, I will time out soon as well | |
| 17:05:13 | bauzas | actually, you know what ? I'm just gonna amend the test now | |
| 17:05:17 | bauzas | and see whether it's seen | |
| 17:17:35 | openstackgerrit | Merged openstack/nova stable/queens: Revert "openstack server create" to "nova boot" in nova docs https://review.opendev.org/693239 | |