<div dir="ltr">Thanks for asking this. It's good to get this documented on the mailing list since most of it is spread across bug reports and commits over the last couple of years.<div><br></div><div>><span style="font-size:12.8px">1. since the retry_if_session_inactive decorator assumes functions </span><span style="font-size:12.8px">that receive a "context", what does it mean that networking_odl uses </span><span style="font-size:12.8px">functions that receive a "session" and not a "context" ? Should </span><span style="font-size:12.8px">networking_odl be modified such that it accepts the same "context" </span><span style="font-size:12.8px">that Neutron passes around?</span></div><div><span style="font-size:12.8px">></span></div><div><div><span style="font-size:12.8px">>2. Alternatively, should the retry_if_session_inactive decorator be </span><span style="font-size:12.8px">modified (or a new decorator added) that provides the identical </span><span style="font-size:12.8px">behavior for functions that receive a "session" argument rather than a </span><span style="font-size:12.8px">"context" argument ? (or should this be done anyway even if </span><span style="font-size:12.8px">networking_odl's pattern is legacy?)</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">So it really depends on how those particular functions are being called. </span><span style="font-size:12.8px">For example, it seems like this function in the review is always going to be called within an active session: <a href="https://review.openstack.org/#/c/500584/3/networking_odl/db/db.py@113">https://review.openstack.org/#/c/500584/3/networking_odl/db/db.py@113</a> </span>If that's the case then the decorator is never going to trigger a retry since is_active will be True. </div><div><br></div><div>Since the goal of that patch is to deal with deadlocks, the retry can't happen down at that level, it will need to be somewhere further up the call stack since the deadlock will put the session in a rollback state.</div><div><br></div><div>For the functions being called outside of an active session, they should switch to accepting a context to handle the enginefacade switch.</div><div><br></div><div>><span style="font-size:12.8px"> 3a. Is this a temporary measure to work around legacy patterns?</span></div><div><br></div></div><div>Yes. When that went in we had very little adoption of the enginefacade. Even now we haven't fully completed the transition so checking for is_active covers the old code paths. </div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">As we continue migrating to the enginefacade, the cases where you can get a reference to a session that actually has 'is_active==False' are going to keep dwindling. </span>Once we complete the switch and remove the legacy session constructor, the decorator will just be adjusted to check if there is a session attached to the context to determine if retries are safe since is_active will always be True.</div><div><br></div><div>><span style="font-size:12.8px"> </span><span style="font-size:12.8px">3b. If 3a., is the pattern in use by networking_odl, receiving a </span><span style="font-size:12.8px">"session" and not "context", the specific legacy pattern in question?</span></div><div><br></div><div>If those functions are called outside of an active session, then yes. Based on the session.begin() call without substransactions=True, I'm guessing that's the case for at least some of them: <a href="https://review.openstack.org/#/c/500584/3/networking_odl/db/db.py@85">https://review.openstack.org/#/c/500584/3/networking_odl/db/db.py@85</a></div><div><br></div><div>><span style="font-size:12.8px">3c. if 3a and 3b, are we expecting all the various plugins to </span><span style="font-size:12.8px">start using "context" at some point ? (and when they come to me with </span><span style="font-size:12.8px">breakage, I can push them in that direction?)</span></div><div><br></div><div>Yeah, for any functions that expect to start their own transaction they should altered to accept contexts and use the reader/writer context managers. If it's a function that is called from within an ongoing transaction, it can obviously still accept a session argument but the 'retry_if_session_inactive' decorator would obviously be irrelevant in that scenario since the session would always be active.</div><div><br></div><div>><span style="font-size:12.8px"> </span><span style="font-size:12.8px">4a. Is it strictly that simple mutable lists and dicts of simple </span><span style="font-size:12.8px">types (ints, strings, etc.) are preserved across retries, assuming the </span><span style="font-size:12.8px">receiving functions might be modifying them?</span></div><div><br></div><div>Yes, this was to deal with functions that would mutate collections passed in. In particular, we have functions that alter the parsed API request as they process them. If they hit a deadlock on commit and we retry at that point the API request would be mangled without the copy. (e.g. <a href="https://bugs.launchpad.net/neutron/+bug/1584920">https://bugs.launchpad.net/neutron/+bug/1584920</a>)</div><div><br></div><div>><span style="font-size:12.8px">4b. Does this deepcopy() approach have any support for functions </span><span style="font-size:12.8px">that are being passed not just simple structures but ORM-mapped </span><span style="font-size:12.8px">objects? Was this use case considered?</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">No, it doesn't support ORM objects. Given that the target use case of the retry decorator was for functions not in an ongoing session, we didn't have a pattern to support in the main tree where an ORM object would be passed as an argument to the function that needed to be protected.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">></span><span style="font-size:12.8px">4c. Does Neutron's model base have any support for deepcopy()? I </span><span style="font-size:12.8px">notice that the base classes in model_base do **not** seem to have a </span><span style="font-size:12.8px">__deepcopy__ present. This would mean that this deepcopy() will </span><span style="font-size:12.8px">definitely break any ORM mapped objects because you need to use an </span><span style="font-size:12.8px">approach like what Nova uses for copy(): </span><a href="https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/models.py#L46" rel="noreferrer" target="_blank" style="font-size:12.8px">https://github.com/openstack/<wbr>nova/blob/master/nova/db/<wbr>sqlalchemy/models.py#L46</a> <span style="font-size:12.8px">. This would be why the developer's objects are all getting kicked</span></div><span style="font-size:12.8px">out of the session. (this is the "is this a bug?" part)</span><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">That's definitely why the objects are being kicked out of the session. </span>I'm not sure if it makes sense to attempt to support copying an ORM object when the decorator is meant for functions starting a completely new session. If there is a use case I'm missing that we do want to support, it probably makes more sense to use a different decorator than to adjust the existing one.</div><div><br></div><div>><span style="font-size:12.8px">4d. How frequent is the problem of mutable lists and dicts, and </span><span style="font-size:12.8px">should this complex and actually pretty expensive behavior be </span><span style="font-size:12.8px">optional, only for those functions that are known to receieve mutable </span><span style="font-size:12.8px">structures and change them in place?</span></div><div><br></div><div>It's difficult to tell. The issue was that the retry logic was added much later to the API level so we needed a generically safe solution to protect every possible API operation. It's copying things passed into the API request so they are usually dictionaries with fewer than 10 keys and values with small strings or lists of only a few entries, making the copy operations cheap.</div><div><br></div><div>If we did want to get rid of the copy operation, it would probably be quicker to add a new decorator that doesn't copy and then slowly opt into for each function that we validate as 'side-effect free' on the inputs.</div><div><br></div><div><br></div><div>I hope that makes sense. Let me know if you have any other questions.</div><div><br></div><div>Cheers,</div><div>Kevin Benton</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 7, 2017 at 8:11 AM, Michael Bayer <span dir="ltr"><<a href="mailto:mbayer@redhat.com" target="_blank">mbayer@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I'm trying to get a handle on neutron's retry decorators. I'm<br>
assisting a developer in the networking_odl project who wishes to make<br>
use of neutron's retry_if_session_inactive decorator.<br>
<br>
The developer has run into several problems, including that<br>
networking_odl methods here seem to accept a "session" directly,<br>
rather than the "context" as is standard throughout neutron, and<br>
retry_if_session_inactive assumes the context calling pattern.<br>
Additionally, the retry_db_errors() decorator which is also used by<br>
retry_if_session_inactive is doing a deepcopy operation on arguments,<br>
which when applied to ORM-mapped entities, makes a copy of them which<br>
are then detached from the Session and they fail to function beyond<br>
that.<br>
<br>
For context, the developer's patch is at:<br>
<a href="https://review.openstack.org/#/c/500584/3" rel="noreferrer" target="_blank">https://review.openstack.org/#<wbr>/c/500584/3</a><br>
<br>
and the neutron features I'm looking at can be seen when they were<br>
reviewed at: <a href="https://review.openstack.org/#/c/356530/22" rel="noreferrer" target="_blank">https://review.openstack.org/#<wbr>/c/356530/22</a><br>
<br>
<br>
So I have a bunch of questions, many are asking the same things: "is<br>
networking_odl's pattern legacy or not?" and "is this retry decorator<br>
buggy?" There's overlap in these questions. But "yes" / "no" to<br>
each would still help me figure out that this is a giraffe and not a<br>
duck :) ("does it have a long neck?" "does it quack?" etc)<br>
<br>
1. since the retry_if_session_inactive decorator assumes functions<br>
that receive a "context", what does it mean that networking_odl uses<br>
functions that receive a "session" and not a "context" ? Should<br>
networking_odl be modified such that it accepts the same "context"<br>
that Neutron passes around?<br>
<br>
2. Alternatively, should the retry_if_session_inactive decorator be<br>
modified (or a new decorator added) that provides the identical<br>
behavior for functions that receive a "session" argument rather than a<br>
"context" argument ? (or should this be done anyway even if<br>
networking_odl's pattern is legacy?)<br>
<br>
3. I notice that the session.is_active flag is considered to be<br>
meaningful. Normally, this flag is of no use, as the enginefacade<br>
pattern only provides for a Session that has had the begin() method<br>
called. However, it looks like in neutron_lib/context.py, the Context<br>
class is providing for a default session<br>
"db_api.get_writer_session()", which I would assume is how we get a<br>
Session with is_active is false:<br>
<br>
3a. Is this a temporary measure to work around legacy patterns?<br>
3b. If 3a., is the pattern in use by networking_odl, receiving a<br>
"session" and not "context", the specific legacy pattern in question?<br>
3c. if 3a and 3b, are we expecting all the various plugins to<br>
start using "context" at some point ? (and when they come to me with<br>
breakage, I can push them in that direction?)<br>
<br>
4. What is the bug we are fixing by using deepcopy() with the<br>
arguments passed to the decorated function:<br>
<br>
4a. Is it strictly that simple mutable lists and dicts of simple<br>
types (ints, strings, etc.) are preserved across retries, assuming the<br>
receiving functions might be modifying them?<br>
<br>
4b. Does this deepcopy() approach have any support for functions<br>
that are being passed not just simple structures but ORM-mapped<br>
objects? Was this use case considered?<br>
<br>
4c. Does Neutron's model base have any support for deepcopy()? I<br>
notice that the base classes in model_base do **not** seem to have a<br>
__deepcopy__ present. This would mean that this deepcopy() will<br>
definitely break any ORM mapped objects because you need to use an<br>
approach like what Nova uses for copy():<br>
<a href="https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/models.py#L46" rel="noreferrer" target="_blank">https://github.com/openstack/<wbr>nova/blob/master/nova/db/<wbr>sqlalchemy/models.py#L46</a><br>
. This would be why the developer's objects are all getting kicked<br>
out of the session. (this is the "is this a bug?" part)<br>
<br>
4d. How frequent is the problem of mutable lists and dicts, and<br>
should this complex and actually pretty expensive behavior be<br>
optional, only for those functions that are known to receieve mutable<br>
structures and change them in place?<br>
<br>
______________________________<wbr>______________________________<wbr>______________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.<wbr>openstack.org?subject:<wbr>unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/<wbr>cgi-bin/mailman/listinfo/<wbr>openstack-dev</a><br>
</blockquote></div><br></div>