14:01:31 <ihrachys> #startmeeting neutron_upgrades
14:01:32 <openstack> Meeting started Thu Nov 30 14:01:31 2017 UTC and is due to finish in 60 minutes.  The chair is ihrachys. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:01:33 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
14:01:35 <openstack> The meeting name has been set to 'neutron_upgrades'
14:01:41 <annp> ihrachys, Hi
14:01:51 <lujinluo> o/
14:01:54 <TuanVu> Hi everybody
14:02:02 <ihrachys> hi!!
14:02:52 * ihrachys waves at hungpv
14:03:07 <ihrachys> #topic Action items from prev meeting
14:03:14 <ihrachys> there was a single one on me
14:03:15 <ihrachys> "ihrachys to categorize remaining models imports"
14:03:21 <ihrachys> I totally forgot about it :-x
14:03:47 <ihrachys> I will take it to the next time
14:03:52 <ihrachys> #action ihrachys to categorize remaining models imports
14:03:55 <ihrachys> sorry for that
14:04:02 <ihrachys> #topic OVO patches
14:04:05 <lujinluo> never mind. ;)
14:04:12 <ihrachys> https://review.openstack.org/#/q/status:open+project:openstack/neutron+branch:master+topic:bp/adopt-oslo-versioned-objects-for-db
14:04:42 <ihrachys> before going through the list, there is another OVO patch here: https://review.openstack.org/#/c/519762/
14:04:55 <ihrachys> the one that reverts the revert of subnet patch
14:05:22 <ihrachys> I think we are waiting for garyk to confirm he is ok with it now, giving him several more days to reply
14:05:45 <ihrachys> ok, as for the list...
14:05:50 <ihrachys> https://review.openstack.org/#/c/521797/ "Use Router OVO in external_net_db"
14:06:26 <ihrachys> hungpv, there are logical issues with the code
14:06:33 <ihrachys> or better, there WERE
14:06:37 <ihrachys> I haven't checked latest version
14:07:04 <hungpv> Yes, I see
14:07:27 <hungpv> I'm trying to figure out on that
14:07:35 <ihrachys> I think there are still issues. you use ~ there on the result of get_objects which is weird.
14:07:51 <ihrachys> hungpv, what surprises me is that no unit tests fail because of the patch
14:07:58 <ihrachys> we probably have bad coverage for the code
14:08:47 <lujinluo> agree
14:08:56 <ihrachys> there are some tempest api test failures here: http://logs.openstack.org/97/521797/5/check/neutron-tempest-plugin-api/7841216/logs/testr_results.html.gz but it would be cool if we could trigger the code with unit tests
14:11:02 <ihrachys> on related note, there is a patch currently in review for a bug that affects the code that adds some unit tests that should trigger the code: https://review.openstack.org/#/c/512484/19/neutron/tests/unit/db/test_rbac_db_mixin.py
14:11:34 <ihrachys> you could try to check whether new tests would trigger a failure with your (broken) patch. if they do, we could then rely on them to prove it works.
14:12:40 <hungpv> Yes, I see. I'll check it to see if it happens
14:12:57 <ihrachys> but if not, it would be nice to see some unit tests contributed as a separate preparatory patch that would cover the code
14:13:18 <ihrachys> so that we are more sure the change doesn't break something
14:13:59 <ihrachys> next is https://review.openstack.org/#/c/407868/ "[WIP] Integration of (Distributed) Port Binding OVO"
14:14:17 <ihrachys> lujinluo, I believe it's still wip?
14:14:26 <lujinluo> yes
14:14:39 <ihrachys> anything we could help or advise?
14:15:32 <lujinluo> i think it is something wrong in db side. it would be appreciated if you could check the ERROR in neutron-server logs
14:15:55 <lujinluo> http://logs.openstack.org/68/407868/75/check/legacy-tempest-dsvm-neutron-dvr/3b5f642/logs/screen-q-svc.txt.gz?level=ERROR
14:16:10 <lujinluo> No details.: InvalidRequestError: Can't attach instance <DistributedPortBinding at 0x7f6a0c3fe6d0>; another instance with key (<class 'neutron.plugins.ml2.models.DistributedPortBinding'>, (u'32b7f57a-6b27-476b-be60-cb6eae576fe9', u'ubuntu-xenial-rax-dfw-0001136185')) is already present in this session.
14:16:26 <lujinluo> i root caused to update_port() in plugin/ml2/plugin.py
14:16:54 <lujinluo> it seems to be the problem of mixing usage of distributed port binding in db and ovo
14:20:20 <ihrachys> it seems to happen on delete_port only
14:23:34 <ihrachys> so for what I understand, you can get this error message when you try to add a model state to the session twice
14:24:03 <lujinluo> eh, right.
14:24:12 <ihrachys> here, since it's delete, it would probably be "a model marked as deleted"
14:24:40 <ihrachys> so there should be some other code somewhere on the code path that also modifies the model, maybe in a slightly different way
14:24:47 <ihrachys> and also adds it to the session
14:26:28 <lujinluo> hmm, so I am confused here. In the code path, we delete port and this triggers the cascade deletion of distributed_port_binding
14:26:54 <ihrachys> here in https://review.openstack.org/#/c/407868/75/neutron/plugins/ml2/plugin.py@1446
14:27:14 <ihrachys> I wonder if this persist_state_to_session is somehow related
14:27:59 <ihrachys> lujinluo, I was thinking, maybe delete_port happens after some other action on the binding model (in the same session / request) that would touch the binding model
14:28:46 <ihrachys> so you first touch the model, e.g. update it somehow, and then delete_port happens a tad later in the code path. then sqlalchemy may be puzzled whether it should persist first or second state.
14:29:55 <ihrachys> I suspect this persist_state_to_session logic is somehow related to the failure.
14:30:13 <lujinluo> hmm, i will check that. So it is possible that we need to add some guards to sessions?
14:30:18 <ihrachys> it's probably yet another kludge we needed to work around sqlalchemy
14:31:11 <ihrachys> lujinluo, we may try to pull kevinbenton on that one since he introduced the persistence "snapshot" concept there
14:31:27 <ihrachys> of course he is not working on openstack but probably could give an advice
14:32:01 <lujinluo> ok. i will try to catch him somehow
14:32:23 <ihrachys> maybe let's describe the issue in an email and send it to him
14:32:40 <ihrachys> we could capture as many details in there
14:32:53 <lujinluo> sure. this would be a good idea
14:32:55 <ihrachys> let's draft one in an etherpad (I will send a link to you later)
14:33:02 <lujinluo> thanks
14:33:26 <ihrachys> ok moving on
14:33:32 <ihrachys> next is https://review.openstack.org/#/c/396351/ "Integration of Floating IP OVO"
14:34:37 <ihrachys> lujinluo, you left a comment there about qos_policy_binding
14:34:50 <lujinluo> yes
14:34:52 <ihrachys> lujinluo, I am not sure what's that about?
14:35:15 <ihrachys> oh I think I follow now
14:35:24 <ihrachys> you were just exploring whether another field is needed
14:35:32 <ihrachys> so no issue in that version of the code
14:35:41 <ihrachys> I see slaweq left some nits in the code
14:35:46 <lujinluo> i editted a little
14:36:23 <lujinluo> cause qos_policy_binding (a newly added table) has added relationship with floatingip table
14:36:38 <lujinluo> i was checking if we need to make qos_policy_binding a synthetic field
14:36:57 <lujinluo> in SQLA side, some codes like floatingip_db.qos_policy_binding appeared
14:37:11 <lujinluo> i modified them to floatingip_obj.db_obj.qos_policy_binding
14:37:44 <lujinluo> https://review.openstack.org/#/c/396351/46/neutron/db/l3_fip_qos.py
14:37:50 <lujinluo> ^ here
14:38:40 <lujinluo> this is the patch adds the new table https://review.openstack.org/#/c/424466/
14:38:48 <ihrachys> yeah. I think we may eventually want to make it similar to how we handle bindings for networks/ports
14:39:51 <ihrachys> https://github.com/openstack/neutron/blob/master/neutron/objects/qos/policy.py#L208-L258
14:40:00 <ihrachys> actually, we already have attach/detach for FIPs there
14:40:35 <ihrachys> then why don't we use them in l3_fip_qos?
14:41:47 <ihrachys> oh we do
14:42:06 <ihrachys> it just updates attributes in db model to reflect changes to bindings table
14:42:30 <lujinluo> yes
14:43:56 <ihrachys> I am not even sure why bother updating the attribute there. we don't seem to use it in caller?
14:45:57 <lujinluo> i am not 100% sure. but before I made those changes, some tests in https://review.openstack.org/#/c/424466/31/neutron/tests/unit/extensions/test_qos_fip.py were failing
14:45:57 <ihrachys> ok I will need some more time on that one
14:46:17 <ihrachys> but there is a definite bug in the new dns code (the new version of it), I posted a comment
14:46:31 <ihrachys> but there may be some more. I need to understand why we need to update the model.
14:47:32 <ihrachys> ok those were all patches that are actionable
14:47:34 <lujinluo> ok. let's continue on gerrit
14:47:44 <ihrachys> #topic Work planning
14:48:06 <ihrachys> I (of course) haven't done remaining model usage classification
14:48:29 <ihrachys> but I would nevertheless like to check if everyone involved has items to work on
14:49:02 <ihrachys> hungpv and lujinluo work on patches already in gerrit
14:49:19 <ihrachys> TuanVu, annp do you have things to work on?
14:49:29 <TuanVu> Hi Ihar
14:49:41 <TuanVu> regarding to this patch
14:50:13 <ihrachys> "this" == ?
14:50:24 <TuanVu> https://review.openstack.org/#/c/507772/
14:50:35 <TuanVu> We’re still working on this patch, there’s 1 blocking point at this moment.
14:50:35 <TuanVu> Here’s the error: http://paste.openstack.org/show/627868/
14:51:13 <TuanVu> webob.exc.HTTPClientError: The server could not comply with the request since it is either malformed or otherwise incorrect.
14:51:35 <lujinluo> this is not the root error
14:52:09 <ihrachys> yeah it's just indication of error bubbled up to api layer
14:52:14 <lujinluo> InvalidRequestError: Instance '<Network at 0x7f6c67b2e250>' is not persistent within this Session
14:52:17 <lujinluo> ^ this is
14:52:31 <ihrachys> +
14:52:45 <lujinluo> i have not checked the patch but i suppose you used something like "network_obj.db_obj"?
14:53:01 <TuanVu> thank you, Luo
14:53:04 <TuanVu> yes, correct
14:53:08 <annp> lujinluo, ihrachys, maybe this related to attach_rbac function
14:53:47 <annp> when i check code, i saw self.obj_context.tenant_id is None.
14:54:17 <annp> But i'm not sure. I need to dig more.
14:55:38 <lujinluo> so from my personal experience, obj.db_obj is not persistent to any session. it cannot be used to be updated, etc. if you do, you will see that error.
14:56:01 <ihrachys> yeah, it's by design
14:56:07 <ihrachys> we detach it after fetching
14:56:15 <lujinluo> exactly.
14:57:08 <ihrachys> it fixes conflicts with other code that may want to work with models directly, and also that we don't fetch additional attributes in extension calls after data is persisted in db
14:58:14 <ihrachys> it's good we can reproduce it with unit test
14:58:15 <annp> lujinluo, ihrachys, Ok, I will dig more. :)
14:58:25 <ihrachys> I will try to poke it in parallel
14:58:33 <TuanVu> awesome!
14:58:39 <TuanVu> thank you so much, Ihar
14:58:50 <ihrachys> ok so it seems like everyone has a thing to work on, that's good
14:58:51 <TuanVu> thank you, too, Luo
14:59:03 <lujinluo> no problem ;)
14:59:06 <ihrachys> we are out of time, so I guess we wrap up?
14:59:14 <lujinluo> yes
14:59:39 <ihrachys> ok, have a nice rest of the week!
14:59:39 <TuanVu> yes
14:59:39 <ihrachys> #endmeeting