)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"031b4ff55a84c6c4ba1a7e57f0aafed6f23d533b","unresolved":false,"context_lines":[{"line_number":12,"context_line":"connect may happen in the future. This change handles when LDAP reports"},{"line_number":13,"context_line":"SERVER_DOWN and removes the connection from the pool before raising"},{"line_number":14,"context_line":"the exception."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I4fb49736e5f32b27e5904c340b434ad4173fb0b4"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"9f6a8fd7_cff62f64","line":15,"updated":"2018-04-23 15:35:30.000000000","message":"Was this discovered by some sort of LDAP functional testing? If so, this sounds like a bug in keystone that we should fix.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":21420,"name":"Gage Hugo","email":"gagehugo@gmail.com","username":"ghugo"},"change_message_id":"79d9bde20326da3a951c96979c472f858a197d6f","unresolved":false,"context_lines":[{"line_number":12,"context_line":"connect may happen in the future. This change handles when LDAP reports"},{"line_number":13,"context_line":"SERVER_DOWN and removes the connection from the pool before raising"},{"line_number":14,"context_line":"the exception."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I4fb49736e5f32b27e5904c340b434ad4173fb0b4"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"9f6a8fd7_0f317da3","line":15,"in_reply_to":"9f6a8fd7_cff62f64","updated":"2018-04-23 22:38:50.000000000","message":"Our cloud was seeing issues with this, opening a bug is probably the correct route for this.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"}],"keystone/identity/backends/ldap/common.py":[{"author":{"_account_id":20466,"name":"Tin Lam","email":"tin@lam.wtf","username":"tinlam"},"change_message_id":"36cfb9b699d75488fa65a9853412e38915b72b63","unresolved":false,"context_lines":[{"line_number":744,"context_line":""},{"line_number":745,"context_line":"    def _remove_conn_from_pool(self, conn, reason\u003dNone):"},{"line_number":746,"context_line":"        LOG.warning(\u0027Removing connection from ldap pool. Reason: %s\u0027, reason)"},{"line_number":747,"context_line":"        self.conn_pool._pool_remove(conn)"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"    def simple_bind_s(self, who\u003d\u0027\u0027, cred\u003d\u0027\u0027,"},{"line_number":750,"context_line":"                      serverctrls\u003dNone, clientctrls\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_463993a1","line":747,"range":{"start_line":747,"start_character":23,"end_line":747,"end_character":35},"updated":"2018-04-13 05:16:49.000000000","message":"where is this method _pool_remove() defined?  I cannot find this in the class definition at [0].\n\n[0] https://github.com/openstack/ldappool/blob/5454ce5922560d3acf7033de97ce22a8ce4399f6/ldappool/__init__.py#L141","commit_id":"6ebb9e69cfe89d28a03e8e3c294ac0948a367786"},{"author":{"_account_id":20466,"name":"Tin Lam","email":"tin@lam.wtf","username":"tinlam"},"change_message_id":"2b0a436119ab862cd7734b73aa8e6bbf906487f7","unresolved":false,"context_lines":[{"line_number":795,"context_line":"                                    timeout, sizelimit)"},{"line_number":796,"context_line":"        except Exception as ex:"},{"line_number":797,"context_line":"            if isinstance(ex, ldap.SERVER_DOWN):"},{"line_number":798,"context_line":"                self.remove_conn_from_pool(conn, \u0027SERVER_DOWN\u0027)"},{"line_number":799,"context_line":"            conn_ctxt.__exit__(*sys.exc_info())"},{"line_number":800,"context_line":"            raise"},{"line_number":801,"context_line":"        res \u003d MsgId((conn, msgid))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_cb784ec5","line":798,"range":{"start_line":798,"start_character":21,"end_line":798,"end_character":41},"updated":"2018-04-13 04:43:24.000000000","message":"_remove_conn_from_pool","commit_id":"6ebb9e69cfe89d28a03e8e3c294ac0948a367786"},{"author":{"_account_id":21420,"name":"Gage Hugo","email":"gagehugo@gmail.com","username":"ghugo"},"change_message_id":"b97726b81b59b91478906dfd5046c1cfa509e3ee","unresolved":false,"context_lines":[{"line_number":795,"context_line":"                                    timeout, sizelimit)"},{"line_number":796,"context_line":"        except Exception as ex:"},{"line_number":797,"context_line":"            if isinstance(ex, ldap.SERVER_DOWN):"},{"line_number":798,"context_line":"                self.remove_conn_from_pool(conn, \u0027SERVER_DOWN\u0027)"},{"line_number":799,"context_line":"            conn_ctxt.__exit__(*sys.exc_info())"},{"line_number":800,"context_line":"            raise"},{"line_number":801,"context_line":"        res \u003d MsgId((conn, msgid))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_a2f64d8e","line":798,"range":{"start_line":798,"start_character":21,"end_line":798,"end_character":41},"in_reply_to":"bf659307_cb784ec5","updated":"2018-04-13 15:25:43.000000000","message":"Done","commit_id":"6ebb9e69cfe89d28a03e8e3c294ac0948a367786"},{"author":{"_account_id":20466,"name":"Tin Lam","email":"tin@lam.wtf","username":"tinlam"},"change_message_id":"1fa7532bceecf46d1db975b23a0bd2dce8788786","unresolved":false,"context_lines":[{"line_number":744,"context_line":""},{"line_number":745,"context_line":"    def _remove_conn_from_pool(self, conn, reason\u003dNone):"},{"line_number":746,"context_line":"        LOG.warning(\u0027Removing connection from ldap pool. Reason: %s\u0027, reason)"},{"line_number":747,"context_line":"        self.conn_pool.purge(conn)"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"    def simple_bind_s(self, who\u003d\u0027\u0027, cred\u003d\u0027\u0027,"},{"line_number":750,"context_line":"                      serverctrls\u003dNone, clientctrls\u003dNone):"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f6a8fd7_0cab83d6","line":747,"range":{"start_line":747,"start_character":29,"end_line":747,"end_character":33},"updated":"2018-04-16 02:11:33.000000000","message":".purge() seems to take a user login (and optionally its password), i.e., something with conn.who, it doesn\u0027t seem to take a connection.  \n\n[0] https://github.com/openstack/ldappool/blob/5454ce5922560d3acf7033de97ce22a8ce4399f6/ldappool/__init__.py#L370","commit_id":"b852c170960a4b7c1cca948dfb2fd2ce7be12555"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"031b4ff55a84c6c4ba1a7e57f0aafed6f23d533b","unresolved":false,"context_lines":[{"line_number":631,"context_line":"        # assert isinstance(self, PooledLDAPHandler)"},{"line_number":632,"context_line":"        with self._get_pool_connection() as conn:"},{"line_number":633,"context_line":"            try:"},{"line_number":634,"context_line":"                self._apply_options(conn)"},{"line_number":635,"context_line":"                return func(self, conn, *args, **kwargs)"},{"line_number":636,"context_line":"            except ldap.SERVER_DOWN:"},{"line_number":637,"context_line":"                self._remove_conn_from_pool(conn, \u0027SERVER_DOWN\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_4fb0df8a","line":634,"range":{"start_line":634,"start_character":16,"end_line":634,"end_character":41},"updated":"2018-04-23 15:35:30.000000000","message":"It\u0027s not apparent that this needs to be in the try/except. I would expect that we only get SERVER_DOWN exceptions when we call the func we\u0027re wrapping, right?","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"031b4ff55a84c6c4ba1a7e57f0aafed6f23d533b","unresolved":false,"context_lines":[{"line_number":634,"context_line":"                self._apply_options(conn)"},{"line_number":635,"context_line":"                return func(self, conn, *args, **kwargs)"},{"line_number":636,"context_line":"            except ldap.SERVER_DOWN:"},{"line_number":637,"context_line":"                self._remove_conn_from_pool(conn, \u0027SERVER_DOWN\u0027)"},{"line_number":638,"context_line":"                raise"},{"line_number":639,"context_line":"    return wrapper"},{"line_number":640,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_2f61eb05","line":637,"range":{"start_line":637,"start_character":51,"end_line":637,"end_character":62},"updated":"2018-04-23 15:35:30.000000000","message":"We should pass this as a kwargs since the method signature defines it at a kwarg. That way if another kwargs is added to the clean up method, it is less likely to break existing callers.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"031b4ff55a84c6c4ba1a7e57f0aafed6f23d533b","unresolved":false,"context_lines":[{"line_number":634,"context_line":"                self._apply_options(conn)"},{"line_number":635,"context_line":"                return func(self, conn, *args, **kwargs)"},{"line_number":636,"context_line":"            except ldap.SERVER_DOWN:"},{"line_number":637,"context_line":"                self._remove_conn_from_pool(conn, \u0027SERVER_DOWN\u0027)"},{"line_number":638,"context_line":"                raise"},{"line_number":639,"context_line":"    return wrapper"},{"line_number":640,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_4f753fca","line":637,"range":{"start_line":637,"start_character":22,"end_line":637,"end_character":43},"updated":"2018-04-23 15:35:30.000000000","message":"nit:\n\n  def _cleanup_connection(\n\nOnly because it makes the name more descriptive of why we\u0027re executing the code instead of just what we\u0027re doing.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"031b4ff55a84c6c4ba1a7e57f0aafed6f23d533b","unresolved":false,"context_lines":[{"line_number":744,"context_line":""},{"line_number":745,"context_line":"    def _remove_conn_from_pool(self, conn, reason\u003dNone):"},{"line_number":746,"context_line":"        LOG.warning(\u0027Removing connection from ldap pool. Reason: %s\u0027, reason)"},{"line_number":747,"context_line":"        self.conn_pool._release_connection(conn)"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"    def simple_bind_s(self, who\u003d\u0027\u0027, cred\u003d\u0027\u0027,"},{"line_number":750,"context_line":"                      serverctrls\u003dNone, clientctrls\u003dNone):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_8f3d97de","line":747,"range":{"start_line":747,"start_character":8,"end_line":747,"end_character":48},"updated":"2018-04-23 15:35:30.000000000","message":"Is it possible for this to throw an exception? Maybe we should only log the warning once it\u0027s successfully cleaned things up.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dcecd4c6f3bf3943518f6b9a625b05cf76631e7f","unresolved":false,"context_lines":[{"line_number":744,"context_line":""},{"line_number":745,"context_line":"    def _remove_conn_from_pool(self, conn, reason\u003dNone):"},{"line_number":746,"context_line":"        LOG.warning(\u0027Removing connection from ldap pool. Reason: %s\u0027, reason)"},{"line_number":747,"context_line":"        self.conn_pool._release_connection(conn)"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"    def simple_bind_s(self, who\u003d\u0027\u0027, cred\u003d\u0027\u0027,"},{"line_number":750,"context_line":"                      serverctrls\u003dNone, clientctrls\u003dNone):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_aab4edf2","line":747,"range":{"start_line":747,"start_character":8,"end_line":747,"end_character":48},"in_reply_to":"9f6a8fd7_4f1a1517","updated":"2018-04-24 15:52:00.000000000","message":"Oh - I meant is it possible for line 747 to thrown an exception from the ldap library or server? If so - it might make sense to move line 746 after 747 to make sure we\u0027re only logging that information once we know we\u0027ve cleaned up the connection.\n\nAs opposed to logging that we\u0027ve cleaned things up properly only to fail with the actual cleanup.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":21420,"name":"Gage Hugo","email":"gagehugo@gmail.com","username":"ghugo"},"change_message_id":"79d9bde20326da3a951c96979c472f858a197d6f","unresolved":false,"context_lines":[{"line_number":744,"context_line":""},{"line_number":745,"context_line":"    def _remove_conn_from_pool(self, conn, reason\u003dNone):"},{"line_number":746,"context_line":"        LOG.warning(\u0027Removing connection from ldap pool. Reason: %s\u0027, reason)"},{"line_number":747,"context_line":"        self.conn_pool._release_connection(conn)"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"    def simple_bind_s(self, who\u003d\u0027\u0027, cred\u003d\u0027\u0027,"},{"line_number":750,"context_line":"                      serverctrls\u003dNone, clientctrls\u003dNone):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_4f1a1517","line":747,"range":{"start_line":747,"start_character":8,"end_line":747,"end_character":48},"in_reply_to":"9f6a8fd7_8f3d97de","updated":"2018-04-23 22:38:50.000000000","message":"I would be fine with throwing an exception as long as keystone can fetch a new connection from the pool and continue on its merry way.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":21420,"name":"Gage Hugo","email":"gagehugo@gmail.com","username":"ghugo"},"change_message_id":"49f37dac80e1855d55f81fad55d18768cd62818c","unresolved":false,"context_lines":[{"line_number":744,"context_line":""},{"line_number":745,"context_line":"    def _remove_conn_from_pool(self, conn, reason\u003dNone):"},{"line_number":746,"context_line":"        LOG.warning(\u0027Removing connection from ldap pool. Reason: %s\u0027, reason)"},{"line_number":747,"context_line":"        self.conn_pool._release_connection(conn)"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"    def simple_bind_s(self, who\u003d\u0027\u0027, cred\u003d\u0027\u0027,"},{"line_number":750,"context_line":"                      serverctrls\u003dNone, clientctrls\u003dNone):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_4abf51a9","line":747,"range":{"start_line":747,"start_character":8,"end_line":747,"end_character":48},"in_reply_to":"9f6a8fd7_aab4edf2","updated":"2018-04-24 15:56:17.000000000","message":"ah ok, good point","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"031b4ff55a84c6c4ba1a7e57f0aafed6f23d533b","unresolved":false,"context_lines":[{"line_number":795,"context_line":"                                    timeout, sizelimit)"},{"line_number":796,"context_line":"        except Exception as ex:"},{"line_number":797,"context_line":"            if isinstance(ex, ldap.SERVER_DOWN):"},{"line_number":798,"context_line":"                self._remove_conn_from_pool(conn, \u0027SERVER_DOWN\u0027)"},{"line_number":799,"context_line":"            conn_ctxt.__exit__(*sys.exc_info())"},{"line_number":800,"context_line":"            raise"},{"line_number":801,"context_line":"        res \u003d MsgId((conn, msgid))"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_ef2bf391","line":798,"range":{"start_line":798,"start_character":51,"end_line":798,"end_character":62},"updated":"2018-04-23 15:35:30.000000000","message":"Same comment here as above.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"031b4ff55a84c6c4ba1a7e57f0aafed6f23d533b","unresolved":false,"context_lines":[{"line_number":824,"context_line":"        try:"},{"line_number":825,"context_line":"            return conn.result3(msg_id, all, timeout)"},{"line_number":826,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":827,"context_line":"            self._remove_conn_from_pool(conn, \u0027SERVER_DOWN\u0027)"},{"line_number":828,"context_line":"            raise"},{"line_number":829,"context_line":""},{"line_number":830,"context_line":"    @use_conn_pool"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f6a8fd7_af1bbb7e","line":827,"range":{"start_line":827,"start_character":47,"end_line":827,"end_character":58},"updated":"2018-04-23 15:35:30.000000000","message":"Same comment here as above.","commit_id":"f0b692efc53346dc316b6ca548f0249e5ab8ecea"}]}