Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-11-15
16:00:48 lyarwood thanks for that mriedem
16:02:27 openstackgerrit Matt Riedemann proposed openstack/nova master: doc: mention that rescuing a volume-backed server is not supported https://review.opendev.org/694584
16:02:39 mriedem np
16:03:03 openstackgerrit Matt Riedemann proposed openstack/nova master: doc: mention that rescuing a volume-backed server is not supported https://review.opendev.org/694584
16:07:36 gibi bauzas: I hit a bug in https://review.opendev.org/#/c/670112/8/nova/cmd/manage.py@2867
16:08:10 mriedem gibi: you were +2 on this before it was rebased a bit in earlier changes https://review.opendev.org/#/c/642591/ - can you hit that again? dansmith - that's also the one where you suggested re-doing the logic so it should be simple and familiar
16:08:21 bauzas gibi: graaah ok, thanks !
16:08:44 gibi mriedem: on it.
16:08:46 bauzas gibi: just respinning, sec
16:09:02 gibi bauzas: sure, I will retry with the new ps
16:09:10 mriedem bauzas: why wouldn't your functional tests hit that?
16:09:33 bauzas mriedem: the CI is still in the weeds
16:09:37 bauzas no result yet
16:09:39 bauzas and I was lazy
16:09:44 mriedem bauzas: i meant locally...
16:09:45 mriedem yeah
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

Earlier   Later