Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-10
12:58:04 efried Look at the signature of e2fsck here https://review.openstack.org/#/c/552242/12/nova/privsep/fs.py
12:58:16 efried accepts (image, flags='-fp')
12:58:51 claudiub sure
12:58:52 efried Then look at the first usage here: https://review.openstack.org/#/c/552242/12/nova/virt/xenapi/vm_utils.py
12:59:03 efried note extra kwarg check_exit_code
12:59:26 efried So I thinks to myself, I thinks, "Okay, let's autospec here: https://review.openstack.org/#/c/552242/12/nova/tests/unit/virt/xenapi/test_vm_utils.py"
12:59:39 efried ...but when I do that, the test still passes.
13:00:05 efried i.e. the autospec doesn't seem to be catching that extra kwarg.
13:00:55 claudiub i might be blind, but where are you autospecing it?
13:00:56 efried It's probably me being blind.
13:01:11 efried @mock.patch('...', autospec=True)
13:01:21 efried is that a legit way to do that?
13:02:02 claudiub i might be really blind as a bat then. but yeah, there's a reason why it passes
13:03:41 claudiub or, wait, that only aplies to object methods. hm. anyways, there is an issue with mock.patch autospec, which i've addressed in oslotest. what happened was that mock.patch's autospec didn't consume the self / cls argument of object / class methods
13:04:08 efried I remember that issue. But in this case there are no classes involved, are there?
13:04:09 claudiub it should be the case now, since it's just a function.
13:04:44 efried it's possible my venv has an old oslotest, lemme check...
13:04:57 claudiub can you check if the mock.patch autospec works as expected with this patch on top? https://review.openstack.org/#/c/470775/
13:04:59 efried finucannot: you around this week?
13:05:16 efried claudiub: okay.
13:05:27 finucannot efried: Yes, but I'm focused on getting the numa-aware-vswitch PoC out the door
13:05:41 stephenfin oops
13:05:50 claudiub that patch basically enforces the oslotest's mock.patch behaviour.
13:07:52 mriedem jianghuaw_: does the citrix xenserver CI have any multinode job to test live migration for this series? https://review.openstack.org/#/c/489451/
13:15:21 efried claudiub: Okay, first I upgraded oslotest in my venv (3.2.0 => 3.4.1). Then I patched in https://review.openstack.org/#/c/557923/ (which presumably also means I'm getting as-yet-unreleased oslotest whatever). Then I merged in https://review.openstack.org/#/c/470775/ with the patch in question.
13:15:28 efried claudiub: None of this yielded the expected failure.
13:16:09 claudiub interesting
13:16:21 claudiub i'll take a look today as well
13:16:44 claudiub but later on, I have a meeting soon, so I have to prepare for that. :)
13:17:01 claudiub but thanks for catching it. :)
13:18:18 openstack Launchpad bug 1698010 in OpenStack Compute (nova) "neutron-based instances should not use the nova-network 'dhcp_domain' option" [High,In progress] - Assigned to Stephen Finucane (stephenfinucane)
13:18:18 madhaviy mriedem: I am checking fix proposed for LP bug https://bugs.launchpad.net/nova/+bug/1698010, by stephenfin , is there any other way to avoid using dhcp_domain from nova.conf during config_drive metadata creation
13:19:08 efried claudiub: Ahcrap, I think I know what's happening.
13:19:28 efried The method in question is decorated with a thing that accepts *a, **k
13:19:56 claudiub oh, I see.
13:20:13 claudiub interesting. :)
13:20:25 efried sure would be nice to be able to get around that somehow. But that sounds like black magic to me.
13:21:01 claudiub also, just an fyi, there are still a few other cases in which autospec is not working properly, for example sqlalchemy tends to have decorators which inject arguments in to the call. can't really autospec that. :)
13:22:40 claudiub well, autospecs are almost useless for methods which have *args / **kwargs
13:23:08 claudiub not entirely, but still.
13:26:59 mriedem madhaviy: i don't remember the details of that, but i do remember that the proposed patch wasn't going to work per garyk's comments. i also seem to remember an openstack-dev ML thread about this, but don't recall those details either. i would have to go back and dig into all of this and load it up into my head, which i'm not going to do right now (busy with other stuff), so unless you can summarize it's going to have to wait.
13:29:10 mriedem i don't see any links to ML discussion in the patches though
13:31:31 mriedem this reminds me, i think it's very weird that the use_neutron config option is deprecated https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.use_neutron even though it's in our install guide and is required while we still have nova-network around
13:31:50 mriedem if anyone is going through their logs and sees a deprecation warning for using use_neutron, there isn't anything they can do about it
13:32:20 mriedem i think oslo.service or one of the oslo libraries even has a flag where you can force services to not start if they are using deprecated options, so you can flush those out in pre-prod
13:33:20 mriedem in other words, wouldn't it make more sense to *not* deprecate options required to run nova with neutron, until at least we've removed nova-network?
13:33:38 mriedem stephenfin: thoughts? ^
13:35:07 stephenfin mriedem: You can filter out those warnings if you want. The intention is "this warning currently exists but is going away soon". The reason it's going away is given in the message
13:35:32 stephenfin *this option currently exists
13:36:15 openstackgerrit Raoul Hidalgo Charman proposed openstack/nova master: Expose shutdown retry interval as config setting https://review.openstack.org/552483
13:37:00 efried claudiub: Yeah, I recognize that; what I'm asking for in this case is a way to signal that I want to get around the decorator and autospec the actual method underneath it.
13:38:58 cdent efried: not really possible, your method has been redefined
13:39:24 cdent the original form is sort of gone
13:39:24 efried cdent: Yeah, hence "black magic"
13:39:41 cdent that would be darker than black
13:41:07 lpetrut efried: you may be able to retrieve the decorated methods, we do it in a few cases to avoid lock decorators within unit tests: https://github.com/openstack/os-win/blob/e0d7032dfb042f56fd02a52796b186cd8d67d240/os_win/_utils.py#L82
13:41:16 mriedem madhaviy: here is that ML thread http://lists.openstack.org/pipermail/openstack-dev/2017-September/121762.html
13:41:38 efried lpetrut: ooooo
13:42:50 cdent efried, lpetrut: too dark
13:43:03 efried ima try it anyway
13:43:08 efried cdent: hold my soul
13:43:23 cdent efried: wouldn't it be better to extract the thing you want to test to an undecorated thing?
13:44:05 efried cdent: I tried that first. Because it's already extracted thusly. But the decorator itself makes the test suite freak out. (It's the privsep entrypoint)
13:44:21 cdent it's all a bit smelly to me (not your soul (but maybe?))
13:44:36 cdent but we already know how I feel about complexity in tests...
13:44:44 efried I'm trying to soften the blows I keep on dishing out to mikal
13:45:06 efried cdent: FYI: https://review.openstack.org/#/c/552242/
13:47:40 mriedem madhaviy: there is also a thread in the operators ML with some other options
13:51:28 claudiub efried: yeah, as lpetrut said, we're getting the undecorated method is some unit tests in os-win, but that would only be needed for decorators which has some sort of special behaviour (adds / injects new arguments). even if we do autospec the undecorated methods, there are still plenty of cases in which the methods expects some sort of key-value argument, something like:
13:51:32 madhaviy mriedem: thanks. But I do not see any conclusion out of this discussion. Can we get back dhcp_domain conf option (not to deprecate)
13:51:33 claudiub if kwargs.get("something"): then do something
13:51:47 claudiub I've seen this in some oslo libs.
13:52:57 mriedem madhaviy: it definitely sounds like the option shouldn't be deprecated
13:53:09 mriedem at least until we know what we're going to do with it
13:53:52 mriedem i thought there was general agreement to use the dns_domain from the neutron network if there was one, and fallback to using the nova dhcp_domain option otherwise, but someone in the ops list punched some holes in that idea, but i'm not sure how valid those holes are, or if they are specific to that person's deployment
13:53:57 efried claudiub: Yeah, I understand that. That's not the case here, though - there are no arbitrary args. In this case, in the course of the review (over many iterations) I saw with my eyeballs that the invocation was wrong, and wanted a way to enforce it via autospec. But the decorator kills doing that directly. Now I'm hacking at it trying to figure out how I would actually use the undecorated method to do what I want.
13:54:28 mriedem madhaviy: if there were going to be a nova/neutron forum session at the summit, i'd put that in the agenda since ops would be in the room
13:54:32 mriedem melwitt: ^
13:55:05 madhaviy mriedem: sure.
13:55:15 claudiub efried: well, instead of autospec=True, you can pass-in a function that has the signature you want to enforce?
13:55:50 efried claudiub: As a side_effect to the mock, you mean?
13:56:11 claudiub something like: @mock.patch('something', autospec=some_function_with_exactly_what_I_want)
13:56:38 claudiub I dunno if that works though.
13:56:50 efried That sort of defeats the purpose, which is to vet whether my (actual) method is being called with the args I expect. If my test is wrong (in that it uses the wrong args) and then I make some_function_with_exactly_what_I_want have those same (wrong) args, then I'm just as effed.
13:59:22 claudiub yeah, a bit, but at least you'll have tests that don't have autospecs with *args / **kwargs in their method signatures.
14:00:29 mriedem madhaviy: melwitt: added https://etherpad.openstack.org/p/YVR-nova-brainstorming
14:00:31 mriedem mlavalle: ^
14:01:51 openstackgerrit Matt Riedemann proposed openstack/nova stable/queens: libvirt: Allow to specify granular CPU feature flags https://review.openstack.org/559700
14:02:04 mlavalle mriedem: cool, thanks!
14:02:41 mriedem dansmith: ^ rev'ed kashyap's backport if you want to hit that
14:03:07 kashyap mriedem: Ah, thanks. Was duking around fixing the compat code
14:03:13 kashyap (After removing the version stuff)
14:03:45 kashyap s/removing/bumping/
14:03:46 dansmith mriedem: done thanks
14:04:15 madhaviy mriedem: thanks!
14:05:29 kashyap mriedem: For the 'pike' change, abandoning it, and doing a fresh cherry-pick from 'queens' is the correct way?
14:05:42 mriedem kashyap: the change id is the same so you don't need to abandon
14:05:50 mriedem just re-do the cherry pick locally
14:05:53 kashyap Ah, right
14:07:13 openstackgerrit Matt Riedemann proposed openstack/nova stable/queens: libvirt: Allow to specify granular CPU feature flags https://review.openstack.org/559700

Earlier   Later