| *** catintheroof has quit IRC | 00:51 | |
| *** x00350071_ has joined #senlin | 01:05 | |
| *** edisonxiang has quit IRC | 01:05 | |
| *** XueFeng has joined #senlin | 01:11 | |
| *** openstackgerrit has quit IRC | 01:23 | |
| *** openstackgerrit has joined #senlin | 01:37 | |
| *** ChanServ sets mode: +v openstackgerrit | 01:37 | |
| openstackgerrit | Qiming Teng proposed openstack/senlin master: Improve the slow path of lock acquire https://review.openstack.org/436721 | 01:37 |
|---|---|---|
| *** yanyanhu has joined #senlin | 01:40 | |
| *** guoshan has joined #senlin | 01:47 | |
| openstackgerrit | Merged openstack/senlin master: Remove 'profile-only' todo item from list https://review.openstack.org/436720 | 02:03 |
| openstackgerrit | Merged openstack/senlin master: Support profile-only to cluster update https://review.openstack.org/436288 | 02:25 |
| *** yuanying has quit IRC | 02:32 | |
| *** Drago has joined #senlin | 02:34 | |
| openstackgerrit | Ethan Lynn proposed openstack/senlin master: Handle network update for VDU https://review.openstack.org/432587 | 02:44 |
| XueFeng | YanYan, around? | 02:46 |
| yanyanhu | hi, XueFeng, sorry just finished a meeting | 03:02 |
| yanyanhu | saw your mail and will update it | 03:02 |
| yanyanhu | done | 03:03 |
| *** guoshan_ has joined #senlin | 03:04 | |
| *** guoshan has quit IRC | 03:04 | |
| XueFeng | ok,thanks:) | 03:06 |
| *** guoshan_ has quit IRC | 03:15 | |
| *** guoshan has joined #senlin | 03:15 | |
| chohoor | About support profile-only to cluster update, related code have be merged. Can I recommit the path(https://review.openstack.org/#/c/436349/) belong to senlinclient? | 03:30 |
| chohoor | Actually, I don't know which project 'SDK' means. | 03:32 |
| Qiming | chohoor, there is another patch to be added at server side, which is bumping api version to 1.6 | 03:33 |
| Qiming | then the support has to be added to python-openstacksdk | 03:34 |
| *** yuanying has joined #senlin | 03:34 | |
| Qiming | we then wait for sdk new release version and the version accpeted into global-requirements | 03:34 |
| Qiming | only after that happens, the client side patch can be accepted | 03:34 |
| chohoor | Qiming, thx. I will read python-openstacksdk code. | 03:38 |
| openstackgerrit | Qiming Teng proposed openstack/senlin master: Fix tags update when join/leave a stack https://review.openstack.org/436745 | 03:48 |
| *** guoshan has quit IRC | 04:25 | |
| *** guoshan has joined #senlin | 04:35 | |
| *** x00350071_ is now known as edisonxiang | 04:37 | |
| *** guoshan has quit IRC | 04:39 | |
| *** Drago has quit IRC | 04:51 | |
| *** guoshan has joined #senlin | 05:09 | |
| openstackgerrit | Hongbin Li proposed openstack/senlin master: Add missing word to document https://review.openstack.org/436763 | 05:13 |
| *** guoshan has quit IRC | 05:45 | |
| openstackgerrit | Qiming Teng proposed openstack/senlin master: Fix LB member status update in error conditions https://review.openstack.org/436772 | 05:53 |
| *** wllabs has quit IRC | 06:01 | |
| *** wllabs has joined #senlin | 06:01 | |
| openstackgerrit | Merged openstack/senlin master: Add missing word to document https://review.openstack.org/436763 | 06:01 |
| *** guoshan has joined #senlin | 06:04 | |
| *** Guest62350 has joined #senlin | 06:18 | |
| *** ruijie has quit IRC | 07:00 | |
| *** ruijie has joined #senlin | 07:11 | |
| *** yuanying has quit IRC | 07:33 | |
| openstackgerrit | Aaron Ding proposed openstack/senlin master: Modify network name in examples files https://review.openstack.org/436805 | 07:49 |
| *** yuanying has joined #senlin | 08:15 | |
| elynn | Hi Qiming around? | 08:24 |
| *** Guest62350 has quit IRC | 08:28 | |
| Qiming | yes | 08:30 |
| openstackgerrit | Merged openstack/senlin master: Fix LB member status update in error conditions https://review.openstack.org/436772 | 08:33 |
| elynn | Qiming, have time to talk about patch https://review.openstack.org/#/c/432587/ ? | 08:48 |
| Qiming | sure | 08:48 |
| elynn | Can I call you via wechat? | 08:49 |
| Qiming | inconenient for voice call atm | 08:49 |
| elynn | okay, Let me try to explain here. | 08:49 |
| elynn | All the logic starts from _update_network() | 08:49 |
| Qiming | then rename all functions called there as _update_something() | 08:50 |
| elynn | It will call _create_interfaces to create newly added networks and call _delete_interfaces() to remove unnecessary networks. | 08:50 |
| elynn | So what I do is changing the logic of _create_interfaces and _delete_interfaces. | 08:51 |
| Qiming | that is fine, if both functions are kept reasonably small | 08:51 |
| Qiming | okay, that part I understand | 08:51 |
| elynn | Qiming, You mean to rename _create_interfaces and _delete_interfaces? | 08:52 |
| Qiming | raising a EResourceUpdate from inside a _create_interfaces function is confusing | 08:52 |
| Qiming | especially when you are again calling server_interface_craete inside that function | 08:52 |
| elynn | _create_interfaces only used in _update_network() | 08:53 |
| elynn | So ... maybe I should rename it... | 08:53 |
| Qiming | that function was added in previous patch | 08:54 |
| Qiming | you mentioned that it was used for both creation and update | 08:54 |
| elynn | The function _create_ports_from_properties is introduced in previous patch. | 08:54 |
| elynn | to create ports | 08:54 |
| Qiming | only for creation's purpose? | 08:55 |
| elynn | yes, only for creation purpos | 08:55 |
| elynn | e | 08:55 |
| Qiming | that is fine | 08:55 |
| elynn | The third parameters only used to raise different error in profile creation or profile update. | 08:56 |
| Qiming | because that function is SHARED in both workflows | 08:56 |
| *** chohoor has quit IRC | 08:56 | |
| Qiming | that is a valid case for validation, because validation is needed no matter you are creating something new or updating something existing | 08:57 |
| elynn | https://github.com/openstack/senlin/blob/master/contrib/vdu/vdu/server.py#L645 | 08:57 |
| elynn | Here. | 08:57 |
| Qiming | yes, that is a validation logic | 08:57 |
| elynn | yes | 08:57 |
| elynn | This functions will create ports and return a list of created ports' dict. | 08:58 |
| elynn | Then later in _create_interfaces L1057 | 08:59 |
| Qiming | which function? | 08:59 |
| elynn | The _create_ports_from_properties function | 08:59 |
| Qiming | okay, that logic is shared between create/update workflows, right? | 08:59 |
| elynn | Yes. | 09:00 |
| elynn | _create_ports_from_properties is shared between create and update | 09:00 |
| Qiming | if that function is already doing port creation, why do we add _create_interfaces ? | 09:01 |
| elynn | _create_interfaces actually doing two things | 09:01 |
| elynn | Create ports and attach to server. | 09:02 |
| Qiming | it is doing several things I'm afraid | 09:02 |
| Qiming | it is even validating if the server is there ... | 09:02 |
| elynn | That's because it is called by _update_network | 09:02 |
| elynn | in profile update workflow. | 09:02 |
| Qiming | why we didn't check if the server is there before updating anything? | 09:03 |
| elynn | So first it will check if server is existed. | 09:03 |
| *** chohoor has joined #senlin | 09:03 | |
| elynn | Hmm, good question, I will check | 09:03 |
| elynn | and remove the server check in _create_interfaces | 09:03 |
| elynn | didn't touch that code. | 09:04 |
| Qiming | the key points ... | 09:04 |
| Qiming | walk through the creation / update workflow | 09:04 |
| Qiming | make sure necessary things are done and only done once | 09:04 |
| elynn | yes, They only share the _create_ports_from_properties part. | 09:05 |
| Qiming | point 2, stop introducing embedded functions, unless they are very straighforward, where even a unit test is not needed | 09:05 |
| elynn | Then in creation workflow, we call server_create to create a new instance with created ports. | 09:05 |
| elynn | Qiming, okay, I will remove the existing two inline functions. | 09:06 |
| Qiming | if there is a need, feel free to move some util functions to neutron driver or even add a utils module | 09:07 |
| Qiming | I see, _delete_interfaces and _add_interfaces were added by me, possibly | 09:08 |
| Qiming | very bad name for code reviewers | 09:08 |
| elynn | Do you have any suggestions to rename it? | 09:08 |
| elynn | And what functions you think are better put into utils module? | 09:09 |
| Qiming | e.g. _update_network_ports_removed, _updated_network_ports_added, | 09:10 |
| elynn | sounds good. | 09:10 |
| Qiming | the _get_port_attrs function doesn't belong to this VDU/server module | 09:11 |
| Qiming | it seems you want to get a port with its floating ips | 09:12 |
| Qiming | that should be moved elsewhere | 09:12 |
| elynn | yes, I want to get the port information with floating IP associated with it. | 09:12 |
| elynn | Put it in neutron driver? | 09:13 |
| elynn | it only return a dict | 09:13 |
| elynn | not a port object. | 09:13 |
| Qiming | it is called on line 1164, which seems the only location | 09:14 |
| elynn | Yes | 09:14 |
| Qiming | take those lines for example | 09:15 |
| Qiming | 1164 gets ports as dict, with floating ips injected | 09:15 |
| Qiming | then 1168 is doing another search | 09:16 |
| Qiming | that search invokes _find_port_by_net_spec | 09:16 |
| elynn | right | 09:16 |
| Qiming | which again invokes network_get func | 09:17 |
| Qiming | what are we doing here? | 09:17 |
| elynn | yes, translate floating_network to ID | 09:17 |
| elynn | When we define a profile | 09:17 |
| elynn | normally we use a name for floating_network | 09:18 |
| Qiming | right | 09:18 |
| Qiming | this is about deleting existing interfaces | 09:19 |
| elynn | hmm, let me see if a name existed in a floating IP dict... | 09:19 |
| elynn | Maybe I could just match the name instead of ID, that would save a network_get call | 09:20 |
| elynn | Can't remember why I add this logic... | 09:20 |
| Qiming | a possible way to move this forward ... | 09:21 |
| elynn | I can optimize that. | 09:21 |
| Qiming | handle one case in one patch | 09:21 |
| Qiming | my brain is too small .... | 09:21 |
| Qiming | need some nutrition I assure you | 09:21 |
| elynn | okay , Let me explain the delete_interface workflow. | 09:21 |
| elynn | It's simple too :) | 09:22 |
| Qiming | not simple | 09:22 |
| elynn | Find a port matching the network property, then delete it. | 09:22 |
| elynn | L1168 is to find the port | 09:22 |
| elynn | L1170 delete it | 09:23 |
| elynn | The complex part is _find_port_by_net_spec | 09:23 |
| Qiming | the port is from profile spec or we created them? | 09:23 |
| openstackgerrit | Merged openstack/senlin master: Modify network name in examples files https://review.openstack.org/436805 | 09:23 |
| Qiming | if, for example, we created them, why don't we save their network id in the beginning to save a network_get call? | 09:24 |
| elynn | save to where? | 09:24 |
| Qiming | if it is "internal created" | 09:25 |
| elynn | Yes, all ports are internal created | 09:26 |
| elynn | except for ' port: xxx' one. | 09:26 |
| elynn | You mean use 'internal_ports' to save the network ID and all parameters? | 09:27 |
| Qiming | I somehow recalled why I was treating update differently from create when validating network | 09:27 |
| elynn | Why? | 09:27 |
| Qiming | try get the ID if possible | 09:27 |
| Qiming | avoid using names when possible | 09:28 |
| Qiming | names requires a cross-check later | 09:28 |
| elynn | But that will store many informations in node.data | 09:28 |
| Qiming | node.data is already polluted | 09:28 |
| Qiming | you are putting port and floating ips separately, aren't you | 09:29 |
| elynn | yes | 09:29 |
| Qiming | is it possible to make floating ip a sub key of port? | 09:30 |
| elynn | I think that's doable | 09:30 |
| Qiming | line 1142 | 09:31 |
| Qiming | it is only about deleting cached ports | 09:31 |
| Qiming | which is also called 'internal_ports' | 09:31 |
| elynn | not exactly... | 09:31 |
| elynn | server_interface_delete detach the port from server. | 09:32 |
| elynn | If a port is created by this profile, then need to call port_delete to delete it. | 09:32 |
| Qiming | but we may leave that port there | 09:32 |
| elynn | Leave that port there? | 09:33 |
| Qiming | leave that port undeleted | 09:33 |
| elynn | Why? | 09:33 |
| Qiming | if the port was provided in the profile spec | 09:33 |
| elynn | Yes, then we don't delete it. | 09:34 |
| elynn | L 1146 | 09:34 |
| Qiming | this is an example of mixing two logics into the same function | 09:35 |
| elynn | it's a inline function, I will expand it in later patch. | 09:35 |
| Qiming | not just about inline or not | 09:36 |
| elynn | Do you suggest to..? | 09:39 |
| Qiming | if I'm revising this logic into a separate function, I'm gonna do this: | 09:43 |
| Qiming | def _update_network_remove_port(node, port_id): | 09:43 |
| Qiming | internal_ports = node.data.get('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 it | 09:43 |
| Qiming | if port_id in internal_ports: | 09:43 |
| Qiming | internal_ports.remove(port_id) | 09:43 |
| Qiming | node.data['internal_ports'] = internal_ports | 09:43 |
| Qiming | no.Node.update(self.context, node.id, {'data': node.data}) | 09:43 |
| Qiming | 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 |
| elynn | That is almost the same, I guess? | 09:45 |
| Qiming | yes | 09:45 |
| Qiming | try write test cases for these two versions | 09:46 |
| Qiming | how many flows you will need to cover? | 09:46 |
| elynn | can not tell.. | 09:47 |
| elynn | Is there any difference? | 09:48 |
| Qiming | readability | 09:49 |
| Qiming | several changes I made | 09:49 |
| Qiming | 1. rename obj to node, because this is a standalone function, it has its own logic, renaming to 'node' helps understanding | 09:50 |
| Qiming | 2. avoid spliting the case of internal_ports into two segments, some inside try except, some left outside | 09:51 |
| Qiming | 3. add a comment before that 'if' statement, so if I'm don't care about internal ports, the flow is very clean | 09:51 |
| elynn | niu! sorry, can't catch your mind just now :P | 09:52 |
| elynn | Now I get it. | 09:52 |
| Qiming | just for your reference | 09:52 |
| elynn | okay, to summarized, two parts need to optimize, 1. reduce API call to get ID, 2. Make code readable. | 09:54 |
| elynn | right? | 09:54 |
| Qiming | yep | 09:57 |
| Qiming | we all forget why we wrote that code | 09:58 |
| elynn | Thanks :) | 09:58 |
| Qiming | code readability is very important | 09:58 |
| Qiming | welcome | 09:58 |
| elynn | agree, and learned. | 09:58 |
| *** yanyanhu has quit IRC | 10:04 | |
| *** XueFeng has quit IRC | 10:30 | |
| *** guoshan has quit IRC | 10:31 | |
| openstackgerrit | XueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None https://review.openstack.org/436888 | 11:38 |
| openstackgerrit | XueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None https://review.openstack.org/436888 | 11:40 |
| *** ruijie has quit IRC | 11:41 | |
| openstackgerrit | XueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None https://review.openstack.org/436888 | 12:06 |
| openstackgerrit | XueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None https://review.openstack.org/436888 | 12:07 |
| openstackgerrit | XueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None https://review.openstack.org/436888 | 13:39 |
| *** zhurong has joined #senlin | 13:47 | |
| openstackgerrit | XueFeng Liu proposed openstack/senlin master: Fix do_recover problem when operation is None https://review.openstack.org/436888 | 14:15 |
| *** Jeffrey4l has quit IRC | 14:38 | |
| *** Jeffrey4l has joined #senlin | 14:50 | |
| *** Jeffrey4l has quit IRC | 14:52 | |
| *** Jeffrey4l has joined #senlin | 14:52 | |
| *** zhurong has quit IRC | 15:07 | |
| *** catintheroof has joined #senlin | 15:43 | |
| *** catintheroof has quit IRC | 16:33 | |
| *** catintheroof has joined #senlin | 16:38 | |
| *** catintheroof has quit IRC | 16:38 | |
| *** catintheroof has joined #senlin | 16:38 | |
| *** catintheroof has quit IRC | 16:43 | |
| *** catintheroof has joined #senlin | 16:53 | |
| *** catintheroof has quit IRC | 16:58 | |
| *** catintheroof has joined #senlin | 17:15 | |
| *** catintheroof has quit IRC | 17:18 | |
| *** catintheroof has joined #senlin | 17:19 | |
| *** edisonxiang has quit IRC | 19:04 | |
| *** edisonxiang has joined #senlin | 19:05 | |
| *** Jeffrey4l_ has joined #senlin | 19:31 | |
| *** Jeffrey4l has quit IRC | 19:31 | |
| *** Jeffrey4l_ has quit IRC | 19:33 | |
| *** Jeffrey4l_ has joined #senlin | 19:33 | |
| *** catintheroof has quit IRC | 19:48 | |
| *** catintheroof has joined #senlin | 20:11 | |
| *** catintheroof has quit IRC | 20:11 | |
| *** catintheroof has joined #senlin | 20:12 | |
| *** Jeffrey4l__ has joined #senlin | 21:34 | |
| *** Jeffrey4l_ has quit IRC | 21:35 | |
| *** catintheroof has quit IRC | 22:47 | |
Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!