From c98c373af0655c785ff3e92a7273eb14274f85e9 Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Wed, 24 May 2023 12:55:16 +0200 Subject: [PATCH 1/9] Remove "dying" units from the planned_units count --- ops/model.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index 465fbab6e..5cb22aa5c 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2934,13 +2934,22 @@ def planned_units(self) -> int: Includes the current unit in the count. """ + # TODO: here, query the "life" of the application, if it's "dying", return -1 + # The goal-state tool will return the information that we need. Goal state as a general # concept is being deprecated, however, in favor of approaches such as the one that we use # here. app_state = self._run('goal-state', return_output=True, use_json=True) app_state = typing.cast(Dict[str, List[str]], app_state) + # Planned units can be zero. We don't need to do error checking here. - return len(app_state.get('units', [])) + # But we need to filter out dying units as they may be reported before being deleted + units = [ + unit_name + for unit_name, unit_info in app_state.get('units', {}) + if unit_info['status'] != 'dying' + ] + return len(units) def update_relation_data(self, relation_id: int, _entity: 'UnitOrApplication', key: str, value: str): From 022f21ea66e83a4d9320cb429029f699d14389d0 Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Wed, 24 May 2023 13:47:11 +0200 Subject: [PATCH 2/9] Remove "dying" units from the planned_units count --- ops/model.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ops/model.py b/ops/model.py index 5cb22aa5c..049dee82a 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2934,8 +2934,6 @@ def planned_units(self) -> int: Includes the current unit in the count. """ - # TODO: here, query the "life" of the application, if it's "dying", return -1 - # The goal-state tool will return the information that we need. Goal state as a general # concept is being deprecated, however, in favor of approaches such as the one that we use # here. @@ -2949,6 +2947,10 @@ def planned_units(self) -> int: for unit_name, unit_info in app_state.get('units', {}) if unit_info['status'] != 'dying' ] + + # TODO: here, if len(units) == 0, query the "life" of the application + # if it's "dying", return -1, otherwise 0 (all units removed but app not removed) + return len(units) def update_relation_data(self, relation_id: int, _entity: 'UnitOrApplication', From 97f61afe18d8ade0aca48f298e2950b6deaeec38 Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Wed, 24 May 2023 13:52:28 +0200 Subject: [PATCH 3/9] Added missing items --- ops/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index 049dee82a..a4a5981e1 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2944,7 +2944,7 @@ def planned_units(self) -> int: # But we need to filter out dying units as they may be reported before being deleted units = [ unit_name - for unit_name, unit_info in app_state.get('units', {}) + for unit_name, unit_info in app_state.get('units', {}).items() if unit_info['status'] != 'dying' ] From a32c68a6e84f84c42e79adfa056420428bd4465c Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Wed, 24 May 2023 14:13:17 +0200 Subject: [PATCH 4/9] Fixed typing issue --- ops/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index a4a5981e1..ee6848c0f 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2938,7 +2938,7 @@ def planned_units(self) -> int: # concept is being deprecated, however, in favor of approaches such as the one that we use # here. app_state = self._run('goal-state', return_output=True, use_json=True) - app_state = typing.cast(Dict[str, List[str]], app_state) + app_state = typing.cast(Dict[str, Dict[str, str]], app_state) # Planned units can be zero. We don't need to do error checking here. # But we need to filter out dying units as they may be reported before being deleted From eb56cd5d394270f4fc02c80e21834b7df49599fd Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Wed, 24 May 2023 15:41:52 +0200 Subject: [PATCH 5/9] Fixed typing issue --- ops/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index ee6848c0f..c7d74389e 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2938,7 +2938,7 @@ def planned_units(self) -> int: # concept is being deprecated, however, in favor of approaches such as the one that we use # here. app_state = self._run('goal-state', return_output=True, use_json=True) - app_state = typing.cast(Dict[str, Dict[str, str]], app_state) + app_state = typing.cast(Dict[str, Dict[str, Any]], app_state) # Planned units can be zero. We don't need to do error checking here. # But we need to filter out dying units as they may be reported before being deleted From c132a111d6792e6f3bfe519a2b4a673dbd28f80e Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Wed, 24 May 2023 17:49:02 +0200 Subject: [PATCH 6/9] Removed todo --- ops/model.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ops/model.py b/ops/model.py index c7d74389e..f55d51c30 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2947,10 +2947,6 @@ def planned_units(self) -> int: for unit_name, unit_info in app_state.get('units', {}).items() if unit_info['status'] != 'dying' ] - - # TODO: here, if len(units) == 0, query the "life" of the application - # if it's "dying", return -1, otherwise 0 (all units removed but app not removed) - return len(units) def update_relation_data(self, relation_id: int, _entity: 'UnitOrApplication', From 976dff9f2da55b4e32236478cbbffc8997aadfdf Mon Sep 17 00:00:00 2001 From: Mehdi Bendriss Date: Thu, 25 May 2023 13:54:22 +0200 Subject: [PATCH 7/9] Simplify filtering Co-authored-by: Ben Hoyt --- ops/model.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/ops/model.py b/ops/model.py index f55d51c30..bb75c2750 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2942,12 +2942,9 @@ def planned_units(self) -> int: # Planned units can be zero. We don't need to do error checking here. # But we need to filter out dying units as they may be reported before being deleted - units = [ - unit_name - for unit_name, unit_info in app_state.get('units', {}).items() - if unit_info['status'] != 'dying' - ] - return len(units) + units = app_state.get('units', {}) + num_alive = sum(1 for unit in units.values() if unit['status'] != 'dying') + return num_alive def update_relation_data(self, relation_id: int, _entity: 'UnitOrApplication', key: str, value: str): From efc06428168d9a302a4e3cd34436d60e87375a0f Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Thu, 25 May 2023 14:32:06 +0200 Subject: [PATCH 8/9] PR feedback: added test cases --- ops/model.py | 2 +- test/test_model.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index bb75c2750..06d689945 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2931,7 +2931,7 @@ def get_pebble(self, socket_path: str) -> 'Client': def planned_units(self) -> int: """Count of "planned" units that will run this application. - Includes the current unit in the count. + Includes the current unit in the count, but does not include dying units. """ # The goal-state tool will return the information that we need. Goal state as a general diff --git a/test/test_model.py b/test/test_model.py index 92a78bd53..7b7344a94 100755 --- a/test/test_model.py +++ b/test/test_model.py @@ -2712,6 +2712,35 @@ def test_relation_remote_app_name_script_errors(self): ['relation-list', '-r', '6', '--app', '--format=json'], ]) + def test_planned_units(self): + # no units + fake_script(self, 'goal-state', """ +echo '{"units":{}, "relations":{}}' +""") + self.assertEqual(self.backend.planned_units(), 0) + + # only active units + fake_script(self, 'goal-state', """ +echo '{ + "units":{ + "app/0": {"status":"active","since":"2023-05-23 17:05:05Z"}, + "app/1": {"status":"active","since":"2023-05-23 17:57:05Z"} + }, + "relations": {} +}'""") + self.assertEqual(self.backend.planned_units(), 2) + + # active and dying units + fake_script(self, 'goal-state', """ +echo '{ + "units":{ + "app/0": {"status":"active","since":"2023-05-23 17:05:05Z"}, + "app/1": {"status":"dying","since":"2023-05-23 17:57:05Z"} + }, + "relations": {} +}'""") + self.assertEqual(self.backend.planned_units(), 1) + class TestLazyMapping(unittest.TestCase): From 82c3cbe5fe94d8af59c683430a8099679d1cc5b3 Mon Sep 17 00:00:00 2001 From: Mehdi-Bendriss Date: Sat, 27 May 2023 16:41:53 +0200 Subject: [PATCH 9/9] Improved docstring with more details. --- ops/model.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index 06d689945..24baca93c 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2931,7 +2931,8 @@ def get_pebble(self, socket_path: str) -> 'Client': def planned_units(self) -> int: """Count of "planned" units that will run this application. - Includes the current unit in the count, but does not include dying units. + This will include the current unit, any units that are alive, units that are in the process + of being started, but will not include units that are being shut down. """ # The goal-state tool will return the information that we need. Goal state as a general