Wednesday, 2018-04-04

*** esberglu has quit IRC00:15
*** esberglu has joined #openstack-powervm00:16
*** esberglu has quit IRC00:20
*** AlexeyAbashkin has joined #openstack-powervm00:58
*** AlexeyAbashkin has quit IRC01:03
*** tonyb has quit IRC04:45
*** chhagarw has joined #openstack-powervm04:47
*** AlexeyAbashkin has joined #openstack-powervm07:44
*** edmondsw has joined #openstack-powervm09:10
*** edmondsw has quit IRC09:15
*** edmondsw has joined #openstack-powervm12:47
openstackgerritChhavi Agarwal proposed openstack/nova-powervm master: WIP: Having iSCSI Initiator locks per VIOS
chhagarwedmondsw: don't review now, its not ready, i uploaded to check some UT errors13:13
edmondswchhagarw you know you can check UT locally, right?13:20
edmondswe.g. tox -e py2713:20
edmondswor to run a specific test... tox -e py27 <testname>13:20
chhagarwyeah i am running on my devenv,13:21
chhagarwi know :)13:21
*** esberglu has joined #openstack-powervm13:29
*** tjakobs has joined #openstack-powervm13:42
openstackgerritChhavi Agarwal proposed openstack/nova-powervm master: WIP: Having iSCSI Initiator locks per VIOS
openstackgerritMatthew Edmonds proposed openstack/nova-powervm master: Use UDID on vscsi mappings
edmondswefried ^14:11
efriededmondsw: Assume UT will be needed for this, yah?14:11
edmondswall UT passed14:11
efriededmondsw: signaling that we're light on UT for this code path :)14:11
edmondswyes :)14:11
efriedAlso, does our CI cover this code path?14:11
edmondswno, CI can't test vscsi14:12
efriedCause if it doesn't, we need live testing.14:12
edmondswcould it possibly hurt?14:12
edmondswI thought this was about as safe as they come14:12
efriedYes, as I was postulating in slack.14:12
efriedsorry, no confidence there.14:12
edmondswI must not have followed that14:12
efriedI 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
efriedthat's just an example off the top of my head.14:13
efriedThis ain't a change in a comment.  We can't let this go with ZERO testing (CI, live, even UT ffs)14:14
efriedwhich is of course not what you want to hear for a 11c change :(14:15
edmondswefried ok I get it. And the explanation we got from the NovaLink guys makes me think this is ok for now. I'll just abandon14:33
efriedsounds good.14:36
*** tjakobs has quit IRC15:01
*** tjakobs has joined #openstack-powervm15:01
efriededmondsw: Are you going to take this over: ?15:58
efriedesberglu: or do you still have room for it?15:59
edmondswI think _all_ we need to do there is figure out how we'll handle dup conf groups/options16:00
edmondswefried and as you just commented, I think that's settled... so just remove -W?16:01
efriededmondsw: Unless we wanted to enhance that docstring.16:02
esbergluedmondsw: Correct. I can probably still take that.16:02
edmondswefried I don't want to change the docstring16:02
esbergluefried: I'm not sure that you can just rip the conf option out of one and add it to the other16:02
edmondswesberglu why not?16:03
esbergluIt _might_ work but the powervm conf group is then being registered twice, once in nova and once in nova-powervm16:03
esbergluWill the second register override and wipe out all of the conf options from the first?16:03
edmondswesberglu no, we would stop registering the group in nova-powervm16:03
edmondswyou should be able to register options separately from registering the group16:04
efrieduhm, oh, right, we decided we weren't sure if that works or not.16:04
esbergluOh yeah duh16:04
esbergluI see what you mean16:04
efriedBecause we don't know what order stuff gets loaded up.16:04
esbergluYeah, what if the nova-powervm conf gets loaded 1st, the group won't exist yet16:04
efriedThe way we left this conversation was: someone was going to go ask dhellmann about it.16:04
efriedbecause obviously the right thing is for both to register the group, and whoever comes second is a no-op.16:05
efriedbut we don't know if that works or note.16:05
efriedAlso, I think Jichen was just requesting that the docstring mention that the value can't be negative.16:05
edmondswefried that just seems silly to me... of course it can't be negative..16:06
edmondswhow can you have negative of something?16:06
efriedor zero16:06
efriedor greater than one16:06
efriedor in increments other than 0.0516:07
edmondswI think that's all covered by the current description, no?16:07
efriedThere's only so much we can be expected to say.  I guess we could ref an IBM doc that explains it.16:07
efriedone way or another, we need jichen to remove his -1.  I don't want to look like we're ignoring him.16:07
edmondswwe replied to his comment, so it's not that we're ignoring him16:08
edmondswprobably need to catch him on irc and ask him if he's ok with it now16:08
edmondswhe did have another comment about test... esberglu have you looked at that one?16:08
esbergluedmondsw: 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 116:10
edmondswyeah, that sounds like a good idea16:11
edmondsw0 < x <= 116:12
edmondswesberglu you gonna do that or should I?16:12
esbergluefried: 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 related16:13
esbergluI can run another later today to confirm16:13
efriedesberglu: Is the group still being registered in both places?16:13 your manual run?16:13
esbergluefried: Yep. Was trying to determine if being registered twice would break it16:14
efriedokay, nice.16:14
openstackgerritMatthew Edmonds proposed openstack/nova-powervm master: Use py3 for pep8
esbergluedmondsw: efried:
esbergluLooks like its fine to register the group twice16:24
efriedI'm sure that was way easier than asking Doug.16:25
*** efried is now known as efried_rollin16:25
esbergluSo much easier ;)16:25
esbergluedmondsw: I can fix up the patch16:26
edmondswesberglu tx16:26
openstackgerritMatthew Edmonds proposed openstack/networking-powervm master: Use py3 for pep8
openstackgerritMatthew Edmonds proposed openstack/ceilometer-powervm master: Use py3 for pep8
*** AlexeyAbashkin has quit IRC16:38
chhagarwedmondsw: 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 used16:41
edmondswI explained that in my comments16:42
edmondswand not being able to use vios_uuids should not be a significant toll on you16:42
chhagarwcurrently 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
chhagarwin that case, everytime this staticmethod needs to be changed, as we need only required vios initiators16:50
esbergluedmondsw: All the py3 changes passed, gonna fast-approve16:58
openstackgerritMerged openstack/ceilometer-powervm master: Use py3 for pep8
chhagarwready for review, please see17:13
openstackgerritMerged openstack/nova-powervm master: Use py3 for pep8
edmondswchhagarw how about creating a _get_vios_uuids static method and having vios_uuids call that? That avoids duplication17:47
chhagarwyes that should also work and will be better17:48
openstackgerritMerged openstack/networking-powervm master: Use py3 for pep8
*** AlexeyAbashkin has joined #openstack-powervm18:08
edmondswtjakobs are we missing a 'not' here?
edmondswor some comment explaining why this seems backwards18:11
*** AlexeyAbashkin has quit IRC18:12
tjakobsedmondsw: if it's multipath the "_iterate_all_targets" is handled at the lower levels. Some clarification around this point should be added.18:14
edmondswso you can have multiple targets even if it's not multipath?18:14
edmondswmaking wrong18:15
tjakobsif chhagarw doesn't add it the current reviews, i'll do something during my rework (unless you want it sooner)18:15
edmondswtjakobs ?18:15
edmondswyou can go ahead and suggest what the comment should look like in that review18:16
edmondswmight as well go ahead and add it there18:16
chhagarwu mentioned wrong, i did not got it18:18
edmondswI don't think the plural forms of target_* are unique to the multipath case, yet you treat them like they are18:18
edmondswtjakobs am I correct on that?18:19
tjakobsedmondsw: I believe you are correct18:21
chhagarwI checked in the cinder storwize code, plurals are returned in the case of multipath18:26
chhagarwtjakobs: in which case u saw the plurals apart from multipath18:27
openstackgerritEric Berglund proposed openstack/nova-powervm master: Remove proc_units_factor from nova-powervm conf
edmondswchhagarw 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
tjakobsfrom 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
tjakobsthe _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
esbergluefried_rollin: edmondsw: Thoughts on merging ?18:35
esbergluI'm not gonna have time to jump back into it18:35
esbergluBut it does add a decent amount of autospec so it doesn't hurt anything18:35
esbergluI would just need to rebase18:35
edmondswesberglu I can't think of a reason not to go ahead there... efried?18:37
edmondswtjakobs chhagarw I found this:
edmondswso it's not just multipath but also "single path with failover on connection failure"18:43
edmondswand dependent on which driver is in use18:43
chhagarwok, so multipath check in _get_iscsi_conn_props need to be removed18:44
edmondswI wonder if we also need to worry about this:
edmondswor if that's already handled somehow18:46
tjakobsthat's handled in the REST layer18:47
*** AlexeyAbashkin has joined #openstack-powervm18:58
*** AlexeyAbashkin has quit IRC19:02
edmondswchhagarw posted my review comments19:28
edmondswsome we already talked about above19:29
*** efried_rollin is now known as efried19:29
efriedesberglu: re - 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
efriededmondsw: Agree ^ ?19:30
efriedesberglu: Oh, yeah, rebase also.19:31
edmondswefried yeah, I think that's basically what we said above... were just waiting to see if you agreed19:31
efriedyes, good by me.19:32
*** esberglu has quit IRC20:03
*** esberglu_ has joined #openstack-powervm20:08
openstackgerritEric Berglund proposed openstack/nova-powervm master: Autospeccing: powervm
*** edmondsw has quit IRC20:50
openstackgerritMerged openstack/nova-powervm master: Autospeccing: powervm
*** tjakobs has quit IRC21:45
*** esberglu_ has quit IRC21:57
*** AlexeyAbashkin has joined #openstack-powervm21:59
*** chhagarw has quit IRC22:00
*** AlexeyAbashkin has quit IRC22:04
openstackgerritKenneth Burger proposed openstack/nova-powervm master: merge conflict
openstackgerritKenneth Burger proposed openstack/nova-powervm master: WIP: Sanitize the config drive UUID

Generated by 2.15.3 by Marius Gedminas - find it at!