)]}'
{"sushy/auth.py":[{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"4acda3a90288da0cb75448246b47462ed7e32b3d","unresolved":false,"context_lines":[{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"},{"line_number":54,"context_line":"        gets removed by this call."},{"line_number":55,"context_line":"        \"\"\""},{"line_number":56,"context_line":"        self._root_resource \u003d self._connector \u003d None"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    def authenticate(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_2e34e646","line":55,"updated":"2018-10-22 09:20:32.000000000","message":"Just thinking if its also sound to do this?\n\n    if self._connector:\n      self._connector.set_auth(None)","commit_id":"6df65e8e91726d76871cb16ecb754a82d37676a4"},{"author":{"_account_id":26340,"name":"Ilya Etingof","email":"etingof@gmail.com","username":"etingof"},"change_message_id":"693e89f5b8a768eb9c86b87c35769a25b5abeb81","unresolved":false,"context_lines":[{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"},{"line_number":54,"context_line":"        gets removed by this call."},{"line_number":55,"context_line":"        \"\"\""},{"line_number":56,"context_line":"        self._root_resource \u003d self._connector \u003d None"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    def authenticate(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_890e548d","line":55,"in_reply_to":"3f79a3b5_2e34e646","updated":"2018-10-22 10:25:02.000000000","message":"Seems to make sense, but probably with conditions...","commit_id":"6df65e8e91726d76871cb16ecb754a82d37676a4"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"3e2b4f5a367b542e2e2bb2a14356d70964b76ee1","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        self._connector \u003d connector"},{"line_number":48,"context_line":"        self._connector.set_auth(self)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def unset_context(self, untie_connection\u003dFalse):"},{"line_number":51,"context_line":"        \"\"\"Reset the context of the authentication object."},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_0c7f9969","line":50,"updated":"2018-10-23 08:26:40.000000000","message":"Why set it to False by default (or even provide this parameter) if in the only place you use it you set it to True? I\u0027d just do it by default.","commit_id":"0680f569034b19d4f1953a2b0a33c63e19c049a0"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"677dd55366e86db8e48206adddbbbcceddf5372d","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        self._connector \u003d connector"},{"line_number":48,"context_line":"        self._connector.set_auth(self)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def unset_context(self, untie_connection\u003dFalse):"},{"line_number":51,"context_line":"        \"\"\"Reset the context of the authentication object."},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_0cceb98e","line":50,"in_reply_to":"3f79a3b5_0c5b19f9","updated":"2018-10-23 08:36:18.000000000","message":"Mm, not much, sorry. What is the use case of connector still linking to an authenticator that is no longer linked to (this) connector?","commit_id":"0680f569034b19d4f1953a2b0a33c63e19c049a0"},{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"96c85f45a1e67407ac332c254795ec0fa4006567","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        self._connector \u003d connector"},{"line_number":48,"context_line":"        self._connector.set_auth(self)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def unset_context(self, untie_connection\u003dFalse):"},{"line_number":51,"context_line":"        \"\"\"Reset the context of the authentication object."},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_ad1de361","line":50,"in_reply_to":"3f79a3b5_0c5d597b","updated":"2018-10-26 18:27:17.000000000","message":"IIRC Auth obj is supposed to be just a wrapper of BMC credentials (and/or session token, id). In order to cache the session token (until it gets expired) for a particular BMC we can take the approach of maintaining the cache with key as ``node-id+redfish_address`` (or some sort of combination of that sort to uniquely identify the node\u0027s BMC) and value as Auth objects. As from the ironic code base [1] what I can notice is we are always creating a new auth object (of type SessionOrBasicAuth) everytime we create the Sushy object. Instead we can use the ``auth`` parameter of Sushy c\u0027tor [2] and can keep passing the cache auth object everytime. And whenever that auth object expires we put in place a mechanism to recreate a new session token pertaining to the node key (of cache) and that gets stored under the same auth object.\n\n    {\n      \"node-id1+redfish_address1\": auth_obj1,  # gets a new session token when the existing one expires,\n                                               # but \u0027auth_obj1\u0027 is still the same.\n      \"node-id2+redfish_address2\": auth_obj2,\n      ...\n    }\n\nSo, inherently we keep creating new Sushy objects and new Connection objects (even though internally urllib will take care of using the same session connection from its connection pool for as long as it can), but we keep reusing the same Auth object (session token) for multiple calls to the same BMC (until that expires) and only attempts to recreate a new session token when the existing one expires (again this recreation can be handled by the auth object as it contains the BMC credentials and hence already has the ability to do so).\n\nAnd for every new Sushy object creation the auth object is set the context of its connection and Redfish root object here [3]. So, I think we can gladly keep creating Sushy and Connection objects.\n\nDo let me know what you think about this approach.\n\n[1] https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/redfish/utils.py#L173-L175\n[2] https://github.com/openstack/sushy/blob/master/sushy/main.py#L97\n[3] https://github.com/openstack/sushy/blob/master/sushy/main.py#L114","commit_id":"0680f569034b19d4f1953a2b0a33c63e19c049a0"},{"author":{"_account_id":26340,"name":"Ilya Etingof","email":"etingof@gmail.com","username":"etingof"},"change_message_id":"ae446a1df6df4e5eed39d192919803773938eebb","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        self._connector \u003d connector"},{"line_number":48,"context_line":"        self._connector.set_auth(self)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def unset_context(self, untie_connection\u003dFalse):"},{"line_number":51,"context_line":"        \"\"\"Reset the context of the authentication object."},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_0c5b19f9","line":50,"in_reply_to":"3f79a3b5_0c7f9969","updated":"2018-10-23 08:31:56.000000000","message":"The idea is to have this method playing on the safe side whenever invoked by the user. With default set to `True`, calling this method would cause the side-effect of  modifying the underlying connection...\n\nDoes it make sense?","commit_id":"0680f569034b19d4f1953a2b0a33c63e19c049a0"},{"author":{"_account_id":26340,"name":"Ilya Etingof","email":"etingof@gmail.com","username":"etingof"},"change_message_id":"d11ad9ac3bb2f8000b6228c2b12bc1774ccc3840","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        self._connector \u003d connector"},{"line_number":48,"context_line":"        self._connector.set_auth(self)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def unset_context(self, untie_connection\u003dFalse):"},{"line_number":51,"context_line":"        \"\"\"Reset the context of the authentication object."},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_6ce96da9","line":50,"in_reply_to":"3f79a3b5_0cceb98e","updated":"2018-10-23 08:47:58.000000000","message":"The use-case is ironic reusing redfish sessions...\n\nWe cache authenticator objects in ironic while connections being pooled by urilib under the hood. We want connections to use authenticators for as long as the connection is alive (which is controlled solely by urlib) therefore we might want to call `unset_context(untie_connection\u003dFalse)`.\n\nBut if the connection ceases to exist (by abrupt urllib\u0027s decision), we do not want to reference the dead connection from the authenticator object to let the connection be GC\u0027ed. That\u0027s why we want to call `unset_context` right after we used the connection.\n\nThis gets quite messy indeed. :-(","commit_id":"0680f569034b19d4f1953a2b0a33c63e19c049a0"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"4a3bf58fedd74bf9982886ebf9fbbffe1bf320f6","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        self._connector \u003d connector"},{"line_number":48,"context_line":"        self._connector.set_auth(self)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def unset_context(self, untie_connection\u003dFalse):"},{"line_number":51,"context_line":"        \"\"\"Reset the context of the authentication object."},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"        Most importantly, internally held reference to sushy `Connector` object"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_0c5d597b","line":50,"in_reply_to":"3f79a3b5_6ce96da9","updated":"2018-10-23 08:54:45.000000000","message":"I think I\u0027m starting to see the problem. We assume one to one relationship between Sushy \u003c-\u003e Connector \u003c-\u003e Authenticator. If we start caching only authenticators, we essentially break this. It may happen that two different Sushy objects will call set_context on the same authenticator in parallel with potentially unpredictable results.\n\nTL;DR we need to either cache the whole Sushy objects or only the session token only :(","commit_id":"0680f569034b19d4f1953a2b0a33c63e19c049a0"}],"sushy/tests/unit/test_auth.py":[{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"4acda3a90288da0cb75448246b47462ed7e32b3d","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    def test_can_refresh_session(self):"},{"line_number":72,"context_line":"        self.assertFalse(self.base_auth.can_refresh_session())"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    @mock.patch.object(auth.BasicAuth, \u0027close\u0027, autospec\u003dTrue)"},{"line_number":75,"context_line":"    def test_context_manager(self, auth_close):"},{"line_number":76,"context_line":"        with auth.BasicAuth(self.username, self.password) as base_auth:"},{"line_number":77,"context_line":"            self.assertEqual(self.username, base_auth._username)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_6ece5e2d","line":74,"range":{"start_line":74,"start_character":4,"end_line":74,"end_character":62},"updated":"2018-10-22 09:20:32.000000000","message":"if you happen to raise another patch-set, can you also remove this mocking and test it against the actual conditions? I think there has to be a call to .set_context() within ``with`` block, otherwise it will fail!","commit_id":"6df65e8e91726d76871cb16ecb754a82d37676a4"},{"author":{"_account_id":26340,"name":"Ilya Etingof","email":"etingof@gmail.com","username":"etingof"},"change_message_id":"693e89f5b8a768eb9c86b87c35769a25b5abeb81","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    def test_can_refresh_session(self):"},{"line_number":72,"context_line":"        self.assertFalse(self.base_auth.can_refresh_session())"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    @mock.patch.object(auth.BasicAuth, \u0027close\u0027, autospec\u003dTrue)"},{"line_number":75,"context_line":"    def test_context_manager(self, auth_close):"},{"line_number":76,"context_line":"        with auth.BasicAuth(self.username, self.password) as base_auth:"},{"line_number":77,"context_line":"            self.assertEqual(self.username, base_auth._username)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_e96b4819","line":74,"range":{"start_line":74,"start_character":4,"end_line":74,"end_character":62},"in_reply_to":"3f79a3b5_6ece5e2d","updated":"2018-10-22 10:25:02.000000000","message":"Right, that should follow up this patch...","commit_id":"6df65e8e91726d76871cb16ecb754a82d37676a4"},{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"4acda3a90288da0cb75448246b47462ed7e32b3d","unresolved":false,"context_lines":[{"line_number":208,"context_line":"        self.assertIsNone(self.sess_auth.get_session_resource_id())"},{"line_number":209,"context_line":"        self.assertIsNone(self.sess_auth.get_session_key())"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    @mock.patch.object(auth.SessionAuth, \u0027close\u0027, autospec\u003dTrue)"},{"line_number":212,"context_line":"    def test_context_manager(self, auth_close):"},{"line_number":213,"context_line":"        with auth.SessionAuth(self.username, self.password) as session_auth:"},{"line_number":214,"context_line":"            self.assertEqual(self.username, session_auth._username)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_2ed066c6","line":211,"range":{"start_line":211,"start_character":4,"end_line":211,"end_character":64},"updated":"2018-10-22 09:20:32.000000000","message":"ditto","commit_id":"6df65e8e91726d76871cb16ecb754a82d37676a4"},{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"4acda3a90288da0cb75448246b47462ed7e32b3d","unresolved":false,"context_lines":[{"line_number":354,"context_line":"        self.assertIsNone(self.sess_basic_auth.get_session_resource_id())"},{"line_number":355,"context_line":"        self.assertIsNone(self.sess_basic_auth.get_session_key())"},{"line_number":356,"context_line":""},{"line_number":357,"context_line":"    @mock.patch.object(auth.SessionOrBasicAuth, \u0027close\u0027, autospec\u003dTrue)"},{"line_number":358,"context_line":"    def test_context_manager(self, auth_close):"},{"line_number":359,"context_line":"        with auth.SessionOrBasicAuth("},{"line_number":360,"context_line":"                self.username, self.password) as session_or_base_auth:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_6ee73ead","line":357,"range":{"start_line":357,"start_character":4,"end_line":357,"end_character":71},"updated":"2018-10-22 09:20:32.000000000","message":"ditto","commit_id":"6df65e8e91726d76871cb16ecb754a82d37676a4"}]}