14:02:16 <ihrachys> #startmeeting neutron_upgrades 14:02:16 <openstack> Meeting started Thu Dec 14 14:02:16 2017 UTC and is due to finish in 60 minutes. The chair is ihrachys. Information about MeetBot at http://wiki.debian.org/MeetBot. 14:02:17 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 14:02:19 <openstack> The meeting name has been set to 'neutron_upgrades' 14:02:24 <annp> hi 14:02:28 <lujinluo> o/ 14:02:31 <TuanVu> Hi guys 14:03:09 <openstack> ihrachys_: Error: Can't start another meeting, one is in progress. Use #endmeeting first. 14:03:27 <ihrachys_> sorry I was dropped 14:03:33 <TuanVu> no problem 14:03:36 <ihrachys_> I presume bot still considers the meeting started :) 14:03:53 <lujinluo> yes 14:04:03 <ihrachys_> there are no AIs 14:04:05 <ihrachys_> #topic OVO 14:04:26 <ihrachys_> https://review.openstack.org/#/q/status:open+project:openstack/neutron+branch:master+topic:bp/adopt-oslo-versioned-objects-for-db 14:04:36 <ihrachys_> going top to botton 14:04:40 <ihrachys_> *bottom 14:04:49 <ihrachys_> https://review.openstack.org/#/c/521797/ "Use Router OVO in external_net_db" 14:05:44 <ihrachys_> https://review.openstack.org/#/c/521797/10/neutron/db/external_net_db.py 14:05:55 <ihrachys_> hungpv, I don't follow what you try to achieve there 14:06:08 <hungpv> Hi 14:06:25 <ihrachys_> you shouldn't need any of that. just build filter dict and pass it to get_objects 14:06:59 <hungpv> I was trying that way 14:07:06 <hungpv> But it does 14:07:07 <ihrachys_> the only complexity there is that filter dict is built in several gos 14:08:15 <hungpv> It didn't pass the UT 14:08:33 <hungpv> So I come up with that so solution 14:08:35 <ihrachys_> hungpv, well, then unit test may need a refinement. do you have a link to the failure? 14:09:34 <ihrachys_> hungpv, we shouldn't routinely add new methods to objects. only when it's absolutely needed. and we should not routinely return sqlalchemy queries from those methods. 14:09:41 <ihrachys_> I think get_objects is fine for the job 14:09:46 <ihrachys_> and you just build the dict and pass it 14:09:54 <ihrachys_> I would need to see the failure to judge more 14:10:08 <ihrachys_> but it seems like the path forward there 14:10:34 <ihrachys> lujinluo, hungpv do I make sense there? 14:10:59 <lujinluo> yes, i think get_objects() would do the work 14:11:43 <lujinluo> also we need to use LIKE statement 14:13:05 <hungpv> Hum I p'm 14:13:16 <hungpv> Thinking about that 14:13:27 <ihrachys> lujinluo, LIKE for what exactly? 14:14:10 <hungpv> But I'm kinda stuck for that way. Trying to build filter dict, and somehow it didn't work 14:14:51 <ihrachys> I see there is some more complexity there for rbac 14:15:00 <lujinluo> i took a glance at Line 235 in old codes "l3_models.Router.tenant_id.in_(tenants_with_entries)" 14:15:19 <lujinluo> tenants_with_entries comes from RBAC 14:15:28 <ihrachys> yeah 14:15:53 <ihrachys> it's SELECT ... WHERE tenant_id IN (SELECT ...) 14:16:17 <ihrachys> this can't be easily modeled directly in OVO 14:17:11 <ihrachys> and there is also NOT there 14:17:20 <ihrachys> ~tenant_id.in_(...) 14:17:48 <ihrachys> that's another thing not available in OVO 14:17:55 <lujinluo> how about we manipulate with queried router ovo objects to find all router whose tenant does not belong to tenants_with_entries 14:19:37 <ihrachys> thinking 14:20:24 <lujinluo> line 206 and 207 is easier 14:21:10 <lujinluo> we use get_objects(), then iterate every one of them to find gw_port_id in ports 14:21:11 <ihrachys> for rbac subquery, we could just split it out 14:21:19 <ihrachys> it shouldn't affect scalability 14:21:29 <ihrachys> because it's O(1) 14:21:43 <ihrachys> as for NOT, I think we may need to add it to OVO layer first 14:23:11 <lujinluo> hmm, is router the only object we used NOT? 14:23:17 <ihrachys> lujinluo, yes, ports calculation can be split too 14:23:34 <ihrachys> lujinluo, apparently. we didn't have a need it seems before. 14:24:34 <lujinluo> ok, then maybe we should add NOT first 14:25:18 <ihrachys> another 'solution' is to actually have a specialized method for that matter (count_by_tenant?) to accommodate for lines 234-240 14:26:13 <ihrachys> and leave NOT for later 14:26:34 <ihrachys> but I am confident that the != '*' case doesn't need any of that, so we could simplify it there 14:27:31 <lujinluo> i would prefer we get things done first, and leave some follow-ups to revisit 14:27:41 <ihrachys> ok let's go this route. 14:27:59 <hoangcx_> +1 14:31:00 <ihrachys> ok next is https://review.openstack.org/#/c/527570/ "Remove _get_subnets_by_cidr from neutron/db/extraroute_db.py" 14:31:09 <ihrachys> I see it's +2, checking if I can nudge right away 14:31:26 <TuanVu> thank you, Ihar 14:31:41 <ihrachys> ok merging 14:31:49 <lujinluo> \o/ 14:31:52 <ihrachys> https://review.openstack.org/#/c/506037/ "Part II of Integrate Port OVO" 14:32:01 <ihrachys> this seems not ready for prime time 14:32:04 <TuanVu> awesome! thanks a lot! 14:32:04 <ihrachys> lujinluo, correct? 14:32:17 <annp> thanks Ihar and Tuanvu :) 14:32:18 <lujinluo> i am pissed off by all the mac_address modification 14:32:27 <lujinluo> it comes back and forth 14:33:14 <lujinluo> anyway, i will try to fix (hopefully) the remaining missing parts 14:33:46 <ihrachys> ok. 14:34:02 <ihrachys> https://review.openstack.org/#/c/407868/ "Integration of (Distributed) Port Binding OVO" was quite close to ready the last time I checked 14:34:15 <lujinluo> slawek pointed a potential race condition 14:34:53 <lujinluo> would you mind have a look? i was thinking making all the delete() + some modifications + create() in one transaction? 14:35:09 <ihrachys> https://review.openstack.org/#/c/407868/80/neutron/objects/ports.py@53 ? 14:35:43 <lujinluo> yes 14:36:53 <ihrachys> ok. I think what slaweq suggests is to have a subtransaction opened for the whole body of update 14:37:38 <ihrachys> like we do here https://github.com/openstack/neutron/blob/master/neutron/objects/base.py#L541 14:37:55 <lujinluo> yes, this is what i was thinking too 14:37:59 <ihrachys> so _load_object is called in scope of the same transaction 14:38:06 <ihrachys> making sure we can load all attributes 14:39:31 <lujinluo> do we also want a test for it? i was not able to come up with one 14:39:56 <ihrachys> I don't think so. it's one of those cases where it just makes sense to have the code to avoid a potential for a race 14:40:15 <ihrachys> also, I think we have a test somewhere that validates a single commit() 14:40:48 <lujinluo> good. i will only add the subtransaction then 14:40:58 <ihrachys> https://github.com/openstack/neutron/blob/master/neutron/tests/unit/objects/test_base.py#L1652-L1665 14:41:38 <ihrachys> though it doesn't seem like it failed anywhere here: http://logs.openstack.org/37/506037/9/check/openstack-tox-py27/ea53df0/testr_results.html.gz 14:42:39 <ihrachys> it's actually surprising 14:43:06 <ihrachys> ah no, it's ot 14:43:07 <ihrachys> *not 14:43:09 <lujinluo> ^ bad link I guess? 14:43:11 <ihrachys> you have some 'new_host' there 14:43:14 <ihrachys> what's that? 14:43:20 <ihrachys> why not reusing 'host'? 14:43:59 <ihrachys> lujinluo, eh, yeah this is correct one: http://logs.openstack.org/68/407868/81/check/openstack-tox-py27/3520ebb/testr_results.html.gz 14:44:03 <ihrachys> but it's still passing 14:44:12 <ihrachys> I think it's because in the test case, we set all fields 14:44:13 <lujinluo> cause then we cannot do self.delete() 14:44:24 <ihrachys> but since new_host is not a field, it's not set and so super().update() is called 14:44:25 <lujinluo> cause the binding with updated host does not exist yet 14:44:59 <ihrachys> lujinluo, what we could do is store the original host in e.g. _load_object 14:45:09 <ihrachys> lujinluo, and then reuse it whenever we update the field 14:45:12 <ihrachys> to delete 14:45:22 <ihrachys> and then use the new field value when creating 14:45:33 <ihrachys> so in that way, you have both values available 14:45:50 <ihrachys> it's actually surprising that we don't have a way to get old values of fields 14:45:57 <ihrachys> in oslo.versionedobjects 14:46:01 <lujinluo> yes 14:46:08 <ihrachys> we can only get a list of field *names* that were touched 14:46:19 <lujinluo> exactly 14:46:31 <ihrachys> so, I think we can make an exception for host attribute and cache it on _load_object 14:46:35 <ihrachys> does it make sense? 14:47:02 <ihrachys> of course, that would belong to the object implementation not base _load_object 14:47:06 <lujinluo> it does, but i also need to double check maybe tmr 14:47:21 <ihrachys> so you would _load_object and then also setattr(obj, '_cached_host', obj.host) 14:49:12 <lujinluo> i am having difficulty understanding '_cached_host' here 14:49:24 <lujinluo> do you still suggest i add it as an attribute? 14:49:34 <ihrachys> as an attribute, yes 14:49:52 <ihrachys> but it would store the original host not the new one 14:50:02 <ihrachys> new one would still be using the host field of the object 14:50:12 <lujinluo> ah, i see your point 14:50:30 <ihrachys> then in update(), when you first need to .delete() you would construct a new object using the cached host value, then call .delete() on it. 14:50:41 <ihrachys> after that, you can move forward with .create() 14:50:58 <lujinluo> it saves the clearance of 'new_host' 14:51:21 <ihrachys> it should also make the UT fail 14:51:44 <ihrachys> I would suggest to do the changes without subtransaction, then validate the test fails, then add subtransaction to check it now passes 14:52:04 <lujinluo> ack! 14:52:13 <ihrachys> cool! 14:52:27 <ihrachys> https://review.openstack.org/#/c/527330/ "l3_agentschedulers_db: convert from Agent model to OVO" 14:52:56 <ihrachys> I had a comment there 14:53:15 <TuanVu> yes, Ihar. Could you please also check comment from An-san 14:53:18 <TuanVu> ? 14:53:29 <ihrachys> basically, since now we fetch using OVO interface, we should check that OVO class has a field, not that it has an attribute with the name 14:55:24 <TuanVu> hmm ... 14:55:35 <ihrachys> TuanVu, is it for [None] value? 14:55:55 <ihrachys> I think value there is expected as None not [None]? 14:56:07 <ihrachys> [None] would actually be true in boolean context 14:57:09 <annp> ihrachys, yes, you're right. 14:57:30 <annp> ihrachys, sorry i misunderstood here. 14:57:34 <ihrachys> np 14:57:51 <TuanVu> ok, I got it 14:57:52 <ihrachys> I am honestly not entirely sure why this code is there but I would be hesitant to touch it 14:59:00 <ihrachys> ok I think we are out of time. anything quick to discuss before we wrap up? 15:00:01 <TuanVu> so we should abandon this patch? 15:00:08 <ihrachys> no why? 15:00:47 <TuanVu> as you mentioned, I guess we don't need to touch that snippet anymore 15:00:54 <lujinluo> no, you just do not use getattr() 15:01:07 <ihrachys> I think it's good. we still should move to OVO object. it just has different way to express presence of a field 15:01:14 <ihrachys> right. 15:01:31 <annp> ihrachys, ok, got it. 15:01:37 <ihrachys> use Agent.fields.get(...) 15:01:44 <TuanVu> thank you, Ihar and Luo 15:01:44 <ihrachys> ok we need to wrap up 15:01:50 <ihrachys> thanks for all the work you do folks! 15:01:52 <ihrachys> #endmeeting