)]}'
{"neutron/agent/linux/daemon.py":[{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class StatusFile(object):"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self,status_file):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAQg%3D","line":31,"updated":"2014-02-17 17:42:37.000000000","message":"First of all, this implementation of Status File, shouldn\u0027t go here, this class is used for the metadata proxy only.  And I was targeting the agents (Manager class)\n\nneutron/services/metering/agents/metering_agent.py:\n   class MeteringAgent(MeteringPluginRpc, manager.Manager):\nneutron/agent/l3_agent.py:\n   class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):\nneutron/agent/dhcp_agent.py:\n    class DhcpAgent(manager.Manager):","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class StatusFile(object):"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self,status_file):"},{"line_number":35,"context_line":"        self._status_file \u003d status_file"},{"line_number":36,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCQc%3D","line":33,"updated":"2014-02-17 15:29:46.000000000","message":"Missing space after the comma. There\u0027s multiple instances of this throughout the file.","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class StatusFile(object):"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self,status_file):"},{"line_number":35,"context_line":"        self._status_file \u003d status_file"},{"line_number":36,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAWY%3D","line":33,"in_reply_to":"AAAAUX%2F%2FCQc%3D","updated":"2014-02-17 17:42:37.000000000","message":"Hmmm, my editor pep8 checking cheated me ;-) It must be disabled...","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self,status_file):"},{"line_number":35,"context_line":"        self._status_file \u003d status_file"},{"line_number":36,"context_line":"        try:"},{"line_number":37,"context_line":"            self.fd \u003d os.open(status_file, os.O_CREAT | os.RDWR)"},{"line_number":38,"context_line":"        except IOError:"},{"line_number":39,"context_line":"            LOG.exception(_(\"Failed to open status file: %s\"), status_file)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCIE%3D","line":36,"updated":"2014-02-17 15:29:46.000000000","message":"Might want to change os.open to open(status_file, \u0027rw\u0027)","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self,status_file):"},{"line_number":35,"context_line":"        self._status_file \u003d status_file"},{"line_number":36,"context_line":"        try:"},{"line_number":37,"context_line":"            self.fd \u003d os.open(status_file, os.O_CREAT | os.RDWR)"},{"line_number":38,"context_line":"        except IOError:"},{"line_number":39,"context_line":"            LOG.exception(_(\"Failed to open status file: %s\"), status_file)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAVo%3D","line":36,"in_reply_to":"AAAAUX%2F%2FCIE%3D","updated":"2014-02-17 17:42:37.000000000","message":"No, I want to use locking on the files, I don\u0027t want several processes writing to the same file. If you check Pidfile is currently implemented with os.open, so I wanted to stick to something similar to what we already had.\n\nMost probably we could access the normal open fd, and do locking, but not sure if that\u0027s a good idea.","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":49,"context_line":"        atexit.register(self._delete_file)"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"    def _delete_file(self):"},{"line_number":52,"context_line":"        self.unlock()"},{"line_number":53,"context_line":"        os.remove(str(self._status_file))"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCG8%3D","line":52,"updated":"2014-02-17 15:29:46.000000000","message":"self._status_file is supposed to be a file path? Why the need for the str operator?","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":49,"context_line":"        atexit.register(self._delete_file)"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"    def _delete_file(self):"},{"line_number":52,"context_line":"        self.unlock()"},{"line_number":53,"context_line":"        os.remove(str(self._status_file))"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAU0%3D","line":52,"in_reply_to":"AAAAUX%2F%2FCG8%3D","updated":"2014-02-17 17:42:37.000000000","message":"As in Pidfile, I was mimicking, probably it\u0027s useful for debugging, but it\u0027s not relevant we don\u0027t need it at this moment.","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":50,"context_line":""},{"line_number":51,"context_line":"    def _delete_file(self):"},{"line_number":52,"context_line":"        self.unlock()"},{"line_number":53,"context_line":"        os.remove(str(self._status_file))"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def lock(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCK0%3D","line":53,"updated":"2014-02-17 15:29:46.000000000","message":"Redundant newline","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":50,"context_line":""},{"line_number":51,"context_line":"    def _delete_file(self):"},{"line_number":52,"context_line":"        self.unlock()"},{"line_number":53,"context_line":"        os.remove(str(self._status_file))"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def lock(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAUw%3D","line":53,"in_reply_to":"AAAAUX%2F%2FCK0%3D","updated":"2014-02-17 17:42:37.000000000","message":"right \u0027:)","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":52,"context_line":"        self.unlock()"},{"line_number":53,"context_line":"        os.remove(str(self._status_file))"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def lock(self):"},{"line_number":57,"context_line":"        if fcntl.flock(self.fd, fcntl.LOCK_EX)!\u003d0:"},{"line_number":58,"context_line":"            raise IOError(_(\u0027Unable to lock status file: %s\u0027),self._status_file)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCJs%3D","line":55,"updated":"2014-02-17 15:29:46.000000000","message":"I think it\u0027s reasonable to remove the locking and unlocking on the status files. Each agent process is supposed to be updating its own status file. If a deployment has different agents pointing to the same status file and you want to catch that, then I\u0027d make that check explicitly in __init__ and change the error message to be more descriptive.\n\nIf you go ahead with the locking approach: I would implement a _change_lock_status function that accepts the lock status (LOCK_EX, LOCK_UN) and have lock and unlock use that internal function. If the new error message is \"Unable to change status file lock\", I think that\u0027d be fine as the exception will contain the stack trace.","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":52,"context_line":"        self.unlock()"},{"line_number":53,"context_line":"        os.remove(str(self._status_file))"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def lock(self):"},{"line_number":57,"context_line":"        if fcntl.flock(self.fd, fcntl.LOCK_EX)!\u003d0:"},{"line_number":58,"context_line":"            raise IOError(_(\u0027Unable to lock status file: %s\u0027),self._status_file)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAT0%3D","line":55,"in_reply_to":"AAAAUX%2F%2FCJs%3D","updated":"2014-02-17 17:42:37.000000000","message":"I wanted to avoid several agents writing to the same file in the event of an user mistake, that would expose something without too much meaning. As the status file syntax is designed to be per-process.\n\nthe _change_lock status seems like a good refactor. We save one translation, and the code is more clear.","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":63,"context_line":"                          self._status_file)"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    def update(self, status_data):"},{"line_number":66,"context_line":""},{"line_number":67,"context_line":"        # TODO(mangelajo): add a file semaphore while we update"},{"line_number":68,"context_line":"        #                  status_file+\u0027.lck\u0027, on which the init script"},{"line_number":69,"context_line":"        #                  could lock also"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCIQ%3D","line":66,"updated":"2014-02-17 15:29:46.000000000","message":"You can use agent/linux/utils.py:replace_file to replace the entire contents of this method, including the future TODO semaphore.","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"76a0812397b514b9dd3a1ca809a376a0f570dace","unresolved":false,"context_lines":[{"line_number":63,"context_line":"                          self._status_file)"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    def update(self, status_data):"},{"line_number":66,"context_line":""},{"line_number":67,"context_line":"        # TODO(mangelajo): add a file semaphore while we update"},{"line_number":68,"context_line":"        #                  status_file+\u0027.lck\u0027, on which the init script"},{"line_number":69,"context_line":"        #                  could lock also"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FANg%3D","line":66,"in_reply_to":"AAAAUX%2F%2FAR4%3D","updated":"2014-02-17 17:46:31.000000000","message":"It actually works, I didn\u0027t know that you could mv a file on top of an existing one.\n\n\u003e\u003e\u003e from neutron.agent.linux.utils import replace_file\n\n\u003e\u003e\u003e replace_file(\"/tmp/aaaa\",\"a\")\n\n\u003e\u003e\u003e replace_file(\"/tmp/aaaa\",\"b\")\n\n[majopela@redcylon neutron]$ cat /tmp/aaaa\nb\n\n[majopela@redcylon neutron]$ echo 3 \u003e/tmp/c\n\n[majopela@redcylon neutron]$ mv /tmp/c /tmp/aaaa\n\n[majopela@redcylon neutron]$ cat /tmp/aaaa\n3","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":63,"context_line":"                          self._status_file)"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    def update(self, status_data):"},{"line_number":66,"context_line":""},{"line_number":67,"context_line":"        # TODO(mangelajo): add a file semaphore while we update"},{"line_number":68,"context_line":"        #                  status_file+\u0027.lck\u0027, on which the init script"},{"line_number":69,"context_line":"        #                  could lock also"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAR4%3D","line":66,"in_reply_to":"AAAAUX%2F%2FCIQ%3D","updated":"2014-02-17 17:42:37.000000000","message":"Wow, that looks like perfect, looking at the implementation of replace file, will it work if we already have a file with those contents?.\n\nCode review is an awesome tool!, Thanks Assaf.","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        os.write(self.fd, status_data)"},{"line_number":73,"context_line":"        os.fsync(self.fd)"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"        # TODO(mangelajo) remove the semaphore now that we\u0027re done"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCKc%3D","line":75,"updated":"2014-02-17 15:29:46.000000000","message":"Two redundant newlines","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        os.write(self.fd, status_data)"},{"line_number":73,"context_line":"        os.fsync(self.fd)"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"        # TODO(mangelajo) remove the semaphore now that we\u0027re done"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FARw%3D","line":75,"in_reply_to":"AAAAUX%2F%2FCKc%3D","updated":"2014-02-17 17:42:37.000000000","message":"Done","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"}],"neutron/agent/linux/process_status.py":[{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"8291d08ccd72e7d636fd70c8049f1f7f4a89cb8b","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# vim: tabstop\u003d4 shiftwidth\u003d4 softtabstop\u003d4"},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"# Copyright 2012 New Dream Network, LLC (DreamHost)"},{"line_number":4,"context_line":"#"},{"line_number":5,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":6,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUX%2F%2B%2Fiw%3D","line":3,"updated":"2014-02-17 19:07:05.000000000","message":"Uhh :-)","commit_id":"a6825f3587e433fb1034de3e4eba2c3483e88cae"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"5cb7de1593d922d96ae9b5eb27ccca4e7544b3a5","unresolved":false,"context_lines":[{"line_number":47,"context_line":"# error codes"},{"line_number":48,"context_line":"OK \u003d 0"},{"line_number":49,"context_line":"RPC_BROKER_DISCONNECTED \u003d 10"},{"line_number":50,"context_line":"CHILD_PROCESS_DIED \u003d 20"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"class ProcessStatus(object):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUX%2F%2B6yc%3D","line":50,"updated":"2014-02-18 07:50:32.000000000","message":"I would change this into \"HEALTH_CHECK_FAILED\" to make something more generic.\n\nCHILD PROCESS is implementation specific to the dnsmasq version, and we could\n \nhave other implementations in the future. (I\u0027m unaware if they already exist)","commit_id":"d794a6defe118d978a1792dfcc34f98cb31cb3ff"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"df2172c2e64854e4b502b37bbc7147db2c9b9bf9","unresolved":false,"context_lines":[{"line_number":35,"context_line":"                   help\u003d_(\"Where to store Neutron status file. \""},{"line_number":36,"context_line":"                          \"This file exposes the current status condition\"))"},{"line_number":37,"context_line":"        ]"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"# error levels , higher \u003d more critical"},{"line_number":40,"context_line":"INFO \u003d 0"},{"line_number":41,"context_line":"WARNING \u003d 10"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F508%3D","line":38,"updated":"2014-02-27 13:44:47.000000000","message":"I\u0027d advise adding a \u0027Status\u0027 class, which holds the error code, message and severity. The class would be able to present itself in the csv format so you could write it to file directly. Finally it\u0027d have factory methods that would return instances of itself with the needed configurations:\n\n(code: OK, level: INFO, message: \u0027Service OK\u0027)\n\n(code: RPC_BROKER_DISCONNECTED, level: CRITICAL, message: \u0027...\u0027)\n\netc...\n\nThis would leave ProcessStatus to manage a set or dict of these Status instances, and not deal with reading, writing, or manipulating the presentation of the data (IE: Get rid of render sorted/unsorted, the STATUS dict).","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"e7d5a5eff0bc17db7ddfd7de61f8e398e2b4b776","unresolved":false,"context_lines":[{"line_number":35,"context_line":"                   help\u003d_(\"Where to store Neutron status file. \""},{"line_number":36,"context_line":"                          \"This file exposes the current status condition\"))"},{"line_number":37,"context_line":"        ]"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"# error levels , higher \u003d more critical"},{"line_number":40,"context_line":"INFO \u003d 0"},{"line_number":41,"context_line":"WARNING \u003d 10"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5m8%3D","line":38,"in_reply_to":"AAAAVX%2F%2F508%3D","updated":"2014-02-27 14:20:23.000000000","message":"Sounds good to me.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"df2172c2e64854e4b502b37bbc7147db2c9b9bf9","unresolved":false,"context_lines":[{"line_number":50,"context_line":"RPC_BROKER_DISCONNECTED \u003d 10"},{"line_number":51,"context_line":"HEALTH_CHECK_FAILED \u003d 20"},{"line_number":52,"context_line":""},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"class ProcessStatus(object):"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    # error codes to messages and levels"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5zg%3D","line":53,"updated":"2014-02-27 13:44:47.000000000","message":"I\u0027d love it if you could add a docstring here that explains how this class is used and for what purposes.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"e7d5a5eff0bc17db7ddfd7de61f8e398e2b4b776","unresolved":false,"context_lines":[{"line_number":50,"context_line":"RPC_BROKER_DISCONNECTED \u003d 10"},{"line_number":51,"context_line":"HEALTH_CHECK_FAILED \u003d 20"},{"line_number":52,"context_line":""},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"class ProcessStatus(object):"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    # error codes to messages and levels"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5mw%3D","line":53,"in_reply_to":"AAAAVX%2F%2F5zg%3D","updated":"2014-02-27 14:20:23.000000000","message":"+1 :)","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"df2172c2e64854e4b502b37bbc7147db2c9b9bf9","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def __init__(self, status_file):"},{"line_number":67,"context_line":"        self._status_file \u003d StatusFile(status_file)"},{"line_number":68,"context_line":"        self._active_status \u003d {}"},{"line_number":69,"context_line":"        self._write_status()"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    def _get_default_status_info(self, status_code):"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F568%3D","line":68,"updated":"2014-02-27 13:44:47.000000000","message":"Here instead of _write_status I would use set_status. In _render_unsorted_status_list I would remove the check for len(status_list) \u003d\u003d 0, and move it to the end of clear_status.\n\nMuch clearer, and solves a (non-interesting, but still) bug with a new instance of ProcessStatus having the default status, but clear_status failing.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"e7d5a5eff0bc17db7ddfd7de61f8e398e2b4b776","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def __init__(self, status_file):"},{"line_number":67,"context_line":"        self._status_file \u003d StatusFile(status_file)"},{"line_number":68,"context_line":"        self._active_status \u003d {}"},{"line_number":69,"context_line":"        self._write_status()"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    def _get_default_status_info(self, status_code):"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5i4%3D","line":68,"in_reply_to":"AAAAVX%2F%2F568%3D","updated":"2014-02-27 14:20:23.000000000","message":"I think you mean doing a \"set_status(OK)\"  when you clear the last status in our list, right?\n\nI believe that: \n1) is not thread safe (unless we lock / check len / set OK / unlock )\n2) the current implementation solves the above and also makes [ ] the implicit OK, but I\u0027m open to discussion on that point.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"174e4972aa13ee6f930fa33fb1d28abc32f3ccfb","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def __init__(self, status_file):"},{"line_number":67,"context_line":"        self._status_file \u003d StatusFile(status_file)"},{"line_number":68,"context_line":"        self._active_status \u003d {}"},{"line_number":69,"context_line":"        self._write_status()"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    def _get_default_status_info(self, status_code):"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5PM%3D","line":68,"in_reply_to":"AAAAVX%2F%2F5fg%3D","updated":"2014-02-27 14:56:33.000000000","message":"The idea is to set the class that any agent can use to expose it\u0027s status, and agents work using threads, so it may happen.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"1194fcae70340a9da3782124b1f87595907ad1df","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def __init__(self, status_file):"},{"line_number":67,"context_line":"        self._status_file \u003d StatusFile(status_file)"},{"line_number":68,"context_line":"        self._active_status \u003d {}"},{"line_number":69,"context_line":"        self._write_status()"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    def _get_default_status_info(self, status_code):"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5fg%3D","line":68,"in_reply_to":"AAAAVX%2F%2F5i4%3D","updated":"2014-02-27 14:26:53.000000000","message":"Do you expect multiple threads to set statuses? I\u0027d think that an agent watcher would have  a single thread?","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"df2172c2e64854e4b502b37bbc7147db2c9b9bf9","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        status_level \u003d self.STATUS[status_code][\u0027level\u0027]"},{"line_number":73,"context_line":"        status_message \u003d self.STATUS[status_code][\u0027message\u0027]"},{"line_number":74,"context_line":"        return status_level, status_message"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def set_status(self, status_code, extended_message\u003d\"\"):"},{"line_number":77,"context_line":"        level, message \u003d self._get_default_status_info(status_code)"},{"line_number":78,"context_line":"        if extended_message !\u003d \"\":"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F56U%3D","line":75,"updated":"2014-02-27 13:44:47.000000000","message":"set_status implies that you can only have one status. Maybe add_status instead?","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"e7d5a5eff0bc17db7ddfd7de61f8e398e2b4b776","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        status_level \u003d self.STATUS[status_code][\u0027level\u0027]"},{"line_number":73,"context_line":"        status_message \u003d self.STATUS[status_code][\u0027message\u0027]"},{"line_number":74,"context_line":"        return status_level, status_message"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def set_status(self, status_code, extended_message\u003d\"\"):"},{"line_number":77,"context_line":"        level, message \u003d self._get_default_status_info(status_code)"},{"line_number":78,"context_line":"        if extended_message !\u003d \"\":"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5kU%3D","line":75,"in_reply_to":"AAAAVX%2F%2F56U%3D","updated":"2014-02-27 14:20:23.000000000","message":"I was thinking more of a bit_mask, when you enable \"bits\" .\n\nadd_status could mean that you can set the same status several times, and that shouldn\u0027t happen.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"174e4972aa13ee6f930fa33fb1d28abc32f3ccfb","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        status_level \u003d self.STATUS[status_code][\u0027level\u0027]"},{"line_number":73,"context_line":"        status_message \u003d self.STATUS[status_code][\u0027message\u0027]"},{"line_number":74,"context_line":"        return status_level, status_message"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def set_status(self, status_code, extended_message\u003d\"\"):"},{"line_number":77,"context_line":"        level, message \u003d self._get_default_status_info(status_code)"},{"line_number":78,"context_line":"        if extended_message !\u003d \"\":"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5Og%3D","line":75,"in_reply_to":"AAAAVX%2F%2F5fA%3D","updated":"2014-02-27 14:56:33.000000000","message":"You\u0027re right","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"1194fcae70340a9da3782124b1f87595907ad1df","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        status_level \u003d self.STATUS[status_code][\u0027level\u0027]"},{"line_number":73,"context_line":"        status_message \u003d self.STATUS[status_code][\u0027message\u0027]"},{"line_number":74,"context_line":"        return status_level, status_message"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def set_status(self, status_code, extended_message\u003d\"\"):"},{"line_number":77,"context_line":"        level, message \u003d self._get_default_status_info(status_code)"},{"line_number":78,"context_line":"        if extended_message !\u003d \"\":"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5fA%3D","line":75,"in_reply_to":"AAAAVX%2F%2F5kU%3D","updated":"2014-02-27 14:26:53.000000000","message":"I think a simple doc would solve any confusion.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"a635bdd6a03df26cb78a1a762541d5670050ddac","unresolved":false,"context_lines":[{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _render_unsorted_status_list(self):"},{"line_number":119,"context_line":"        status_list \u003d []"},{"line_number":120,"context_line":"        for status_code in self._active_status.keys():"},{"line_number":121,"context_line":"            status_data \u003d self._active_status[status_code]"},{"line_number":122,"context_line":"            level \u003d status_data[\u0027level\u0027]"},{"line_number":123,"context_line":"            message \u003d status_data[\u0027message\u0027]"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5GU%3D","line":120,"updated":"2014-02-27 15:11:36.000000000","message":"Ok, we must keep thread-safety in mind here, to avoid a \nRuntimeError: dictionary changed size during iteration","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"546b8fe39d3100c7e6045d9a7ae83139fbf58b34","unresolved":false,"context_lines":[{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _render_unsorted_status_list(self):"},{"line_number":119,"context_line":"        status_list \u003d []"},{"line_number":120,"context_line":"        for status_code in self._active_status.keys():"},{"line_number":121,"context_line":"            status_data \u003d self._active_status[status_code]"},{"line_number":122,"context_line":"            level \u003d status_data[\u0027level\u0027]"},{"line_number":123,"context_line":"            message \u003d status_data[\u0027message\u0027]"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5CY%3D","line":120,"in_reply_to":"AAAAVX%2F%2F5GU%3D","updated":"2014-02-27 15:14:55.000000000","message":"(As we discussed privately), I suggest you add neutron/common/utils.py:synchronized decorator to the public methods of ProcessStatus - Simplest solution. Then you can use set_status in __init__.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"df2172c2e64854e4b502b37bbc7147db2c9b9bf9","unresolved":false,"context_lines":[{"line_number":155,"context_line":"        tmp_file.write(status_data)"},{"line_number":156,"context_line":"        tmp_file.close()"},{"line_number":157,"context_line":"        os.chmod(tmp_file.name, 0o644)"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"        # open \u0026 lock it to avoid any other process writting to the same file"},{"line_number":160,"context_line":"        try:"},{"line_number":161,"context_line":"            new_fd \u003d os.open(tmp_file.name, os.O_CREAT | os.O_RDWR)"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5zI%3D","line":158,"updated":"2014-02-27 13:44:47.000000000","message":"I understand the lock is to prevent multiple agents of the same type writing statuses to the same file. Personally, I think that if you have multiple agents running on the same host the status file is the least of your worries. For code simplicity I\u0027d get rid of the lock.","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"e7d5a5eff0bc17db7ddfd7de61f8e398e2b4b776","unresolved":false,"context_lines":[{"line_number":155,"context_line":"        tmp_file.write(status_data)"},{"line_number":156,"context_line":"        tmp_file.close()"},{"line_number":157,"context_line":"        os.chmod(tmp_file.name, 0o644)"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"        # open \u0026 lock it to avoid any other process writting to the same file"},{"line_number":160,"context_line":"        try:"},{"line_number":161,"context_line":"            new_fd \u003d os.open(tmp_file.name, os.O_CREAT | os.O_RDWR)"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVX%2F%2F5ls%3D","line":158,"in_reply_to":"AAAAVX%2F%2F5zI%3D","updated":"2014-02-27 14:20:23.000000000","message":"Yes, you convinced me, let\u0027s take of of here this complexity, and let the process manager that launch us take care of not launching two of us...","commit_id":"40f6fd788b6e3737a36b71c9c85cefd63baffb84"}],"neutron/common/config.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"17b812e74f17be47d31745252d1a7997ea2fa1c6","unresolved":false,"context_lines":[{"line_number":91,"context_line":"    cfg.StrOpt(\u0027state_path\u0027,"},{"line_number":92,"context_line":"               default\u003d\u0027/var/lib/neutron\u0027,"},{"line_number":93,"context_line":"               help\u003d_(\"Where to store Neutron state files. \""},{"line_number":94,"context_line":"                      \"This directory must be writable by the agent.\")),"},{"line_number":95,"context_line":"    cfg.StrOpt(\u0027status_file\u0027,"},{"line_number":96,"context_line":"               default\u003dNone,"},{"line_number":97,"context_line":"               help\u003d_(\"Where to store Neutron status file. \""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FCXs%3D","line":94,"updated":"2014-02-17 15:29:46.000000000","message":"I\u0027d expect to find this in /etc/dhcp_agent.ini?","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"7f1662368f415488bf8c14cf55e0e3f8af654f7b","unresolved":false,"context_lines":[{"line_number":91,"context_line":"    cfg.StrOpt(\u0027state_path\u0027,"},{"line_number":92,"context_line":"               default\u003d\u0027/var/lib/neutron\u0027,"},{"line_number":93,"context_line":"               help\u003d_(\"Where to store Neutron state files. \""},{"line_number":94,"context_line":"                      \"This directory must be writable by the agent.\")),"},{"line_number":95,"context_line":"    cfg.StrOpt(\u0027status_file\u0027,"},{"line_number":96,"context_line":"               default\u003dNone,"},{"line_number":97,"context_line":"               help\u003d_(\"Where to store Neutron status file. \""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FAPg%3D","line":94,"in_reply_to":"AAAAUX%2F%2FBkk%3D","updated":"2014-02-17 17:42:37.000000000","message":"Yes, I was aiming to use this status file also with the neutron-server, but I\u0027m unsure yet if I will be able to do it at the same time.\n\nIf I cant, I will move it to be only for the manager.Manager, or each agent individually (I\u0027d try to avoid this).","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"0a703fc1f8305e63440d6eb528e292d5d599fa69","unresolved":false,"context_lines":[{"line_number":91,"context_line":"    cfg.StrOpt(\u0027state_path\u0027,"},{"line_number":92,"context_line":"               default\u003d\u0027/var/lib/neutron\u0027,"},{"line_number":93,"context_line":"               help\u003d_(\"Where to store Neutron state files. \""},{"line_number":94,"context_line":"                      \"This directory must be writable by the agent.\")),"},{"line_number":95,"context_line":"    cfg.StrOpt(\u0027status_file\u0027,"},{"line_number":96,"context_line":"               default\u003dNone,"},{"line_number":97,"context_line":"               help\u003d_(\"Where to store Neutron status file. \""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FBkk%3D","line":94,"in_reply_to":"AAAAUX%2F%2FCXs%3D","updated":"2014-02-17 15:30:37.000000000","message":"I keep forgetting this isn\u0027t just for the dhcp agent. Ignore this comment :)","commit_id":"338b4b265f0c14eac9f92ca690faa5a2dd2eda7d"}]}