From 913fd9767928ed96082486c9c0ee010447525182 Mon Sep 17 00:00:00 2001 From: FileMagic <22534836+FileMagic@users.noreply.github.com> Date: Tue, 13 Aug 2024 13:45:14 +0000 Subject: [PATCH 01/20] [Broken] Attempt to add redis and add stdout printing to debug issues --- Makefile | 4 ++++ st2actions/tests/unit/test_workflow_engine.py | 15 ++++++++------- .../services/test_workflow_service_retries.py | 5 +++-- st2tests/st2tests/config.py | 9 +++++++++ 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 303cc5111f..a5a2afa521 100644 --- a/Makefile +++ b/Makefile @@ -53,6 +53,10 @@ COVERAGE_GLOBS_QUOTED := $(foreach glob,$(COVERAGE_GLOBS),'$(glob)') REQUIREMENTS := test-requirements.txt requirements.txt +# Redis config for testing +ST2_OVERRIDE_COORDINATOR_REDIS_HOST := ${REDIS_HOST:-"127.0.0.1"} +ST2_OVERRIDE_COORDINATOR_REDIS_PORT := ${REDIS_PORT:-"6379"} + # Pin common pip version here across all the targets # Note! Periodic maintenance pip upgrades are required to be up-to-date with the latest pip security fixes and updates PIP_VERSION ?= 24.2 diff --git a/st2actions/tests/unit/test_workflow_engine.py b/st2actions/tests/unit/test_workflow_engine.py index 68d68a3b2f..c86e24f556 100644 --- a/st2actions/tests/unit/test_workflow_engine.py +++ b/st2actions/tests/unit/test_workflow_engine.py @@ -24,6 +24,7 @@ from orquesta import statuses as wf_statuses from oslo_config import cfg from tooz import coordination +from tooz.drivers.redis import RedisDriver from st2actions.workflows import workflows from st2common.bootstrap import actionsregistrar @@ -142,7 +143,7 @@ def test_process(self): lv_ac_db = lv_db_access.LiveAction.get_by_id(str(lv_ac_db.id)) self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_SUCCEEDED) - @mock.patch.object(coordination_service.NoOpDriver, "get_lock") + @mock.patch.object(RedisDriver, "get_lock") def test_process_error_handling(self, mock_get_lock): expected_errors = [ { @@ -200,7 +201,7 @@ def test_process_error_handling(self, mock_get_lock): self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_FAILED) @mock.patch.object( - coordination_service.NoOpDriver, + RedisDriver, "get_lock", ) @mock.patch.object( @@ -263,7 +264,7 @@ def test_process_error_handling_has_error(self, mock_get_lock): self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_CANCELED) @mock.patch.object( - coordination_service.NoOpDriver, + RedisDriver, "get_members", mock.MagicMock(return_value=coordination_service.NoOpAsyncResult("")), ) @@ -325,7 +326,7 @@ def test_workflow_engine_shutdown(self): ) @mock.patch.object( - coordination_service.NoOpDriver, + RedisDriver, "get_members", mock.MagicMock(return_value=coordination_service.NoOpAsyncResult("member-1")), ) @@ -399,7 +400,7 @@ def test_workflow_engine_shutdown_with_service_registry_disabled(self): self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_RUNNING) @mock.patch.object( - coordination_service.NoOpDriver, + RedisDriver, "get_lock", mock.MagicMock(return_value=coordination_service.NoOpLock(name="noop")), ) @@ -456,7 +457,7 @@ def test_workflow_engine_shutdown_first_then_start(self): ) @mock.patch.object( - coordination_service.NoOpDriver, + RedisDriver, "get_lock", mock.MagicMock(return_value=coordination_service.NoOpLock(name="noop")), ) @@ -485,7 +486,7 @@ def test_workflow_engine_start_first_then_shutdown(self): eventlet.spawn(workflow_engine.start, True) eventlet.spawn_after(1, workflow_engine.shutdown) - coordination_service.NoOpDriver.get_members = mock.MagicMock( + RedisDriver.get_members = mock.MagicMock( return_value=coordination_service.NoOpAsyncResult("member-1") ) diff --git a/st2common/tests/unit/services/test_workflow_service_retries.py b/st2common/tests/unit/services/test_workflow_service_retries.py index ca2fab6f9f..5364043db7 100644 --- a/st2common/tests/unit/services/test_workflow_service_retries.py +++ b/st2common/tests/unit/services/test_workflow_service_retries.py @@ -27,6 +27,7 @@ from orquesta import statuses as wf_statuses from tooz import coordination +from tooz.drivers.redis import RedisDriver import st2tests @@ -123,7 +124,7 @@ def setUpClass(cls): for pack in PACKS: actions_registrar.register_from_pack(pack) - @mock.patch.object(coord_svc.NoOpDriver, "get_lock") + @mock.patch.object(RedisDriver, "get_lock") def test_recover_from_coordinator_connection_error(self, mock_get_lock): mock_get_lock.side_effect = coord_svc.NoOpLock(name="noop") wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") @@ -157,7 +158,7 @@ def test_recover_from_coordinator_connection_error(self, mock_get_lock): tk1_ex_db = wf_db_access.TaskExecution.get_by_id(tk1_ex_db.id) self.assertEqual(tk1_ex_db.status, wf_statuses.SUCCEEDED) - @mock.patch.object(coord_svc.NoOpDriver, "get_lock") + @mock.patch.object(RedisDriver, "get_lock") def test_retries_exhausted_from_coordinator_connection_error(self, mock_get_lock): mock_get_lock.side_effect = coord_svc.NoOpLock(name="noop") wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") diff --git a/st2tests/st2tests/config.py b/st2tests/st2tests/config.py index bef5197bcf..87d0679355 100644 --- a/st2tests/st2tests/config.py +++ b/st2tests/st2tests/config.py @@ -139,6 +139,15 @@ def _override_scheduler_opts(): def _override_coordinator_opts(noop=False): driver = None if noop else "zake://" + + ST2_OVERRIDE_COORDINATOR_REDIS_HOST = os.environ.get("ST2_OVERRIDE_COORDINATOR_REDIS_HOST", False) + if ST2_OVERRIDE_COORDINATOR_REDIS_HOST: + + ST2_OVERRIDE_COORDINATOR_REDIS_PORT = os.environ.get("ST2_OVERRIDE_COORDINATOR_REDIS_PORT", "6379") + driver=f"redis://{ST2_OVERRIDE_COORDINATOR_REDIS_HOST}:{ST2_OVERRIDE_COORDINATOR_REDIS_PORT}" + assert False + print(f"Redis is being used with the following cord: {driver}") + CONF.set_override(name="url", override=driver, group="coordination") CONF.set_override(name="lock_timeout", override=1, group="coordination") From 913b5f01bead435161faf2cd2a61c9cd4641fce8 Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Mon, 9 Sep 2024 13:51:22 -0400 Subject: [PATCH 02/20] working unit tests --- Makefile | 6 ++-- st2actions/tests/unit/test_worker.py | 22 +++++++++++- st2actions/tests/unit/test_workflow_engine.py | 35 +++++++++++-------- .../services/test_workflow_service_retries.py | 1 - st2common/tests/unit/test_service_setup.py | 1 + st2tests/st2tests/config.py | 4 +-- 6 files changed, 48 insertions(+), 21 deletions(-) diff --git a/Makefile b/Makefile index a5a2afa521..04ba496d30 100644 --- a/Makefile +++ b/Makefile @@ -54,8 +54,8 @@ COVERAGE_GLOBS_QUOTED := $(foreach glob,$(COVERAGE_GLOBS),'$(glob)') REQUIREMENTS := test-requirements.txt requirements.txt # Redis config for testing -ST2_OVERRIDE_COORDINATOR_REDIS_HOST := ${REDIS_HOST:-"127.0.0.1"} -ST2_OVERRIDE_COORDINATOR_REDIS_PORT := ${REDIS_PORT:-"6379"} +ST2_OVERRIDE_COORDINATOR_REDIS_HOST := 127.0.0.1 +ST2_OVERRIDE_COORDINATOR_REDIS_PORT := 6379 # Pin common pip version here across all the targets # Note! Periodic maintenance pip upgrades are required to be up-to-date with the latest pip security fixes and updates @@ -828,6 +828,8 @@ unit-tests: requirements .unit-tests echo "Running tests in" $$component; \ echo "-----------------------------------------------------------"; \ . $(VIRTUALENV_DIR)/bin/activate; \ + ST2_OVERRIDE_COORDINATOR_REDIS_HOST=$(ST2_OVERRIDE_COORDINATOR_REDIS_HOST) \ + ST2_OVERRIDE_COORDINATOR_REDIS_PORT=$(ST2_OVERRIDE_COORDINATOR_REDIS_PORT) \ nosetests $(NOSE_OPTS) -s -v \ $$component/tests/unit || ((failed+=1)); \ echo "-----------------------------------------------------------"; \ diff --git a/st2actions/tests/unit/test_worker.py b/st2actions/tests/unit/test_worker.py index b335b6f2be..3a6a7fae3c 100644 --- a/st2actions/tests/unit/test_worker.py +++ b/st2actions/tests/unit/test_worker.py @@ -20,6 +20,7 @@ import mock import os from oslo_config import cfg +from tooz.drivers.redis import RedisDriver import tempfile # This import must be early for import-time side-effects. @@ -169,7 +170,7 @@ def test_worker_shutdown(self): runner_thread.wait() @mock.patch.object( - coordination.NoOpDriver, + RedisDriver, "get_members", mock.MagicMock(return_value=coordination.NoOpAsyncResult("member-1")), ) @@ -177,6 +178,15 @@ def test_worker_graceful_shutdown_with_multiple_runners(self): cfg.CONF.set_override( name="graceful_shutdown", override=True, group="actionrunner" ) + cfg.CONF.set_override( + name="service_registry", override=True, group="coordination" + ) + cfg.CONF.set_override( + name="exit_still_active_check", override=10, group="actionrunner" + ) + cfg.CONF.set_override( + name="still_active_check_interval", override=1, group="actionrunner" + ) action_worker = actions_worker.get_worker() temp_file = None @@ -237,6 +247,16 @@ def test_worker_graceful_shutdown_with_single_runner(self): cfg.CONF.set_override( name="graceful_shutdown", override=True, group="actionrunner" ) + cfg.CONF.set_override( + name="service_registry", override=True, group="coordination" + ) + cfg.CONF.set_override( + name="exit_still_active_check", override=10, group="actionrunner" + ) + cfg.CONF.set_override( + name="still_active_check_interval", override=1, group="actionrunner" + ) + action_worker = actions_worker.get_worker() temp_file = None diff --git a/st2actions/tests/unit/test_workflow_engine.py b/st2actions/tests/unit/test_workflow_engine.py index c86e24f556..63848af3bc 100644 --- a/st2actions/tests/unit/test_workflow_engine.py +++ b/st2actions/tests/unit/test_workflow_engine.py @@ -145,6 +145,10 @@ def test_process(self): @mock.patch.object(RedisDriver, "get_lock") def test_process_error_handling(self, mock_get_lock): + tests_config.parse_args() + cfg.CONF.set_override( + name="service_registry", override=True, group="coordination" + ) expected_errors = [ { "message": "Execution failed. See result for details.", @@ -158,7 +162,6 @@ def test_process_error_handling(self, mock_get_lock): "route": 0, }, ] - mock_get_lock.side_effect = coordination_service.NoOpLock(name="noop") wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) @@ -179,8 +182,6 @@ def test_process_error_handling(self, mock_get_lock): task_execution=str(t1_ex_db.id) )[0] mock_get_lock.side_effect = [ - coordination.ToozConnectionError("foobar"), - coordination.ToozConnectionError("foobar"), coordination.ToozConnectionError("foobar"), coordination.ToozConnectionError("foobar"), coordination.ToozConnectionError("foobar"), @@ -213,6 +214,8 @@ def test_process_error_handling_has_error(self, mock_get_lock): mock_get_lock.side_effect = coordination_service.NoOpLock(name="noop") wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) + + lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) # Assert action execution is running. @@ -263,15 +266,20 @@ def test_process_error_handling_has_error(self, mock_get_lock): lv_ac_db = lv_db_access.LiveAction.get_by_id(str(lv_ac_db.id)) self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_CANCELED) - @mock.patch.object( - RedisDriver, - "get_members", - mock.MagicMock(return_value=coordination_service.NoOpAsyncResult("")), - ) def test_workflow_engine_shutdown(self): + cfg.CONF.set_override( + name="graceful_shutdown", override=True, group="actionrunner" + ) cfg.CONF.set_override( name="service_registry", override=True, group="coordination" ) + cfg.CONF.set_override( + name="exit_still_active_check", override=4, group="workflow_engine" + ) + cfg.CONF.set_override( + name="still_active_check_interval", override=1, group="workflow_engine" + ) + wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) @@ -284,11 +292,10 @@ def test_workflow_engine_shutdown(self): )[0] self.assertEqual(wf_ex_db.status, action_constants.LIVEACTION_STATUS_RUNNING) workflow_engine = workflows.get_engine() - eventlet.spawn(workflow_engine.shutdown) # Sleep for few seconds to ensure execution transitions to pausing. - eventlet.sleep(5) + eventlet.sleep(8) lv_ac_db = lv_db_access.LiveAction.get_by_id(str(lv_ac_db.id)) self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_PAUSING) @@ -481,15 +488,15 @@ def test_workflow_engine_start_first_then_shutdown(self): self.assertEqual(wf_ex_db.status, action_constants.LIVEACTION_STATUS_RUNNING) workflow_engine = workflows.get_engine() + RedisDriver.get_members = mock.MagicMock( + return_value=coordination_service.NoOpAsyncResult("member-1") + ) + workflow_engine._delay = 0 # Initiate start first eventlet.spawn(workflow_engine.start, True) eventlet.spawn_after(1, workflow_engine.shutdown) - RedisDriver.get_members = mock.MagicMock( - return_value=coordination_service.NoOpAsyncResult("member-1") - ) - lv_ac_db = lv_db_access.LiveAction.get_by_id(str(lv_ac_db.id)) # Startup routine acquires the lock first and shutdown routine sees a new member present in registry. diff --git a/st2common/tests/unit/services/test_workflow_service_retries.py b/st2common/tests/unit/services/test_workflow_service_retries.py index 5364043db7..4de49fcf3f 100644 --- a/st2common/tests/unit/services/test_workflow_service_retries.py +++ b/st2common/tests/unit/services/test_workflow_service_retries.py @@ -196,7 +196,6 @@ def test_retries_exhausted_from_coordinator_connection_error(self, mock_get_lock "update_task_state", mock.MagicMock( side_effect=[ - mongoengine.connection.ConnectionFailure(), mongoengine.connection.ConnectionFailure(), None, ] diff --git a/st2common/tests/unit/test_service_setup.py b/st2common/tests/unit/test_service_setup.py index 0fa413ca5d..4638be2f88 100644 --- a/st2common/tests/unit/test_service_setup.py +++ b/st2common/tests/unit/test_service_setup.py @@ -217,6 +217,7 @@ def test_deregister_service_when_service_registry_enabled(self): members = coordinator.get_members(service.encode("utf-8")) self.assertEqual(len(list(members.get())), 1) service_setup.deregister_service(service) + members = coordinator.get_members(service.encode("utf-8")) self.assertEqual(len(list(members.get())), 0) def test_deregister_service_when_service_registry_disables(self): diff --git a/st2tests/st2tests/config.py b/st2tests/st2tests/config.py index 87d0679355..33149d8d0c 100644 --- a/st2tests/st2tests/config.py +++ b/st2tests/st2tests/config.py @@ -145,15 +145,13 @@ def _override_coordinator_opts(noop=False): ST2_OVERRIDE_COORDINATOR_REDIS_PORT = os.environ.get("ST2_OVERRIDE_COORDINATOR_REDIS_PORT", "6379") driver=f"redis://{ST2_OVERRIDE_COORDINATOR_REDIS_HOST}:{ST2_OVERRIDE_COORDINATOR_REDIS_PORT}" - assert False - print(f"Redis is being used with the following cord: {driver}") CONF.set_override(name="url", override=driver, group="coordination") CONF.set_override(name="lock_timeout", override=1, group="coordination") def _override_workflow_engine_opts(): - cfg.CONF.set_override("retry_stop_max_msec", 500, group="workflow_engine") + cfg.CONF.set_override("retry_stop_max_msec", 200, group="workflow_engine") cfg.CONF.set_override("retry_wait_fixed_msec", 100, group="workflow_engine") cfg.CONF.set_override("retry_max_jitter_msec", 100, group="workflow_engine") cfg.CONF.set_override("gc_max_idle_sec", 1, group="workflow_engine") From 7f50e44221732f8ed5be3150faa30be4f9b82751 Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Tue, 10 Sep 2024 15:20:39 -0400 Subject: [PATCH 03/20] lint fixes --- st2actions/tests/unit/test_workflow_engine.py | 3 +-- st2tests/st2tests/config.py | 10 +++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/st2actions/tests/unit/test_workflow_engine.py b/st2actions/tests/unit/test_workflow_engine.py index 63848af3bc..eba108080b 100644 --- a/st2actions/tests/unit/test_workflow_engine.py +++ b/st2actions/tests/unit/test_workflow_engine.py @@ -215,7 +215,6 @@ def test_process_error_handling_has_error(self, mock_get_lock): wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) - lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) # Assert action execution is running. @@ -279,7 +278,7 @@ def test_workflow_engine_shutdown(self): cfg.CONF.set_override( name="still_active_check_interval", override=1, group="workflow_engine" ) - + wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) diff --git a/st2tests/st2tests/config.py b/st2tests/st2tests/config.py index 33149d8d0c..856ffbabf2 100644 --- a/st2tests/st2tests/config.py +++ b/st2tests/st2tests/config.py @@ -140,11 +140,15 @@ def _override_scheduler_opts(): def _override_coordinator_opts(noop=False): driver = None if noop else "zake://" - ST2_OVERRIDE_COORDINATOR_REDIS_HOST = os.environ.get("ST2_OVERRIDE_COORDINATOR_REDIS_HOST", False) + ST2_OVERRIDE_COORDINATOR_REDIS_HOST = os.environ.get( + "ST2_OVERRIDE_COORDINATOR_REDIS_HOST", False + ) if ST2_OVERRIDE_COORDINATOR_REDIS_HOST: - ST2_OVERRIDE_COORDINATOR_REDIS_PORT = os.environ.get("ST2_OVERRIDE_COORDINATOR_REDIS_PORT", "6379") - driver=f"redis://{ST2_OVERRIDE_COORDINATOR_REDIS_HOST}:{ST2_OVERRIDE_COORDINATOR_REDIS_PORT}" + ST2_OVERRIDE_COORDINATOR_REDIS_PORT = os.environ.get( + "ST2_OVERRIDE_COORDINATOR_REDIS_PORT", "6379" + ) + driver = f"redis://{ST2_OVERRIDE_COORDINATOR_REDIS_HOST}:{ST2_OVERRIDE_COORDINATOR_REDIS_PORT}" CONF.set_override(name="url", override=driver, group="coordination") CONF.set_override(name="lock_timeout", override=1, group="coordination") From 4ca0722d171c25488588e72f96e8e2d087649f3f Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Mon, 9 Sep 2024 15:43:45 -0400 Subject: [PATCH 04/20] Add redis vars to more Makefile targets --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 04ba496d30..26710744e5 100644 --- a/Makefile +++ b/Makefile @@ -854,6 +854,8 @@ endif echo "Running tests in" $$component; \ echo "-----------------------------------------------------------"; \ . $(VIRTUALENV_DIR)/bin/activate; \ + ST2_OVERRIDE_COORDINATOR_REDIS_HOST=$(ST2_OVERRIDE_COORDINATOR_REDIS_HOST) \ + ST2_OVERRIDE_COORDINATOR_REDIS_PORT=$(ST2_OVERRIDE_COORDINATOR_REDIS_PORT) \ COVERAGE_FILE=.coverage.unit.$$(echo $$component | tr '/' '.') \ nosetests $(NOSE_OPTS) -s -v $(NOSE_COVERAGE_FLAGS) \ $(NOSE_COVERAGE_PACKAGES) \ From c91491d50c5fef9700eeb0a35befc25f2d1a3492 Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Tue, 10 Sep 2024 14:55:58 -0400 Subject: [PATCH 05/20] enable redis --- .github/workflows/ci.yaml | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 12e0b61ff2..2e681dc649 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -474,18 +474,18 @@ jobs: # Used for the coordination backend for integration tests # NOTE: To speed things up, we only start redis for integration tests # where it's needed - # redis: - # # Docker Hub image - # image: redis - # # Set health checks to wait until redis has started - # options: >- - # --name "redis" - # --health-cmd "redis-cli ping" - # --health-interval 10s - # --health-timeout 5s - # --health-retries 5 - # ports: - # - 6379:6379/tcp + redis: + # Docker Hub image + image: redis + # Set health checks to wait until redis has started + options: >- + --name "redis" + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6379:6379/tcp env: TASK: '${{ matrix.task }}' From 668a90e2ea23494dcb718825117572175b5167ac Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Tue, 10 Sep 2024 15:03:27 -0400 Subject: [PATCH 06/20] Revert "enable redis" This reverts commit 820001d0a9daa7a7ca7e9788cbed1fbb179d76b2. --- .github/workflows/ci.yaml | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 2e681dc649..12e0b61ff2 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -474,18 +474,18 @@ jobs: # Used for the coordination backend for integration tests # NOTE: To speed things up, we only start redis for integration tests # where it's needed - redis: - # Docker Hub image - image: redis - # Set health checks to wait until redis has started - options: >- - --name "redis" - --health-cmd "redis-cli ping" - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 6379:6379/tcp + # redis: + # # Docker Hub image + # image: redis + # # Set health checks to wait until redis has started + # options: >- + # --name "redis" + # --health-cmd "redis-cli ping" + # --health-interval 10s + # --health-timeout 5s + # --health-retries 5 + # ports: + # - 6379:6379/tcp env: TASK: '${{ matrix.task }}' From f05f081d429103f1cf2935cb4276fdff21cc1341 Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Tue, 10 Sep 2024 15:32:32 -0400 Subject: [PATCH 07/20] add back redis --- .github/workflows/ci.yaml | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 12e0b61ff2..265038a54a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -287,6 +287,19 @@ jobs: image: mongo:4.4 ports: - 27017:27017 + redis-server: + # Docker Hub image + image: redis + # Set health checks to wait until redis has started + options: >- + --name "redis" + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6379:6379/tcp + rabbitmq: image: rabbitmq:3.8-management @@ -474,18 +487,18 @@ jobs: # Used for the coordination backend for integration tests # NOTE: To speed things up, we only start redis for integration tests # where it's needed - # redis: - # # Docker Hub image - # image: redis - # # Set health checks to wait until redis has started - # options: >- - # --name "redis" - # --health-cmd "redis-cli ping" - # --health-interval 10s - # --health-timeout 5s - # --health-retries 5 - # ports: - # - 6379:6379/tcp + redis-server: + # Docker Hub image + image: redis + # Set health checks to wait until redis has started + options: >- + --name "redis" + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6379:6379/tcp env: TASK: '${{ matrix.task }}' From e647d5621599860a0123c30eeafb05ead758042c Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Tue, 10 Sep 2024 15:42:52 -0400 Subject: [PATCH 08/20] already redis running integration job --- .github/workflows/ci.yaml | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 265038a54a..f5d1ad7592 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -487,18 +487,18 @@ jobs: # Used for the coordination backend for integration tests # NOTE: To speed things up, we only start redis for integration tests # where it's needed - redis-server: - # Docker Hub image - image: redis - # Set health checks to wait until redis has started - options: >- - --name "redis" - --health-cmd "redis-cli ping" - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 6379:6379/tcp + #redis-server: + # # Docker Hub image + # image: redis + # # Set health checks to wait until redis has started + # options: >- + # --name "redis" + # --health-cmd "redis-cli ping" + # --health-interval 10s + # --health-timeout 5s + # --health-retries 5 + # ports: + # - 6379:6379/tcp env: TASK: '${{ matrix.task }}' From 914b0d8caa09a15ffcc4c098d77b7ea4a1273ba4 Mon Sep 17 00:00:00 2001 From: guzzijones12 Date: Tue, 10 Sep 2024 15:46:44 -0400 Subject: [PATCH 09/20] remove bad comment in ci --- .github/workflows/ci.yaml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f5d1ad7592..c4223cd4ef 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -484,9 +484,6 @@ jobs: #- 4369:4369/tcp # epmd # - # Used for the coordination backend for integration tests - # NOTE: To speed things up, we only start redis for integration tests - # where it's needed #redis-server: # # Docker Hub image # image: redis From 009f64371ef7c0b22a97dffc570f477e711fc15d Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Tue, 17 Sep 2024 15:55:59 -0500 Subject: [PATCH 10/20] GHA: always run redis container instead of starting as-needed All tests (unit, integration, pack) need redis now. --- .github/workflows/ci.yaml | 54 ++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c4223cd4ef..35187cc7de 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -133,6 +133,18 @@ jobs: - 5672:5672/tcp # AMQP standard port - 15672:15672/tcp # Management: HTTP, CLI + redis: + # Docker Hub image + image: redis + # Set health checks to wait until redis has started + options: >- + --name "redis" + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6379:6379/tcp env: # CI st2.conf (with ST2_CI_USER user instead of stanley) ST2_CONF: 'conf/st2.ci.conf' @@ -163,11 +175,6 @@ jobs: - name: Install requirements run: | ./scripts/ci/install-requirements.sh - - name: Run Redis Service Container - timeout-minutes: 2 - run: | - docker run --rm --detach -p 127.0.0.1:6379:6379/tcp --name redis redis:latest - until [ "$(docker inspect -f {{.State.Running}} redis)" == "true" ]; do sleep 0.1; done - name: Setup Tests run: | # prep a ci-specific dev conf file that uses runner instead of stanley @@ -234,9 +241,6 @@ jobs: name: logs-py${{ matrix.python-version }} path: logs.tar.gz retention-days: 7 - - name: Stop Redis Service Container - if: "${{ always() }}" - run: docker rm --force redis || true unit-tests: needs: pre_job @@ -287,7 +291,7 @@ jobs: image: mongo:4.4 ports: - 27017:27017 - redis-server: + redis: # Docker Hub image image: redis # Set health checks to wait until redis has started @@ -484,18 +488,18 @@ jobs: #- 4369:4369/tcp # epmd # - #redis-server: - # # Docker Hub image - # image: redis - # # Set health checks to wait until redis has started - # options: >- - # --name "redis" - # --health-cmd "redis-cli ping" - # --health-interval 10s - # --health-timeout 5s - # --health-retries 5 - # ports: - # - 6379:6379/tcp + redis: + # Docker Hub image + image: redis + # Set health checks to wait until redis has started + options: >- + --name "redis" + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6379:6379/tcp env: TASK: '${{ matrix.task }}' @@ -548,11 +552,6 @@ jobs: cp conf/st2.dev.conf "${ST2_CONF}" ; sed -i -e "s/stanley/${ST2_CI_USER}/" "${ST2_CONF}" sudo -E ./scripts/ci/add-itest-user-key.sh - - name: Run Redis Service Container - timeout-minutes: 2 - run: | - docker run --rm --detach -p 127.0.0.1:6379:6379/tcp --name redis redis:latest - until [ "$(docker inspect -f {{.State.Running}} redis)" == "true" ]; do sleep 0.1; done - name: Permissions Workaround run: | echo "$ST2_CI_REPO_PATH" @@ -595,9 +594,6 @@ jobs: name: logs-py${{ matrix.python-version }}-nose-${{ matrix.nosetests_node_index }} path: logs.tar.gz retention-days: 7 - - name: Stop Redis Service Container - if: "${{ always() }}" - run: docker rm --force redis || true slack-notification: name: Slack notification for failed master builds From 6750d6833869932bd89b4b791f50d2435f3fc00c Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Tue, 17 Sep 2024 16:03:06 -0500 Subject: [PATCH 11/20] rename ST2_OVERRIDE_... env vars to follow ST2TESTS_* convention --- Makefile | 12 ++++++------ st2tests/st2tests/config.py | 13 ++++--------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index 26710744e5..9a46a71d4b 100644 --- a/Makefile +++ b/Makefile @@ -54,8 +54,8 @@ COVERAGE_GLOBS_QUOTED := $(foreach glob,$(COVERAGE_GLOBS),'$(glob)') REQUIREMENTS := test-requirements.txt requirements.txt # Redis config for testing -ST2_OVERRIDE_COORDINATOR_REDIS_HOST := 127.0.0.1 -ST2_OVERRIDE_COORDINATOR_REDIS_PORT := 6379 +ST2TESTS_REDIS_HOST := 127.0.0.1 +ST2TESTS_REDIS_PORT := 6379 # Pin common pip version here across all the targets # Note! Periodic maintenance pip upgrades are required to be up-to-date with the latest pip security fixes and updates @@ -828,8 +828,8 @@ unit-tests: requirements .unit-tests echo "Running tests in" $$component; \ echo "-----------------------------------------------------------"; \ . $(VIRTUALENV_DIR)/bin/activate; \ - ST2_OVERRIDE_COORDINATOR_REDIS_HOST=$(ST2_OVERRIDE_COORDINATOR_REDIS_HOST) \ - ST2_OVERRIDE_COORDINATOR_REDIS_PORT=$(ST2_OVERRIDE_COORDINATOR_REDIS_PORT) \ + ST2TESTS_REDIS_HOST=$(ST2TESTS_REDIS_HOST) \ + ST2TESTS_REDIS_PORT=$(ST2TESTS_REDIS_PORT) \ nosetests $(NOSE_OPTS) -s -v \ $$component/tests/unit || ((failed+=1)); \ echo "-----------------------------------------------------------"; \ @@ -854,8 +854,8 @@ endif echo "Running tests in" $$component; \ echo "-----------------------------------------------------------"; \ . $(VIRTUALENV_DIR)/bin/activate; \ - ST2_OVERRIDE_COORDINATOR_REDIS_HOST=$(ST2_OVERRIDE_COORDINATOR_REDIS_HOST) \ - ST2_OVERRIDE_COORDINATOR_REDIS_PORT=$(ST2_OVERRIDE_COORDINATOR_REDIS_PORT) \ + ST2TESTS_REDIS_HOST=$(ST2TESTS_REDIS_HOST) \ + ST2TESTS_REDIS_PORT=$(ST2TESTS_REDIS_PORT) \ COVERAGE_FILE=.coverage.unit.$$(echo $$component | tr '/' '.') \ nosetests $(NOSE_OPTS) -s -v $(NOSE_COVERAGE_FLAGS) \ $(NOSE_COVERAGE_PACKAGES) \ diff --git a/st2tests/st2tests/config.py b/st2tests/st2tests/config.py index 856ffbabf2..40c82151e9 100644 --- a/st2tests/st2tests/config.py +++ b/st2tests/st2tests/config.py @@ -140,15 +140,10 @@ def _override_scheduler_opts(): def _override_coordinator_opts(noop=False): driver = None if noop else "zake://" - ST2_OVERRIDE_COORDINATOR_REDIS_HOST = os.environ.get( - "ST2_OVERRIDE_COORDINATOR_REDIS_HOST", False - ) - if ST2_OVERRIDE_COORDINATOR_REDIS_HOST: - - ST2_OVERRIDE_COORDINATOR_REDIS_PORT = os.environ.get( - "ST2_OVERRIDE_COORDINATOR_REDIS_PORT", "6379" - ) - driver = f"redis://{ST2_OVERRIDE_COORDINATOR_REDIS_HOST}:{ST2_OVERRIDE_COORDINATOR_REDIS_PORT}" + redis_host = os.environ.get("ST2TESTS_REDIS_HOST", False) + if redis_host: + redis_port = os.environ.get("ST2TESTS_REDIS_PORT", "6379") + driver = f"redis://{redis_host}:{redis_port}" CONF.set_override(name="url", override=driver, group="coordination") CONF.set_override(name="lock_timeout", override=1, group="coordination") From 70fa0ba788555288ae65b3ebaa67826658594e0a Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Tue, 17 Sep 2024 17:06:56 -0500 Subject: [PATCH 12/20] comment out parse_args tests_config.parse_args is called when importing st2tests and again in setUpClass. if this is needed, try self.reset() --- st2actions/tests/unit/test_workflow_engine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2actions/tests/unit/test_workflow_engine.py b/st2actions/tests/unit/test_workflow_engine.py index eba108080b..d98a549265 100644 --- a/st2actions/tests/unit/test_workflow_engine.py +++ b/st2actions/tests/unit/test_workflow_engine.py @@ -145,7 +145,7 @@ def test_process(self): @mock.patch.object(RedisDriver, "get_lock") def test_process_error_handling(self, mock_get_lock): - tests_config.parse_args() + # tests_config.parse_args() # maybe call self.reset() cfg.CONF.set_override( name="service_registry", override=True, group="coordination" ) From 3944b784362aef360185f06bac02692a8acc4990 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 12:13:18 -0500 Subject: [PATCH 13/20] reset config between actionrunner worker tests --- st2actions/tests/unit/test_worker.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/st2actions/tests/unit/test_worker.py b/st2actions/tests/unit/test_worker.py index 3a6a7fae3c..89d4bb728a 100644 --- a/st2actions/tests/unit/test_worker.py +++ b/st2actions/tests/unit/test_worker.py @@ -27,6 +27,7 @@ from st2tests.base import DbTestCase import st2actions.worker as actions_worker +import st2tests.config as tests_config from st2common.constants import action as action_constants from st2common.models.db.liveaction import LiveActionDB from st2common.models.system.common import ResourceReference @@ -67,6 +68,11 @@ def setUpClass(cls): ) WorkerTestCase.local_action_db = models["actions"]["local.yaml"] + @staticmethod + def reparse_config(): + tests_config.reset() + tests_config.parse_args() + def _get_liveaction_model(self, action_db, params): status = action_constants.LIVEACTION_STATUS_REQUESTED start_timestamp = date_utils.get_datetime_utc_now() @@ -117,6 +123,7 @@ def test_non_utf8_action_result_string(self): ) def test_worker_shutdown(self): + self.reparse_config() cfg.CONF.set_override( name="graceful_shutdown", override=False, group="actionrunner" ) @@ -175,6 +182,7 @@ def test_worker_shutdown(self): mock.MagicMock(return_value=coordination.NoOpAsyncResult("member-1")), ) def test_worker_graceful_shutdown_with_multiple_runners(self): + self.reparse_config() cfg.CONF.set_override( name="graceful_shutdown", override=True, group="actionrunner" ) @@ -244,6 +252,7 @@ def test_worker_graceful_shutdown_with_multiple_runners(self): shutdown_thread.kill() def test_worker_graceful_shutdown_with_single_runner(self): + self.reparse_config() cfg.CONF.set_override( name="graceful_shutdown", override=True, group="actionrunner" ) @@ -321,6 +330,7 @@ def test_worker_graceful_shutdown_with_single_runner(self): mock.MagicMock(return_value=coordination.NoOpAsyncResult("member-1")), ) def test_worker_graceful_shutdown_exit_timeout(self): + self.reparse_config() cfg.CONF.set_override( name="graceful_shutdown", override=True, group="actionrunner" ) From 7d1b1f4481d18b60e0f5a4250f10e3756574f7cc Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 12:35:33 -0500 Subject: [PATCH 14/20] DRY config setup in test_worker --- st2actions/tests/unit/test_worker.py | 74 ++++++++++++++-------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/st2actions/tests/unit/test_worker.py b/st2actions/tests/unit/test_worker.py index 89d4bb728a..3b986cf9bd 100644 --- a/st2actions/tests/unit/test_worker.py +++ b/st2actions/tests/unit/test_worker.py @@ -69,9 +69,33 @@ def setUpClass(cls): WorkerTestCase.local_action_db = models["actions"]["local.yaml"] @staticmethod - def reparse_config(): + def reset_config( + graceful_shutdown=True, # default is True (st2common.config) + exit_still_active_check=None, # default is 300 (st2common.config) + still_active_check_interval=None, # default is 2 (st2common.config) + service_registry=None, # default is False (st2common.config) + ): tests_config.reset() tests_config.parse_args() + cfg.CONF.set_override( + name="graceful_shutdown", override=graceful_shutdown, group="actionrunner" + ) + if exit_still_active_check is not None: + cfg.CONF.set_override( + name="exit_still_active_check", + override=exit_still_active_check, + group="actionrunner", + ) + if still_active_check_interval is not None: + cfg.CONF.set_override( + name="still_active_check_interval", + override=still_active_check_interval, + group="actionrunner", + ) + if service_registry is not None: + cfg.CONF.set_override( + name="service_registry", override=service_registry, group="coordination" + ) def _get_liveaction_model(self, action_db, params): status = action_constants.LIVEACTION_STATUS_REQUESTED @@ -123,10 +147,8 @@ def test_non_utf8_action_result_string(self): ) def test_worker_shutdown(self): - self.reparse_config() - cfg.CONF.set_override( - name="graceful_shutdown", override=False, group="actionrunner" - ) + self.reset_config(graceful_shutdown=False) + action_worker = actions_worker.get_worker() temp_file = None @@ -182,19 +204,12 @@ def test_worker_shutdown(self): mock.MagicMock(return_value=coordination.NoOpAsyncResult("member-1")), ) def test_worker_graceful_shutdown_with_multiple_runners(self): - self.reparse_config() - cfg.CONF.set_override( - name="graceful_shutdown", override=True, group="actionrunner" - ) - cfg.CONF.set_override( - name="service_registry", override=True, group="coordination" - ) - cfg.CONF.set_override( - name="exit_still_active_check", override=10, group="actionrunner" - ) - cfg.CONF.set_override( - name="still_active_check_interval", override=1, group="actionrunner" + self.reset_config( + exit_still_active_check=10, + still_active_check_interval=1, + service_registry=True, ) + action_worker = actions_worker.get_worker() temp_file = None @@ -252,18 +267,10 @@ def test_worker_graceful_shutdown_with_multiple_runners(self): shutdown_thread.kill() def test_worker_graceful_shutdown_with_single_runner(self): - self.reparse_config() - cfg.CONF.set_override( - name="graceful_shutdown", override=True, group="actionrunner" - ) - cfg.CONF.set_override( - name="service_registry", override=True, group="coordination" - ) - cfg.CONF.set_override( - name="exit_still_active_check", override=10, group="actionrunner" - ) - cfg.CONF.set_override( - name="still_active_check_interval", override=1, group="actionrunner" + self.reset_config( + exit_still_active_check=10, + still_active_check_interval=1, + service_registry=True, ) action_worker = actions_worker.get_worker() @@ -330,13 +337,8 @@ def test_worker_graceful_shutdown_with_single_runner(self): mock.MagicMock(return_value=coordination.NoOpAsyncResult("member-1")), ) def test_worker_graceful_shutdown_exit_timeout(self): - self.reparse_config() - cfg.CONF.set_override( - name="graceful_shutdown", override=True, group="actionrunner" - ) - cfg.CONF.set_override( - name="exit_still_active_check", override=5, group="actionrunner" - ) + self.reset_config(exit_still_active_check=5) + action_worker = actions_worker.get_worker() temp_file = None From 869818cf63238a7b228a76f1a542ed372aedb68c Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 14:11:46 -0500 Subject: [PATCH 15/20] coord.get_members returns NoOpAsyncResult(iterable) get_members should return a list or tuple, not a string. I noticed while debugging that get_members output ended up as ['m', 'e', 'm', 'b', 'e', 'r', '-', '1'] because it listified the string. So, use a tuple when creating the mocked NoOpAsyncResult so it is closer to the actual return values. --- st2actions/tests/unit/test_worker.py | 4 ++-- st2actions/tests/unit/test_workflow_engine.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/st2actions/tests/unit/test_worker.py b/st2actions/tests/unit/test_worker.py index 3b986cf9bd..b9891e4f21 100644 --- a/st2actions/tests/unit/test_worker.py +++ b/st2actions/tests/unit/test_worker.py @@ -201,7 +201,7 @@ def test_worker_shutdown(self): @mock.patch.object( RedisDriver, "get_members", - mock.MagicMock(return_value=coordination.NoOpAsyncResult("member-1")), + mock.MagicMock(return_value=coordination.NoOpAsyncResult(("member-1", "member-2"))), ) def test_worker_graceful_shutdown_with_multiple_runners(self): self.reset_config( @@ -334,7 +334,7 @@ def test_worker_graceful_shutdown_with_single_runner(self): @mock.patch.object( coordination.NoOpDriver, "get_members", - mock.MagicMock(return_value=coordination.NoOpAsyncResult("member-1")), + mock.MagicMock(return_value=coordination.NoOpAsyncResult(("member-1",))), ) def test_worker_graceful_shutdown_exit_timeout(self): self.reset_config(exit_still_active_check=5) diff --git a/st2actions/tests/unit/test_workflow_engine.py b/st2actions/tests/unit/test_workflow_engine.py index d98a549265..a50eee2538 100644 --- a/st2actions/tests/unit/test_workflow_engine.py +++ b/st2actions/tests/unit/test_workflow_engine.py @@ -334,7 +334,7 @@ def test_workflow_engine_shutdown(self): @mock.patch.object( RedisDriver, "get_members", - mock.MagicMock(return_value=coordination_service.NoOpAsyncResult("member-1")), + mock.MagicMock(return_value=coordination_service.NoOpAsyncResult(("member-1",))), ) def test_workflow_engine_shutdown_with_multiple_members(self): cfg.CONF.set_override( @@ -488,7 +488,7 @@ def test_workflow_engine_start_first_then_shutdown(self): workflow_engine = workflows.get_engine() RedisDriver.get_members = mock.MagicMock( - return_value=coordination_service.NoOpAsyncResult("member-1") + return_value=coordination_service.NoOpAsyncResult(("member-1",)) ) workflow_engine._delay = 0 From 847907fe2274015c8750f09200c3c8a79209723a Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 17:29:36 -0500 Subject: [PATCH 16/20] tests_config.parse_args is required. ugh --- st2actions/tests/unit/test_policies.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/st2actions/tests/unit/test_policies.py b/st2actions/tests/unit/test_policies.py index a2c828b39b..09a401e6b0 100644 --- a/st2actions/tests/unit/test_policies.py +++ b/st2actions/tests/unit/test_policies.py @@ -28,6 +28,7 @@ from st2common.transport.publishers import CUDPublisher from st2common.bootstrap import runnersregistrar as runners_registrar from st2tests import ExecutionDbTestCase +from st2tests import config as tests_config from st2tests.fixtures.generic.fixture import PACK_NAME as PACK from st2tests.fixturesloader import FixturesLoader from st2tests.mocks.runners import runner @@ -36,6 +37,8 @@ from st2tests.policies.concurrency import FakeConcurrencyApplicator from st2tests.policies.mock_exception import RaiseExceptionApplicator +# This needs to run before creating FakeConcurrencyApplicator below. +tests_config.parse_args() TEST_FIXTURES = { "actions": ["action1.yaml"], From c35d4463033c036b4206bbec136052769579374f Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 17:33:38 -0500 Subject: [PATCH 17/20] Use RedisDriver instead of NoOpDriver --- st2actions/tests/unit/test_worker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2actions/tests/unit/test_worker.py b/st2actions/tests/unit/test_worker.py index b9891e4f21..f3c488f478 100644 --- a/st2actions/tests/unit/test_worker.py +++ b/st2actions/tests/unit/test_worker.py @@ -332,7 +332,7 @@ def test_worker_graceful_shutdown_with_single_runner(self): shutdown_thread.kill() @mock.patch.object( - coordination.NoOpDriver, + RedisDriver, "get_members", mock.MagicMock(return_value=coordination.NoOpAsyncResult(("member-1",))), ) From 223c7a683ebb4d296f095b17f6ff334acf30b9ef Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 22:19:15 -0500 Subject: [PATCH 18/20] DRY config setup in test_workflow_engine --- st2actions/tests/unit/test_workflow_engine.py | 83 +++++++++++-------- 1 file changed, 50 insertions(+), 33 deletions(-) diff --git a/st2actions/tests/unit/test_workflow_engine.py b/st2actions/tests/unit/test_workflow_engine.py index a50eee2538..f747f6c333 100644 --- a/st2actions/tests/unit/test_workflow_engine.py +++ b/st2actions/tests/unit/test_workflow_engine.py @@ -26,6 +26,7 @@ from tooz import coordination from tooz.drivers.redis import RedisDriver +import st2tests.config as tests_config from st2actions.workflows import workflows from st2common.bootstrap import actionsregistrar from st2common.bootstrap import runnersregistrar @@ -92,7 +93,39 @@ def setUpClass(cls): for pack in PACKS: actions_registrar.register_from_pack(pack) + @staticmethod + def reset_config( + graceful_shutdown=None, # default is True (st2common.config) + exit_still_active_check=None, # default is 300 (st2common.config) + still_active_check_interval=None, # default is 2 (st2common.config) + service_registry=None, # default is False (st2common.config) + ): + tests_config.reset() + tests_config.parse_args() + if graceful_shutdown is not None: + cfg.CONF.set_override( + name="graceful_shutdown", override=graceful_shutdown, group="actionrunner" + ) + if exit_still_active_check is not None: + cfg.CONF.set_override( + name="exit_still_active_check", + override=exit_still_active_check, + group="workflow_engine", + ) + if still_active_check_interval is not None: + cfg.CONF.set_override( + name="still_active_check_interval", + override=still_active_check_interval, + group="workflow_engine", + ) + if service_registry is not None: + cfg.CONF.set_override( + name="service_registry", override=service_registry, group="coordination" + ) + def test_process(self): + self.reset_config() + wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) @@ -145,10 +178,8 @@ def test_process(self): @mock.patch.object(RedisDriver, "get_lock") def test_process_error_handling(self, mock_get_lock): - # tests_config.parse_args() # maybe call self.reset() - cfg.CONF.set_override( - name="service_registry", override=True, group="coordination" - ) + self.reset_config(service_registry=True) + expected_errors = [ { "message": "Execution failed. See result for details.", @@ -211,6 +242,8 @@ def test_process_error_handling(self, mock_get_lock): mock.MagicMock(side_effect=Exception("Unexpected error.")), ) def test_process_error_handling_has_error(self, mock_get_lock): + self.reset_config() + mock_get_lock.side_effect = coordination_service.NoOpLock(name="noop") wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) @@ -266,17 +299,11 @@ def test_process_error_handling_has_error(self, mock_get_lock): self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_CANCELED) def test_workflow_engine_shutdown(self): - cfg.CONF.set_override( - name="graceful_shutdown", override=True, group="actionrunner" - ) - cfg.CONF.set_override( - name="service_registry", override=True, group="coordination" - ) - cfg.CONF.set_override( - name="exit_still_active_check", override=4, group="workflow_engine" - ) - cfg.CONF.set_override( - name="still_active_check_interval", override=1, group="workflow_engine" + self.reset_config( + graceful_shutdown=True, + exit_still_active_check=4, + still_active_check_interval=1, + service_registry=True, ) wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") @@ -337,9 +364,8 @@ def test_workflow_engine_shutdown(self): mock.MagicMock(return_value=coordination_service.NoOpAsyncResult(("member-1",))), ) def test_workflow_engine_shutdown_with_multiple_members(self): - cfg.CONF.set_override( - name="service_registry", override=True, group="coordination" - ) + self.reset_config(service_registry=True) + wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) @@ -380,9 +406,8 @@ def test_workflow_engine_shutdown_with_multiple_members(self): self.assertEqual(lv_ac_db.status, action_constants.LIVEACTION_STATUS_RUNNING) def test_workflow_engine_shutdown_with_service_registry_disabled(self): - cfg.CONF.set_override( - name="service_registry", override=False, group="coordination" - ) + self.reset_config(service_registry=False) + wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) @@ -411,12 +436,8 @@ def test_workflow_engine_shutdown_with_service_registry_disabled(self): mock.MagicMock(return_value=coordination_service.NoOpLock(name="noop")), ) def test_workflow_engine_shutdown_first_then_start(self): - cfg.CONF.set_override( - name="service_registry", override=True, group="coordination" - ) - cfg.CONF.set_override( - name="exit_still_active_check", override=0, group="workflow_engine" - ) + self.reset_config(service_registry=True, exit_still_active_check=0) + wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) @@ -468,12 +489,8 @@ def test_workflow_engine_shutdown_first_then_start(self): mock.MagicMock(return_value=coordination_service.NoOpLock(name="noop")), ) def test_workflow_engine_start_first_then_shutdown(self): - cfg.CONF.set_override( - name="service_registry", override=True, group="coordination" - ) - cfg.CONF.set_override( - name="exit_still_active_check", override=0, group="workflow_engine" - ) + self.reset_config(service_registry=True, exit_still_active_check=0) + wf_meta = self.get_wf_fixture_meta_data(TEST_PACK_PATH, "sequential.yaml") lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta["name"]) lv_ac_db, ac_ex_db = action_service.request(lv_ac_db) From 78c3248bf8c887add3b5bf569983f2c6f4421c6e Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 22:43:21 -0500 Subject: [PATCH 19/20] fmt --- st2actions/tests/unit/test_worker.py | 4 +++- st2actions/tests/unit/test_workflow_engine.py | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/st2actions/tests/unit/test_worker.py b/st2actions/tests/unit/test_worker.py index f3c488f478..917d0683e1 100644 --- a/st2actions/tests/unit/test_worker.py +++ b/st2actions/tests/unit/test_worker.py @@ -201,7 +201,9 @@ def test_worker_shutdown(self): @mock.patch.object( RedisDriver, "get_members", - mock.MagicMock(return_value=coordination.NoOpAsyncResult(("member-1", "member-2"))), + mock.MagicMock( + return_value=coordination.NoOpAsyncResult(("member-1", "member-2")) + ), ) def test_worker_graceful_shutdown_with_multiple_runners(self): self.reset_config( diff --git a/st2actions/tests/unit/test_workflow_engine.py b/st2actions/tests/unit/test_workflow_engine.py index f747f6c333..ed6440bd57 100644 --- a/st2actions/tests/unit/test_workflow_engine.py +++ b/st2actions/tests/unit/test_workflow_engine.py @@ -104,7 +104,9 @@ def reset_config( tests_config.parse_args() if graceful_shutdown is not None: cfg.CONF.set_override( - name="graceful_shutdown", override=graceful_shutdown, group="actionrunner" + name="graceful_shutdown", + override=graceful_shutdown, + group="actionrunner", ) if exit_still_active_check is not None: cfg.CONF.set_override( @@ -361,7 +363,9 @@ def test_workflow_engine_shutdown(self): @mock.patch.object( RedisDriver, "get_members", - mock.MagicMock(return_value=coordination_service.NoOpAsyncResult(("member-1",))), + mock.MagicMock( + return_value=coordination_service.NoOpAsyncResult(("member-1",)) + ), ) def test_workflow_engine_shutdown_with_multiple_members(self): self.reset_config(service_registry=True) From 774f45bb8805e9368a6e8571a6fb51b88b5e2043 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 18 Sep 2024 23:52:19 -0500 Subject: [PATCH 20/20] add changelog entry --- CHANGELOG.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4b4dd3961a..43f654af7c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -28,6 +28,9 @@ Changed * Update st2client deps: editor and prompt-toolkit. #6189 (by @nzlosh) * Updated dependency oslo.config to prepare for python 3.10 support. #6193 (by @nzlosh) +* Updated unit tests to use redis for coordination instead of the NoOp driver. This will hopefully make CI more stable. #6245 + Contributed by @FileMagic, @guzzijones, and @cognifloyd + Added ~~~~~ * Continue introducing `pants `_ to improve DX (Developer Experience)