)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"f4b57e9ca6f4010371a0416e10a0058ebc9a27bd","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Refactor VIFIDPortMixin: vif_attach and vif_detach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This change adds the vif_attach and vif_detach methods to the"},{"line_number":10,"context_line":"non-neutron VIFPortIDMixin mixin class."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"vif_attach uses the existing port selection algorithm, ignoring physical"},{"line_number":13,"context_line":"networks."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ff346bd7_1d317b0c","line":10,"updated":"2017-07-24 10:23:25.000000000","message":"Maybe I\u0027m missing something, but are these used anywhere? I don\u0027t see them called. Or is it for the future?","commit_id":"66f837dcb40e7f4d8d37a6fd6ce7ab287c7607e5"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"c61040ce1af8c42acba7e79d1745115da3efb040","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Refactor VIFIDPortMixin: vif_attach and vif_detach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This change adds the vif_attach and vif_detach methods to the"},{"line_number":10,"context_line":"non-neutron VIFPortIDMixin mixin class."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"vif_attach uses the existing port selection algorithm, ignoring physical"},{"line_number":13,"context_line":"networks."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ff346bd7_cf0babd3","line":10,"in_reply_to":"ff346bd7_1d317b0c","updated":"2017-07-27 17:15:01.000000000","message":"I\u0027ll improve the commit message for this series to add some context.","commit_id":"66f837dcb40e7f4d8d37a6fd6ce7ab287c7607e5"}],"ironic/drivers/modules/network/common.py":[{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"568abe8c21b64ec11d4fcd9dd208ab5cb68ffda5","unresolved":false,"context_lines":[{"line_number":404,"context_line":"        :raises: NetworkError, VifAlreadyAttached, NoFreePhysicalPorts"},{"line_number":405,"context_line":"        \"\"\""},{"line_number":406,"context_line":"        vif_id \u003d vif_info[\u0027id\u0027]"},{"line_number":407,"context_line":"        port_like_obj \u003d get_free_port_like_object(task, vif_id, set())"},{"line_number":408,"context_line":"        self._save_vif_to_port_like_obj(port_like_obj, vif_id)"},{"line_number":409,"context_line":""},{"line_number":410,"context_line":"    def vif_detach(self, task, vif_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"df3967d1_b325b6ad","line":407,"range":{"start_line":407,"start_character":64,"end_line":407,"end_character":69},"updated":"2017-08-02 10:02:32.000000000","message":"should we also introduce something like get_physnets?","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"214187dffa20d8654dbc43c0be4465967abaca85","unresolved":false,"context_lines":[{"line_number":404,"context_line":"        :raises: NetworkError, VifAlreadyAttached, NoFreePhysicalPorts"},{"line_number":405,"context_line":"        \"\"\""},{"line_number":406,"context_line":"        vif_id \u003d vif_info[\u0027id\u0027]"},{"line_number":407,"context_line":"        port_like_obj \u003d get_free_port_like_object(task, vif_id, set())"},{"line_number":408,"context_line":"        self._save_vif_to_port_like_obj(port_like_obj, vif_id)"},{"line_number":409,"context_line":""},{"line_number":410,"context_line":"    def vif_detach(self, task, vif_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f436f4f_c840ca0a","line":407,"range":{"start_line":407,"start_character":64,"end_line":407,"end_character":69},"in_reply_to":"df3967d1_b325b6ad","updated":"2017-08-09 09:25:24.000000000","message":"See final comment. I think in the framework model you propose this fits (actually it\u0027s required). In this model it seems simpler to keep it inline.","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"568abe8c21b64ec11d4fcd9dd208ab5cb68ffda5","unresolved":false,"context_lines":[{"line_number":604,"context_line":""},{"line_number":605,"context_line":"        port_like_obj \u003d get_free_port_like_object(task, vif_id, physnets)"},{"line_number":606,"context_line":""},{"line_number":607,"context_line":"        # Address is optional for portgroups"},{"line_number":608,"context_line":"        if port_like_obj.address:"},{"line_number":609,"context_line":"            # Check if the requested vif_id is a neutron port. If it is"},{"line_number":610,"context_line":"            # then attempt to update the port\u0027s MAC address."},{"line_number":611,"context_line":"            try:"},{"line_number":612,"context_line":"                client.show_port(vif_id)"},{"line_number":613,"context_line":"            except neutron_exceptions.NeutronClientException:"},{"line_number":614,"context_line":"                # TODO(mgoddard): Remove this except clause and handle errors"},{"line_number":615,"context_line":"                # properly. We can do this once a strategy has been determined"},{"line_number":616,"context_line":"                # for handling the tempest VIF tests in an environment that"},{"line_number":617,"context_line":"                # may not support neutron."},{"line_number":618,"context_line":"                # NOTE(sambetts): If a client error occurs this is because"},{"line_number":619,"context_line":"                # either neutron doesn\u0027t exist because we\u0027re running in"},{"line_number":620,"context_line":"                # standalone environment or we can\u0027t find a matching neutron"},{"line_number":621,"context_line":"                # port which means a user might be requesting a non-neutron"},{"line_number":622,"context_line":"                # port. So skip trying to update the neutron port MAC address"},{"line_number":623,"context_line":"                # in these cases."},{"line_number":624,"context_line":"                pass"},{"line_number":625,"context_line":"            else:"},{"line_number":626,"context_line":"                try:"},{"line_number":627,"context_line":"                    neutron.update_port_address(vif_id, port_like_obj.address)"},{"line_number":628,"context_line":"                except exception.FailedToUpdateMacOnPort:"},{"line_number":629,"context_line":"                    raise exception.NetworkError(_("},{"line_number":630,"context_line":"                        \"Unable to attach VIF %(vif)s because Ironic can not \""},{"line_number":631,"context_line":"                        \"update Neutron port %(port)s MAC address to match \""},{"line_number":632,"context_line":"                        \"physical MAC address %(mac)s\") % {"},{"line_number":633,"context_line":"                            \u0027vif\u0027: vif_id, \u0027port\u0027: vif_id,"},{"line_number":634,"context_line":"                            \u0027mac\u0027: port_like_obj.address})"},{"line_number":635,"context_line":""},{"line_number":636,"context_line":"        self._save_vif_to_port_like_obj(port_like_obj, vif_id)"},{"line_number":637,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"df3967d1_d3ccbabd","line":634,"range":{"start_line":607,"start_character":8,"end_line":634,"end_character":58},"updated":"2017-08-02 10:02:32.000000000","message":"I also wonder how to fit this bit and at the same time avoid complete override of base vif_attach method.","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"214187dffa20d8654dbc43c0be4465967abaca85","unresolved":false,"context_lines":[{"line_number":604,"context_line":""},{"line_number":605,"context_line":"        port_like_obj \u003d get_free_port_like_object(task, vif_id, physnets)"},{"line_number":606,"context_line":""},{"line_number":607,"context_line":"        # Address is optional for portgroups"},{"line_number":608,"context_line":"        if port_like_obj.address:"},{"line_number":609,"context_line":"            # Check if the requested vif_id is a neutron port. If it is"},{"line_number":610,"context_line":"            # then attempt to update the port\u0027s MAC address."},{"line_number":611,"context_line":"            try:"},{"line_number":612,"context_line":"                client.show_port(vif_id)"},{"line_number":613,"context_line":"            except neutron_exceptions.NeutronClientException:"},{"line_number":614,"context_line":"                # TODO(mgoddard): Remove this except clause and handle errors"},{"line_number":615,"context_line":"                # properly. We can do this once a strategy has been determined"},{"line_number":616,"context_line":"                # for handling the tempest VIF tests in an environment that"},{"line_number":617,"context_line":"                # may not support neutron."},{"line_number":618,"context_line":"                # NOTE(sambetts): If a client error occurs this is because"},{"line_number":619,"context_line":"                # either neutron doesn\u0027t exist because we\u0027re running in"},{"line_number":620,"context_line":"                # standalone environment or we can\u0027t find a matching neutron"},{"line_number":621,"context_line":"                # port which means a user might be requesting a non-neutron"},{"line_number":622,"context_line":"                # port. So skip trying to update the neutron port MAC address"},{"line_number":623,"context_line":"                # in these cases."},{"line_number":624,"context_line":"                pass"},{"line_number":625,"context_line":"            else:"},{"line_number":626,"context_line":"                try:"},{"line_number":627,"context_line":"                    neutron.update_port_address(vif_id, port_like_obj.address)"},{"line_number":628,"context_line":"                except exception.FailedToUpdateMacOnPort:"},{"line_number":629,"context_line":"                    raise exception.NetworkError(_("},{"line_number":630,"context_line":"                        \"Unable to attach VIF %(vif)s because Ironic can not \""},{"line_number":631,"context_line":"                        \"update Neutron port %(port)s MAC address to match \""},{"line_number":632,"context_line":"                        \"physical MAC address %(mac)s\") % {"},{"line_number":633,"context_line":"                            \u0027vif\u0027: vif_id, \u0027port\u0027: vif_id,"},{"line_number":634,"context_line":"                            \u0027mac\u0027: port_like_obj.address})"},{"line_number":635,"context_line":""},{"line_number":636,"context_line":"        self._save_vif_to_port_like_obj(port_like_obj, vif_id)"},{"line_number":637,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f436f4f_08fff201","line":634,"range":{"start_line":607,"start_character":8,"end_line":634,"end_character":58},"in_reply_to":"df3967d1_d3ccbabd","updated":"2017-08-09 09:25:24.000000000","message":"See next comment :)","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cd310f3e115c5cfa51e4e25acc82ea3e5ae7acd4","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        # attached, and fail if not."},{"line_number":652,"context_line":"        port_like_obj \u003d self._get_port_like_obj_by_vif_id(task, vif_id)"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        self._clear_vif_from_port_like_obj(port_like_obj)"},{"line_number":655,"context_line":""},{"line_number":656,"context_line":"        # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance."},{"line_number":657,"context_line":"        if task.node.provision_state \u003d\u003d states.ACTIVE:"}],"source_content_type":"text/x-python","patch_set":2,"id":"df3967d1_dcdd0c15","line":654,"updated":"2017-08-01 12:44:47.000000000","message":"this should be replaced by\n\n super(...).vif_detach(task, vif_id)","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"214187dffa20d8654dbc43c0be4465967abaca85","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        # attached, and fail if not."},{"line_number":652,"context_line":"        port_like_obj \u003d self._get_port_like_obj_by_vif_id(task, vif_id)"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        self._clear_vif_from_port_like_obj(port_like_obj)"},{"line_number":655,"context_line":""},{"line_number":656,"context_line":"        # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance."},{"line_number":657,"context_line":"        if task.node.provision_state \u003d\u003d states.ACTIVE:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f436f4f_2d07681f","line":654,"in_reply_to":"df3967d1_13ea420e","updated":"2017-08-09 09:25:24.000000000","message":"I think that is roughly the approach I took in my original PoC. Rather than redefining the interface methods, I replaced neutron-specific behaviour with method call stubs that could be overridden, e.g. bind/unbind_vif().\n\nBased on feedback from sambetts, we ended up with this approach.\n\nI guess it\u0027s a question of toolkit (this) vs. framework (your suggestion).\n\nWhile I don\u0027t feel entirely wed to either solution, some thoughts:\n* The framework model implies a new interface (all those new bind/unbind_vif etc. methods). Interfaces should be well thought out and are hard to change.\n* The toolkit model is inherently more flexible, as you get top-down control and don\u0027t need to use the tools provided if you choose not to.\n* This solution exists already...\n\nPerhaps there is some logic in having a \u0027network provider\u0027 driver layer which provides low level operations, but that feels like a spec.","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"4eaecb75d94ac1d1f464555a80d512ce98ca04dd","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        # attached, and fail if not."},{"line_number":652,"context_line":"        port_like_obj \u003d self._get_port_like_obj_by_vif_id(task, vif_id)"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        self._clear_vif_from_port_like_obj(port_like_obj)"},{"line_number":655,"context_line":""},{"line_number":656,"context_line":"        # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance."},{"line_number":657,"context_line":"        if task.node.provision_state \u003d\u003d states.ACTIVE:"}],"source_content_type":"text/x-python","patch_set":2,"id":"df3967d1_dc950ce6","line":654,"in_reply_to":"df3967d1_9c0e648b","updated":"2017-08-01 12:56:39.000000000","message":"oh, right. then it can be left as it is","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"568abe8c21b64ec11d4fcd9dd208ab5cb68ffda5","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        # attached, and fail if not."},{"line_number":652,"context_line":"        port_like_obj \u003d self._get_port_like_obj_by_vif_id(task, vif_id)"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        self._clear_vif_from_port_like_obj(port_like_obj)"},{"line_number":655,"context_line":""},{"line_number":656,"context_line":"        # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance."},{"line_number":657,"context_line":"        if task.node.provision_state \u003d\u003d states.ACTIVE:"}],"source_content_type":"text/x-python","patch_set":2,"id":"df3967d1_13ea420e","line":654,"in_reply_to":"df3967d1_dc950ce6","updated":"2017-08-02 10:02:32.000000000","message":"I don\u0027t think it\u0027s good to do the complete override here. maybe introduce something like pre_clear_function passed as a parameter to this method, which is called prior to _clear_vif_from_port_like_obj? Or maybe even this can be introduced as a parameter to _clear_vif_from_port_like_obj itself.","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"9a09fa56eefac4041b3962d8af2039e8e21df0d7","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        # attached, and fail if not."},{"line_number":652,"context_line":"        port_like_obj \u003d self._get_port_like_obj_by_vif_id(task, vif_id)"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        self._clear_vif_from_port_like_obj(port_like_obj)"},{"line_number":655,"context_line":""},{"line_number":656,"context_line":"        # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance."},{"line_number":657,"context_line":"        if task.node.provision_state \u003d\u003d states.ACTIVE:"}],"source_content_type":"text/x-python","patch_set":2,"id":"df3967d1_9c0e648b","line":654,"in_reply_to":"df3967d1_dcdd0c15","updated":"2017-08-01 12:52:57.000000000","message":"Ah yes, it now looks like that would be better but I\u0027m about to push a patch which swaps the order to:\n\n        port_like_obj \u003d self._get_port_like_obj_by_vif_id(task, vif_id)\n\n        # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance.\n        if task.node.provision_state \u003d\u003d states.ACTIVE:\n            neutron.unbind_neutron_port(vif_id)\n\n        self._clear_vif_from_port_like_obj(port_like_obj)\n\nShall I leave it as it is to save the churn?","commit_id":"65d81d087814de1215266f098e774a32ceb7ce64"}]}