| *** esberglu has quit IRC | 00:15 | |
| *** esberglu has joined #openstack-powervm | 00:16 | |
| *** esberglu has quit IRC | 00:20 | |
| *** AlexeyAbashkin has joined #openstack-powervm | 00:58 | |
| *** AlexeyAbashkin has quit IRC | 01:03 | |
| *** tonyb has quit IRC | 04:45 | |
| *** chhagarw has joined #openstack-powervm | 04:47 | |
| *** AlexeyAbashkin has joined #openstack-powervm | 07:44 | |
| *** edmondsw has joined #openstack-powervm | 09:10 | |
| *** edmondsw has quit IRC | 09:15 | |
| *** edmondsw has joined #openstack-powervm | 12:47 | |
| openstackgerrit | Chhavi Agarwal proposed openstack/nova-powervm master: WIP: Having iSCSI Initiator locks per VIOS https://review.openstack.org/557800 | 13:09 |
|---|---|---|
| chhagarw | edmondsw: don't review now, its not ready, i uploaded to check some UT errors | 13:13 |
| edmondsw | chhagarw you know you can check UT locally, right? | 13:20 |
| edmondsw | e.g. tox -e py27 | 13:20 |
| edmondsw | or to run a specific test... tox -e py27 <testname> | 13:20 |
| chhagarw | yeah i am running on my devenv, | 13:21 |
| chhagarw | i know :) | 13:21 |
| edmondsw | :) | 13:24 |
| *** esberglu has joined #openstack-powervm | 13:29 | |
| *** tjakobs has joined #openstack-powervm | 13:42 | |
| openstackgerrit | Chhavi Agarwal proposed openstack/nova-powervm master: WIP: Having iSCSI Initiator locks per VIOS https://review.openstack.org/557800 | 13:46 |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: Use UDID on vscsi mappings https://review.openstack.org/558832 | 14:10 |
| edmondsw | efried ^ | 14:11 |
| efried | edmondsw: Assume UT will be needed for this, yah? | 14:11 |
| edmondsw | all UT passed | 14:11 |
| efried | edmondsw: signaling that we're light on UT for this code path :) | 14:11 |
| edmondsw | yes :) | 14:11 |
| efried | Also, does our CI cover this code path? | 14:11 |
| edmondsw | no, CI can't test vscsi | 14:12 |
| efried | Cause if it doesn't, we need live testing. | 14:12 |
| edmondsw | could it possibly hurt? | 14:12 |
| edmondsw | I thought this was about as safe as they come | 14:12 |
| efried | Yes, as I was postulating in slack. | 14:12 |
| efried | sorry, no confidence there. | 14:12 |
| edmondsw | I must not have followed that | 14:12 |
| efried | I know for sure that there are *some* places where UDIDs need to be encoded/decoded in non-obvious ways. So if you added the UDID here and it's in the wrong format, you'll suddenly stop matching any disks with this statement, and not map the disk, and fail the world. | 14:13 |
| efried | that's just an example off the top of my head. | 14:13 |
| efried | This ain't a change in a comment. We can't let this go with ZERO testing (CI, live, even UT ffs) | 14:14 |
| efried | which is of course not what you want to hear for a 11c change :( | 14:15 |
| edmondsw | efried ok I get it. And the explanation we got from the NovaLink guys makes me think this is ok for now. I'll just abandon | 14:33 |
| efried | sounds good. | 14:36 |
| *** tjakobs has quit IRC | 15:01 | |
| *** tjakobs has joined #openstack-powervm | 15:01 | |
| efried | edmondsw: Are you going to take this over: https://review.openstack.org/#/c/554688/ ? | 15:58 |
| efried | esberglu: or do you still have room for it? | 15:59 |
| edmondsw | I think _all_ we need to do there is figure out how we'll handle dup conf groups/options | 16:00 |
| edmondsw | efried and as you just commented, I think that's settled... so just remove -W? | 16:01 |
| efried | edmondsw: Unless we wanted to enhance that docstring. | 16:02 |
| esberglu | edmondsw: Correct. I can probably still take that. | 16:02 |
| edmondsw | efried I don't want to change the docstring | 16:02 |
| esberglu | efried: I'm not sure that you can just rip the conf option out of one and add it to the other | 16:02 |
| edmondsw | esberglu why not? | 16:03 |
| esberglu | It _might_ work but the powervm conf group is then being registered twice, once in nova and once in nova-powervm | 16:03 |
| esberglu | Will the second register override and wipe out all of the conf options from the first? | 16:03 |
| edmondsw | esberglu no, we would stop registering the group in nova-powervm | 16:03 |
| edmondsw | you should be able to register options separately from registering the group | 16:04 |
| efried | uhm, oh, right, we decided we weren't sure if that works or not. | 16:04 |
| esberglu | Oh yeah duh | 16:04 |
| esberglu | I see what you mean | 16:04 |
| efried | Because we don't know what order stuff gets loaded up. | 16:04 |
| esberglu | Yeah, what if the nova-powervm conf gets loaded 1st, the group won't exist yet | 16:04 |
| efried | The way we left this conversation was: someone was going to go ask dhellmann about it. | 16:04 |
| efried | because obviously the right thing is for both to register the group, and whoever comes second is a no-op. | 16:05 |
| efried | but we don't know if that works or note. | 16:05 |
| efried | not. | 16:05 |
| efried | Also, I think Jichen was just requesting that the docstring mention that the value can't be negative. | 16:05 |
| edmondsw | efried that just seems silly to me... of course it can't be negative.. | 16:06 |
| edmondsw | how can you have negative of something? | 16:06 |
| efried | or zero | 16:06 |
| efried | or greater than one | 16:06 |
| efried | or in increments other than 0.05 | 16:07 |
| edmondsw | I think that's all covered by the current description, no? | 16:07 |
| efried | There's only so much we can be expected to say. I guess we could ref an IBM doc that explains it. | 16:07 |
| efried | one way or another, we need jichen to remove his -1. I don't want to look like we're ignoring him. | 16:07 |
| edmondsw | we replied to his comment, so it's not that we're ignoring him | 16:08 |
| edmondsw | probably need to catch him on irc and ask him if he's ok with it now | 16:08 |
| edmondsw | he did have another comment about test... esberglu have you looked at that one? | 16:08 |
| esberglu | edmondsw: Yeah his comment was about validating the conf option and then adding tests for the validation. I guess we could check that it is between 0 and 1 | 16:10 |
| edmondsw | yeah, that sounds like a good idea | 16:11 |
| edmondsw | 0 < x <= 1 | 16:12 |
| edmondsw | esberglu you gonna do that or should I? | 16:12 |
| esberglu | efried: edmondsw: FWIW I tried a manual CI run with the nova patch and proc_units_factor remove from nova-powervm. It failed, but the failures didn't seem to be related | 16:13 |
| esberglu | I can run another later today to confirm | 16:13 |
| efried | esberglu: Is the group still being registered in both places? | 16:13 |
| efried | ...in your manual run? | 16:13 |
| esberglu | efried: Yep. Was trying to determine if being registered twice would break it | 16:14 |
| efried | okay, nice. | 16:14 |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: Use py3 for pep8 https://review.openstack.org/558868 | 16:22 |
| esberglu | edmondsw: efried: https://github.com/openstack/oslo.config/blob/master/oslo_config/cfg.py#L1897-L1910 | 16:24 |
| esberglu | https://github.com/openstack/oslo.config/blob/master/oslo_config/cfg.py#L2659-L2670 | 16:24 |
| esberglu | Looks like its fine to register the group twice | 16:24 |
| efried | beaut | 16:25 |
| efried | I'm sure that was way easier than asking Doug. | 16:25 |
| *** efried is now known as efried_rollin | 16:25 | |
| esberglu | So much easier ;) | 16:25 |
| esberglu | edmondsw: I can fix up the patch | 16:26 |
| edmondsw | esberglu tx | 16:26 |
| openstackgerrit | Matthew Edmonds proposed openstack/networking-powervm master: Use py3 for pep8 https://review.openstack.org/558872 | 16:34 |
| openstackgerrit | Matthew Edmonds proposed openstack/ceilometer-powervm master: Use py3 for pep8 https://review.openstack.org/558875 | 16:37 |
| *** AlexeyAbashkin has quit IRC | 16:38 | |
| chhagarw | edmondsw: why we need to have the get_iscsi_initiators to be staticmethod, i want to use vios_uuids, in this method so later if we need to have a specific implementation of vios_uuids for iscsi it can be used | 16:41 |
| edmondsw | I explained that in my comments | 16:42 |
| edmondsw | and not being able to use vios_uuids should not be a significant toll on you | 16:42 |
| chhagarw | currently we do not have specific implementation of vios_uuids for iscsi, so it can be easy to pull directly from pvm_partition, but we may need vios_uuids specific implementation for iSCSI in future. | 16:50 |
| chhagarw | in that case, everytime this staticmethod needs to be changed, as we need only required vios initiators | 16:50 |
| esberglu | edmondsw: All the py3 changes passed, gonna fast-approve | 16:58 |
| openstackgerrit | Merged openstack/ceilometer-powervm master: Use py3 for pep8 https://review.openstack.org/558875 | 17:12 |
| chhagarw | https://review.openstack.org/#/c/557800/ | 17:12 |
| chhagarw | ready for review, please see | 17:13 |
| openstackgerrit | Merged openstack/nova-powervm master: Use py3 for pep8 https://review.openstack.org/558868 | 17:13 |
| edmondsw | chhagarw how about creating a _get_vios_uuids static method and having vios_uuids call that? That avoids duplication | 17:47 |
| chhagarw | yes that should also work and will be better | 17:48 |
| openstackgerrit | Merged openstack/networking-powervm master: Use py3 for pep8 https://review.openstack.org/558872 | 17:52 |
| *** AlexeyAbashkin has joined #openstack-powervm | 18:08 | |
| edmondsw | tjakobs are we missing a 'not' here? https://github.com/openstack/nova-powervm/blob/master/nova_powervm/virt/powervm/volume/iscsi.py#L132 | 18:10 |
| edmondsw | or some comment explaining why this seems backwards | 18:11 |
| *** AlexeyAbashkin has quit IRC | 18:12 | |
| tjakobs | edmondsw: if it's multipath the "_iterate_all_targets" is handled at the lower levels. Some clarification around this point should be added. | 18:14 |
| edmondsw | so you can have multiple targets even if it's not multipath? | 18:14 |
| edmondsw | making https://review.openstack.org/#/c/557800/8/nova_powervm/virt/powervm/volume/iscsi.py@127 wrong | 18:15 |
| tjakobs | if chhagarw doesn't add it the current reviews, i'll do something during my rework (unless you want it sooner) | 18:15 |
| edmondsw | tjakobs ? | 18:15 |
| edmondsw | you can go ahead and suggest what the comment should look like in that review | 18:16 |
| edmondsw | might as well go ahead and add it there | 18:16 |
| chhagarw | edmondsw: https://review.openstack.org/#/c/557800/8/nova_powervm/virt/powervm/volume/iscsi.py@127 | 18:18 |
| chhagarw | u mentioned wrong, i did not got it | 18:18 |
| edmondsw | I don't think the plural forms of target_* are unique to the multipath case, yet you treat them like they are | 18:18 |
| edmondsw | tjakobs am I correct on that? | 18:19 |
| tjakobs | edmondsw: I believe you are correct | 18:21 |
| chhagarw | I checked in the cinder storwize code, plurals are returned in the case of multipath | 18:26 |
| chhagarw | tjakobs: in which case u saw the plurals apart from multipath | 18:27 |
| openstackgerrit | Eric Berglund proposed openstack/nova-powervm master: Remove proc_units_factor from nova-powervm conf https://review.openstack.org/558890 | 18:28 |
| edmondsw | chhagarw if plurals are only in the multipath case, then why do we need _iterate_all_targets (which isn't called in the multipath case)? | 18:29 |
| tjakobs | from what I can tell multipath have have some or all of the plurals, and single_path will have (at least) all the singluars (which should be sufficient). But this model was supposed to "just work" for any of the cinder volumes, and I haven't looked at them all to verify the first sentence is always true. | 18:33 |
| tjakobs | the _iterate_all_targets is to handle some edge cases (it was taken from os-brick and they go through it on the singular path case) | 18:34 |
| esberglu | efried_rollin: edmondsw: Thoughts on merging https://review.openstack.org/#/c/536178/ ? | 18:35 |
| esberglu | I'm not gonna have time to jump back into it | 18:35 |
| esberglu | But it does add a decent amount of autospec so it doesn't hurt anything | 18:35 |
| esberglu | I would just need to rebase | 18:35 |
| edmondsw | esberglu I can't think of a reason not to go ahead there... efried? | 18:37 |
| edmondsw | tjakobs chhagarw I found this: https://github.com/openstack/cinder/blob/master/cinder/volume/targets/iscsi.py#L72-L80 | 18:42 |
| edmondsw | so it's not just multipath but also "single path with failover on connection failure" | 18:43 |
| edmondsw | and dependent on which driver is in use | 18:43 |
| chhagarw | ok, so multipath check in _get_iscsi_conn_props need to be removed | 18:44 |
| edmondsw | yep | 18:45 |
| edmondsw | I wonder if we also need to worry about this: https://github.com/openstack/cinder/blob/master/cinder/volume/targets/iscsi.py#L78-L80 | 18:46 |
| edmondsw | or if that's already handled somehow | 18:46 |
| tjakobs | that's handled in the REST layer | 18:47 |
| *** AlexeyAbashkin has joined #openstack-powervm | 18:58 | |
| *** AlexeyAbashkin has quit IRC | 19:02 | |
| edmondsw | chhagarw posted my review comments | 19:28 |
| edmondsw | some we already talked about above | 19:29 |
| *** efried_rollin is now known as efried | 19:29 | |
| efried | esberglu: re https://review.openstack.org/#/c/536178/ - looks like we were holding it up to do same in more places, which can always be done later. So my take is: edit the commit message to remove the WIP, let's see if it still passes, and then we can merge it as is. | 19:30 |
| efried | edmondsw: Agree ^ ? | 19:30 |
| efried | esberglu: Oh, yeah, rebase also. | 19:31 |
| chhagarw | ok | 19:31 |
| edmondsw | efried yeah, I think that's basically what we said above... were just waiting to see if you agreed | 19:31 |
| efried | yes, good by me. | 19:32 |
| *** esberglu has quit IRC | 20:03 | |
| *** esberglu_ has joined #openstack-powervm | 20:08 | |
| openstackgerrit | Eric Berglund proposed openstack/nova-powervm master: Autospeccing: powervm https://review.openstack.org/536178 | 20:14 |
| *** edmondsw has quit IRC | 20:50 | |
| openstackgerrit | Merged openstack/nova-powervm master: Autospeccing: powervm https://review.openstack.org/536178 | 21:09 |
| *** tjakobs has quit IRC | 21:45 | |
| *** esberglu_ has quit IRC | 21:57 | |
| *** AlexeyAbashkin has joined #openstack-powervm | 21:59 | |
| *** chhagarw has quit IRC | 22:00 | |
| *** AlexeyAbashkin has quit IRC | 22:04 | |
| openstackgerrit | Kenneth Burger proposed openstack/nova-powervm master: merge conflict https://review.openstack.org/558950 | 22:19 |
| openstackgerrit | Kenneth Burger proposed openstack/nova-powervm master: WIP: Sanitize the config drive UUID https://review.openstack.org/428433 | 22:24 |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!