14:00:43 <ihrachys> #startmeeting neutron_upgrades
14:00:44 <openstack> Meeting started Thu Aug 24 14:00:43 2017 UTC and is due to finish in 60 minutes.  The chair is ihrachys. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:00:45 <ihrachys> lujinluo, hi
14:00:45 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
14:00:47 <openstack> The meeting name has been set to 'neutron_upgrades'
14:00:53 <ihrachys> manjeets, is off today
14:00:55 <lujinluo> ihrachys: o/
14:00:58 <TuanVu> Hi everybody
14:01:01 <lujinluo> i see
14:01:05 <ihrachys> TuanVu, hey!
14:01:38 <ihrachys> we have no action items from prev meeting. I think we can just run through patches up for review.
14:01:39 <annp_> Hi
14:01:43 <ihrachys> annp_, hey!
14:01:49 <TuanVu> Hi ihrachys
14:02:00 <annp_> Hi ihrachys. :)
14:02:32 <ihrachys> #topic OVO patches
14:02:48 <ihrachys> https://review.openstack.org/#/q/status:open+project:openstack/neutron+branch:master+topic:bp/adopt-oslo-versioned-objects-for-db
14:03:08 <ihrachys> Floating IP: https://review.openstack.org/#/c/396351/
14:03:24 <lujinluo> I have a question regarding floating ip
14:03:31 <lujinluo> i met check policy failure
14:03:40 <lujinluo> because of the absence of tenant_id
14:03:56 <ihrachys> where is the error?
14:04:12 <lujinluo> e.g http://logs.openstack.org/51/396351/28/check/gate-tempest-dsvm-neutron-full-ubuntu-xenial/0e31319/console.html#_2017-08-24_09_10_47_777599
14:04:39 <lujinluo> It ha in API layer, as I observed
14:04:44 <lujinluo> It is*
14:05:27 <ihrachys> lujinluo, we probably return a dict from plugin to api layer that doesn't have tenant_id in it
14:05:55 <lujinluo> yes, but how can we mitigate that? i mean we switched from tenant_id to project_id in ovo
14:06:15 <lujinluo> there is no longer tenant_id in floating ip ovo
14:06:21 <ihrachys> I think we still return tenant_id (in addition to project_id) in dicts generated with .to_dict()
14:06:30 <lujinluo> i see.
14:06:42 <lujinluo> i will add tenant_id to .to_dict() then
14:07:08 <ihrachys> I think it should already be there
14:07:15 <ihrachys> sec
14:07:32 <lujinluo> it is possible that i deleted it, not 100% sure though
14:08:04 <ihrachys> this is generic implementation for tenant_id field: https://github.com/openstack/neutron/blob/master/neutron/objects/base.py#L244-L247
14:08:39 <ihrachys> and this, I think, should add tenant_id to the dict: https://github.com/openstack/neutron/blob/master/neutron/objects/base.py#L121-L125
14:09:33 <lujinluo> i did not modify base.py, then they are not working as expected..?
14:10:41 <ihrachys> for what I understand, the request that fails is to get floatingip
14:10:49 <ihrachys> which should be get_floatingip in plugin code
14:11:41 <ihrachys> this: https://github.com/openstack/neutron/blob/master/neutron/db/l3_db.py#L1390-L1393
14:12:03 <ihrachys> in your patch, you changed the type of result of _get_floatingip to an OVO object
14:12:13 <ihrachys> (before the patch, it was a sqlalchemy model)
14:12:31 <lujinluo> Ahh, i see. https://review.openstack.org/#/c/396351/28/neutron/db/l3_db.py@1037 i should change this line as well
14:12:36 <ihrachys> that being said, _make_floatingip_dict should have correctly extract tenant_id: https://github.com/openstack/neutron/blob/master/neutron/db/l3_db.py#L1036
14:13:11 <ihrachys> lujinluo, yeah, I am not sure we implement __getitem__ for tenant_id
14:13:29 <lujinluo> it should be floatingip['project_id']. thanks, i will double check tmr
14:13:34 <ihrachys> cool
14:13:52 <ihrachys> next is https://review.openstack.org/#/c/495810/ "Use Agent OVO in agents_db and test_agents_db"
14:14:17 <ihrachys> it's pretty red
14:14:25 <TuanVu_> hi, we're still working on it
14:14:34 <annp__> I and Tuan just working on it
14:14:46 <annp__> we have issue with _get_dict
14:14:57 <ihrachys> ack, we'll look closer when it's ready
14:15:10 <ihrachys> next is https://review.openstack.org/#/c/370452/ "OVO for NetworkDhcpAgentBinding"
14:15:19 <TuanVu_> hi Ihrachys
14:15:25 <TuanVu_> could you please wait
14:15:43 <ihrachys> TuanVu_, eh, sure. what's up?
14:15:54 <TuanVu_> we have one concern and hopefully you could help
14:16:13 <TuanVu_> yes, regarding to _get_dict
14:16:49 <ihrachys> TuanVu_, which patch do we talk about, agent one?
14:16:55 <TuanVu_> https://review.openstack.org/#/c/495810/5/neutron/db/agents_db.py
14:16:59 <TuanVu_> yes
14:17:10 <TuanVu_> line 178
14:17:42 <TuanVu_> for this function
14:17:44 <TuanVu_> conf = jsonutils.loads(json_value)
14:18:03 <TuanVu_> we cannot load json_value
14:18:21 <TuanVu_> it looks like there's problem with unicode string
14:18:41 <ihrachys> have you checked that json_value is a legit JSON string?
14:18:45 <TuanVu_> therefore, there are several tests failed and we still couldn't figure out how to fix it
14:19:37 <lujinluo> is is because in OVO, "configuraton" is DictOfMiscValuesField?
14:20:00 <TuanVu_> hmm, we've tried to dump it, and it looks legit
14:20:16 <TuanVu_> could you please clarify more, Ihra and Luo ?
14:20:21 <ihrachys> TuanVu_, I think lujinluo is onto smth here. maybe the attribute you extract is already a dict
14:20:30 <ihrachys> so you may not need to do .loads()
14:20:47 <ihrachys> have you checked its type?
14:20:59 <TuanVu_> what do you think, An-san?
14:21:03 <lujinluo> https://review.openstack.org/#/c/411830/ this patch declares the type pf "configurations"
14:21:19 <ihrachys> TuanVu_, f.e see https://review.openstack.org/#/c/495810/5/neutron/objects/agent.py@75
14:21:36 <annp> sorry, i just lost connection. :)
14:21:53 <annp> let me see
14:22:16 <TuanVu_> thank you, Luo and Ihra, I'm also checking
14:23:01 <annp> I will try with ihrachys's suggestion tomorrow,
14:23:09 <annp> Please go ahead
14:23:13 <ihrachys> ok, thanks
14:23:21 <ihrachys> next is https://review.openstack.org/#/c/370452/ "OVO for NetworkDhcpAgentBinding"
14:23:34 <ihrachys> I already +2d it, we need 2nd opinion to merge
14:24:11 <ihrachys> moving on. next is https://review.openstack.org/#/c/361443/, "OVO for L3HARouter"
14:24:42 <ihrachys> I need to repeat review for the patch, I see TuanVu_ respinned it yesterday
14:24:51 <TuanVu_> yes, I did
14:25:16 <TuanVu_> thanks for your insightful review, Ihrachys
14:25:30 <ihrachys> I see some discussion here: https://review.openstack.org/#/c/361443/46/neutron/objects/l3_hamode.py@53 give me a second to read it
14:26:23 <TuanVu_> ok
14:26:52 <ihrachys> ok I see what's going on there. I think it's fine to leave it as-is. I would suggest we leave a TODO here to get back to it, but it's not a must.
14:27:15 <ihrachys> I think it's the right thing not to try to unravel the whole stack of interdependencies in one go\
14:27:32 <TuanVu_> ok then
14:27:51 <ihrachys> next is https://review.openstack.org/#/c/429829/, "Allow Agent object to be queryable by dict's key field"
14:28:08 <ihrachys> TuanVu_, you respinned it
14:28:20 <ihrachys> I wonder whether it's still needed
14:28:39 <ihrachys> the patch is quite old, not sure it applies now. do we have some code that would benefit from it?
14:28:54 <lujinluo> i went through the unittest of agent, and it seems we do not need it anymore
14:29:27 <TuanVu_> yeah, I've checked that too
14:29:47 <ihrachys> ok, then let's abandon? we can always revive if needed.
14:29:51 <TuanVu_> but since being a newbie, I'm not sure if we still need it
14:30:17 <TuanVu_> ok, no problem
14:30:45 <TuanVu_> I'll abandon this patch
14:30:49 <ihrachys> thanks!
14:31:08 <ihrachys> there are no more patches in the review queue that have new developments
14:31:36 <ihrachys> any patch from those I haven't mentioned that you would like to discuss?
14:32:08 <lujinluo> ihrachys: thanks for changing the topic of Port patch! I always forgot to do it  ;)
14:32:14 <TuanVu_> Hi
14:32:20 <TuanVu_> I have one question
14:32:23 <ihrachys> TuanVu_, shoot
14:32:52 <TuanVu_> about the strategy for updating patches for Agent (eg: the order of files to be updated)
14:34:39 <ihrachys> TuanVu_, what's the question?
14:36:01 <ihrachys> have we lost TuanVu_ ?
14:36:13 <TuanVu_> after working on "agents_db", I realize that when continue working on other files (eg: l3agentscheduler), there are lots of changes need to be done on agents_db
14:36:40 <TuanVu_> sorry, I'm trying to explain my concern in a simple way
14:36:54 <ihrachys> TuanVu_, np, I thought that's connectivity issues
14:37:12 <ihrachys> TuanVu_, so in general, I think it's fine to split patches as much as to simplify review / reduce their size
14:37:17 <ihrachys> it's easier to land small patches
14:37:30 <TuanVu_> yes, I totally agree with that idea
14:37:46 <ihrachys> there is no requirement that you switch all uses of a model in one go
14:37:51 <TuanVu_> but regarding to the order of file to be updated
14:38:33 <ihrachys> how does the order play out in this particular case?
14:38:54 <TuanVu_> especially for Agent OVO which is pretty big and complicated, do you have any suggestion for "the order of files to be updated"
14:39:07 <TuanVu_> eg: which one first, then what is the next one
14:39:27 <TuanVu_> yes, the order plays a pretty big deal
14:39:42 <ihrachys> there are two files at play, agents_db and l3agentscheduler? or there are more?
14:40:35 <TuanVu_> well, there are lots of files need to be updated for Agent OVO (about 10, I think)
14:40:58 <TuanVu_> https://review.openstack.org/#/c/495810/
14:41:32 <TuanVu_> after above patch, I'm about to move on to l3agentscheduler
14:41:49 <TuanVu_> and when working with l3agentscheduler, I'll need to update agents_db again
14:42:04 <TuanVu_> so that's the problem I would like to mention here
14:42:13 <ihrachys> ok. I don't have specifics to give very specific answer, but what I would do is I would try to take the smallest bit you can tackle from one of those files [f.e. a single method], try to migrate it to OVO and see where it leads you (and do the minimal necessary change in other files to adopt to the change and pass CI without touching other code)
14:42:38 <ihrachys> TuanVu_, oh, you mean you want to base your scheduler work on that other patch?
14:42:40 <TuanVu_> I mean, looks like it would be better to update agents_db at the end
14:42:51 <ihrachys> if so, you can stack them in git
14:43:51 <TuanVu_> thank you, Ihar
14:43:59 <TuanVu_> I got your idea :)
14:44:17 <ihrachys> TuanVu_, in the agents_db patch, if the only remaining issue is with json.loads() and it will be ready to merge, I think that's what we should concentrate on
14:44:17 <TuanVu_> "I would try to take the smallest bit you can tackle from one of those files [f.e. a single method], try to migrate it to OVO and see where it leads you (and do the minimal necessary change in other files to adopt to the change and pass CI without touching other code)"
14:44:33 <ihrachys> but in the meantime, you can work on scheduler patch, just make sure you base it on the agents_db one
14:45:02 <TuanVu_> yes, thanks for your advice
14:45:08 <TuanVu_> I really appreciate it :)
14:45:16 <ihrachys> you do it with: git review -d 495810 ; <edit scheduler code>; git commit -a -m "blablabla"; git review
14:45:41 <ihrachys> the first command (git review -d ...) will download the state of git tree with the agents_db patch on top
14:45:51 <TuanVu_> yeah, I got your idea
14:46:10 <ihrachys> so when you execute the last command (git review), it will send both patches, but the first is the same, so it is not affected
14:46:15 <ihrachys> ok, cool
14:46:32 <ihrachys> I guess that's all we had for today unless someone has anything more to discuss
14:46:43 <lujinluo> none from me
14:46:51 <annp_> me too
14:46:57 <TuanVu_> same here
14:47:01 <ihrachys> TuanVu_, annp_, lujinluo thanks for all the effort
14:47:10 <ihrachys> #endmeeting