)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"2eabb60e0ee670d1dcd57fa08038268e1068382a","unresolved":false,"context_lines":[{"line_number":10,"context_line":"on the node to validate and start the task, even if a task was"},{"line_number":11,"context_line":"asynchronous. This allows node vendor passthru methods to specify"},{"line_number":12,"context_line":"exclusive_task_required\u003dFalse in their passthru decorators to let the"},{"line_number":13,"context_line":"conductor acquire a shared lock on a node for asynchronous passthru"},{"line_number":14,"context_line":"methods. This is useful in cases like the agent heartbeat where no"},{"line_number":15,"context_line":"validation is performed on a node. The agent heartbeat now uses a shared"},{"line_number":16,"context_line":"lock, as an example of this."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"9a1a9d01_f681b94e","line":13,"updated":"2015-09-24 16:31:46.000000000","message":"IIUC you also allow it for synchronous methods now?","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"1377ef3ec8c218c0a6635f5c53afc846f90c1d09","unresolved":false,"context_lines":[{"line_number":10,"context_line":"on the node to validate and start the task, even if a task was"},{"line_number":11,"context_line":"asynchronous. This allows node vendor passthru methods to specify"},{"line_number":12,"context_line":"exclusive_task_required\u003dFalse in their passthru decorators to let the"},{"line_number":13,"context_line":"conductor acquire a shared lock on a node for asynchronous passthru"},{"line_number":14,"context_line":"methods. This is useful in cases like the agent heartbeat where no"},{"line_number":15,"context_line":"validation is performed on a node. The agent heartbeat now uses a shared"},{"line_number":16,"context_line":"lock, as an example of this."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"9a1a9d01_e43475df","line":13,"in_reply_to":"9a1a9d01_f681b94e","updated":"2015-09-25 17:48:10.000000000","message":"Correct, thanks for catching this.","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"}],"doc/source/dev/drivers.rst":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"24f067d50cee242f4246fdfec82e4d4c5cb406bb","unresolved":false,"context_lines":[{"line_number":53,"context_line":"  + For synchronous methods, a 200 (OK) HTTP status code is returned to"},{"line_number":54,"context_line":"    indicate that the request was fulfilled. The response may include a body."},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"While performing the request, a lock is held on the node, and other"},{"line_number":57,"context_line":"requests for the node will be delayed and may fail with an HTTP 409"},{"line_number":58,"context_line":"(Conflict) error code. This only occurs if the lock is an exclusive lock (if"},{"line_number":59,"context_line":"the method doesn\u0027t specify require_exclusive_lock\u003dFalse)."}],"source_content_type":"text/x-rst","patch_set":14,"id":"ba63e191_56786ac4","line":56,"updated":"2016-01-19 22:31:18.000000000","message":"I would actually make this one of the points like the above, and reword it. eg:\n\n* can require an exclusive lock on then node....","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"555042c165f8a8acea1394b1fdd670fc138a7c39","unresolved":false,"context_lines":[{"line_number":53,"context_line":"  + For synchronous methods, a 200 (OK) HTTP status code is returned to"},{"line_number":54,"context_line":"    indicate that the request was fulfilled. The response may include a body."},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"While performing the request, a lock is held on the node, and other"},{"line_number":57,"context_line":"requests for the node will be delayed and may fail with an HTTP 409"},{"line_number":58,"context_line":"(Conflict) error code. This only occurs if the lock is an exclusive lock (if"},{"line_number":59,"context_line":"the method doesn\u0027t specify require_exclusive_lock\u003dFalse)."}],"source_content_type":"text/x-rst","patch_set":14,"id":"dab17558_f5049415","line":56,"in_reply_to":"ba63e191_56786ac4","updated":"2016-05-11 15:32:55.000000000","message":"Done, please let me know if it needs more rewording.","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":5805,"name":"Chris Krelle","email":"nobodycam@gmail.com","username":"nobodycam"},"change_message_id":"e207ade87f5a9a29d19961fb7e213e1a2d43492c","unresolved":false,"context_lines":[{"line_number":56,"context_line":"* can require an exclusive lock on the node. This only occurs if the method"},{"line_number":57,"context_line":"  doesn\u0027t specify require_exclusive_lock\u003dFalse in the decorator. If an"},{"line_number":58,"context_line":"  exclusive lock is held on the node, other requests for the node will be"},{"line_number":59,"context_line":"  delayed and may fail with an HTTP 409 (Conflict) error code."},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"This endpoint exposes a node\u0027s driver directly, and as such, it is"},{"line_number":62,"context_line":"expressly not part of Ironic\u0027s standard REST API. There is only a"}],"source_content_type":"text/x-rst","patch_set":16,"id":"dab17558_594f17bd","line":59,"updated":"2016-05-13 19:46:07.000000000","message":"Nit: the wording here seems odd given that the default is to take an exclusive lock. \n\nMaybe something like:\n* can request a shared lock on the node. this can be done by setting the require_exclusive_lock parameter to false...","commit_id":"ea06393fe853d6b54ab1e2f6d5c1f39e8c61907c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"e1b2806679ecf0f487bf7751ce75688ceaba671e","unresolved":false,"context_lines":[{"line_number":56,"context_line":"* can require an exclusive lock on the node. This only occurs if the method"},{"line_number":57,"context_line":"  doesn\u0027t specify require_exclusive_lock\u003dFalse in the decorator. If an"},{"line_number":58,"context_line":"  exclusive lock is held on the node, other requests for the node will be"},{"line_number":59,"context_line":"  delayed and may fail with an HTTP 409 (Conflict) error code."},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"This endpoint exposes a node\u0027s driver directly, and as such, it is"},{"line_number":62,"context_line":"expressly not part of Ironic\u0027s standard REST API. There is only a"}],"source_content_type":"text/x-rst","patch_set":16,"id":"dab17558_cf578b58","line":59,"in_reply_to":"dab17558_594f17bd","updated":"2016-05-16 09:25:24.000000000","message":"+1 sounds more comprehensible","commit_id":"ea06393fe853d6b54ab1e2f6d5c1f39e8c61907c"}],"doc/source/dev/vendor-passthru.rst":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"24f067d50cee242f4246fdfec82e4d4c5cb406bb","unresolved":false,"context_lines":[{"line_number":120,"context_line":"* async: A boolean value to determine whether this method should run"},{"line_number":121,"context_line":"  asynchronously or synchronously. Defaults to True (Asynchronously)."},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"The node passthru decorator (`@passthru`) also accepts the following parameter:"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"* require_exclusive_lock: A boolean value determining whether this method"},{"line_number":126,"context_line":"  should require an exclusive lock on a node between validate() and the"}],"source_content_type":"text/x-rst","patch_set":14,"id":"ba63e191_d6ee9a8a","line":123,"updated":"2016-01-19 22:31:18.000000000","message":"nit, node/node vendor/","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"555042c165f8a8acea1394b1fdd670fc138a7c39","unresolved":false,"context_lines":[{"line_number":120,"context_line":"* async: A boolean value to determine whether this method should run"},{"line_number":121,"context_line":"  asynchronously or synchronously. Defaults to True (Asynchronously)."},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"The node passthru decorator (`@passthru`) also accepts the following parameter:"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"* require_exclusive_lock: A boolean value determining whether this method"},{"line_number":126,"context_line":"  should require an exclusive lock on a node between validate() and the"}],"source_content_type":"text/x-rst","patch_set":14,"id":"dab17558_953d48c6","line":123,"in_reply_to":"ba63e191_d6ee9a8a","updated":"2016-05-11 15:32:55.000000000","message":"Fixed","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"}],"ironic/conductor/manager.py":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"75f3abbb8ec230fbce8e31aec25f5f3080e51455","unresolved":false,"context_lines":[{"line_number":463,"context_line":"        invoke the vendor method. If the method mode is \u0027async\u0027 the conductor"},{"line_number":464,"context_line":"        will start background worker to perform vendor action. If the method"},{"line_number":465,"context_line":"        mode is \u0027async\u0027 and the driver has the property exclusive_lock\u003dFalse,"},{"line_number":466,"context_line":"        an exclusive lock will not be acquired on the node."},{"line_number":467,"context_line":""},{"line_number":468,"context_line":"        :param context: an admin context."},{"line_number":469,"context_line":"        :param node_id: the id or uuid of a node."}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_7572387e","line":466,"updated":"2015-09-22 07:35:36.000000000","message":"I don\u0027t quite get why sync methods can\u0027t have a shared lock..","commit_id":"31590baf0842cc200bde3f1e037475da3ba23fe3"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"99cca915e336b63e96fc7bd6a3234406b3280c6b","unresolved":false,"context_lines":[{"line_number":463,"context_line":"        invoke the vendor method. If the method mode is \u0027async\u0027 the conductor"},{"line_number":464,"context_line":"        will start background worker to perform vendor action. If the method"},{"line_number":465,"context_line":"        mode is \u0027async\u0027 and the driver has the property exclusive_lock\u003dFalse,"},{"line_number":466,"context_line":"        an exclusive lock will not be acquired on the node."},{"line_number":467,"context_line":""},{"line_number":468,"context_line":"        :param context: an admin context."},{"line_number":469,"context_line":"        :param node_id: the id or uuid of a node."}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_c5523d35","line":466,"in_reply_to":"ba15a1d1_63449e1a","updated":"2015-09-23 09:44:03.000000000","message":"Hi Mario,\n\nYeah I also don\u0027t get why we have this sync vs async for shared or exclusive locks. This is actually changing the behavior because up to now all methods (sync or async) gets an exclusive lock and people may be expecting that. \n\nWhat if, we allow the vendor methods to specify whether they want a shared or exclusive lock in a per-method base and not in a driver base ?\n\nFor example, we can extend the @passthru decorator [1] to have a parameter such as \"require_exclusive_lock\u003dTrue/False\" (defaulting to True since that was the behavior before).\n\nSo in this method we can do, e.g:\n\n if vendor_opts[\u0027require_exclusive_lock\u0027]:\n     task.upgrade_lock()\n\n\nWhat do you think? That\u0027s good because each vendor method have the power to say what it needs and what it needs not.\n\n[1] https://github.com/openstack/ironic/blob/38a4ebcdf8cf1c8b47bf1f9bd32e4d55b5e31a43/ironic/drivers/base.py#L642-L643","commit_id":"31590baf0842cc200bde3f1e037475da3ba23fe3"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"20c78614baeeb2b12c21df27e20fa14a2ffda035","unresolved":false,"context_lines":[{"line_number":463,"context_line":"        invoke the vendor method. If the method mode is \u0027async\u0027 the conductor"},{"line_number":464,"context_line":"        will start background worker to perform vendor action. If the method"},{"line_number":465,"context_line":"        mode is \u0027async\u0027 and the driver has the property exclusive_lock\u003dFalse,"},{"line_number":466,"context_line":"        an exclusive lock will not be acquired on the node."},{"line_number":467,"context_line":""},{"line_number":468,"context_line":"        :param context: an admin context."},{"line_number":469,"context_line":"        :param node_id: the id or uuid of a node."}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_63449e1a","line":466,"in_reply_to":"ba15a1d1_7572387e","updated":"2015-09-22 20:19:12.000000000","message":"That should be OK I think. I just didn\u0027t want to introduce a race condition of some sort like:\n\n1. driver A starts and completes validation on node X in state 1\n\n2. driver B starts and finishes validation on node X in state 1\n\n3. driver A, method foo starts, changes node X to state 2. state 2 is invalid for driver B.\n\n4. driver B, method bar starts on an invalid node\n\nThinking about it more, though, that type of race could happen independently of whether the calls are synchronous or not. For example, if A is synchronous and fast, and B is async and slow, step 1 could still execute after step 2:\n\n1. driver B is async, starts and finishes validation on node X in state 1, doesn\u0027t lock X\n\n2. driver A is sync and locks X, validates X, executes foo, puts X into invalid state 2, unlocks X\n\n3. driver B executes bar on X, a now invalid node according to B\u0027s validate()\n\nSo, the fact that A is synchronous and locks the node doesn\u0027t help B. The race condition would happens because bar allows a shared lock and depends on the node being valid even with the state change.\n\nSo, what you mentioned below about making allowing shared locks depend on methods is a better strategy. Then, a method should only allow shared lock use if it doesn\u0027t care if the state changes between validate() and the execution. In other words, for the method to safely use a shared lock, it should never care whether the node passes validate() or not.\n\nThat moves the assumption about whether a method is safe to execute on a potentially invalid node onto the method itself. It\u0027s probably safe to do that as long as we still default to locking a node unless a driver method specifies that it doesn\u0027t care if the lock is shared (meaning it doesn\u0027t care whether the node is still valid for its driver or not).","commit_id":"31590baf0842cc200bde3f1e037475da3ba23fe3"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"0da12fe6a5b1090a0e7564c29ac8e9358a12dc9b","unresolved":false,"context_lines":[{"line_number":463,"context_line":"        invoke the vendor method. If the method mode is \u0027async\u0027 the conductor"},{"line_number":464,"context_line":"        will start background worker to perform vendor action. If the method"},{"line_number":465,"context_line":"        mode is \u0027async\u0027 and the driver has the property exclusive_lock\u003dFalse,"},{"line_number":466,"context_line":"        an exclusive lock will not be acquired on the node."},{"line_number":467,"context_line":""},{"line_number":468,"context_line":"        :param context: an admin context."},{"line_number":469,"context_line":"        :param node_id: the id or uuid of a node."}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_f6b032ff","line":466,"in_reply_to":"ba15a1d1_c5523d35","updated":"2015-09-23 22:50:31.000000000","message":"That makes sense. Per-method specification is much more flexible. The latest patch set should preserve the original behavior except for the agent heartbeat method, for which I added require_exclusive_lock\u003dFalse.","commit_id":"31590baf0842cc200bde3f1e037475da3ba23fe3"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"75f3abbb8ec230fbce8e31aec25f5f3080e51455","unresolved":false,"context_lines":[{"line_number":519,"context_line":"            # requires an exclusive lock on the node, change the shared task to"},{"line_number":520,"context_line":"            # an exclusive one"},{"line_number":521,"context_line":"            is_async \u003d vendor_opts[\u0027async\u0027]"},{"line_number":522,"context_line":"            if not is_async or vendor_iface.exclusive_task:"},{"line_number":523,"context_line":"                task.upgrade_lock()"},{"line_number":524,"context_line":""},{"line_number":525,"context_line":"            vendor_iface.validate(task, method\u003ddriver_method,"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_956d8c9b","line":522,"updated":"2015-09-22 07:35:36.000000000","message":"this should be set in vendor_opts for a method, not globally on an interface","commit_id":"31590baf0842cc200bde3f1e037475da3ba23fe3"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"20c78614baeeb2b12c21df27e20fa14a2ffda035","unresolved":false,"context_lines":[{"line_number":519,"context_line":"            # requires an exclusive lock on the node, change the shared task to"},{"line_number":520,"context_line":"            # an exclusive one"},{"line_number":521,"context_line":"            is_async \u003d vendor_opts[\u0027async\u0027]"},{"line_number":522,"context_line":"            if not is_async or vendor_iface.exclusive_task:"},{"line_number":523,"context_line":"                task.upgrade_lock()"},{"line_number":524,"context_line":""},{"line_number":525,"context_line":"            vendor_iface.validate(task, method\u003ddriver_method,"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_835e92e7","line":522,"in_reply_to":"ba15a1d1_956d8c9b","updated":"2015-09-22 20:19:12.000000000","message":"I agree, given the above discussion. Change coming in next patch set.","commit_id":"31590baf0842cc200bde3f1e037475da3ba23fe3"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"e0507f79a79db9461614a1056bb04fdbaf7c13ab","unresolved":false,"context_lines":[{"line_number":515,"context_line":""},{"line_number":516,"context_line":"            # Change shared lock to exclusive if a vendor method requires it."},{"line_number":517,"context_line":"            # Vendor methods default to requiring an exclusive lock."},{"line_number":518,"context_line":"            if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":519,"context_line":"                task.upgrade_lock()"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"            vendor_iface.validate(task, method\u003ddriver_method,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a1a9d01_db28e3e4","line":518,"updated":"2015-09-28 23:20:49.000000000","message":"Just wondering if this should be a get?  Are we sure that this will always exist in the dictionary?\n\nMaybe:\n\n if vendor_opts.get(\u0027require_exclusive_lock\u0027):","commit_id":"ca71fb77e4858cfe8fcd3a73df2743e1bf3def76"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"8ea87f33b041ccb5033cb61a4012f2b65c624b38","unresolved":false,"context_lines":[{"line_number":515,"context_line":""},{"line_number":516,"context_line":"            # Change shared lock to exclusive if a vendor method requires it."},{"line_number":517,"context_line":"            # Vendor methods default to requiring an exclusive lock."},{"line_number":518,"context_line":"            if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":519,"context_line":"                task.upgrade_lock()"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"            vendor_iface.validate(task, method\u003ddriver_method,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a1a9d01_375314e4","line":518,"in_reply_to":"9a1a9d01_db28e3e4","updated":"2015-09-29 16:01:11.000000000","message":"It\u0027s a method internal to Ironic that\u0027s defined (with a default) in the `passthru` method in ironic/drivers/base.py, so it should always exist regardless of whether it\u0027s specified by the driver or not.\n\nRather than using `.get` it might be better to raise an error as we do above with vendor_opts[\u0027func\u0027], since if this is missing there\u0027s something wrong with the code in Ironic itself. That makes it more consistent with what was here previously.\n\nWhat do you think of the new code here?","commit_id":"ca71fb77e4858cfe8fcd3a73df2743e1bf3def76"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"cf085cd75436e98a8e1cc19adfd1c1615ead9e91","unresolved":false,"context_lines":[{"line_number":509,"context_line":"                if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_c9946f44","line":512,"updated":"2015-09-29 20:44:39.000000000","message":"Not 100% sure I like this. Is this documented somewhere? Though it seems to work in Python 2 \u0026 3","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"2cd7401eaf6621b8bea4355083537136af727e5d","unresolved":false,"context_lines":[{"line_number":509,"context_line":"                if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_ef8053c1","line":512,"in_reply_to":"9a1a9d01_af4d7b3f","updated":"2015-09-29 21:51:20.000000000","message":"It seems fine to me as is. Works in both Python 2 \u0026 3, so I\u0027m good.\n\nI\u0027ll be 95% I like this :)","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"7d0d5b8376f1b71f50bbb539c8ee9222d2754105","unresolved":false,"context_lines":[{"line_number":509,"context_line":"                if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_af4d7b3f","line":512,"in_reply_to":"9a1a9d01_c9946f44","updated":"2015-09-29 21:40:28.000000000","message":"It seems to be all over the internet, but I can\u0027t find anything that\u0027s very specific in the Python docs. This is the closest thing I found: https://docs.python.org/2/library/exceptions.html#exceptions.BaseException.args\n\nWould you rather I do multiple try/except clauses? Or, I could just re-raise with the original error message.","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"cf085cd75436e98a8e1cc19adfd1c1615ead9e91","unresolved":false,"context_lines":[{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_c9a22f99","line":513,"updated":"2015-09-29 20:44:39.000000000","message":"Should we have a unit test for this case?","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"2cd7401eaf6621b8bea4355083537136af727e5d","unresolved":false,"context_lines":[{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_8fed17b9","line":513,"in_reply_to":"9a1a9d01_4f7b7f8c","updated":"2015-09-29 21:51:20.000000000","message":"Sorry, I didn\u0027t realize it was a decorator.\n\nAs mentioned in IRC, maybe call it as a normal function for difficult cases like this.","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"4e0e39365ca24540a970cbe46ad51c66b2d0dcc7","unresolved":false,"context_lines":[{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_ea6b61b7","line":513,"in_reply_to":"9a1a9d01_8fed17b9","updated":"2015-09-29 22:05:51.000000000","message":"Good points in IRC. I added the test for require_exclusive_lock. For the other 2 cases, I believe that these are already tested in ironic/tests/conductor/test_manager.py.\n\nIf you grep for \"InvalidParameterValue\" in that test, a few cases related to this behavior should come up.","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"7d0d5b8376f1b71f50bbb539c8ee9222d2754105","unresolved":false,"context_lines":[{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_4f7b7f8c","line":513,"in_reply_to":"9a1a9d01_c9a22f99","updated":"2015-09-29 21:40:28.000000000","message":"That would have the same issue I mentioned before with the test failing automatically on import (before any of the unit tests execute), since the handler lookups and check for require_exclusive_lock parameter all happen upon class import or declaration. The comment in patch set 4 addresses what happens when you try to add a test like that: https://gist.github.com/supermari0/deeb1d6d0aee4fa971e9\n\nI\u0027d like to add a test if someone can think of a good way to do this. The risk of a regression happening right now is low, however, since a vendor writing their own tests or even running their code would see an immediate failure whenever any module tries to import or run their driver code.","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"cf085cd75436e98a8e1cc19adfd1c1615ead9e91","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"},{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_89a8b778","line":516,"updated":"2015-09-29 20:44:39.000000000","message":"Should we have a unit test for this case?","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"7d0d5b8376f1b71f50bbb539c8ee9222d2754105","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"},{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_6f3203f4","line":516,"in_reply_to":"9a1a9d01_89a8b778","updated":"2015-09-29 21:40:28.000000000","message":"see above","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"cf085cd75436e98a8e1cc19adfd1c1615ead9e91","unresolved":false,"context_lines":[{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"},{"line_number":520,"context_line":"                else:"},{"line_number":521,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":522,"context_line":"                        _(\u0027Driver method {} not found in vendor interface \u0027"},{"line_number":523,"context_line":"                          \u0027{}\u0027).format(driver_method, vendor_iface))"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_89d677ef","line":520,"updated":"2015-09-29 20:44:39.000000000","message":"Should we have a unit test for this case?","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"7d0d5b8376f1b71f50bbb539c8ee9222d2754105","unresolved":false,"context_lines":[{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"},{"line_number":520,"context_line":"                else:"},{"line_number":521,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":522,"context_line":"                        _(\u0027Driver method {} not found in vendor interface \u0027"},{"line_number":523,"context_line":"                          \u0027{}\u0027).format(driver_method, vendor_iface))"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a1a9d01_4f54dffd","line":520,"in_reply_to":"9a1a9d01_89d677ef","updated":"2015-09-29 21:40:28.000000000","message":"see above","commit_id":"2466cc048655b7b024d7f0243d87401e6154a1b7"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":506,"context_line":"                vendor_func \u003d vendor_opts[\u0027func\u0027]"},{"line_number":507,"context_line":"                # Change shared lock to exclusive if a vendor method requires"},{"line_number":508,"context_line":"                # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":509,"context_line":"                if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_d6822b00","line":509,"updated":"2015-10-07 09:12:31.000000000","message":"It will be better if we just move the 2 lines of code outside the try/except block as we don\u0027t except the 2 lines to raise KeyError.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":506,"context_line":"                vendor_func \u003d vendor_opts[\u0027func\u0027]"},{"line_number":507,"context_line":"                # Change shared lock to exclusive if a vendor method requires"},{"line_number":508,"context_line":"                # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":509,"context_line":"                if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_a34d8f3f","line":509,"in_reply_to":"7a2fa921_d6822b00","updated":"2015-10-16 18:19:34.000000000","message":"Done","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"a0fec2441302f2b8e1697ddd2ec1864f647d2d2b","unresolved":false,"context_lines":[{"line_number":507,"context_line":"                # Change shared lock to exclusive if a vendor method requires"},{"line_number":508,"context_line":"                # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":509,"context_line":"                if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_10a9df23","line":510,"updated":"2015-10-01 11:25:34.000000000","message":"Is there an unhandled node already lock error here?","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"f1965fe497e18350173c7a5908672e19fcb27902","unresolved":false,"context_lines":[{"line_number":507,"context_line":"                # Change shared lock to exclusive if a vendor method requires"},{"line_number":508,"context_line":"                # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":509,"context_line":"                if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":510,"context_line":"                    task.upgrade_lock()"},{"line_number":511,"context_line":"            except KeyError as e:"},{"line_number":512,"context_line":"                missing_key \u003d e.args[0]"},{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_39fba249","line":510,"in_reply_to":"9a1a9d01_10a9df23","updated":"2015-10-01 15:31:45.000000000","message":"Yes. That was the same behavior as before, however, and I think it\u0027s correct to leave it that way. The error wouldn\u0027t have any side effects since all we do here is deserialize some vendor information.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"},{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_76a9d77a","line":516,"updated":"2015-10-07 09:12:31.000000000","message":"Do we really expect it to raise KeyError. In any case we will adding the key either True or False.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                if missing_key \u003d\u003d \u0027func\u0027:"},{"line_number":514,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":515,"context_line":"                        _(\u0027No handler for method %s\u0027) % driver_method)"},{"line_number":516,"context_line":"                elif missing_key \u003d\u003d \u0027require_exclusive_lock\u0027:"},{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_e3c70796","line":516,"in_reply_to":"7a2fa921_76a9d77a","updated":"2015-10-16 18:19:34.000000000","message":"Hm. I think another reviewer may have asked me to add this.\n\nLooking at it, I think I was concerned about upgrade behavior. Specifically, if a vendor passthru method is called and the conductor it executes on hasn\u0027t been updated yet, I thought there could be a case where it\u0027s missing the parameter.\n\nSince it appears that the same conductor would be the one handling both the decorator in base.py which adds the require_exclusive_lock parameter and this call, though, I don\u0027t think that behavior is possible.\n\nI\u0027ll remove this check.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"},{"line_number":520,"context_line":"                else:"},{"line_number":521,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":522,"context_line":"                        _(\u0027Driver method {} not found in vendor interface \u0027"},{"line_number":523,"context_line":"                          \u0027{}\u0027).format(driver_method, vendor_iface))"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_36ba7fc1","line":520,"updated":"2015-10-07 09:12:31.000000000","message":"This is not required in my opinion.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":517,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":518,"context_line":"                        _(\u0027Required parameter require_exclusive_lock not \u0027"},{"line_number":519,"context_line":"                          \u0027found for method %s\u0027) % driver_method)"},{"line_number":520,"context_line":"                else:"},{"line_number":521,"context_line":"                    raise exception.InvalidParameterValue("},{"line_number":522,"context_line":"                        _(\u0027Driver method {} not found in vendor interface \u0027"},{"line_number":523,"context_line":"                          \u0027{}\u0027).format(driver_method, vendor_iface))"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_b6670fe8","line":520,"in_reply_to":"7a2fa921_36ba7fc1","updated":"2015-10-16 18:19:34.000000000","message":"You\u0027re right, I think the original try/except behavior is fine.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"a829917c3690873dddfb0da5b2b738105295193e","unresolved":false,"context_lines":[{"line_number":479,"context_line":"        :raises: UnsupportedDriverExtension if current driver does not have"},{"line_number":480,"context_line":"                 vendor interface or method is unsupported."},{"line_number":481,"context_line":"        :raises: NoFreeConductorWorker when there is no free worker to start"},{"line_number":482,"context_line":"                 async task."},{"line_number":483,"context_line":"        :raises: NodeLocked if node is locked by another conductor."},{"line_number":484,"context_line":"        :returns: A dictionary containing:"},{"line_number":485,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9a8ffd7b_61758f96","line":482,"updated":"2015-11-26 07:53:03.000000000","message":"nit: maybe it\u0027s better to add more detail:\n* NodeLocked exception is because unable to upgrade task lock...","commit_id":"223f30cf3414b60c5b41e809b34439e4afd1dd38"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"efb8b419ad11135792b2ca4f37ae2521c24bdcfe","unresolved":false,"context_lines":[{"line_number":479,"context_line":"        :raises: UnsupportedDriverExtension if current driver does not have"},{"line_number":480,"context_line":"                 vendor interface or method is unsupported."},{"line_number":481,"context_line":"        :raises: NoFreeConductorWorker when there is no free worker to start"},{"line_number":482,"context_line":"                 async task."},{"line_number":483,"context_line":"        :raises: NodeLocked if node is locked by another conductor."},{"line_number":484,"context_line":"        :returns: A dictionary containing:"},{"line_number":485,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9a8ffd7b_eaefc21b","line":482,"in_reply_to":"9a8ffd7b_61758f96","updated":"2015-11-27 17:57:15.000000000","message":"Added more detail.","commit_id":"223f30cf3414b60c5b41e809b34439e4afd1dd38"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"24f067d50cee242f4246fdfec82e4d4c5cb406bb","unresolved":false,"context_lines":[{"line_number":470,"context_line":"        :raises: NoFreeConductorWorker when there is no free worker to start"},{"line_number":471,"context_line":"                 async task."},{"line_number":472,"context_line":"        :raises: NodeLocked if a shared lock is unable to be upgraded if an"},{"line_number":473,"context_line":"                 exclusive lock is required for this method."},{"line_number":474,"context_line":"        :returns: A dictionary containing:"},{"line_number":475,"context_line":""},{"line_number":476,"context_line":"            :return: The response of the invoked vendor method"}],"source_content_type":"text/x-python","patch_set":14,"id":"ba63e191_cbea77ac","line":473,"updated":"2016-01-19 22:31:18.000000000","message":"that\u0027s awkward. How about \u0027if the vendor passthru method requires an exclusive lock but the node is locked by another conductor.\u0027\n\nI don\u0027t think it needs to mention the shared lock being upgraded to an exclusive lock.","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"555042c165f8a8acea1394b1fdd670fc138a7c39","unresolved":false,"context_lines":[{"line_number":470,"context_line":"        :raises: NoFreeConductorWorker when there is no free worker to start"},{"line_number":471,"context_line":"                 async task."},{"line_number":472,"context_line":"        :raises: NodeLocked if a shared lock is unable to be upgraded if an"},{"line_number":473,"context_line":"                 exclusive lock is required for this method."},{"line_number":474,"context_line":"        :returns: A dictionary containing:"},{"line_number":475,"context_line":""},{"line_number":476,"context_line":"            :return: The response of the invoked vendor method"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_d54a3054","line":473,"in_reply_to":"ba63e191_cbea77ac","updated":"2016-05-11 15:32:55.000000000","message":"Done","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"24f067d50cee242f4246fdfec82e4d4c5cb406bb","unresolved":false,"context_lines":[{"line_number":484,"context_line":"        \"\"\""},{"line_number":485,"context_line":"        LOG.debug(\"RPC vendor_passthru called for node %s.\" % node_id)"},{"line_number":486,"context_line":"        # NOTE(mariojv): Not all vendor passthru require an exclusive lock on a"},{"line_number":487,"context_line":"        # node, so we acquire a shared task initially. If a method requires an"},{"line_number":488,"context_line":"        # exclusive lock, we\u0027ll acquire one after checking vendor_opts before"},{"line_number":489,"context_line":"        # starting validation."},{"line_number":490,"context_line":"        with task_manager.acquire(context, node_id, shared\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":14,"id":"ba63e191_ab0ce394","line":487,"updated":"2016-01-19 22:31:18.000000000","message":"the task isn\u0027t shared is it? s/task/lock/","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"555042c165f8a8acea1394b1fdd670fc138a7c39","unresolved":false,"context_lines":[{"line_number":484,"context_line":"        \"\"\""},{"line_number":485,"context_line":"        LOG.debug(\"RPC vendor_passthru called for node %s.\" % node_id)"},{"line_number":486,"context_line":"        # NOTE(mariojv): Not all vendor passthru require an exclusive lock on a"},{"line_number":487,"context_line":"        # node, so we acquire a shared task initially. If a method requires an"},{"line_number":488,"context_line":"        # exclusive lock, we\u0027ll acquire one after checking vendor_opts before"},{"line_number":489,"context_line":"        # starting validation."},{"line_number":490,"context_line":"        with task_manager.acquire(context, node_id, shared\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_f5793494","line":487,"in_reply_to":"ba63e191_ab0ce394","updated":"2016-05-11 15:32:55.000000000","message":"Done","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"24f067d50cee242f4246fdfec82e4d4c5cb406bb","unresolved":false,"context_lines":[{"line_number":506,"context_line":"            # Change shared lock to exclusive if a vendor method requires"},{"line_number":507,"context_line":"            # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":508,"context_line":"            if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":509,"context_line":"                task.upgrade_lock()"},{"line_number":510,"context_line":""},{"line_number":511,"context_line":"            http_method \u003d http_method.upper()"},{"line_number":512,"context_line":"            if http_method not in vendor_opts[\u0027http_methods\u0027]:"}],"source_content_type":"text/x-python","patch_set":14,"id":"ba63e191_6b034b43","line":509,"updated":"2016-01-19 22:31:18.000000000","message":"Out of curiosity, why didn\u0027t you change to an exclusive lock after the http_method check below? (Ie, after line 515)","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"555042c165f8a8acea1394b1fdd670fc138a7c39","unresolved":false,"context_lines":[{"line_number":506,"context_line":"            # Change shared lock to exclusive if a vendor method requires"},{"line_number":507,"context_line":"            # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":508,"context_line":"            if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":509,"context_line":"                task.upgrade_lock()"},{"line_number":510,"context_line":""},{"line_number":511,"context_line":"            http_method \u003d http_method.upper()"},{"line_number":512,"context_line":"            if http_method not in vendor_opts[\u0027http_methods\u0027]:"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_956b88b6","line":509,"in_reply_to":"ba63e191_6b034b43","updated":"2016-05-11 15:32:55.000000000","message":"No reason. Moved to below.","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"24f067d50cee242f4246fdfec82e4d4c5cb406bb","unresolved":false,"context_lines":[{"line_number":514,"context_line":"                    _(\u0027The method %(method)s does not support HTTP %(http)s\u0027) %"},{"line_number":515,"context_line":"                    {\u0027method\u0027: driver_method, \u0027http\u0027: http_method})"},{"line_number":516,"context_line":""},{"line_number":517,"context_line":"            vendor_iface.validate(task, method\u003ddriver_method,"},{"line_number":518,"context_line":"                                  http_method\u003dhttp_method, **info)"},{"line_number":519,"context_line":""},{"line_number":520,"context_line":"            # Inform the vendor method which HTTP method it was invoked with"}],"source_content_type":"text/x-python","patch_set":14,"id":"ba63e191_8b08ff2e","line":517,"updated":"2016-01-19 22:31:18.000000000","message":"we don\u0027t seem to have anything that explicitly says that validate() shouldn\u0027t muck around with a resource but I guess we can assume they don\u0027t.\n\nBut if we assume they don\u0027t, why not defer the shared lock -\u003e exclusive lock, until after the validation succeeds?","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"555042c165f8a8acea1394b1fdd670fc138a7c39","unresolved":false,"context_lines":[{"line_number":514,"context_line":"                    _(\u0027The method %(method)s does not support HTTP %(http)s\u0027) %"},{"line_number":515,"context_line":"                    {\u0027method\u0027: driver_method, \u0027http\u0027: http_method})"},{"line_number":516,"context_line":""},{"line_number":517,"context_line":"            vendor_iface.validate(task, method\u003ddriver_method,"},{"line_number":518,"context_line":"                                  http_method\u003dhttp_method, **info)"},{"line_number":519,"context_line":""},{"line_number":520,"context_line":"            # Inform the vendor method which HTTP method it was invoked with"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_55dc8045","line":517,"in_reply_to":"ba63e191_8b08ff2e","updated":"2016-05-11 15:32:55.000000000","message":"There could be a race condition if we defer the lock upgrade, even if validate doesn\u0027t interfere with the resource. We\u0027d violate repeatable read, in a sense.\n\nThe following scenario could occur:\n\n-\u003e validate is called and succeeds\n-\u003e another operation on the node does something like remove IPMI credentials\n-\u003e no longer valid\n-\u003e we proceed even though validate would now fail","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"6a7633095d64ce394e808baef46793d7beecacc9","unresolved":false,"context_lines":[{"line_number":279,"context_line":"                        http_method, info):"},{"line_number":280,"context_line":"        \"\"\"RPC method to encapsulate vendor action."},{"line_number":281,"context_line":""},{"line_number":282,"context_line":"        Validate driver specific info or get driver status, and if successful"},{"line_number":283,"context_line":"        invoke the vendor method. If the method mode is \u0027async\u0027 the conductor"},{"line_number":284,"context_line":"        will start background worker to perform vendor action."},{"line_number":285,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_8cd67fef","line":282,"updated":"2016-05-13 09:24:26.000000000","message":"Please remove \"or get driver status\", this is not true for current code.","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c7abb067d9206bab51eee48c22c8723511aad483","unresolved":false,"context_lines":[{"line_number":279,"context_line":"                        http_method, info):"},{"line_number":280,"context_line":"        \"\"\"RPC method to encapsulate vendor action."},{"line_number":281,"context_line":""},{"line_number":282,"context_line":"        Validate driver specific info or get driver status, and if successful"},{"line_number":283,"context_line":"        invoke the vendor method. If the method mode is \u0027async\u0027 the conductor"},{"line_number":284,"context_line":"        will start background worker to perform vendor action."},{"line_number":285,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_cb221ba3","line":282,"in_reply_to":"dab17558_8cd67fef","updated":"2016-05-13 14:21:06.000000000","message":"Done","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"6a7633095d64ce394e808baef46793d7beecacc9","unresolved":false,"context_lines":[{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        \"\"\""},{"line_number":310,"context_line":"        LOG.debug(\"RPC vendor_passthru called for node %s.\" % node_id)"},{"line_number":311,"context_line":"        # NOTE(mariojv): Not all vendor passthru require an exclusive lock on a"},{"line_number":312,"context_line":"        # node, so we acquire a shared lock initially. If a method requires an"},{"line_number":313,"context_line":"        # exclusive lock, we\u0027ll acquire one after checking vendor_opts before"},{"line_number":314,"context_line":"        # starting validation."}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_ac76e33b","line":311,"range":{"start_line":311,"start_character":40,"end_line":311,"end_character":48},"updated":"2016-05-13 09:24:26.000000000","message":"Looks like missing \"methods\" word after selection.","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c7abb067d9206bab51eee48c22c8723511aad483","unresolved":false,"context_lines":[{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        \"\"\""},{"line_number":310,"context_line":"        LOG.debug(\"RPC vendor_passthru called for node %s.\" % node_id)"},{"line_number":311,"context_line":"        # NOTE(mariojv): Not all vendor passthru require an exclusive lock on a"},{"line_number":312,"context_line":"        # node, so we acquire a shared lock initially. If a method requires an"},{"line_number":313,"context_line":"        # exclusive lock, we\u0027ll acquire one after checking vendor_opts before"},{"line_number":314,"context_line":"        # starting validation."}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_2059465c","line":311,"range":{"start_line":311,"start_character":40,"end_line":311,"end_character":48},"in_reply_to":"dab17558_ac76e33b","updated":"2016-05-13 14:21:06.000000000","message":"Fixed","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"6a7633095d64ce394e808baef46793d7beecacc9","unresolved":false,"context_lines":[{"line_number":335,"context_line":"                    {\u0027method\u0027: driver_method, \u0027http\u0027: http_method})"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"            # Change shared lock to exclusive if a vendor method requires"},{"line_number":338,"context_line":"            # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":339,"context_line":"            if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":340,"context_line":"                task.upgrade_lock()"},{"line_number":341,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_4c3ad735","line":338,"updated":"2016-05-13 09:24:26.000000000","message":"nit: extra space","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c7abb067d9206bab51eee48c22c8723511aad483","unresolved":false,"context_lines":[{"line_number":335,"context_line":"                    {\u0027method\u0027: driver_method, \u0027http\u0027: http_method})"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"            # Change shared lock to exclusive if a vendor method requires"},{"line_number":338,"context_line":"            # it.  Vendor methods default to requiring an exclusive lock."},{"line_number":339,"context_line":"            if vendor_opts[\u0027require_exclusive_lock\u0027]:"},{"line_number":340,"context_line":"                task.upgrade_lock()"},{"line_number":341,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_2b3ac7cd","line":338,"in_reply_to":"dab17558_4c3ad735","updated":"2016-05-13 14:21:06.000000000","message":"Fixed","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"}],"ironic/conductor/task_manager.py":[{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"e63f59020424d86f8fdd310af7c53959f1f5e640","unresolved":false,"context_lines":[{"line_number":254,"context_line":"            self.upgrade_lock()"},{"line_number":255,"context_line":"            return True"},{"line_number":256,"context_line":"        except exception.NodeLocked:"},{"line_number":257,"context_line":"            LOG.info(\u0027try_upgrade_lock was unable to upgrade lock from shared \u0027"},{"line_number":258,"context_line":"                     \u0027to exclusive\u0027)"},{"line_number":259,"context_line":"            return False"},{"line_number":260,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_7e257ec0","line":257,"updated":"2015-10-18 04:27:15.000000000","message":"In most use-cases, this would be an error. So may be s/info/error ?\n\nAlso if you are going ahead with info, it needs localization with _LI(). Also better to log self.node.uuid here as well.","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"caabb8885c9c4022a210a826f3a5d0b4daa6a0b6","unresolved":false,"context_lines":[{"line_number":254,"context_line":"            self.upgrade_lock()"},{"line_number":255,"context_line":"            return True"},{"line_number":256,"context_line":"        except exception.NodeLocked:"},{"line_number":257,"context_line":"            LOG.info(\u0027try_upgrade_lock was unable to upgrade lock from shared \u0027"},{"line_number":258,"context_line":"                     \u0027to exclusive\u0027)"},{"line_number":259,"context_line":"            return False"},{"line_number":260,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_ed08f82f","line":257,"in_reply_to":"3a29b11f_7e257ec0","updated":"2015-10-28 01:33:56.000000000","message":"How about a warning? https://wiki.openstack.org/wiki/LoggingStandards#Log_level_definitions\n\nIt\u0027s not always an error that the operator would need to investigate, so a warning meets the wiki guidelines. \n\nFor example, there might be a case where we always want to do some part of an action regardless of whether a node is locked, and after that, attempt to do an action with a lock that is desired but not strictly needed for a successful operation.","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"76d648d998461a9d8890db5350c2693b779ac2f8","unresolved":false,"context_lines":[{"line_number":255,"context_line":"            return True"},{"line_number":256,"context_line":"        except exception.NodeLocked:"},{"line_number":257,"context_line":"            LOG.warning(_LW(\u0027try_upgrade_lock was unable to upgrade lock from \u0027"},{"line_number":258,"context_line":"                        \u0027shared to exclusive for node %s\u0027) % self.node_uuid)"},{"line_number":259,"context_line":"            return False"},{"line_number":260,"context_line":""},{"line_number":261,"context_line":"    def upgrade_lock(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_19423a3f","line":258,"updated":"2015-11-04 14:00:51.000000000","message":"s/%/,\n\nAlso formatting to align single quotes in this line to the single quotes in the above line.\n\nWondering if tox -epep8 should be catching this","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"a4d7d578a2ad00a651c5b1d53b145a8488db3dbc","unresolved":false,"context_lines":[{"line_number":255,"context_line":"            return True"},{"line_number":256,"context_line":"        except exception.NodeLocked:"},{"line_number":257,"context_line":"            LOG.warning(_LW(\u0027try_upgrade_lock was unable to upgrade lock from \u0027"},{"line_number":258,"context_line":"                        \u0027shared to exclusive for node %s\u0027) % self.node_uuid)"},{"line_number":259,"context_line":"            return False"},{"line_number":260,"context_line":""},{"line_number":261,"context_line":"    def upgrade_lock(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_98ab6f89","line":258,"in_reply_to":"fa80f949_19423a3f","updated":"2015-11-05 23:50:30.000000000","message":"Done","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"76d648d998461a9d8890db5350c2693b779ac2f8","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        Also reloads node object from the database."},{"line_number":265,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"        :raises NodeLocked"},{"line_number":268,"context_line":"        \"\"\""},{"line_number":269,"context_line":"        if self.shared:"},{"line_number":270,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_59b442f1","line":267,"updated":"2015-11-04 14:00:51.000000000","message":"s/:raises/:raises:","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"a4d7d578a2ad00a651c5b1d53b145a8488db3dbc","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        Also reloads node object from the database."},{"line_number":265,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"        :raises NodeLocked"},{"line_number":268,"context_line":"        \"\"\""},{"line_number":269,"context_line":"        if self.shared:"},{"line_number":270,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_18a0df9e","line":267,"in_reply_to":"fa80f949_59b442f1","updated":"2015-11-05 23:50:30.000000000","message":"Done","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"44fa5239fa236ae68c123556d5f38b6b853bad91","unresolved":false,"context_lines":[{"line_number":247,"context_line":"        Also reloads node object from the database."},{"line_number":248,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":249,"context_line":""},{"line_number":250,"context_line":"        :raises: NodeLocked"},{"line_number":251,"context_line":"        \"\"\""},{"line_number":252,"context_line":"        if self.shared:"},{"line_number":253,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":12,"id":"9a8ffd7b_6607131d","line":250,"updated":"2015-11-25 13:32:57.000000000","message":"\".. if the node is already locked\"","commit_id":"223f30cf3414b60c5b41e809b34439e4afd1dd38"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"efb8b419ad11135792b2ca4f37ae2521c24bdcfe","unresolved":false,"context_lines":[{"line_number":247,"context_line":"        Also reloads node object from the database."},{"line_number":248,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":249,"context_line":""},{"line_number":250,"context_line":"        :raises: NodeLocked"},{"line_number":251,"context_line":"        \"\"\""},{"line_number":252,"context_line":"        if self.shared:"},{"line_number":253,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":12,"id":"9a8ffd7b_4ae12e2c","line":250,"in_reply_to":"9a8ffd7b_6607131d","updated":"2015-11-27 17:57:15.000000000","message":"Done","commit_id":"223f30cf3414b60c5b41e809b34439e4afd1dd38"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"24f067d50cee242f4246fdfec82e4d4c5cb406bb","unresolved":false,"context_lines":[{"line_number":248,"context_line":"        Also reloads node object from the database."},{"line_number":249,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        :raises: NodeLocked if the node is already locked."},{"line_number":252,"context_line":"        \"\"\""},{"line_number":253,"context_line":"        if self.shared:"},{"line_number":254,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":14,"id":"ba63e191_769fae50","line":251,"updated":"2016-01-19 22:31:18.000000000","message":"well, more specifically (since this has a lock on the node but it is a shared lock)\n\nraises NodeLocked if there is already an exclusive lock on the node.","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"555042c165f8a8acea1394b1fdd670fc138a7c39","unresolved":false,"context_lines":[{"line_number":248,"context_line":"        Also reloads node object from the database."},{"line_number":249,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        :raises: NodeLocked if the node is already locked."},{"line_number":252,"context_line":"        \"\"\""},{"line_number":253,"context_line":"        if self.shared:"},{"line_number":254,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_15a4b8b7","line":251,"in_reply_to":"ba63e191_769fae50","updated":"2016-05-11 15:32:55.000000000","message":"Done","commit_id":"ee7d1ccc18091b2e946feb0e23423fc267a5c0a0"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"6a7633095d64ce394e808baef46793d7beecacc9","unresolved":false,"context_lines":[{"line_number":259,"context_line":"        Also reloads node object from the database."},{"line_number":260,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"        :raises: NodeLocked if there is already an exclusive lock on the node."},{"line_number":263,"context_line":"        \"\"\""},{"line_number":264,"context_line":"        if self.shared:"},{"line_number":265,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_32f36e0c","line":262,"updated":"2016-05-13 09:24:26.000000000","message":"This is not very correct. This exception raises if existing lock remains after \"node_locked_retry_attempts\".","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c7abb067d9206bab51eee48c22c8723511aad483","unresolved":false,"context_lines":[{"line_number":259,"context_line":"        Also reloads node object from the database."},{"line_number":260,"context_line":"        Does nothing if lock is already exclusive."},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"        :raises: NodeLocked if there is already an exclusive lock on the node."},{"line_number":263,"context_line":"        \"\"\""},{"line_number":264,"context_line":"        if self.shared:"},{"line_number":265,"context_line":"            LOG.debug(\u0027Upgrading shared lock on node %(uuid)s for %(purpose)s \u0027"}],"source_content_type":"text/x-python","patch_set":15,"id":"dab17558_0bf423cd","line":262,"in_reply_to":"dab17558_32f36e0c","updated":"2016-05-13 14:21:06.000000000","message":"I agree, I will change this. @Ruby are you OK with this change too?","commit_id":"85e5f93ffc32c7137ededa4fe4a421954f75ed85"}],"ironic/drivers/base.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"4e3ca8e49fa879341cfab4cdb4c79e7f1fcaa398","unresolved":false,"context_lines":[{"line_number":605,"context_line":"    :param description: a string shortly describing what the method does."},{"line_number":606,"context_line":"    :param require_exclusive_lock: Boolean value. Only valid for node passthru"},{"line_number":607,"context_line":"                                   methods. If True, lock the node in between"},{"line_number":608,"context_line":"                                   validate() and calling the passthru. The"},{"line_number":609,"context_line":"                                   node remains locked during execution for a"},{"line_number":610,"context_line":"                                   synchronous passthru method. If False,"},{"line_number":611,"context_line":"                                   don\u0027t lock the node. Defaults to True."}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_91dacc77","line":608,"updated":"2015-09-24 15:28:00.000000000","message":"Maybe:\n\ns/calling the passthru/invoking the vendor method","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"1377ef3ec8c218c0a6635f5c53afc846f90c1d09","unresolved":false,"context_lines":[{"line_number":605,"context_line":"    :param description: a string shortly describing what the method does."},{"line_number":606,"context_line":"    :param require_exclusive_lock: Boolean value. Only valid for node passthru"},{"line_number":607,"context_line":"                                   methods. If True, lock the node in between"},{"line_number":608,"context_line":"                                   validate() and calling the passthru. The"},{"line_number":609,"context_line":"                                   node remains locked during execution for a"},{"line_number":610,"context_line":"                                   synchronous passthru method. If False,"},{"line_number":611,"context_line":"                                   don\u0027t lock the node. Defaults to True."}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_2c1a8718","line":608,"in_reply_to":"9a1a9d01_91dacc77","updated":"2015-09-25 17:48:10.000000000","message":"Done","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"2eabb60e0ee670d1dcd57fa08038268e1068382a","unresolved":false,"context_lines":[{"line_number":628,"context_line":"                                                   \u0027attach\u0027: attach})"},{"line_number":629,"context_line":"            func._driver_metadata \u003d metadata"},{"line_number":630,"context_line":"        else:"},{"line_number":631,"context_line":"            metadata[1][\u0027require_exclusive_lock\u0027] \u003d require_exclusive_lock"},{"line_number":632,"context_line":"            func._vendor_metadata \u003d metadata"},{"line_number":633,"context_line":""},{"line_number":634,"context_line":"        passthru_logmessage \u003d _LE(\u0027vendor_passthru failed with method %s\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_6303ad10","line":631,"updated":"2015-09-24 16:31:46.000000000","message":"Could you also add a test for it in tests.drivers.test_base?","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"1377ef3ec8c218c0a6635f5c53afc846f90c1d09","unresolved":false,"context_lines":[{"line_number":628,"context_line":"                                                   \u0027attach\u0027: attach})"},{"line_number":629,"context_line":"            func._driver_metadata \u003d metadata"},{"line_number":630,"context_line":"        else:"},{"line_number":631,"context_line":"            metadata[1][\u0027require_exclusive_lock\u0027] \u003d require_exclusive_lock"},{"line_number":632,"context_line":"            func._vendor_metadata \u003d metadata"},{"line_number":633,"context_line":""},{"line_number":634,"context_line":"        passthru_logmessage \u003d _LE(\u0027vendor_passthru failed with method %s\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_9117f950","line":631,"in_reply_to":"9a1a9d01_6303ad10","updated":"2015-09-25 17:48:10.000000000","message":"Done","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"4e3ca8e49fa879341cfab4cdb4c79e7f1fcaa398","unresolved":false,"context_lines":[{"line_number":629,"context_line":"            func._driver_metadata \u003d metadata"},{"line_number":630,"context_line":"        else:"},{"line_number":631,"context_line":"            metadata[1][\u0027require_exclusive_lock\u0027] \u003d require_exclusive_lock"},{"line_number":632,"context_line":"            func._vendor_metadata \u003d metadata"},{"line_number":633,"context_line":""},{"line_number":634,"context_line":"        passthru_logmessage \u003d _LE(\u0027vendor_passthru failed with method %s\u0027)"},{"line_number":635,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_513194eb","line":632,"updated":"2015-09-24 15:28:00.000000000","message":"Avoid duplicating the common metadata dictionary\n\n def handle_passthru(func):\n     ...\n     supported_ \u003d [i.upper() for i in http_methods]\n     description_ \u003d description or \u0027\u0027\n     metadata \u003d {\u0027http_methods\u0027: supported_,\n                 \u0027async\u0027: async,\n                 \u0027description\u0027: description_,\n                 \u0027attach\u0027: attach}\n\n     if driver_passthru:\n         func._driver_metadata \u003d VendorMetadata(api_method, metadata)\n     else:\n         metadata[\u0027require_exclusive_lock] \u003d require_exclusive_lock\n         func._vendor_metadata \u003d VendorMetadata(api_method, metadata)","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"1377ef3ec8c218c0a6635f5c53afc846f90c1d09","unresolved":false,"context_lines":[{"line_number":629,"context_line":"            func._driver_metadata \u003d metadata"},{"line_number":630,"context_line":"        else:"},{"line_number":631,"context_line":"            metadata[1][\u0027require_exclusive_lock\u0027] \u003d require_exclusive_lock"},{"line_number":632,"context_line":"            func._vendor_metadata \u003d metadata"},{"line_number":633,"context_line":""},{"line_number":634,"context_line":"        passthru_logmessage \u003d _LE(\u0027vendor_passthru failed with method %s\u0027)"},{"line_number":635,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_ac59b790","line":632,"in_reply_to":"9a1a9d01_513194eb","updated":"2015-09-25 17:48:10.000000000","message":"Oops, thanks - forgot to remove the driver_passthru metadata definition when I was testing.\n\nI do have to add the metadata like this, though:\n\nmetadata[1][\u0027require_exclusive_lock\u0027] \u003d require_exclusive_lock\n\nsince VendorMetadata is a namedtuple and not a regular dict (see the definition above this method).","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"a267e8864ad0deb8e089b0439b05335b5c7556e8","unresolved":false,"context_lines":[{"line_number":622,"context_line":"                                               \u0027description\u0027: description_,"},{"line_number":623,"context_line":"                                               \u0027attach\u0027: attach})"},{"line_number":624,"context_line":"        if driver_passthru:"},{"line_number":625,"context_line":"            func._driver_metadata \u003d metadata"},{"line_number":626,"context_line":"        else:"},{"line_number":627,"context_line":"            metadata[1][\u0027require_exclusive_lock\u0027] \u003d require_exclusive_lock"},{"line_number":628,"context_line":"            func._vendor_metadata \u003d metadata"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_2b077f1d","line":625,"updated":"2015-09-28 02:46:50.000000000","message":"require_exclusive_lock doesn\u0027t make sense for driver passthru, but do we need to throw an exception here to make sure someone is informed that they passed an argument that isn\u0027t supported for driver passthru. May be a ValueError ?","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"3dfe89d97a8bcc27c00a44bf2b7f292edc89b921","unresolved":false,"context_lines":[{"line_number":622,"context_line":"                                               \u0027description\u0027: description_,"},{"line_number":623,"context_line":"                                               \u0027attach\u0027: attach})"},{"line_number":624,"context_line":"        if driver_passthru:"},{"line_number":625,"context_line":"            func._driver_metadata \u003d metadata"},{"line_number":626,"context_line":"        else:"},{"line_number":627,"context_line":"            metadata[1][\u0027require_exclusive_lock\u0027] \u003d require_exclusive_lock"},{"line_number":628,"context_line":"            func._vendor_metadata \u003d metadata"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_f0687889","line":625,"in_reply_to":"9a1a9d01_2b077f1d","updated":"2015-09-28 16:39:52.000000000","message":"Good call. I added this in driver_passthru below so that we can keep default arguments in this function. I raise a VendorPassthruException since it\u0027s more descriptive, and ValueError is supposed to be only for built-in operations: https://docs.python.org/2/library/exceptions.html#exceptions.ValueError","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"e0507f79a79db9461614a1056bb04fdbaf7c13ab","unresolved":false,"context_lines":[{"line_number":653,"context_line":""},{"line_number":654,"context_line":"def driver_passthru(http_methods, method\u003dNone, async\u003dTrue, description\u003dNone,"},{"line_number":655,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":656,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":657,"context_line":"        raise exception.VendorPassthruException(message\u003d_LE("},{"line_number":658,"context_line":"            \u0027require_exclusive_lock not supported for driver vendor passthru \u0027"},{"line_number":659,"context_line":"            \u0027methods, only node passthru\u0027))"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a1a9d01_5b061387","line":656,"updated":"2015-09-28 23:20:49.000000000","message":"Is there a unit test for this?","commit_id":"ca71fb77e4858cfe8fcd3a73df2743e1bf3def76"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"8ea87f33b041ccb5033cb61a4012f2b65c624b38","unresolved":false,"context_lines":[{"line_number":653,"context_line":""},{"line_number":654,"context_line":"def driver_passthru(http_methods, method\u003dNone, async\u003dTrue, description\u003dNone,"},{"line_number":655,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":656,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":657,"context_line":"        raise exception.VendorPassthruException(message\u003d_LE("},{"line_number":658,"context_line":"            \u0027require_exclusive_lock not supported for driver vendor passthru \u0027"},{"line_number":659,"context_line":"            \u0027methods, only node passthru\u0027))"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a1a9d01_f2328a0f","line":656,"in_reply_to":"9a1a9d01_5b061387","updated":"2015-09-29 16:01:11.000000000","message":"There isn\u0027t. That\u0027s actually hard to test because the error happens when the class is imported or declared, not when the method is called. Here\u0027s what happens when I add a test, even using an assertRaises: https://gist.github.com/supermari0/deeb1d6d0aee4fa971e9\n\nSince that happens during this test, the exception should be seen whenever a vendor is testing their interface (either through unit tests or running Ironic with the offending vendor driver).\n\nAny other way you can think of testing this? If not, I think it\u0027s still safe from regressions since it would always be caught by the vendor trying to run their code or tests.","commit_id":"ca71fb77e4858cfe8fcd3a73df2743e1bf3def76"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"e53943b849c8073e5b15921d1421ca806f6508e2","unresolved":false,"context_lines":[{"line_number":653,"context_line":""},{"line_number":654,"context_line":"def driver_passthru(http_methods, method\u003dNone, async\u003dTrue, description\u003dNone,"},{"line_number":655,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":656,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":657,"context_line":"        raise exception.VendorPassthruException(message\u003d_LE("},{"line_number":658,"context_line":"            \u0027require_exclusive_lock not supported for driver vendor passthru \u0027"},{"line_number":659,"context_line":"            \u0027methods, only node passthru\u0027))"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a1a9d01_a9dfbb67","line":656,"in_reply_to":"9a1a9d01_f2328a0f","updated":"2015-09-29 21:44:30.000000000","message":"That makes total sense now because it is a decorator. When a decorator with parameters is applied (@driver_passthru) it is run during the import phase.\n\nYes that would be difficult to test. Thanks","commit_id":"ca71fb77e4858cfe8fcd3a73df2743e1bf3def76"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":655,"context_line":"def driver_passthru(http_methods, method\u003dNone, async\u003dTrue, description\u003dNone,"},{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        raise exception.VendorPassthruException(message\u003d_LE("},{"line_number":659,"context_line":"            \u0027require_exclusive_lock not supported for driver vendor passthru \u0027"},{"line_number":660,"context_line":"            \u0027methods, only node passthru\u0027))"},{"line_number":661,"context_line":"    return _passthru(http_methods, method, async, driver_passthru\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_b6fe8f36","line":658,"updated":"2015-10-07 09:12:31.000000000","message":"I am not sure if it should be a exception VendorPassthruException. This is a coding error and should be ValueError in my opinion.  It will be detected at the start of Ironic anyway.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":655,"context_line":"def driver_passthru(http_methods, method\u003dNone, async\u003dTrue, description\u003dNone,"},{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        raise exception.VendorPassthruException(message\u003d_LE("},{"line_number":659,"context_line":"            \u0027require_exclusive_lock not supported for driver vendor passthru \u0027"},{"line_number":660,"context_line":"            \u0027methods, only node passthru\u0027))"},{"line_number":661,"context_line":"    return _passthru(http_methods, method, async, driver_passthru\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_56e31ba4","line":658,"in_reply_to":"7a2fa921_b6fe8f36","updated":"2015-10-16 18:19:34.000000000","message":"Done","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"e63f59020424d86f8fdd310af7c53959f1f5e640","unresolved":false,"context_lines":[{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        # Regular driver vendor passthru methods don\u0027t accept this parameter"},{"line_number":659,"context_line":"        raise ValueError"},{"line_number":660,"context_line":"    return _passthru(http_methods, method, async, driver_passthru\u003dTrue,"},{"line_number":661,"context_line":"                     description\u003ddescription, attach\u003dattach)"},{"line_number":662,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_9e22dab8","line":659,"updated":"2015-10-18 04:27:15.000000000","message":"May be an error message for the driver developer to understand the mistake ?","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"caabb8885c9c4022a210a826f3a5d0b4daa6a0b6","unresolved":false,"context_lines":[{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        # Regular driver vendor passthru methods don\u0027t accept this parameter"},{"line_number":659,"context_line":"        raise ValueError"},{"line_number":660,"context_line":"    return _passthru(http_methods, method, async, driver_passthru\u003dTrue,"},{"line_number":661,"context_line":"                     description\u003ddescription, attach\u003dattach)"},{"line_number":662,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_ad02000a","line":659,"in_reply_to":"3a29b11f_9e22dab8","updated":"2015-10-28 01:33:56.000000000","message":"Done","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"76d648d998461a9d8890db5350c2693b779ac2f8","unresolved":false,"context_lines":[{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        # Regular driver vendor passthru methods don\u0027t accept this parameter"},{"line_number":659,"context_line":"        raise ValueError(_LE(\"Driver vendor passthru methods don\u0027t accept the \""},{"line_number":660,"context_line":"                             \"require_exclusive_lock parameter\"))"},{"line_number":661,"context_line":"    return _passthru(http_methods, method, async, driver_passthru\u003dTrue,"},{"line_number":662,"context_line":"                     description\u003ddescription, attach\u003dattach)"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_39adbee9","line":659,"updated":"2015-11-04 14:00:51.000000000","message":"May be include method name in here since it is available.  Also s/_LE()/_()\n\n  \"Driver vendor passthru method %s cannot accept require_exclusive_lock parameter\"","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"a4d7d578a2ad00a651c5b1d53b145a8488db3dbc","unresolved":false,"context_lines":[{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        # Regular driver vendor passthru methods don\u0027t accept this parameter"},{"line_number":659,"context_line":"        raise ValueError(_LE(\"Driver vendor passthru methods don\u0027t accept the \""},{"line_number":660,"context_line":"                             \"require_exclusive_lock parameter\"))"},{"line_number":661,"context_line":"    return _passthru(http_methods, method, async, driver_passthru\u003dTrue,"},{"line_number":662,"context_line":"                     description\u003ddescription, attach\u003dattach)"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_586e6730","line":659,"in_reply_to":"fa80f949_39adbee9","updated":"2015-11-05 23:50:30.000000000","message":"Changed _LE() to _().\n\nI don\u0027t think the decorated method\u0027s name is available from this scope (\"method\" doesn\u0027t actually refer to the decorated method\u0027s name). We could retrieve the method name if this message was moved into handle_passthru above, where the original function\u0027s in scope. Would that be preferable?","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"44fa5239fa236ae68c123556d5f38b6b853bad91","unresolved":false,"context_lines":[{"line_number":653,"context_line":""},{"line_number":654,"context_line":""},{"line_number":655,"context_line":"def driver_passthru(http_methods, method\u003dNone, async\u003dTrue, description\u003dNone,"},{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        # Regular driver vendor passthru methods don\u0027t accept this parameter"},{"line_number":659,"context_line":"        raise ValueError(_(\"Driver vendor passthru method cannot accept \""}],"source_content_type":"text/x-python","patch_set":12,"id":"9a8ffd7b_8602370a","line":656,"updated":"2015-11-25 13:32:57.000000000","message":"just remove this argument and people won\u0027t use it :)","commit_id":"223f30cf3414b60c5b41e809b34439e4afd1dd38"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"efb8b419ad11135792b2ca4f37ae2521c24bdcfe","unresolved":false,"context_lines":[{"line_number":653,"context_line":""},{"line_number":654,"context_line":""},{"line_number":655,"context_line":"def driver_passthru(http_methods, method\u003dNone, async\u003dTrue, description\u003dNone,"},{"line_number":656,"context_line":"                    attach\u003dFalse, require_exclusive_lock\u003dNone):"},{"line_number":657,"context_line":"    if require_exclusive_lock is not None:"},{"line_number":658,"context_line":"        # Regular driver vendor passthru methods don\u0027t accept this parameter"},{"line_number":659,"context_line":"        raise ValueError(_(\"Driver vendor passthru method cannot accept \""}],"source_content_type":"text/x-python","patch_set":12,"id":"9a8ffd7b_ca619eaa","line":656,"in_reply_to":"9a8ffd7b_8602370a","updated":"2015-11-27 17:57:15.000000000","message":"Ramakrishnan actually wanted me to add this quite a few patch sets ago so that people don\u0027t add this to driver vendor passthru accidentally without some sort of error occurring. \n\nI just tested removing this arg and using it with a driver_passthru decorator, and it errored out with a helpful message about unexpected kwarg, so I agree that this should be removed.","commit_id":"223f30cf3414b60c5b41e809b34439e4afd1dd38"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"c5bb4e7c58ad7b9ededee7302fad853010b17b54","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                   Defaults to False."},{"line_number":614,"context_line":"    :param description: a string shortly describing what the method does."},{"line_number":615,"context_line":"    :param require_exclusive_lock: Boolean value. Only valid for node passthru"},{"line_number":616,"context_line":"                                   methods. If True, lock the node in between"},{"line_number":617,"context_line":"                                   validate() and invoking the vendor method."},{"line_number":618,"context_line":"                                   The node remains locked during execution"},{"line_number":619,"context_line":"                                   for a synchronous passthru method. If False,"}],"source_content_type":"text/x-python","patch_set":13,"id":"7a740942_2e2fdca2","line":616,"updated":"2015-12-03 17:48:42.000000000","message":"Before validate in the code.","commit_id":"266615fa82c8e5b83b3966c34b435543718e3f05"}],"ironic/drivers/modules/agent_base_vendor.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"4e3ca8e49fa879341cfab4cdb4c79e7f1fcaa398","unresolved":false,"context_lines":[{"line_number":341,"context_line":"                return"},{"line_number":342,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":343,"context_line":"                  not self.deploy_has_started(task)):"},{"line_number":344,"context_line":"                msg \u003d _(\u0027Node failed to get image for deploy.\u0027)"},{"line_number":345,"context_line":"                self.continue_deploy(task, **kwargs)"},{"line_number":346,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":347,"context_line":"                  self.deploy_is_done(task)):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_d19da45a","line":344,"updated":"2015-09-24 15:28:00.000000000","message":"Somewhere in this method we require an exclusive lock, I think that the heartbeat() method still should have an exclusive lock for it\n\nhttp://logs.openstack.org/58/226058/2/check/gate-tempest-dsvm-ironic-agent_ssh/89207e4/logs/screen-ir-cond.txt.gz#_2015-09-24_12_47_39_408","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"1377ef3ec8c218c0a6635f5c53afc846f90c1d09","unresolved":false,"context_lines":[{"line_number":341,"context_line":"                return"},{"line_number":342,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":343,"context_line":"                  not self.deploy_has_started(task)):"},{"line_number":344,"context_line":"                msg \u003d _(\u0027Node failed to get image for deploy.\u0027)"},{"line_number":345,"context_line":"                self.continue_deploy(task, **kwargs)"},{"line_number":346,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":347,"context_line":"                  self.deploy_is_done(task)):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_99e42e0e","line":344,"in_reply_to":"9a1a9d01_d19da45a","updated":"2015-09-25 17:48:10.000000000","message":"Thanks for finding the failure in the logs.\n\nI think we can still get away with only exclusively locking in a few cases here, since heartbeat doesn\u0027t really need to lock in states other than the ones we check for here. I\u0027ve updated the task to only lock on deploy-related tasks. That should cover it, but I\u0027ll look into it more if the gate doesn\u0027t pass. Maybe we\u0027ll also need to lock on the cleaning tasks below?\n\nIf we really don\u0027t want to change the lock for heartbeat, can you think of another place where I can allow a shared lock to at least have one use of the change?\n\nI would also be OK with making a separate patch for the heartbeat non-exclusive lock if we find another candidate to use the change on in this patch.","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"a267e8864ad0deb8e089b0439b05335b5c7556e8","unresolved":false,"context_lines":[{"line_number":240,"context_line":""},{"line_number":241,"context_line":"        if not command:"},{"line_number":242,"context_line":"            # Command is not done yet"},{"line_number":243,"context_line":"            return"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        if command.get(\u0027command_status\u0027) \u003d\u003d \u0027FAILED\u0027:"},{"line_number":246,"context_line":"            msg \u003d (_(\u0027Agent returned error for clean step %(step)s on node \u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_eb63c718","line":243,"updated":"2015-09-28 02:46:50.000000000","message":"It\u0027s better to upgrade the lock in this path too as in failure cases to continue_cleaning, we end up calling cleaning_error_handler.  It\u0027s efficient to do here because we do upgrade the lock only when command has completed.","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"3dfe89d97a8bcc27c00a44bf2b7f292edc89b921","unresolved":false,"context_lines":[{"line_number":240,"context_line":""},{"line_number":241,"context_line":"        if not command:"},{"line_number":242,"context_line":"            # Command is not done yet"},{"line_number":243,"context_line":"            return"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        if command.get(\u0027command_status\u0027) \u003d\u003d \u0027FAILED\u0027:"},{"line_number":246,"context_line":"            msg \u003d (_(\u0027Agent returned error for clean step %(step)s on node \u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_4142b811","line":243,"in_reply_to":"9a1a9d01_eb63c718","updated":"2015-09-28 16:39:52.000000000","message":"I\u0027m not sure what you mean. Are you saying that we should lock on every potential state transition?\n\nSo, in this case, we would lock on every call to cleaning_error_handler, and when cleaning completes?\n\nIf so, I could upgrade the lock in cleaning_error_handler itself and when the node finishes cleaning (haven\u0027t found that section of the code yet). Is that reasonable?\n\nIt also wouldn\u0027t be too bad to lock a node for the entire cleaning process during the heartbeat, I think, but I\u0027m not sure.","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"a267e8864ad0deb8e089b0439b05335b5c7556e8","unresolved":false,"context_lines":[{"line_number":342,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":343,"context_line":"                  not self.deploy_has_started(task)):"},{"line_number":344,"context_line":"                msg \u003d _(\u0027Node failed to get image for deploy.\u0027)"},{"line_number":345,"context_line":"                # Deploy actions require exclusive lock"},{"line_number":346,"context_line":"                task.upgrade_lock()"},{"line_number":347,"context_line":"                self.continue_deploy(task, **kwargs)"},{"line_number":348,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_261ed663","line":345,"updated":"2015-09-28 02:46:50.000000000","message":"No test cases for these changes asserting when do we need only shared lock, when do we upgrade to exclusive lock.","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"3dfe89d97a8bcc27c00a44bf2b7f292edc89b921","unresolved":false,"context_lines":[{"line_number":342,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":343,"context_line":"                  not self.deploy_has_started(task)):"},{"line_number":344,"context_line":"                msg \u003d _(\u0027Node failed to get image for deploy.\u0027)"},{"line_number":345,"context_line":"                # Deploy actions require exclusive lock"},{"line_number":346,"context_line":"                task.upgrade_lock()"},{"line_number":347,"context_line":"                self.continue_deploy(task, **kwargs)"},{"line_number":348,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_e14aecab","line":345,"in_reply_to":"9a1a9d01_261ed663","updated":"2015-09-28 16:39:52.000000000","message":"Done. I also added comments for where the tests should go when we decide what to do in the case of cleaning.","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"a267e8864ad0deb8e089b0439b05335b5c7556e8","unresolved":false,"context_lines":[{"line_number":372,"context_line":"                    msg \u003d _(\u0027Node failed to check cleaning progress.\u0027)"},{"line_number":373,"context_line":"                    self.continue_cleaning(task, **kwargs)"},{"line_number":374,"context_line":""},{"line_number":375,"context_line":"        except Exception as e:"},{"line_number":376,"context_line":"            err_info \u003d {\u0027node\u0027: node.uuid, \u0027msg\u0027: msg, \u0027e\u0027: e}"},{"line_number":377,"context_line":"            last_error \u003d _(\u0027Asynchronous exception for node %(node)s: \u0027"},{"line_number":378,"context_line":"                           \u0027%(msg)s exception: %(e)s\u0027) % err_info"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_4bf8b349","line":375,"updated":"2015-09-28 02:46:50.000000000","message":"We need to upgrade the lock in case of exception because the whole block of code is in try/except block.","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"3dfe89d97a8bcc27c00a44bf2b7f292edc89b921","unresolved":false,"context_lines":[{"line_number":372,"context_line":"                    msg \u003d _(\u0027Node failed to check cleaning progress.\u0027)"},{"line_number":373,"context_line":"                    self.continue_cleaning(task, **kwargs)"},{"line_number":374,"context_line":""},{"line_number":375,"context_line":"        except Exception as e:"},{"line_number":376,"context_line":"            err_info \u003d {\u0027node\u0027: node.uuid, \u0027msg\u0027: msg, \u0027e\u0027: e}"},{"line_number":377,"context_line":"            last_error \u003d _(\u0027Asynchronous exception for node %(node)s: \u0027"},{"line_number":378,"context_line":"                           \u0027%(msg)s exception: %(e)s\u0027) % err_info"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_5384ced2","line":375,"in_reply_to":"9a1a9d01_4bf8b349","updated":"2015-09-28 16:39:52.000000000","message":"Done. You\u0027re right, we wouldn\u0027t want to allow a race condition where the failed state gets overwritten because of a missing lock.","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"140723af8edf2a7d6541d44121c7436cb940018a","unresolved":false,"context_lines":[{"line_number":372,"context_line":"                    msg \u003d _(\u0027Node failed to check cleaning progress.\u0027)"},{"line_number":373,"context_line":"                    self.continue_cleaning(task, **kwargs)"},{"line_number":374,"context_line":""},{"line_number":375,"context_line":"        except Exception as e:"},{"line_number":376,"context_line":"            # Upgrade the lock to disallow operations on a node with erroneous"},{"line_number":377,"context_line":"            # heartbeat behavior"},{"line_number":378,"context_line":"            task.upgrade_lock()"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_3edafa4c","line":375,"updated":"2015-09-29 22:43:38.000000000","message":"If the upgrade_lock() calls at :346 or :352 fail, they will raise an exception, which will be caught here -- and just tried again. That may not be what you want.","commit_id":"3fc807f34653c4aa6be415603dc8e1742f60a57d"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"ce47cb076d5ac3fea0c4b9d8e76539a9bd21e0d6","unresolved":false,"context_lines":[{"line_number":372,"context_line":"                    msg \u003d _(\u0027Node failed to check cleaning progress.\u0027)"},{"line_number":373,"context_line":"                    self.continue_cleaning(task, **kwargs)"},{"line_number":374,"context_line":""},{"line_number":375,"context_line":"        except Exception as e:"},{"line_number":376,"context_line":"            # Upgrade the lock to disallow operations on a node with erroneous"},{"line_number":377,"context_line":"            # heartbeat behavior"},{"line_number":378,"context_line":"            task.upgrade_lock()"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_add9e258","line":375,"in_reply_to":"9a1a9d01_3edafa4c","updated":"2015-09-30 18:31:51.000000000","message":"That\u0027s a really good point, I didn\u0027t think about that. The adverse effects would be minimal (nonexistent?) since nothing important is modified there before upgrading a lock, but we should at least log a helpful warning message. I put it as a warning for now, but I can change it to a different log level if someone wants.","commit_id":"3fc807f34653c4aa6be415603dc8e1742f60a57d"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"140723af8edf2a7d6541d44121c7436cb940018a","unresolved":false,"context_lines":[{"line_number":375,"context_line":"        except Exception as e:"},{"line_number":376,"context_line":"            # Upgrade the lock to disallow operations on a node with erroneous"},{"line_number":377,"context_line":"            # heartbeat behavior"},{"line_number":378,"context_line":"            task.upgrade_lock()"},{"line_number":379,"context_line":"            err_info \u003d {\u0027node\u0027: node.uuid, \u0027msg\u0027: msg, \u0027e\u0027: e}"},{"line_number":380,"context_line":"            last_error \u003d _(\u0027Asynchronous exception for node %(node)s: \u0027"},{"line_number":381,"context_line":"                           \u0027%(msg)s exception: %(e)s\u0027) % err_info"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_dedade4a","line":378,"updated":"2015-09-29 22:43:38.000000000","message":"What if this call fails (because something else managed to acquire a lock right there) and raises an exception?","commit_id":"3fc807f34653c4aa6be415603dc8e1742f60a57d"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"ce47cb076d5ac3fea0c4b9d8e76539a9bd21e0d6","unresolved":false,"context_lines":[{"line_number":375,"context_line":"        except Exception as e:"},{"line_number":376,"context_line":"            # Upgrade the lock to disallow operations on a node with erroneous"},{"line_number":377,"context_line":"            # heartbeat behavior"},{"line_number":378,"context_line":"            task.upgrade_lock()"},{"line_number":379,"context_line":"            err_info \u003d {\u0027node\u0027: node.uuid, \u0027msg\u0027: msg, \u0027e\u0027: e}"},{"line_number":380,"context_line":"            last_error \u003d _(\u0027Asynchronous exception for node %(node)s: \u0027"},{"line_number":381,"context_line":"                           \u0027%(msg)s exception: %(e)s\u0027) % err_info"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_6dad0a9a","line":378,"in_reply_to":"9a1a9d01_dedade4a","updated":"2015-09-30 18:31:51.000000000","message":"In this case, the behavior wouldn\u0027t be as graceful since we aren\u0027t calling the error handlers or putting the node in a failed state. It\u0027s possible (probable?) that the next heartbeat after the node is unlocked would catch it, but the behavior still isn\u0027t good.\n\nTo mitigate this, we can lock the node during cleaning. Then, a node\u0027s lock will persist through a cleaning attempt and failure, so the behavior would be the same as before this patch. We should also lock during a deploy that\u0027s already started to avoid the following race condition:\n\n1. Something goes wrong during node.touch_provisioning\n\n2. We try to upgrade the lock here to set the fail state\n\n3. We fail to upgrade the lock, and a failed deploy isn\u0027t properly handled\n\nI\u0027ll keep the lock upgrade in the exception as an extra precaution, but now the node should already be locked by the heartbeat during every action except updating the heartbeat time itself.\n\nIn the case that the NodeLockedException is thrown here, I think the best policy would still be to attempt to fail the node gracefully as is already done in the method.","commit_id":"3fc807f34653c4aa6be415603dc8e1742f60a57d"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":240,"context_line":""},{"line_number":241,"context_line":"        if not command:"},{"line_number":242,"context_line":"            # Command is not done yet"},{"line_number":243,"context_line":"            return"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        if command.get(\u0027command_status\u0027) \u003d\u003d \u0027FAILED\u0027:"},{"line_number":246,"context_line":"            msg \u003d (_(\u0027Agent returned error for clean step %(step)s on node \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_e0bc406e","line":243,"updated":"2015-10-07 09:12:31.000000000","message":"Continuing discussion from patch set 3, there are places below from here where we need an exclusive lock once we found that the last cleaning command has completed.  See below...","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":240,"context_line":""},{"line_number":241,"context_line":"        if not command:"},{"line_number":242,"context_line":"            # Command is not done yet"},{"line_number":243,"context_line":"            return"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        if command.get(\u0027command_status\u0027) \u003d\u003d \u0027FAILED\u0027:"},{"line_number":246,"context_line":"            msg \u003d (_(\u0027Agent returned error for clean step %(step)s on node \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_d69c0b10","line":243,"in_reply_to":"7a2fa921_e0bc406e","updated":"2015-10-16 18:19:34.000000000","message":"Thanks for the detailed responses.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":249,"context_line":"                    \u0027err\u0027: command.get(\u0027command_error\u0027),"},{"line_number":250,"context_line":"                    \u0027step\u0027: node.clean_step})"},{"line_number":251,"context_line":"            LOG.error(msg)"},{"line_number":252,"context_line":"            return manager.cleaning_error_handler(task, msg)"},{"line_number":253,"context_line":"        elif command.get(\u0027command_status\u0027) \u003d\u003d \u0027CLEAN_VERSION_MISMATCH\u0027:"},{"line_number":254,"context_line":"            # Restart cleaning, agent must have rebooted to new version"},{"line_number":255,"context_line":"            LOG.info(_LI(\u0027Node %s detected a clean version mismatch, \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_c0b9047b","line":252,"updated":"2015-10-07 09:12:31.000000000","message":"We need an exclusive lock here as it might end up doing tear_down_cleaning.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":249,"context_line":"                    \u0027err\u0027: command.get(\u0027command_error\u0027),"},{"line_number":250,"context_line":"                    \u0027step\u0027: node.clean_step})"},{"line_number":251,"context_line":"            LOG.error(msg)"},{"line_number":252,"context_line":"            return manager.cleaning_error_handler(task, msg)"},{"line_number":253,"context_line":"        elif command.get(\u0027command_status\u0027) \u003d\u003d \u0027CLEAN_VERSION_MISMATCH\u0027:"},{"line_number":254,"context_line":"            # Restart cleaning, agent must have rebooted to new version"},{"line_number":255,"context_line":"            LOG.info(_LI(\u0027Node %s detected a clean version mismatch, \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_ca6a18ed","line":252,"in_reply_to":"7a2fa921_c0b9047b","updated":"2015-10-16 18:19:34.000000000","message":"Done","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":263,"context_line":"                       {\u0027node\u0027: node.uuid,"},{"line_number":264,"context_line":"                        \u0027err\u0027: command.get(\u0027command_error\u0027),"},{"line_number":265,"context_line":"                        \u0027step\u0027: node.clean_step})"},{"line_number":266,"context_line":"                LOG.exception(msg)"},{"line_number":267,"context_line":"                return manager.cleaning_error_handler(task, msg)"},{"line_number":268,"context_line":"            self._notify_conductor_resume_clean(task)"},{"line_number":269,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_80ac6c36","line":266,"updated":"2015-10-07 09:12:31.000000000","message":"We need an exclusive lock here as it might end up doing tear_down_cleaning.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":263,"context_line":"                       {\u0027node\u0027: node.uuid,"},{"line_number":264,"context_line":"                        \u0027err\u0027: command.get(\u0027command_error\u0027),"},{"line_number":265,"context_line":"                        \u0027step\u0027: node.clean_step})"},{"line_number":266,"context_line":"                LOG.exception(msg)"},{"line_number":267,"context_line":"                return manager.cleaning_error_handler(task, msg)"},{"line_number":268,"context_line":"            self._notify_conductor_resume_clean(task)"},{"line_number":269,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_ca41b864","line":266,"in_reply_to":"7a2fa921_80ac6c36","updated":"2015-10-16 18:19:34.000000000","message":"Done","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":275,"context_line":"                          {\u0027method\u0027: clean_step_hook.__name__,"},{"line_number":276,"context_line":"                           \u0027node\u0027: node.uuid,"},{"line_number":277,"context_line":"                           \u0027step\u0027: node.clean_step})"},{"line_number":278,"context_line":"                try:"},{"line_number":279,"context_line":"                    clean_step_hook(task, command)"},{"line_number":280,"context_line":"                except Exception as e:"},{"line_number":281,"context_line":"                    msg \u003d (_(\u0027For node %(node)s, post clean step hook \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_004e1c96","line":278,"updated":"2015-10-07 09:12:31.000000000","message":"We need an exclusive lock here as it might end up doing tear_down_cleaning as post clean hook assumes that you have exclusive lock (though not mentioned anywhere, this was the norm in liberty).","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":275,"context_line":"                          {\u0027method\u0027: clean_step_hook.__name__,"},{"line_number":276,"context_line":"                           \u0027node\u0027: node.uuid,"},{"line_number":277,"context_line":"                           \u0027step\u0027: node.clean_step})"},{"line_number":278,"context_line":"                try:"},{"line_number":279,"context_line":"                    clean_step_hook(task, command)"},{"line_number":280,"context_line":"                except Exception as e:"},{"line_number":281,"context_line":"                    msg \u003d (_(\u0027For node %(node)s, post clean step hook \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_45491b5f","line":278,"in_reply_to":"7a2fa921_004e1c96","updated":"2015-10-16 18:19:34.000000000","message":"Done","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":285,"context_line":"                            \u0027node\u0027: node.uuid,"},{"line_number":286,"context_line":"                            \u0027error\u0027: e,"},{"line_number":287,"context_line":"                            \u0027step\u0027: node.clean_step})"},{"line_number":288,"context_line":"                    LOG.exception(msg)"},{"line_number":289,"context_line":"                    return manager.cleaning_error_handler(task, msg)"},{"line_number":290,"context_line":""},{"line_number":291,"context_line":"            LOG.info(_LI(\u0027Agent on node %s returned cleaning command success, \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_a0440874","line":288,"updated":"2015-10-07 09:12:31.000000000","message":"We need an exclusive lock here as it might end up doing tear_down_cleaning.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":285,"context_line":"                            \u0027node\u0027: node.uuid,"},{"line_number":286,"context_line":"                            \u0027error\u0027: e,"},{"line_number":287,"context_line":"                            \u0027step\u0027: node.clean_step})"},{"line_number":288,"context_line":"                    LOG.exception(msg)"},{"line_number":289,"context_line":"                    return manager.cleaning_error_handler(task, msg)"},{"line_number":290,"context_line":""},{"line_number":291,"context_line":"            LOG.info(_LI(\u0027Agent on node %s returned cleaning command success, \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_0576c3a3","line":288,"in_reply_to":"7a2fa921_a0440874","updated":"2015-10-16 18:19:34.000000000","message":"Done","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":297,"context_line":"                   {\u0027node\u0027: node.uuid,"},{"line_number":298,"context_line":"                    \u0027err\u0027: command.get(\u0027command_status\u0027),"},{"line_number":299,"context_line":"                    \u0027step\u0027: node.clean_step})"},{"line_number":300,"context_line":"            LOG.error(msg)"},{"line_number":301,"context_line":"            return manager.cleaning_error_handler(task, msg)"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":"    @base.passthru([\u0027POST\u0027], require_exclusive_lock\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_605fd041","line":300,"updated":"2015-10-07 09:12:31.000000000","message":"We need an exclusive lock here as it might end up doing tear_down_cleaning.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":297,"context_line":"                   {\u0027node\u0027: node.uuid,"},{"line_number":298,"context_line":"                    \u0027err\u0027: command.get(\u0027command_status\u0027),"},{"line_number":299,"context_line":"                    \u0027step\u0027: node.clean_step})"},{"line_number":300,"context_line":"            LOG.error(msg)"},{"line_number":301,"context_line":"            return manager.cleaning_error_handler(task, msg)"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":"    @base.passthru([\u0027POST\u0027], require_exclusive_lock\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_257bbf8d","line":300,"in_reply_to":"7a2fa921_605fd041","updated":"2015-10-16 18:19:34.000000000","message":"Done","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"f28afab81936f60d9600fe8f111de150f0ee4fc0","unresolved":false,"context_lines":[{"line_number":342,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":343,"context_line":"                  not self.deploy_has_started(task)):"},{"line_number":344,"context_line":"                msg \u003d _(\u0027Node failed to get image for deploy.\u0027)"},{"line_number":345,"context_line":"                try:"},{"line_number":346,"context_line":"                    # Deploy actions require exclusive lock"},{"line_number":347,"context_line":"                    task.upgrade_lock()"},{"line_number":348,"context_line":"                except exception.NodeLockedException:"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_fb2f153c","line":345,"updated":"2015-10-07 09:12:31.000000000","message":"Since we have this 7 lines of code reperated in most places, it might be worth moving to a common function and into something like below everywhere:\n\n  if not _acquire_exclusive_lock(task):\n    return","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"c939d8387d6a373288526a90015c491a727d37e8","unresolved":false,"context_lines":[{"line_number":342,"context_line":"            elif (node.provision_state \u003d\u003d states.DEPLOYWAIT and"},{"line_number":343,"context_line":"                  not self.deploy_has_started(task)):"},{"line_number":344,"context_line":"                msg \u003d _(\u0027Node failed to get image for deploy.\u0027)"},{"line_number":345,"context_line":"                try:"},{"line_number":346,"context_line":"                    # Deploy actions require exclusive lock"},{"line_number":347,"context_line":"                    task.upgrade_lock()"},{"line_number":348,"context_line":"                except exception.NodeLockedException:"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a2ca52d_768e57d1","line":345,"in_reply_to":"7a2fa921_fb2f153c","updated":"2015-10-16 18:19:34.000000000","message":"OK. I wanted to keep a specific error message for each case, but that may not be necessary.","commit_id":"0589c09eee83983a9aeaaabcdf8e2e8872b3f05c"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"e63f59020424d86f8fdd310af7c53959f1f5e640","unresolved":false,"context_lines":[{"line_number":249,"context_line":"                    \u0027err\u0027: command.get(\u0027command_error\u0027),"},{"line_number":250,"context_line":"                    \u0027step\u0027: node.clean_step})"},{"line_number":251,"context_line":"            LOG.error(msg)"},{"line_number":252,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":253,"context_line":"                return"},{"line_number":254,"context_line":"            return manager.cleaning_error_handler(task, msg)"},{"line_number":255,"context_line":"        elif command.get(\u0027command_status\u0027) \u003d\u003d \u0027CLEAN_VERSION_MISMATCH\u0027:"}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_313417dd","line":252,"updated":"2015-10-18 04:27:15.000000000","message":"I saw a comment in test_agent_base_vendor.py which I hope is regarding this.  I think we could move the below 4 calls to one place after lin 243 above as almost all of the time, we would require an exlusive lock if a cleaning complete just completed (regardless of whether success/failure).","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"caabb8885c9c4022a210a826f3a5d0b4daa6a0b6","unresolved":false,"context_lines":[{"line_number":249,"context_line":"                    \u0027err\u0027: command.get(\u0027command_error\u0027),"},{"line_number":250,"context_line":"                    \u0027step\u0027: node.clean_step})"},{"line_number":251,"context_line":"            LOG.error(msg)"},{"line_number":252,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":253,"context_line":"                return"},{"line_number":254,"context_line":"            return manager.cleaning_error_handler(task, msg)"},{"line_number":255,"context_line":"        elif command.get(\u0027command_status\u0027) \u003d\u003d \u0027CLEAN_VERSION_MISMATCH\u0027:"}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_9ba5f3b6","line":252,"in_reply_to":"3a29b11f_313417dd","updated":"2015-10-28 01:33:56.000000000","message":"That sounds good. I was thinking there are a lot of calls to try_upgrade_lock here, so that will make it cleaner.","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"026649b0c2d0b18683e847854455b0db80220965","unresolved":false,"context_lines":[{"line_number":406,"context_line":"            last_error \u003d _(\u0027Asynchronous exception for node %(node)s: \u0027"},{"line_number":407,"context_line":"                           \u0027%(msg)s exception: %(e)s\u0027) % err_info"},{"line_number":408,"context_line":""},{"line_number":409,"context_line":"            # We don\u0027t return here, as we still want to try to fail gracefully"},{"line_number":410,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":411,"context_line":"                LOG.exception(_LE(\u0027Node {} failed to lock during heartbeat \u0027"},{"line_number":412,"context_line":"                                  \u0027exception: {}\u0027).format(node.get("}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_4db5c17b","line":409,"updated":"2015-10-16 18:26:05.000000000","message":"Ramakrishnan - do you think that this is a good way to handle the edge case here?\n\nThere is a chance that we would call cleaning_error_handler without acquiring the lock, but I can\u0027t think of another way to handle this except possibly returning and having the heartbeat try again another time. I\u0027m fine with either option.","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"caabb8885c9c4022a210a826f3a5d0b4daa6a0b6","unresolved":false,"context_lines":[{"line_number":406,"context_line":"            last_error \u003d _(\u0027Asynchronous exception for node %(node)s: \u0027"},{"line_number":407,"context_line":"                           \u0027%(msg)s exception: %(e)s\u0027) % err_info"},{"line_number":408,"context_line":""},{"line_number":409,"context_line":"            # We don\u0027t return here, as we still want to try to fail gracefully"},{"line_number":410,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":411,"context_line":"                LOG.exception(_LE(\u0027Node {} failed to lock during heartbeat \u0027"},{"line_number":412,"context_line":"                                  \u0027exception: {}\u0027).format(node.get("}],"source_content_type":"text/x-python","patch_set":8,"id":"1a26ad4f_16931afc","line":409,"in_reply_to":"3a29b11f_91108b50","updated":"2015-10-28 01:33:56.000000000","message":"Done","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"e63f59020424d86f8fdd310af7c53959f1f5e640","unresolved":false,"context_lines":[{"line_number":406,"context_line":"            last_error \u003d _(\u0027Asynchronous exception for node %(node)s: \u0027"},{"line_number":407,"context_line":"                           \u0027%(msg)s exception: %(e)s\u0027) % err_info"},{"line_number":408,"context_line":""},{"line_number":409,"context_line":"            # We don\u0027t return here, as we still want to try to fail gracefully"},{"line_number":410,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":411,"context_line":"                LOG.exception(_LE(\u0027Node {} failed to lock during heartbeat \u0027"},{"line_number":412,"context_line":"                                  \u0027exception: {}\u0027).format(node.get("}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_91108b50","line":409,"in_reply_to":"5a2ca52d_4db5c17b","updated":"2015-10-18 04:27:15.000000000","message":"I think we should return here as well.  Calling cleaning_error_handler() without lock is going to be more dangerous in my opinion than doing anything at all.  We can hope that we would get another heartbeat by which we would be triggering something again.\n\nBut another way to look at this is, would we ever have a shared lock at this point.  All the above places which can raise exceptions would be called only if we acquired exclusive locks.  So we might not be here with a shared lock.\n\nSo in my opinion:\n\n1) Add a comment above saying this is theoretically not possible to have a shared lock unless we have some coding error (someone missed to upgrade the lock when they added a new condition)\n\n2) Return if we cannot upgrade lock.","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"e63f59020424d86f8fdd310af7c53959f1f5e640","unresolved":false,"context_lines":[{"line_number":408,"context_line":""},{"line_number":409,"context_line":"            # We don\u0027t return here, as we still want to try to fail gracefully"},{"line_number":410,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":411,"context_line":"                LOG.exception(_LE(\u0027Node {} failed to lock during heartbeat \u0027"},{"line_number":412,"context_line":"                                  \u0027exception: {}\u0027).format(node.get("},{"line_number":413,"context_line":"                                      \u0027uuid\u0027, \u0027UNKNOWN\u0027), last_error))"},{"line_number":414,"context_line":"            LOG.exception(last_error)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3a29b11f_311df766","line":411,"updated":"2015-10-18 04:27:15.000000000","message":"Formatting should be done outside of localization call (I think this would still do the formatting and then pass on formatted string to LOG.exception).","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"caabb8885c9c4022a210a826f3a5d0b4daa6a0b6","unresolved":false,"context_lines":[{"line_number":408,"context_line":""},{"line_number":409,"context_line":"            # We don\u0027t return here, as we still want to try to fail gracefully"},{"line_number":410,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":411,"context_line":"                LOG.exception(_LE(\u0027Node {} failed to lock during heartbeat \u0027"},{"line_number":412,"context_line":"                                  \u0027exception: {}\u0027).format(node.get("},{"line_number":413,"context_line":"                                      \u0027uuid\u0027, \u0027UNKNOWN\u0027), last_error))"},{"line_number":414,"context_line":"            LOG.exception(last_error)"}],"source_content_type":"text/x-python","patch_set":8,"id":"1a26ad4f_369096ec","line":411,"in_reply_to":"3a29b11f_311df766","updated":"2015-10-28 01:33:56.000000000","message":"It is happening outside the localization call already, if you look at the location of the parentheses. I\u0027m just using .format instead of %.\n\nI\u0027ll switch it to using % and move the formatting outside of the method call so that this is more obvious.","commit_id":"dd80ee62dbc3396b73a038de21a682fa0eb780d4"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"3d4a7bfb60a0e99f923b42582a3d43f108435222","unresolved":false,"context_lines":[{"line_number":331,"context_line":"                                                    \u0027\"agent_url\" must be \u0027"},{"line_number":332,"context_line":"                                                    \u0027specified.\u0027))"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"        node.driver_internal_info \u003d driver_internal_info"},{"line_number":335,"context_line":"        node.save()"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"        # Async call backs don\u0027t set error state on their own"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_d95592c6","line":334,"updated":"2015-11-04 13:47:16.000000000","message":"I\u0027m afraid about race here.\n - Driver \"foo\" acquires exclusive lock.\n - Agent driver acquires non-exclusive lock on the same node.\n - Driver \"foo\" changes node.driver_internal_info.\n - Driver \"foo\" saves the node.\n - Agent driver saves the node.\n\nAll modification of node.driver_internal_info from driver \"foo\" will be lost.","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"76d648d998461a9d8890db5350c2693b779ac2f8","unresolved":false,"context_lines":[{"line_number":331,"context_line":"                                                    \u0027\"agent_url\" must be \u0027"},{"line_number":332,"context_line":"                                                    \u0027specified.\u0027))"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"        node.driver_internal_info \u003d driver_internal_info"},{"line_number":335,"context_line":"        node.save()"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"        # Async call backs don\u0027t set error state on their own"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_d9811291","line":334,"in_reply_to":"fa80f949_d95592c6","updated":"2015-11-04 14:00:51.000000000","message":"Hmm. That\u0027s true.  Does it help if we move the heartbeat to a separate field on the node table ?","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"a4d7d578a2ad00a651c5b1d53b145a8488db3dbc","unresolved":false,"context_lines":[{"line_number":331,"context_line":"                                                    \u0027\"agent_url\" must be \u0027"},{"line_number":332,"context_line":"                                                    \u0027specified.\u0027))"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"        node.driver_internal_info \u003d driver_internal_info"},{"line_number":335,"context_line":"        node.save()"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"        # Async call backs don\u0027t set error state on their own"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_fea4e3b9","line":334,"in_reply_to":"fa80f949_d9811291","updated":"2015-11-05 23:50:30.000000000","message":"That\u0027s a really good point Yuriy, thanks. I left it how it is for now but commented about some potential strategies below. \n\nMoving the heartbeat to a separate field would avoid that race condition, but I\u0027m not sure it\u0027s appropriate since not every Ironic operator uses the agent, and the fields in driver_internal_info we\u0027re modifying *are* related to the internals of this driver.\n\nIf we leave the DB as-is, I see 2 options here:\n\n1. Lock above this part of the method, returning if it upgrading the lock fails, and remove the lock upgrades below (since we would be upgrading on every heartbeat). This would eliminate the race condition, but I don\u0027t think that it\u0027s necessarily the best path because then the only area of code that would execute asynchronously against the node would be the small bit above that updates the heartbeat time variable.\n\n2. Don\u0027t use require_exclusive_lock\u003dFalse with this method. We could then either just leave the rest of the change as an option for a future driver, or I\u0027d be willing to try require_exclusive_lock\u003dFalse on another node vendor passthru method, if we can find any good candidates.\n\nWhat do you two think? 1 is easier but 2 seems better if we find a candidate method.","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"76d648d998461a9d8890db5350c2693b779ac2f8","unresolved":false,"context_lines":[{"line_number":409,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":410,"context_line":"                err_info \u003d {\u0027node\u0027: node.uuid, \u0027err\u0027: last_error}"},{"line_number":411,"context_line":"                err_message \u003d _LE(\u0027Node {node} failed to lock during \u0027"},{"line_number":412,"context_line":"                                  \u0027heartbeat \u0027\u0027exception: {err}\u0027) % err_info"},{"line_number":413,"context_line":"                LOG.exception(err_message)"},{"line_number":414,"context_line":"                return"},{"line_number":415,"context_line":"            LOG.exception(last_error)"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_5941a20a","line":412,"updated":"2015-11-04 14:00:51.000000000","message":"Formatting should be done within LOG.exception call.\n\n  LOG.exception(_LE(\"Node %(node)s failed to lock during \"\n      \"heartbeat exception: %(err)s\", err_info)\n\nMay be also pull this up before line 401, i.e first thing we do is to try and upgrade the lock.","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"a4d7d578a2ad00a651c5b1d53b145a8488db3dbc","unresolved":false,"context_lines":[{"line_number":409,"context_line":"            if not task.try_upgrade_lock():"},{"line_number":410,"context_line":"                err_info \u003d {\u0027node\u0027: node.uuid, \u0027err\u0027: last_error}"},{"line_number":411,"context_line":"                err_message \u003d _LE(\u0027Node {node} failed to lock during \u0027"},{"line_number":412,"context_line":"                                  \u0027heartbeat \u0027\u0027exception: {err}\u0027) % err_info"},{"line_number":413,"context_line":"                LOG.exception(err_message)"},{"line_number":414,"context_line":"                return"},{"line_number":415,"context_line":"            LOG.exception(last_error)"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_7b36c5e8","line":412,"in_reply_to":"fa80f949_5941a20a","updated":"2015-11-05 23:50:30.000000000","message":"Changed to format within LOG.exception.\n\nI think we need to keep this below when err_info and last_err are defined so we can keep the reason that the heartbeat raised an exception in the first place (last_error) in the error message.","commit_id":"7d87ed0336775efc6f3f7cea70a5805855a9b9d1"}],"ironic/drivers/modules/fake.py":[{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"2eabb60e0ee670d1dcd57fa08038268e1068382a","unresolved":false,"context_lines":[{"line_number":132,"context_line":"        return {\u0027B1\u0027: \u0027B1 description. Required.\u0027,"},{"line_number":133,"context_line":"                \u0027B2\u0027: \u0027B2 description. Required.\u0027}"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"    def validate(self, task, method, **kwargs):"},{"line_number":136,"context_line":"        if method in (\u0027second_method\u0027, \u0027third_method_sync\u0027):"},{"line_number":137,"context_line":"            bar \u003d kwargs.get(\u0027bar\u0027)"},{"line_number":138,"context_line":"            if not bar:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_c3595990","line":135,"updated":"2015-09-24 16:31:46.000000000","message":"fourth_method_shared_lock should be added too.","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"1377ef3ec8c218c0a6635f5c53afc846f90c1d09","unresolved":false,"context_lines":[{"line_number":132,"context_line":"        return {\u0027B1\u0027: \u0027B1 description. Required.\u0027,"},{"line_number":133,"context_line":"                \u0027B2\u0027: \u0027B2 description. Required.\u0027}"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"    def validate(self, task, method, **kwargs):"},{"line_number":136,"context_line":"        if method in (\u0027second_method\u0027, \u0027third_method_sync\u0027):"},{"line_number":137,"context_line":"            bar \u003d kwargs.get(\u0027bar\u0027)"},{"line_number":138,"context_line":"            if not bar:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a1a9d01_d4d80456","line":135,"in_reply_to":"9a1a9d01_c3595990","updated":"2015-09-25 17:48:10.000000000","message":"Done","commit_id":"26e826c8b93ead2a48120c467225d46e0d1bd518"}],"ironic/tests/conductor/test_manager.py":[{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"a267e8864ad0deb8e089b0439b05335b5c7556e8","unresolved":false,"context_lines":[{"line_number":630,"context_line":"        self.assertTrue(response[\u0027async\u0027])"},{"line_number":631,"context_line":""},{"line_number":632,"context_line":"        # Assert lock was never upgraded to an exclusive one"},{"line_number":633,"context_line":"        self.assertEqual(0, mock_upgrade.call_count)"},{"line_number":634,"context_line":""},{"line_number":635,"context_line":"        node.refresh()"},{"line_number":636,"context_line":"        self.assertIsNone(node.last_error)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_2b59bf5a","line":633,"updated":"2015-09-28 02:46:50.000000000","message":"nit:  below seems more natural to me :)\n\n  self.assertFalse(mock_upgrade.called)","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"},{"author":{"_account_id":13295,"name":"Mario Villaplana","email":"mario.villaplana@gmail.com","username":"mariojv"},"change_message_id":"3dfe89d97a8bcc27c00a44bf2b7f292edc89b921","unresolved":false,"context_lines":[{"line_number":630,"context_line":"        self.assertTrue(response[\u0027async\u0027])"},{"line_number":631,"context_line":""},{"line_number":632,"context_line":"        # Assert lock was never upgraded to an exclusive one"},{"line_number":633,"context_line":"        self.assertEqual(0, mock_upgrade.call_count)"},{"line_number":634,"context_line":""},{"line_number":635,"context_line":"        node.refresh()"},{"line_number":636,"context_line":"        self.assertIsNone(node.last_error)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a1a9d01_a1c074e1","line":633,"in_reply_to":"9a1a9d01_2b59bf5a","updated":"2015-09-28 16:39:52.000000000","message":"Done","commit_id":"00828d74ae1f9e81f1350530ab3dfc6807efbbed"}],"ironic/tests/unit/conductor/test_manager.py":[{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"ff55e0df0f5541621527eb389b462e1b0f95d42a","unresolved":false,"context_lines":[{"line_number":309,"context_line":"        self.assertTrue(response[\u0027async\u0027])"},{"line_number":310,"context_line":""},{"line_number":311,"context_line":"        # Assert lock was upgraded to an exclusive one"},{"line_number":312,"context_line":"        self.assertEqual(1, mock_upgrade.call_count)"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        node.refresh()"},{"line_number":315,"context_line":"        self.assertIsNone(node.last_error)"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_b9237a57","line":312,"updated":"2016-05-16 08:28:32.000000000","message":"This is not common to use like this, what about use mock_upgrade.assert_called_once_with()\nOr self.assertTrue(mock_upgrade.called)","commit_id":"ea06393fe853d6b54ab1e2f6d5c1f39e8c61907c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"e1b2806679ecf0f487bf7751ce75688ceaba671e","unresolved":false,"context_lines":[{"line_number":309,"context_line":"        self.assertTrue(response[\u0027async\u0027])"},{"line_number":310,"context_line":""},{"line_number":311,"context_line":"        # Assert lock was upgraded to an exclusive one"},{"line_number":312,"context_line":"        self.assertEqual(1, mock_upgrade.call_count)"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        node.refresh()"},{"line_number":315,"context_line":"        self.assertIsNone(node.last_error)"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_6ff49f5a","line":312,"in_reply_to":"dab17558_b9237a57","updated":"2016-05-16 09:25:24.000000000","message":"+1","commit_id":"ea06393fe853d6b54ab1e2f6d5c1f39e8c61907c"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"ff55e0df0f5541621527eb389b462e1b0f95d42a","unresolved":false,"context_lines":[{"line_number":335,"context_line":"        self.assertFalse(response[\u0027async\u0027])"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"        # Assert lock was upgraded to an exclusive one"},{"line_number":338,"context_line":"        self.assertEqual(1, mock_upgrade.call_count)"},{"line_number":339,"context_line":""},{"line_number":340,"context_line":"        node.refresh()"},{"line_number":341,"context_line":"        self.assertIsNone(node.last_error)"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_99df9639","line":338,"updated":"2016-05-16 08:28:32.000000000","message":"ditto","commit_id":"ea06393fe853d6b54ab1e2f6d5c1f39e8c61907c"}]}