)]}'
{"ironic/conductor/task_manager.py":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"7290cde0a46d784545d0108d5d0bfb2174217564","unresolved":false,"context_lines":[{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def __init__(self, context, node_id, shared\u003dFalse,"},{"line_number":173,"context_line":"                 purpose\u003d\u0027unspecified action\u0027, retry\u003dTrue, patient\u003dFalse,"},{"line_number":174,"context_line":"                 load_driver\u003dTrue, nodes_only\u003dFalse):"},{"line_number":175,"context_line":"        \"\"\"Create a new TaskManager."},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"        Acquire a lock on a node. The lock can be either shared or"}],"source_content_type":"text/x-python","patch_set":1,"id":"cd68282e_8de7ffcd","line":174,"updated":"2021-02-24 12:00:25.000000000","message":"An alternative proposal (which I like more) is to lazy-load everything except for the node.","commit_id":"f98569fa38f7afac338ca885a28acfe3e5aa2848"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"c8f7e506fbb6eb713818bc3cbe2cc9e2e29d6a72","unresolved":true,"context_lines":[{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def __init__(self, context, node_id, shared\u003dFalse,"},{"line_number":173,"context_line":"                 purpose\u003d\u0027unspecified action\u0027, retry\u003dTrue, patient\u003dFalse,"},{"line_number":174,"context_line":"                 load_driver\u003dTrue, nodes_only\u003dFalse):"},{"line_number":175,"context_line":"        \"\"\"Create a new TaskManager."},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"        Acquire a lock on a node. The lock can be either shared or"}],"source_content_type":"text/x-python","patch_set":1,"id":"67bb5562_b1d6cbad","line":174,"updated":"2021-02-22 17:55:36.000000000","message":"i\u0027m not fond of this -- worried that it will be set to True, then someone later adds code to access self.ports. If we go with it, maybe \u0027node_only\u0027 (cuz this doesn\u0027t get more than one node).","commit_id":"f98569fa38f7afac338ca885a28acfe3e5aa2848"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"c8f7e506fbb6eb713818bc3cbe2cc9e2e29d6a72","unresolved":true,"context_lines":[{"line_number":243,"context_line":"                    objects.VolumeConnector.list_by_node_id(context,"},{"line_number":244,"context_line":"                                                            self.node.id)"},{"line_number":245,"context_line":"                self.volume_targets \u003d objects.VolumeTarget.list_by_node_id("},{"line_number":246,"context_line":"                    context, self.node.id)"},{"line_number":247,"context_line":""},{"line_number":248,"context_line":"            if load_driver:"},{"line_number":249,"context_line":"                self.driver \u003d driver_factory.build_driver_for_task(self)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9994dd64_1d29015c","line":246,"updated":"2021-02-22 17:55:36.000000000","message":"what are the values of self.ports if nodes_only is True? Wondering if we can hide this from the conductor with some sort of \u0027lazy loading...\u0027. ie, if self.ports is used, then we set it.\n\nSince we need to lock the node, I suppose it makes sense to set self._node.","commit_id":"f98569fa38f7afac338ca885a28acfe3e5aa2848"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"c8f7e506fbb6eb713818bc3cbe2cc9e2e29d6a72","unresolved":true,"context_lines":[{"line_number":256,"context_line":""},{"line_number":257,"context_line":"    @property"},{"line_number":258,"context_line":"    def node(self):"},{"line_number":259,"context_line":"        return self._node"},{"line_number":260,"context_line":""},{"line_number":261,"context_line":"    @node.setter"},{"line_number":262,"context_line":"    def node(self, node):"}],"source_content_type":"text/x-python","patch_set":1,"id":"a1fec5ca_af36892a","line":259,"updated":"2021-02-22 17:55:36.000000000","message":"eg, what about changing ports, portgroups etc to @property and fetching the info from db. eg \u0027if not self._ports (or not hasattr(_ports)): self._ports \u003d objects.Port. etc. return self._ports)","commit_id":"f98569fa38f7afac338ca885a28acfe3e5aa2848"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"5a3d2a4b0cb5c0d5ca998c47562d6269deaed4bc","unresolved":true,"context_lines":[{"line_number":266,"context_line":"        except Exception:"},{"line_number":267,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":268,"context_line":"                self.release_resources()"},{"line_number":269,"context_line":"        return self._ports"},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    @ports.setter"},{"line_number":272,"context_line":"    def ports(self, ports):"}],"source_content_type":"text/x-python","patch_set":4,"id":"77359fc6_d66b5ab0","line":269,"updated":"2021-03-01 16:16:13.000000000","message":"This looks good. My only question is what happens when this barfs later (as opposed to failing when the TaskManager object is being created). If it happens in real life, I guess we\u0027ll find out... (e.g. if conductor was in the middle of modifying the node and ports, if the node \u0027state\u0027 will be out of sync)","commit_id":"017f751465da52dae975d1966f908cc0222e639e"},{"author":{"_account_id":11292,"name":"Arne Wiebalck","email":"Arne.Wiebalck@cern.ch","username":"wiebalck"},"change_message_id":"01d48c24c8faf781de3db3b92cd76dd47db9a486","unresolved":true,"context_lines":[{"line_number":266,"context_line":"        except Exception:"},{"line_number":267,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":268,"context_line":"                self.release_resources()"},{"line_number":269,"context_line":"        return self._ports"},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    @ports.setter"},{"line_number":272,"context_line":"    def ports(self, ports):"}],"source_content_type":"text/x-python","patch_set":4,"id":"bff23b16_3d85ec17","line":269,"in_reply_to":"77359fc6_d66b5ab0","updated":"2021-03-02 08:12:48.000000000","message":"Yeah, I thought about this as well ... but I guess we need to still fail none the less.","commit_id":"017f751465da52dae975d1966f908cc0222e639e"}],"ironic/tests/unit/conductor/test_manager.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"0ecda6585b49506c8362499a2d376775dad8dedd","unresolved":true,"context_lines":[{"line_number":3405,"context_line":"        self.service.set_console_mode(self.context, node.uuid, False)"},{"line_number":3406,"context_line":"        self._stop_service()"},{"line_number":3407,"context_line":"        self.assertFalse(mock_sc.called)"},{"line_number":3408,"context_line":"        self.assertFalse(mock_notify.called)"},{"line_number":3409,"context_line":""},{"line_number":3410,"context_line":"    @mock.patch.object(fake.FakeConsole, \u0027get_console\u0027, autospec\u003dTrue)"},{"line_number":3411,"context_line":"    def test_get_console(self, mock_gc):"}],"source_content_type":"text/x-python","patch_set":4,"id":"31b674b7_c35804ef","line":3408,"updated":"2021-03-01 16:54:14.000000000","message":"how consistently did the above test_enable_console_already_enabled() fail for you without the above change? Because this test is basically the same as the above one.\n\nAlso, what was failing? Both mock_sc \u0026 mock_notify were being called?","commit_id":"017f751465da52dae975d1966f908cc0222e639e"},{"author":{"_account_id":11292,"name":"Arne Wiebalck","email":"Arne.Wiebalck@cern.ch","username":"wiebalck"},"change_message_id":"01d48c24c8faf781de3db3b92cd76dd47db9a486","unresolved":true,"context_lines":[{"line_number":3405,"context_line":"        self.service.set_console_mode(self.context, node.uuid, False)"},{"line_number":3406,"context_line":"        self._stop_service()"},{"line_number":3407,"context_line":"        self.assertFalse(mock_sc.called)"},{"line_number":3408,"context_line":"        self.assertFalse(mock_notify.called)"},{"line_number":3409,"context_line":""},{"line_number":3410,"context_line":"    @mock.patch.object(fake.FakeConsole, \u0027get_console\u0027, autospec\u003dTrue)"},{"line_number":3411,"context_line":"    def test_get_console(self, mock_gc):"}],"source_content_type":"text/x-python","patch_set":4,"id":"68e37633_3b696bf0","line":3408,"in_reply_to":"31b674b7_c35804ef","updated":"2021-03-02 08:12:48.000000000","message":"Thanks for having a look! With the old order (stop_service before the asserts) the test failed 100% for me. With the new order it succeeds 100%.\n\n\u003e Both mock_sc \u0026 mock_notify were being called?\n\nYes.\n\n\u003e Because this test is basically the same as the above one.\n\nI totally agree :) As mentioned, there seems to be another task manager instantiated when stop_service() is called. This task manager has the console not enabled, but it gets enabled when init_host and start_consoles is invoked. And these are the calls to mock_sc and mock_notify we see.","commit_id":"017f751465da52dae975d1966f908cc0222e639e"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e03d5b13483e672f9cf2b29085117950024ac9d7","unresolved":true,"context_lines":[{"line_number":3405,"context_line":"        self.service.set_console_mode(self.context, node.uuid, False)"},{"line_number":3406,"context_line":"        self._stop_service()"},{"line_number":3407,"context_line":"        self.assertFalse(mock_sc.called)"},{"line_number":3408,"context_line":"        self.assertFalse(mock_notify.called)"},{"line_number":3409,"context_line":""},{"line_number":3410,"context_line":"    @mock.patch.object(fake.FakeConsole, \u0027get_console\u0027, autospec\u003dTrue)"},{"line_number":3411,"context_line":"    def test_get_console(self, mock_gc):"}],"source_content_type":"text/x-python","patch_set":4,"id":"e88897c8_ae20a659","line":3408,"in_reply_to":"68e37633_3b696bf0","updated":"2021-03-02 20:31:03.000000000","message":"Ah. ok. So in test_enable_console_already_enabled(), the node has console_enabled\u003dTrue. My guess is that when _stop_service() happens, this [1] is invoked, which causes the console to be enabled -- in a thread. With your change, this [2] is faster and hence the mocked console calls are made before the test does the .assertFalse() stuff.\n\nI\u0027m not quite sure what causes [1] to be invoked but 😊\n\nAnd it doesn\u0027t fail in this test, cuz the console is disabled for this node, so no calls are made to enable it.\n\n[1] https://opendev.org/openstack/ironic/src/commit/5857fa802d42a49aa4cd2a0b2b0618c0845f6c65/ironic/conductor/base_manager.py#L215-L216\n[2] https://opendev.org/openstack/ironic/src/commit/5857fa802d42a49aa4cd2a0b2b0618c0845f6c65/ironic/conductor/base_manager.py#L568","commit_id":"017f751465da52dae975d1966f908cc0222e639e"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e03d5b13483e672f9cf2b29085117950024ac9d7","unresolved":true,"context_lines":[{"line_number":3403,"context_line":"                                          console_enabled\u003dFalse)"},{"line_number":3404,"context_line":"        self._start_service()"},{"line_number":3405,"context_line":"        self.service.set_console_mode(self.context, node.uuid, False)"},{"line_number":3406,"context_line":"        self._stop_service()"},{"line_number":3407,"context_line":"        self.assertFalse(mock_sc.called)"},{"line_number":3408,"context_line":"        self.assertFalse(mock_notify.called)"},{"line_number":3409,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"35e78e51_9401c046","line":3406,"updated":"2021-03-02 20:31:03.000000000","message":"To be consistent, I think we should move this to the end too.","commit_id":"82cab603bbb21d81769cdc4f50d4fa445d026e5c"}],"releasenotes/notes/taskmanager-lazy-load-32a14526c647c2f0.yaml":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"5a3d2a4b0cb5c0d5ca998c47562d6269deaed4bc","unresolved":true,"context_lines":[{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Introduces lazy-loading of ports, portgroups, volume connections"},{"line_number":5,"context_line":"    and volume targets in task manager. As the power sync periodic"},{"line_number":6,"context_line":"    job is creating a task manager object for every node on each power"},{"line_number":7,"context_line":"    sync cycle, but does not require the aforementioned data, this"},{"line_number":8,"context_line":"    change should reduce the number of database interactions by around"}],"source_content_type":"text/x-yaml","patch_set":4,"id":"66b1ef3a_8be09ad1","line":5,"updated":"2021-03-01 16:16:13.000000000","message":"my guess is that it isn\u0027t only the power sync task that will be sped up. Most periodic tasks probably will (the ones that only need info from the node object itself).\n\nMaybe we can reword this to be a bit more general. (Might even improve non-periodic tasks but...)\n\nFor periodic tasks (e.g. power sync) that create a task manager object but doesn\u0027t require the aforementioned data, this change should reduce the number of database interactions by around two thirds, speeding up execution.","commit_id":"017f751465da52dae975d1966f908cc0222e639e"},{"author":{"_account_id":11292,"name":"Arne Wiebalck","email":"Arne.Wiebalck@cern.ch","username":"wiebalck"},"change_message_id":"01d48c24c8faf781de3db3b92cd76dd47db9a486","unresolved":false,"context_lines":[{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Introduces lazy-loading of ports, portgroups, volume connections"},{"line_number":5,"context_line":"    and volume targets in task manager. As the power sync periodic"},{"line_number":6,"context_line":"    job is creating a task manager object for every node on each power"},{"line_number":7,"context_line":"    sync cycle, but does not require the aforementioned data, this"},{"line_number":8,"context_line":"    change should reduce the number of database interactions by around"}],"source_content_type":"text/x-yaml","patch_set":4,"id":"30bb9f77_4b3e5d26","line":5,"in_reply_to":"66b1ef3a_8be09ad1","updated":"2021-03-02 08:12:48.000000000","message":"OK, done.","commit_id":"017f751465da52dae975d1966f908cc0222e639e"}]}