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