Cleanup: no mixed use of setters and properties.

pgbouncemgr/manager.py

@@ -38,8 +38,8 @@ class Manager():
         self.state_store.load()
 
     def run(self):
-        """Starts the manager."""
+        """Starts the manager process."""
-        self.drop_privileges(self.config.run_user, self.config.run_group)
+        drop_privileges(self.config.run_user, self.config.run_group)
         while True:
             self.node_poller.poll()
             sleep(self.config.poll_interval_in_sec)

pgbouncemgr/node_config.py

@@ -29,12 +29,3 @@ class NodeConfig():
                 "pgbouncer_config path must exist",
                 "nodes[%s]" % self.node_id, "pgbouncer_config", path)
         self._pgbouncer_config = path
-
-    def export(self):
-        """Exports the data for the node configuration, that we want
-           to end up in the state data that is stored in the state store."""
-        return {
-            "pgbouncer_config": self.pgbouncer_config,
-            "host": self.host,
-            "port": self.port
-        }

pgbouncemgr/state.py

@@ -170,16 +170,12 @@ class State():
         self.modified = True
         self._timeline_id = timeline_id
 
-    def add_node(self, node):
+    def add_node(self, node_config):
-        """Add a node to the state. Node can be a NodeConfig object or
+        """Add a node to the state, based on the provided NodeConfig object."""
-           a node_id. In case a node_id is provided, an NodeConfig object
+        if node_config.node_id in self.nodes:
-           will be created automatically."""
+            raise DuplicateNodeAdded(node_config.node_id)
-        if not isinstance(node, NodeConfig):
+        node_state = NodeState(node_config, self)
-            node = NodeConfig(node)
+        self.nodes[node_config.node_id] = node_state
-        if node.node_id in self.nodes:
-            raise DuplicateNodeAdded(node.node_id)
-        node_state = NodeState(node, self)
-        self.nodes[node.node_id] = node_state
         self.modified = True
         return node_state
 
@@ -305,21 +301,21 @@ class NodeState():
     """This class encapsulates the information for a single node in a
        PostgreSQL cluster."""
 
-    def __init__(self, config, parent_state):
+    def __init__(self, node_config, parent_state):
-        self.node_id = config.node_id
+        self.node_id = node_config.node_id
-        self.config = config
+        self.config = node_config
         self.parent_state = parent_state
         self._system_id = None
         self._timeline_id = None
-        self.status = NODE_UNKNOWN
+        self._status = NODE_UNKNOWN
-        self.err = None
+        self._error = None
 
     def reset(self):
         """Reset the data for the node."""
         self.system_id = None
         self.timeline_id = None
         self.status = NODE_UNKNOWN
-        self.err = None
+        self.error = None
         self.notify_parent()
 
     def notify_parent(self):
@@ -365,25 +361,35 @@ class NodeState():
         self._timeline_id = timeline_id
         self.notify_parent()
 
-    def set_status(self, status):
+    @property
+    def status(self):
+        return self._status
+
+    @status.setter
+    def status(self, status):
         """Set the connection status for the node. This is used to indicate
            whether or not a connection can be setup to the node from the
-           pgbouncemgr application and if the node is running in primary
+           pgbouncemgr application and if the node is found to be running
-           or fallback mode. Possible values are: NODE_UNKNOWN, NODE_OFFLINE,
+           in primary or fallback mode. Possible values are: NODE_UNKNOWN,
-           NODE_PRIMARY and NODE_STANDBY."""
+           NODE_OFFLINE, NODE_PRIMARY and NODE_STANDBY."""
-        if self.status == status:
+        if self._status == status:
             return
         if status not in NODE_STATUSES:
             raise InvalidNodeStatus(status)
-        self.status = status
+        self._status = status
         self.notify_parent()
 
-    def set_error(self, err):
+    @property
+    def error(self):
+        return self._error
+
+    @error.setter
+    def error(self, msg):
         """When there is some problem with this node, this method can be used
            to set an error message for it, explaining the problem."""
-        if self.err == err:
+        if self._error == msg:
             return
-        self.err = err
+        self._error = msg
         self.notify_parent()
 
     def promote(self):
@@ -397,10 +403,16 @@ class NodeState():
            state storage."""
         return {
             "node_id": self.node_id,
-            "config": self.config.export(),
+            "config": {
+                # Not all config data are exported, just a few that might
+                # be useful to find back in the state export.
+                "pgbouncer_config": self.config.pgbouncer_config,
+                "host": self.config.host,
+                "port": self.config.port
+            },
             "system_id": self.system_id,
             "timeline_id": self.timeline_id,
             "is_leader": self.parent_state.leader_node_id == self.node_id,
             "status": self.status,
-            "error": self.err,
+            "error": self._error,
         }

tests/test_state.py

@@ -138,17 +138,17 @@ class StateTests(unittest.TestCase):
 class NodeCollectionTests(unittest.TestCase):
     def test_WithNodeNotYetInState_AddNode_AddsNodeState(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
 
         self.assertEqual(1, node.node_id)
         self.assertTrue(state.modified)
 
     def test_WithNodeAlreadyInState_AddNode_RaisesException(self):
         state = State(Logger())
-        state.add_node(123)
+        state.add_node(NodeConfig(123))
 
         with self.assertRaises(DuplicateNodeAdded) as context:
-            state.add_node(123)
+            state.add_node(NodeConfig(123))
         self.assertIn("duplicate node_id=123", str(context.exception))
 
     def test_WithNodeNotInState_getNode_RaisesException(self):
@@ -159,7 +159,7 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_WithNodeInState_getNode_ReturnsNode(self):
         state = State(Logger())
-        node = state.add_node("that does exist")
+        node = state.add_node(NodeConfig("that does exist"))
 
         node_from_state = state.get_node("that does exist")
 
@@ -168,14 +168,14 @@ class NodeCollectionTests(unittest.TestCase):
     def test_WithUnknownNode_SetLeaderNode_RaisesException(self):
         state1 = State(Logger())
         state2 = State(Logger())
-        node = state2.add_node(1337)
+        node = state2.add_node(NodeConfig(1337))
         with self.assertRaises(UnknownNodeRequested) as context:
             state1.promote_node(node)
         self.assertIn("unknown node_id=1337", str(context.exception))
 
     def test_WithNodeWithoutSystemId_SetLeaderNode_RaisesException(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         node.timeline_id = 1
         node.config.pgbouncer_config = PGBOUNCER_CONFIG
         with self.assertRaises(NodeCannotBePromoted) as context:
@@ -185,7 +185,7 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_WithNodeWithoutTimelineId_SetLeaderNode_RaisesException(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         node.system_id = "system"
         node.config.pgbouncer_config = PGBOUNCER_CONFIG
         with self.assertRaises(NodeCannotBePromoted) as context:
@@ -195,7 +195,7 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_WithNodeWithoutPgbouncerConfig_SetLeaderNode_RaisesException(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         node.system_id = "a7d8a9347df789saghdfs"
         node.timeline_id = 11111111111
         with self.assertRaises(NodeCannotBePromoted) as context:
@@ -205,7 +205,7 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_SetLeaderNode_SetsLeaderNode_WithUnknownLeaderStatus(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         node.config.pgbouncer_config = PGBOUNCER_CONFIG
         node.system_id = "SuperCluster"
         node.timeline_id = " 005   "
@@ -220,7 +220,7 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_SetLeaderNode_ToSameLeader_ResetsLeaderNode_WithUnknownLeaderStatus(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         node.config.pgbouncer_config = PGBOUNCER_CONFIG
         node.system_id = "1.2.3.4.5.6.7.8.9.10.11"
         node.timeline_id = 12
@@ -236,11 +236,11 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_SetLeaderNode_ToNodeWithDifferentSystemId_RaisesException(self):
         state = State(Logger())
-        node1 = state.add_node(1)
+        node1 = state.add_node(NodeConfig(1))
         node1.config.pgbouncer_config = PGBOUNCER_CONFIG
         node1.system_id = "systemA"
         node1.timeline_id = 10
-        node2 = state.add_node(2)
+        node2 = state.add_node(NodeConfig(2))
         node2.config.pgbouncer_config = PGBOUNCER_CONFIG
         node2.system_id = "systemB"
         node2.timeline_id = 10
@@ -251,11 +251,11 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_SetLeaderNode_ToNodeWithLowerTimelineId_RaisesException(self):
         state = State(Logger())
-        node1 = state.add_node(1)
+        node1 = state.add_node(NodeConfig(1))
         node1.config.pgbouncer_config = PGBOUNCER_CONFIG
         node1.system_id = "systemX"
         node1.timeline_id = 10
-        node2 = state.add_node(2)
+        node2 = state.add_node(NodeConfig(2))
         node2.config.pgbouncer_config = PGBOUNCER_CONFIG
         node2.system_id = "systemX"
         node2.timeline_id = 9
@@ -266,15 +266,15 @@ class NodeCollectionTests(unittest.TestCase):
 
     def test_SetLeaderNode_ToNodeWithSameOrHigherTimelineId_IsOk(self):
         state = State(Logger())
-        node1 = state.add_node(1)
+        node1 = state.add_node(NodeConfig(1))
         node1.config.pgbouncer_config = PGBOUNCER_CONFIG
         node1.system_id = "systemX"
         node1.timeline_id = 42
-        node2 = state.add_node(2)
+        node2 = state.add_node(NodeConfig(2))
         node2.config.pgbouncer_config = PGBOUNCER_CONFIG
         node2.system_id = "systemX"
         node2.timeline_id = 42
-        node3 = state.add_node(3)
+        node3 = state.add_node(NodeConfig(3))
         node3.config.pgbouncer_config = PGBOUNCER_CONFIG
         node3.system_id = "systemX"
         node3.timeline_id = 43
@@ -301,7 +301,7 @@ class NodeCollectionTests(unittest.TestCase):
 class NodeTests(unittest.TestCase):
     def test_WithNoneValue_SetSystemId_RaisesException(self):
         state = State(Logger())
-        node = state.add_node("break me")
+        node = state.add_node(NodeConfig("break me"))
         state.modified = False
 
         with self.assertRaises(InvalidSystemId) as context:
@@ -311,7 +311,7 @@ class NodeTests(unittest.TestCase):
 
     def test_WithEmptyString_SetSystemId_RaisesException(self):
         state = State(Logger())
-        node = state.add_node("break me")
+        node = state.add_node(NodeConfig("break me"))
         state.modified = False
 
         with self.assertRaises(InvalidSystemId) as context:
@@ -321,7 +321,7 @@ class NodeTests(unittest.TestCase):
 
     def test_SetSystemId_SetsSystemId_AndNotifiesChangeToState(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         state.modified = False
 
         node.system_id = "X"
@@ -331,7 +331,7 @@ class NodeTests(unittest.TestCase):
 
     def test_WithNonIntegerInput_SetTimelineId_RaisesException(self):
         state = State(Logger())
-        node = state.add_node("break me")
+        node = state.add_node(NodeConfig("break me"))
 
         with self.assertRaises(InvalidTimelineId) as context:
             node.timeline_id = "TARDIS"
@@ -339,7 +339,7 @@ class NodeTests(unittest.TestCase):
     
     def test_SetTimelineId_SetsTimelineId_AndNotifiesChangeToState(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         state.modified = False
 
         node.timeline_id = 25
@@ -349,58 +349,58 @@ class NodeTests(unittest.TestCase):
 
     def test_SetStatus_SetsStatus_AndNotifiesChangeToState(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         state.modified = False
 
-        node.set_status(NODE_PRIMARY)
+        node.status = NODE_PRIMARY
 
         self.assertEqual(NODE_PRIMARY, node.status)
         self.assertTrue(state.modified)
 
     def test_WithInvalidStatus_SetStatus_RaisesException(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         state.modified = False
 
         with self.assertRaises(InvalidNodeStatus) as context:
-            node.set_status("DERAILED")
+            node.status = "DERAILED"
         self.assertIn("'DERAILED'", str(context.exception))
         self.assertFalse(state.modified)
 
     def test_SetError_ToString_SetsError_AndNotifiesChangeToState(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
         state.modified = False
 
-        node.set_error("Found some spare bits at IKEA.py line 141")
+        node.error = "Found some spare bits at IKEA.py line 141"
 
-        self.assertEqual("Found some spare bits at IKEA.py line 141", node.err)
+        self.assertEqual("Found some spare bits at IKEA.py line 141", node.error)
         self.assertTrue(state.modified)
 
     def test_SetError_ToNone_ClearsError_AndNotifiesChangeToState(self):
         state = State(Logger())
-        node = state.add_node(1)
+        node = state.add_node(NodeConfig(1))
-        node.set_error("Mouse lost its ball")
+        node.error = "Mouse lost its ball"
         state.modified = False
 
-        node.set_error(None)
+        node.error = None
 
-        self.assertEqual(None, node.err)
+        self.assertEqual(None, node.error)
         self.assertTrue(state.modified)
 
     def test_WhenNothingChanges_NoChangeIsNotifiedToState(self):
         state = State(Logger())
-        node = state.add_node("x")
+        node = state.add_node(NodeConfig("x"))
         node.system_id = "aaaaaaa"
         node.timeline_id = 55
-        node.set_status(NODE_PRIMARY)
+        node.status = NODE_PRIMARY
-        node.set_error("Just testin'")
+        node.error = "Just testin'"
         state.modified = False
 
         node.system_id = "aaaaaaa"
         node.timeline_id = 55
-        node.set_status(NODE_PRIMARY)
+        node.status = NODE_PRIMARY
-        node.set_error("Just testin'")
+        node.error = "Just testin'"
 
         self.assertFalse(state.modified)
 
@@ -411,19 +411,19 @@ class StateExportTests(unittest.TestCase):
         self.maxDiff = 5000;
         state = State(Logger())
 
-        node1 = state.add_node(1)
+        node1 = state.add_node(NodeConfig(1))
         node1.config.pgbouncer_config = PGBOUNCER_CONFIG
         node1.system_id = "System X"
         node1.timeline_id = 555
-        node1.set_status(NODE_PRIMARY)
+        node1.status = NODE_PRIMARY
-        node1.set_error("Some error for 1")
+        node1.error = "Some error for 1"
 
-        node2 = state.add_node(2)
+        node2 = state.add_node(NodeConfig(2))
         node2.config.pgbouncer_config = PGBOUNCER_CONFIG
         node2.system_id = "System Y"
         node2.timeline_id = 1
-        node2.set_status(NODE_STANDBY)
+        node2.status = NODE_STANDBY
-        node2.set_error("Some error for 2")
+        node2.error = "Some error for 2"
 
         node1.promote()