| *** k0da has quit IRC | 00:30 | |
| *** mdrabe has quit IRC | 00:35 | |
| *** mdrabe has joined #openstack-powervm | 00:38 | |
| *** mdrabe has quit IRC | 00:45 | |
| *** mdrabe has joined #openstack-powervm | 00:48 | |
| *** mdrabe has quit IRC | 01:07 | |
| *** mdrabe has joined #openstack-powervm | 01:10 | |
| *** mdrabe has quit IRC | 01:22 | |
| *** mdrabe has joined #openstack-powervm | 01:26 | |
| *** mdrabe_ has joined #openstack-powervm | 02:03 | |
| *** mdrabe has quit IRC | 02:05 | |
| *** edmondsw has joined #openstack-powervm | 03:07 | |
| *** edmondsw has quit IRC | 03:11 | |
| *** mdrabe_ has quit IRC | 03:24 | |
| *** mdrabe has joined #openstack-powervm | 03:25 | |
| *** mdrabe has quit IRC | 03:32 | |
| *** mdrabe has joined #openstack-powervm | 03:34 | |
| *** mdrabe has quit IRC | 03:43 | |
| *** mdrabe has joined #openstack-powervm | 03:51 | |
| *** mdrabe has quit IRC | 04:05 | |
| *** mdrabe has joined #openstack-powervm | 04:07 | |
| *** mdrabe has quit IRC | 04:32 | |
| *** mdrabe has joined #openstack-powervm | 04:34 | |
| *** mdrabe has quit IRC | 04:44 | |
| *** mdrabe has joined #openstack-powervm | 04:50 | |
| *** mdrabe_ has joined #openstack-powervm | 05:17 | |
| *** mdrabe has quit IRC | 05:17 | |
| *** mdrabe_ has quit IRC | 05:22 | |
| *** mdrabe has joined #openstack-powervm | 05:22 | |
| *** mdrabe has quit IRC | 05:58 | |
| *** mdrabe has joined #openstack-powervm | 06:00 | |
| *** AlexeyAbashkin has joined #openstack-powervm | 07:51 | |
| *** k0da has joined #openstack-powervm | 08:28 | |
| *** catmando has quit IRC | 11:25 | |
| *** catmando has joined #openstack-powervm | 11:26 | |
| *** svenkat has joined #openstack-powervm | 13:13 | |
| *** edmondsw has joined #openstack-powervm | 13:15 | |
| *** edmondsw has quit IRC | 13:15 | |
| *** edmondsw_ has joined #openstack-powervm | 13:16 | |
| *** svenkat has quit IRC | 13:18 | |
| *** svenkat has joined #openstack-powervm | 13:28 | |
| *** edmondsw_ is now known as edmondsw | 13:37 | |
| *** apearson_ has joined #openstack-powervm | 13:44 | |
| *** mdrabe_ has joined #openstack-powervm | 14:17 | |
| *** mdrabe has quit IRC | 14:20 | |
| *** mdrabe_ is now known as mdrabe | 14:23 | |
| *** edmondsw has quit IRC | 14:38 | |
| *** svenkat has quit IRC | 14:39 | |
| *** edmondsw has joined #openstack-powervm | 14:52 | |
| *** svenkat has joined #openstack-powervm | 14:53 | |
| *** esberglu has joined #openstack-powervm | 14:59 | |
| *** tjakobs has joined #openstack-powervm | 15:01 | |
| openstackgerrit | Eric Berglund proposed openstack/nova-powervm master: Update UPPER_CONSTRAINTS_FILE for stable/queens https://review.openstack.org/543482 | 15:10 |
|---|---|---|
| openstackgerrit | Eric Berglund proposed openstack/networking-powervm master: Update UPPER_CONSTRAINTS_FILE for stable/queens https://review.openstack.org/543483 | 15:14 |
| efried | esberglu: What are you doing? | 15:15 |
| esberglu | efried: I'm not sure why, I have stable/queens checked out locally, but the git command pushed the tox changes to master | 15:16 |
| efried | ah | 15:16 |
| efried | esberglu: looking... | 15:16 |
| esberglu | efried: I'm sure I just messed something up | 15:17 |
| efried | esberglu: Yeah, no, I see 543482 tracked against master. | 15:17 |
| efried | oh, hm, now I don't, after a fetch. | 15:17 |
| efried | esberglu: When you push, are you saying `git review stable/queens` ? | 15:18 |
| esberglu | efried: I need to do the .gitreview change first probably | 15:21 |
| esberglu | Yeah | 15:21 |
| edmondsw | yeah, make the .gitreview change. | 15:22 |
| efried | eh? | 15:22 |
| esberglu | efried: https://review.openstack.org/#/c/542880/1 | 15:22 |
| esberglu | ^ need that in nova/net-powervm first | 15:22 |
| efried | When have we ever done that before? | 15:23 |
| efried | I don't see it in pike or ocata | 15:23 |
| efried | or newton or mitaka... | 15:23 |
| edmondsw | hmmm... we *should* have done that... | 15:23 |
| efried | Shrug, I see Nova does it. | 15:24 |
| edmondsw | it's just a convenience, though | 15:24 |
| efried | I guess just for conv... yeah | 15:24 |
| efried | ...so that esberglu doesn't have to remember to say stable/queens when he does his git review command. | 15:24 |
| edmondsw | :) | 15:24 |
| esberglu | Apparently I don't work on the stable branches much :) | 15:28 |
| edmondsw | esberglu do you have any idea where stable-ocata_a comes from vs. stable-ocata? here: https://readthedocs.org/projects/ceilometer-powervm/versions/ | 15:31 |
| edmondsw | I noticed neither were public. I'm going to mark one public so it can be seen... I assume stable-ocata | 15:32 |
| edmondsw | s/assume/hope/ | 15:32 |
| esberglu | edmondsw: Not sure what you're looking at? | 15:33 |
| edmondsw | edmondsw scroll down to inactive versions | 15:33 |
| edmondsw | esberglu ^ | 15:34 |
| esberglu | Not on my page | 15:34 |
| edmondsw | did you login? | 15:34 |
| esberglu | Oh lol | 15:34 |
| edmondsw | I'm also thinking we probably need to deactivate stable-newton and older and instead active *-eol | 15:37 |
| edmondsw | esberglu which brings another intesteresting question... there are a couple different liberty-eol there | 15:37 |
| edmondsw | oh, one uses underscore... I assume liberty-eol is the right one? | 15:38 |
| esberglu | edmondsw: No idea on the _a question | 15:40 |
| edmondsw | esberglu k, I'll just ignore them and activate the ones without the _a, hope they're the latest | 15:40 |
| esberglu | edmondsw: And yeah we should deactivate newton and earlier | 15:41 |
| edmondsw | esberglu doing that now | 15:41 |
| esberglu | edmondsw: And activate stable-ocata | 15:41 |
| edmondsw | done | 15:41 |
| edmondsw | esberglu there is a "wipe" link on these... I may try that on the _a and see if it removes them. I don't think we need them | 15:42 |
| edmondsw | esberglu whatever is creating the _a must still be there with queens... see https://readthedocs.org/projects/nova-powervm/versions/ | 15:47 |
| edmondsw | I guess I'll just leave them, they're not hurting anything | 15:48 |
| *** k0da has quit IRC | 16:40 | |
| esberglu | edmondsw: I think I finally figured out what is going wrong with that update_server_name tempest test | 17:19 |
| esberglu | https://github.com/openstack/nova/blob/master/nova/virt/powervm/tasks/storage.py#L200 | 17:20 |
| esberglu | We shouldn't be passing instance into that init | 17:20 |
| esberglu | Haven't tested the fix yet, but I'm pretty sure that's where the issue is coming from | 17:21 |
| efried | ooo | 17:22 |
| efried | nice | 17:22 |
| efried | esberglu: OOT doesn't have that | 17:23 |
| esberglu | efried: Yep | 17:24 |
| esberglu | I'm pretty sure that the all of the taskflow work happened while the Config Drive patch was sitting as WIP | 17:24 |
| efried | esberglu: While you're at it, name the freaking kwarg if you please. | 17:24 |
| esberglu | So it slipped through the cracks | 17:24 |
| esberglu | I'll check and make sure it wasn't missed anywhere else | 17:24 |
| efried | Had we been doing that all along as a habit, that never would have happened. | 17:24 |
| efried | Again, the OOT version is doing that properly. | 17:25 |
| efried | ...since 2015. Not sure how that slipped through. | 17:25 |
| *** AlexeyAbashkin has quit IRC | 17:36 | |
| edmondsw | esberglul why would update_server_name involve DeleteVOpt? | 17:58 |
| esberglu | edmondsw: The test isn't actually failing, it's deleting the server afterwards | 17:58 |
| edmondsw | that's a good catch | 17:58 |
| esberglu | that is failing | 17:58 |
| edmondsw | ah, ok | 17:58 |
| esberglu | So we're passing in instance as the task name | 17:59 |
| esberglu | Which has some non-ascii stuff in it from the test | 17:59 |
| edmondsw | open a LP bug | 17:59 |
| edmondsw | and we can maybe get this fixed for rc2 | 17:59 |
| efried | That's backport-worthy. | 18:00 |
| efried | Yeah | 18:00 |
| efried | Did it just land in Q? | 18:00 |
| esberglu | efried: Yeah | 18:00 |
| edmondsw | efried yep, if they don't take it for rc2 we can backport | 18:00 |
| edmondsw | esberglu so does the server delete work but leave the VOPt behind? | 18:01 |
| edmondsw | cuz surely there are tests to make sure server delete works... | 18:02 |
| edmondsw | how did we not have a UT fail? | 18:02 |
| esberglu | edmondsw: This only fails because of the non-ascii rename | 18:02 |
| edmondsw | that will need to be part of the fix | 18:02 |
| edmondsw | why would it only be an issue for non-ascii? | 18:02 |
| edmondsw | I'm missing something | 18:03 |
| esberglu | DeleteVopt is passing in the full instance as the task name. Which is weird but doesn't actually break anything | 18:03 |
| esberglu | It's just a string representation of the instance | 18:03 |
| esberglu | But when there is non-ascii in that string it breaks the taskflow compilation | 18:04 |
| edmondsw | ah | 18:04 |
| esberglu | edmondsw: http://184.172.12.213/17/535517/21/check/nova-in-tree-pvm/fedbf46/logs/n-cpu.txt.gz#_Feb_08_12_17_49_639756 | 18:05 |
| esberglu | You can see what I mean there | 18:05 |
| edmondsw | yeah, I got it | 18:05 |
| edmondsw | esberglu I assume you'll fix the other places as well, e.g. https://github.com/openstack/nova/blob/master/nova/virt/powervm/tasks/storage.py#L160 | 18:06 |
| esberglu | edmondsw: Yep | 18:06 |
| edmondsw | tx | 18:07 |
| edmondsw | good catch | 18:07 |
| esberglu | efried: edmondsw: Remind me how backports work right now. Is the fix supposed to merge to master before being propsed to queens? | 18:18 |
| edmondsw | yes | 18:19 |
| edmondsw | once it merges to master, you cherry-pick it to stable/queens | 18:20 |
| esberglu | edmondsw: Yeah just wasn't sure if I had to wait to cherry-pick or not. Tx | 18:20 |
| edmondsw | I don't know that you have to wait to propose it, but you do have to wait to merge it | 18:21 |
| edmondsw | so may make sense to wait on proposal... not sure on etiquette there | 18:21 |
| edmondsw | I usually wait | 18:21 |
| *** k0da has joined #openstack-powervm | 18:33 | |
| *** k0da has quit IRC | 18:58 | |
| *** apearson_ has quit IRC | 18:58 | |
| esberglu | edmondsw: We aren't mocking taskflow, we are executing the tasks, not sure how autospec would help here | 19:20 |
| *** k0da has joined #openstack-powervm | 19:32 | |
| edmondsw | esberglu looking | 19:34 |
| edmondsw | esberglu yeah looks like we've got tests that mock tasks, but at the execute method level instead of the class level | 19:35 |
| *** k0da has quit IRC | 19:39 | |
| efried | autospec wouldn't help. | 19:54 |
| efried | I mean, you could add a bug-specific test by creating an instance with deliberately non-ascii chars in the name and making sure you can delete its VOpt. | 19:54 |
| efried | But that seems really narrow. | 19:55 |
| *** apearson_ has joined #openstack-powervm | 20:05 | |
| *** svenkat_ has joined #openstack-powervm | 20:09 | |
| *** svenkat has quit IRC | 20:10 | |
| *** svenkat_ has quit IRC | 20:13 | |
| esberglu | edmondsw: Tried a couple variations on what you sent me in slack, haven't got them working yet | 20:17 |
| esberglu | I've got to relocate quick, will pick back up after | 20:18 |
| *** esberglu has quit IRC | 20:19 | |
| *** esberglu has joined #openstack-powervm | 20:30 | |
| edmondsw | efried this is what I was thinking and had suggested to esberglu... but apparently he hasn't gotten it to work: http://paste.openstack.org/show/670559/ | 20:37 |
| efried | edmondsw: How would that help to validate anything? | 20:38 |
| efried | Task.__init__ takes a bunch of arguments. If you're not specifying the kwarg keys, and they (usually) cast correctly, it'll happily take it. | 20:38 |
| edmondsw | efried yeah, true | 20:39 |
| efried | In order to make this test just the bug (which, again, I think is kind of narrow), we should *not* mock Task, deliberately create an instance with non-ascii, and then validate that our task passes. | 20:39 |
| efried | Break it first to validate that it can be broken thusly. | 20:39 |
| efried | Then apply the fix and make sure it fixes it. | 20:40 |
| efried | You know, The Gibi Process. | 20:40 |
| edmondsw | I suppose we could just test that the task name is what it's supposed to be | 20:41 |
| efried | Yeah. Which is also a little goofy. | 20:41 |
| esberglu | efried: I don't think trying to catch the non-ascii bug is what we should be doing here though. The problem was that we were passing bad args | 20:41 |
| efried | Yeah | 20:41 |
| efried | So you could mock Task.__init__ and make sure the first arg is instanceof str. | 20:41 |
| edmondsw | is there a way to combine the autospec test with enforcement of using kwargs instead of passing as args? | 20:42 |
| efried | and/or not instanceof Instance. | 20:42 |
| efried | Both of which are goofy. | 20:42 |
| efried | edmondsw: Enforcing kwargs can be done with @positional | 20:42 |
| efried | But people are shying away from that | 20:42 |
| efried | in the community | 20:42 |
| efried | it seems | 20:42 |
| edmondsw | any idea why? | 20:42 |
| efried | even though IMO it's a pretty good idea. | 20:42 |
| efried | not sure, let me dig up some history from ksa... | 20:42 |
| edmondsw | maybe doesn't make as much sense in some places as it does here? | 20:42 |
| efried | shrug | 20:42 |
| edmondsw | I'd think that was generally more of a hacking test than a UT, but in this case it's needed in UT | 20:43 |
| efried | The positional decorator results in poorly maintainable code in | 20:43 |
| efried | a misguided effort to emulate python3's key-word-arg only notation | 20:43 |
| efried | and functionality. This patch removes keysteonauth's dependance | 20:43 |
| efried | on the positional decorator. | 20:43 |
| efried | I20106345747860365cd0203ba1b33a2900e045b9 | 20:43 |
| efried | edmondsw: I guess you could ask Morgan about it. | 20:43 |
| edmondsw | will do | 20:44 |
| efried | edmondsw: However, I think in this case the decorator would have to be on Task.__init__, so wouldn't help us. | 20:44 |
| efried | We can assert that we called it with kwargs, though. | 20:44 |
| edmondsw | well, and positional was removed from g-r... https://github.com/openstack/requirements/commit/f8ea0a17752a01ce60c4a3dd4fa53ad4f452d2a9 | 20:45 |
| efried | Cause if you assert called_with('name') it'll fail if you actually called it with name='name' and vice versa. | 20:45 |
| edmondsw | oh, there you go | 20:45 |
| efried | So yeah, we can mock Task.__init__ and make sure we're calling it with a single arg with the key specified. But are we really gonna do that friggin everywhere? | 20:46 |
| edmondsw | efried kinda pointless unless you do it everywhere | 20:50 |
| edmondsw | we have what, about a dozen tasks? | 20:50 |
| edmondsw | looks like 13 in nova today | 20:50 |
| efried | It's more than just TaskFlow, really. | 20:51 |
| efried | I mean, we could bite off TaskFlow for one. | 20:51 |
| edmondsw | I would hope that we're mocking out most things besides TaskFlow | 20:51 |
| efried | How does that help our cause? | 20:52 |
| edmondsw | e.g. pypowervm | 20:52 |
| efried | Are we really going to assert every constructor call we make to anything we mock? | 20:52 |
| edmondsw | assert_called_with | 20:52 |
| edmondsw | yeah, at some point things just need to work | 20:53 |
| edmondsw | UT can't catch everything | 20:53 |
| efried | There's a move in Nova to do much more functional testing. | 20:56 |
| efried | We would need to add some serious fixture framework to make that happen for us. | 20:57 |
| esberglu | efried: edmondsw: So you guys thinking we add tests for the task constructor calls? | 21:02 |
| *** ChanServ has quit IRC | 21:02 | |
| efried | esberglu: IMO add one for this specific one and put a low-priority TODO to add it for every init of every constructor of every mocked class everywhere. | 21:03 |
| efried | For the sake of this patch, we don't want to extend the scope too far. | 21:03 |
| edmondsw | agreed | 21:03 |
| *** ChanServ has joined #openstack-powervm | 21:10 | |
| *** barjavel.freenode.net sets mode: +o ChanServ | 21:10 | |
| esberglu | efried: edmondsw: http://paste.openstack.org/show/670603/ | 21:14 |
| esberglu | Any idea why B is failing? Only diff is adding autospec=True | 21:14 |
| efried | Ugh, could it be because Task inherits its .name attr from *its* parent class? | 21:16 |
| *** k0da has joined #openstack-powervm | 21:17 | |
| edmondsw | it actually says CreateAndConnectCfgDrive doesn't have name, not Task... so may indeed be a parent/child thing but the other direction | 21:17 |
| edmondsw | mock doesn't seem to deal with __init__ well in general :( | 21:18 |
| efried | You can try addressing your mock just to Task, not Task.__init__. | 21:19 |
| efried | That's and add instance=True | 21:19 |
| efried | s/That's a/A/ | 21:19 |
| efried | That's how I normally do constructor mocks, I think. | 21:19 |
| esberglu | Idk that mriedem is going to be cool with just fixing the specific one and not adding tests for the rest | 21:36 |
| esberglu | We shall see | 21:37 |
| *** edmondsw has quit IRC | 21:39 | |
| efried | esberglu: He'll be cooler with that than extending the tests to unrelated code paths in this same patch. | 21:40 |
| efried | esberglu: If you wanted to be super stringent and proper about it, you would first propose the patch that does it for everything, with the failing test assertions commented out with a bug number; then propose the fix on top and include uncommenting those assertions in that fix. | 21:41 |
| efried | I seriously doubt he'd demand that, but he would probably be more impressed if you did it that way. | 21:41 |
| esberglu | efried: Would that 1st patch be allowed through at this point in the release? Not sure if patches have to be directly fixing bugs | 21:46 |
| esberglu | efried: Also, tried your instance=True suggestion, still getting the same | 21:48 |
| *** apearson_ has quit IRC | 22:26 | |
| *** tjakobs has quit IRC | 23:06 | |
| *** k0da has quit IRC | 23:19 | |
| *** AlexeyAbashkin has joined #openstack-powervm | 23:21 | |
| *** AlexeyAbashkin has quit IRC | 23:26 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!