Wednesday, 2017-02-22

*** catintheroof has quit IRC00:51
*** x00350071_ has joined #senlin01:05
*** edisonxiang has quit IRC01:05
*** XueFeng has joined #senlin01:11
*** openstackgerrit has quit IRC01:23
*** openstackgerrit has joined #senlin01:37
*** ChanServ sets mode: +v openstackgerrit01:37
openstackgerritQiming Teng proposed openstack/senlin master: Improve the slow path of lock acquire
*** yanyanhu has joined #senlin01:40
*** guoshan has joined #senlin01:47
openstackgerritMerged openstack/senlin master: Remove 'profile-only' todo item from list
openstackgerritMerged openstack/senlin master: Support profile-only to cluster update
*** yuanying has quit IRC02:32
*** Drago has joined #senlin02:34
openstackgerritEthan Lynn proposed openstack/senlin master: Handle network update for VDU
XueFengYanYan, around?02:46
yanyanhuhi, XueFeng, sorry just finished a meeting03:02
yanyanhusaw your mail and will update it03:02
*** guoshan_ has joined #senlin03:04
*** guoshan has quit IRC03:04
*** guoshan_ has quit IRC03:15
*** guoshan has joined #senlin03:15
chohoorAbout support profile-only to cluster update, related code have be merged. Can I recommit the path( belong to senlinclient?03:30
chohoorActually, I don't know which project 'SDK' means.03:32
Qimingchohoor, there is another patch to be added at server side, which is bumping api version to 1.603:33
Qimingthen the support has to be added to python-openstacksdk03:34
*** yuanying has joined #senlin03:34
Qimingwe then wait for sdk new release version and the version accpeted into global-requirements03:34
Qimingonly after that happens, the client side patch can be accepted03:34
chohoorQiming, thx. I will read python-openstacksdk code.03:38
openstackgerritQiming Teng proposed openstack/senlin master: Fix tags update when join/leave a stack
*** guoshan has quit IRC04:25
*** guoshan has joined #senlin04:35
*** x00350071_ is now known as edisonxiang04:37
*** guoshan has quit IRC04:39
*** Drago has quit IRC04:51
*** guoshan has joined #senlin05:09
openstackgerritHongbin Li proposed openstack/senlin master: Add missing word to document
*** guoshan has quit IRC05:45
openstackgerritQiming Teng proposed openstack/senlin master: Fix LB member status update in error conditions
*** wllabs has quit IRC06:01
*** wllabs has joined #senlin06:01
openstackgerritMerged openstack/senlin master: Add missing word to document
*** guoshan has joined #senlin06:04
*** Guest62350 has joined #senlin06:18
*** ruijie has quit IRC07:00
*** ruijie has joined #senlin07:11
*** yuanying has quit IRC07:33
openstackgerritAaron Ding proposed openstack/senlin master: Modify network name in examples files
*** yuanying has joined #senlin08:15
elynnHi Qiming around?08:24
*** Guest62350 has quit IRC08:28
openstackgerritMerged openstack/senlin master: Fix LB member status update in error conditions
elynnQiming, have time to talk about patch ?08:48
elynnCan I call you via wechat?08:49
Qiminginconenient for voice call atm08:49
elynnokay, Let me try to explain here.08:49
elynnAll the logic starts from _update_network()08:49
Qimingthen rename all functions called there as _update_something()08:50
elynnIt will call _create_interfaces to create newly added networks and call _delete_interfaces() to remove unnecessary networks.08:50
elynnSo what I do is changing the logic of _create_interfaces and _delete_interfaces.08:51
Qimingthat is fine, if both functions are kept reasonably small08:51
Qimingokay, that part I understand08:51
elynnQiming, You mean to rename _create_interfaces and _delete_interfaces?08:52
Qimingraising a EResourceUpdate from inside a _create_interfaces function is confusing08:52
Qimingespecially when you are again calling server_interface_craete inside that function08:52
elynn_create_interfaces only used in _update_network()08:53
elynnSo ... maybe I should rename it...08:53
Qimingthat function was added in previous patch08:54
Qimingyou mentioned that it was used for both creation and update08:54
elynnThe function _create_ports_from_properties is introduced in previous patch.08:54
elynnto create ports08:54
Qimingonly for creation's purpose?08:55
elynnyes, only for creation purpos08:55
Qimingthat is fine08:55
elynnThe third parameters only used to raise different error in profile creation or profile update.08:56
Qimingbecause that function is SHARED in both workflows08:56
*** chohoor has quit IRC08:56
Qimingthat is a valid case for validation, because validation is needed no matter you are creating something new or updating something existing08:57
Qimingyes, that is a validation logic08:57
elynnThis functions will create ports and return a list of created ports' dict.08:58
elynnThen later in _create_interfaces L105708:59
Qimingwhich function?08:59
elynnThe _create_ports_from_properties function08:59
Qimingokay, that logic is shared between create/update workflows, right?08:59
elynn_create_ports_from_properties is shared between create and update09:00
Qimingif that function is already doing port creation, why do we add _create_interfaces ?09:01
elynn_create_interfaces actually doing two things09:01
elynnCreate ports and attach to server.09:02
Qimingit is doing several things I'm afraid09:02
Qimingit is even validating if the server is there ...09:02
elynnThat's because it is called by _update_network09:02
elynnin profile update workflow.09:02
Qimingwhy we didn't check if the server is there before updating anything?09:03
elynnSo first it will check if server is existed.09:03
*** chohoor has joined #senlin09:03
elynnHmm, good question, I will check09:03
elynnand remove the server check in _create_interfaces09:03
elynndidn't touch that code.09:04
Qimingthe key points ...09:04
Qimingwalk through the creation / update workflow09:04
Qimingmake sure necessary things are done and only done once09:04
elynnyes, They only share the _create_ports_from_properties part.09:05
Qimingpoint 2, stop introducing embedded functions, unless they are very straighforward, where even a unit test is not needed09:05
elynnThen in creation workflow, we call server_create to create a new instance with created ports.09:05
elynnQiming, okay, I will remove the existing two inline functions.09:06
Qimingif there is a need, feel free to move some util functions to neutron driver or even add a utils module09:07
QimingI see, _delete_interfaces and _add_interfaces were added by me, possibly09:08
Qimingvery bad name for code reviewers09:08
elynnDo you have any suggestions to rename it?09:08
elynnAnd what functions you think are better put into utils module?09:09
Qiminge.g. _update_network_ports_removed, _updated_network_ports_added,09:10
elynnsounds good.09:10
Qimingthe _get_port_attrs function doesn't belong to this VDU/server module09:11
Qimingit seems you want to get a port with its floating ips09:12
Qimingthat should be moved elsewhere09:12
elynnyes, I want to get the port information with floating IP associated with it.09:12
elynnPut it in neutron driver?09:13
elynnit only return a dict09:13
elynnnot a port object.09:13
Qimingit is called on line 1164, which seems the only location09:14
Qimingtake those lines for example09:15
Qiming1164 gets ports as dict, with floating ips injected09:15
Qimingthen 1168 is doing another search09:16
Qimingthat search invokes _find_port_by_net_spec09:16
Qimingwhich again invokes network_get func09:17
Qimingwhat are we doing here?09:17
elynnyes, translate floating_network to ID09:17
elynnWhen we define a profile09:17
elynnnormally we use a name for floating_network09:18
Qimingthis is about deleting existing interfaces09:19
elynnhmm, let me see if a name existed in a floating IP dict...09:19
elynnMaybe I could just match the name instead of ID, that would save a network_get call09:20
elynnCan't remember why I add this logic...09:20
Qiminga possible way to move this forward ...09:21
elynnI can optimize that.09:21
Qiminghandle one case in one patch09:21
Qimingmy brain is too small ....09:21
Qimingneed some nutrition I assure you09:21
elynnokay , Let me explain the delete_interface workflow.09:21
elynnIt's simple too :)09:22
Qimingnot simple09:22
elynnFind a port matching the network property, then delete it.09:22
elynnL1168 is to find the port09:22
elynnL1170 delete it09:23
elynnThe complex part is _find_port_by_net_spec09:23
Qimingthe port is from profile spec or we created them?09:23
openstackgerritMerged openstack/senlin master: Modify network name in examples files
Qimingif, for example, we created them, why don't we save their network id in the beginning to save a network_get call?09:24
elynnsave to where?09:24
Qimingif it is "internal created"09:25
elynnYes, all ports are internal created09:26
elynnexcept for ' port: xxx' one.09:26
elynnYou mean use 'internal_ports' to save the network ID and all parameters?09:27
QimingI somehow recalled why I was treating update differently from create when validating network09:27
Qimingtry get the ID if possible09:27
Qimingavoid using names when possible09:28
Qimingnames requires a cross-check later09:28
elynnBut that will store many informations in node.data09:28 is already polluted09:28
Qimingyou are putting port and floating ips separately, aren't you09:29
Qimingis it possible to make floating ip a sub key of port?09:30
elynnI think that's doable09:30
Qimingline 114209:31
Qimingit is only about deleting cached ports09:31
Qimingwhich is also called 'internal_ports'09:31
elynnnot exactly...09:31
elynnserver_interface_delete detach the port from server.09:32
elynnIf a port is created by this profile, then need to call port_delete to delete it.09:32
Qimingbut we may leave that port there09:32
elynnLeave that port there?09:33
Qimingleave that port undeleted09:33
Qimingif the port was provided in the profile spec09:33
elynnYes, then we don't delete it.09:34
elynnL 114609:34
Qimingthis is an example of mixing two logics into the same function09:35
elynnit's a inline function, I will expand it in later patch.09:35
Qimingnot just about inline or not09:36
elynnDo you suggest to..?09:39
Qimingif I'm revising this logic into a separate function, I'm gonna do this:09:43
Qimingdef _update_network_remove_port(node, port_id):09:43
Qiming   internal_ports ='internal_ports', [])09:43
Qiming   cc =09:43
Qiming   nc =09:43
Qiming   try:09:43
Qiming      cc.server_interface_delete(port_id, node.physical_id)09:43
Qiming      # remove port if we created it09:43
Qiming      if port_id in internal_ports:09:43
Qiming         internal_ports.remove(port_id)09:43
Qiming['internal_ports'] = internal_ports09:43
Qiming         no.Node.update(self.context,, {'data':})09:43
Qiming        nc.port_delete(port_id, True)09:43
Qiming   except exc.InternalError as ex:09:43
Qiming     raise exc.EResourceUpdate()09:43
elynnThat is almost the same, I guess?09:45
Qimingtry write test cases for these two versions09:46
Qiminghow many flows you will need to cover?09:46
elynncan not tell..09:47
elynnIs there any difference?09:48
Qimingseveral changes I made09:49
Qiming1. rename obj to node, because this is a standalone function, it has its own logic, renaming to 'node' helps understanding09:50
Qiming2. avoid spliting the case of internal_ports into two segments, some inside try except, some left outside09:51
Qiming3. add a comment before that 'if' statement, so if I'm don't care about internal ports, the flow is very clean09:51
elynnniu! sorry, can't catch your mind just now :P09:52
elynnNow I get it.09:52
Qimingjust for your reference09:52
elynnokay, to summarized, two parts need to optimize, 1. reduce API call to get ID,  2. Make code readable.09:54
Qimingwe all forget why we wrote that code09:58
elynnThanks :)09:58
Qimingcode readability is very important09:58
elynnagree, and learned.09:58
*** yanyanhu has quit IRC10:04
*** XueFeng has quit IRC10:30
*** guoshan has quit IRC10:31
openstackgerritXueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None
openstackgerritXueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None
*** ruijie has quit IRC11:41
openstackgerritXueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None
openstackgerritXueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None
openstackgerritXueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None
*** zhurong has joined #senlin13:47
openstackgerritXueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None
*** Jeffrey4l has quit IRC14:38
*** Jeffrey4l has joined #senlin14:50
*** Jeffrey4l has quit IRC14:52
*** Jeffrey4l has joined #senlin14:52
*** zhurong has quit IRC15:07
*** catintheroof has joined #senlin15:43
*** catintheroof has quit IRC16:33
*** catintheroof has joined #senlin16:38
*** catintheroof has quit IRC16:38
*** catintheroof has joined #senlin16:38
*** catintheroof has quit IRC16:43
*** catintheroof has joined #senlin16:53
*** catintheroof has quit IRC16:58
*** catintheroof has joined #senlin17:15
*** catintheroof has quit IRC17:18
*** catintheroof has joined #senlin17:19
*** edisonxiang has quit IRC19:04
*** edisonxiang has joined #senlin19:05
*** Jeffrey4l_ has joined #senlin19:31
*** Jeffrey4l has quit IRC19:31
*** Jeffrey4l_ has quit IRC19:33
*** Jeffrey4l_ has joined #senlin19:33
*** catintheroof has quit IRC19:48
*** catintheroof has joined #senlin20:11
*** catintheroof has quit IRC20:11
*** catintheroof has joined #senlin20:12
*** Jeffrey4l__ has joined #senlin21:34
*** Jeffrey4l_ has quit IRC21:35
*** catintheroof has quit IRC22:47

Generated by 2.14.0 by Marius Gedminas - find it at!