From 78fb27567d0b39fe8873be935c916f88ff7f43f6 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 13:34:45 +0100 Subject: [PATCH 01/14] Configure redis coordination backend for orquesta integration tests to see if it helps with race conditions. --- .github/workflows/ci.yaml | 43 +++++++++++++++++++-------- scripts/github/prepare-integration.sh | 4 +++ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8347f8a860..74b4ea991f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -20,6 +20,9 @@ on: # run every night at midnight - cron: '0 0 * * *' +# TODO: Our workflow is far from ideal. We need to refactor it into multiple +# ones and only run commands which are needed for some steps for those steps and +# not for all jobs: ci: name: '${{ matrix.name }} - python (${{ matrix.python-version }})' @@ -28,18 +31,18 @@ jobs: fail-fast: false matrix: include: - - name: 'Lint Checks' - task: 'ci-checks' - python-version: '3.6' - - name: 'Compile' - task: 'ci-compile' - python-version: '3.6' - - name: 'Pack Tests' - task: 'ci-packs-tests' - python-version: '3.6' - - name: 'Unit Tests' - task: 'ci-unit' - python-version: '3.6' + # - name: 'Lint Checks' + # task: 'ci-checks' + # python-version: '3.6' + # - name: 'Compile' + # task: 'ci-compile' + # python-version: '3.6' + # - name: 'Pack Tests' + # task: 'ci-packs-tests' + # python-version: '3.6' + # - name: 'Unit Tests' + # task: 'ci-unit' + # python-version: '3.6' # This job is slow so we only run in on a daily basis # - name: 'Micro Benchmarks' # task: 'micro-benchmarks' @@ -82,6 +85,22 @@ jobs: #- 15671:15671/tcp # Management: SSL port #- 25672:25672/tcp # inter-node or CLI #- 4369:4369/tcp # epmd + # + + # Used for the coordination backend for integration tests + # TODO: Only start this for integration tests via job step + # https://github.community/t/conditional-services-in-a-job/135301/3 + redis: + # Docker Hub image + image: redis + # Set health checks to wait until redis has started + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6379:6379/tcp env: TASK: '${{ matrix.task }}' diff --git a/scripts/github/prepare-integration.sh b/scripts/github/prepare-integration.sh index 767a4999b7..dc1b073123 100755 --- a/scripts/github/prepare-integration.sh +++ b/scripts/github/prepare-integration.sh @@ -10,6 +10,10 @@ fi # shellcheck disable=SC1091 source ./virtualenv/bin/activate +# Enable coordination backend to avoid race conditions with orquesta tests due +# to the lack of the coordination backend +sed -i "s#\#url = redis://localhost#url = redis://127.0.0.1#g" conf/st2.dev.conf + # install st2 client python ./st2client/setup.py develop st2 --version From 97297d32217b018c32c7845bd66ac379cb6c0dd1 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 13:50:50 +0100 Subject: [PATCH 02/14] Add skip duplicate jobs pre-job to avoid duplicate and unncessary GHA workflow runs. --- .github/workflows/ci.yaml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 74b4ea991f..5130d2aa41 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -24,7 +24,20 @@ on: # ones and only run commands which are needed for some steps for those steps and # not for all jobs: + pre_job: + name: Skip Duplicate Jobs Pre Job + runs-on: ubuntu-latest + outputs: + should_skip: ${{ steps.skip_check.outputs.should_skip }} + steps: + - id: skip_check + uses: fkirc/skip-duplicate-actions@4c656bbdb6906310fa6213604828008bc28fe55d # v3.3.0 + with: + github_token: ${{ github.token }} + ci: + needs: pre_job + if: ${{ needs.pre_job.outputs.should_skip != 'true' }} name: '${{ matrix.name }} - python (${{ matrix.python-version }})' runs-on: ubuntu-latest strategy: From ca9ffbbac6a918f16e7919a736e5ae36723c7bc0 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 13:59:56 +0100 Subject: [PATCH 03/14] Add additional logging. --- scripts/github/prepare-integration.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/github/prepare-integration.sh b/scripts/github/prepare-integration.sh index dc1b073123..e579643043 100755 --- a/scripts/github/prepare-integration.sh +++ b/scripts/github/prepare-integration.sh @@ -12,7 +12,12 @@ source ./virtualenv/bin/activate # Enable coordination backend to avoid race conditions with orquesta tests due # to the lack of the coordination backend -sed -i "s#\#url = redis://localhost#url = redis://127.0.0.1#g" conf/st2.dev.conf +sed -i "s#\#url = redis://localhost#url = redis://127.0.0.1#g" ./conf/st2.dev.conf + +echo "Used config for the tests" +echo "" +cat conf/st2.dev.conf +echo "" # install st2 client python ./st2client/setup.py develop From 963707f709bf71915f3ca308c6a114736539f958 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 14:01:04 +0100 Subject: [PATCH 04/14] Make sure redis lib is installed. --- scripts/github/prepare-integration.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/github/prepare-integration.sh b/scripts/github/prepare-integration.sh index e579643043..fd5340bf40 100755 --- a/scripts/github/prepare-integration.sh +++ b/scripts/github/prepare-integration.sh @@ -19,6 +19,9 @@ echo "" cat conf/st2.dev.conf echo "" +# Needed by the coordination backend +pip install "redis==3.5.3" + # install st2 client python ./st2client/setup.py develop st2 --version From 31416086959d74973178b87f4658365187e94f5e Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 15:57:17 +0100 Subject: [PATCH 05/14] Also need to set url in the ci conf. --- scripts/github/prepare-integration.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/github/prepare-integration.sh b/scripts/github/prepare-integration.sh index fd5340bf40..fe04b6c518 100755 --- a/scripts/github/prepare-integration.sh +++ b/scripts/github/prepare-integration.sh @@ -13,6 +13,7 @@ source ./virtualenv/bin/activate # Enable coordination backend to avoid race conditions with orquesta tests due # to the lack of the coordination backend sed -i "s#\#url = redis://localhost#url = redis://127.0.0.1#g" ./conf/st2.dev.conf +sed -i "s#\#url = redis://localhost#url = redis://127.0.0.1#g" ./conf/st2.ci.conf || true echo "Used config for the tests" echo "" From 984633a53d83be38f972ae511bf0317835264139 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 15:57:59 +0100 Subject: [PATCH 06/14] Try only running it for integration tests. --- .github/workflows/ci.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 5130d2aa41..d2d3ed41dc 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -223,7 +223,8 @@ jobs: - name: Install requirements run: | ./scripts/ci/install-requirements.sh - - name: Setup integration tests + - name: Setup Integration Tests + if: "${{ env.TASK == 'ci-packs-tests' || env.TASK == 'ci-integration' }}" run: | # prep a ci-specific dev conf file that uses runner instead of stanley # this user is the username of the user in GitHub actions, used for SSH, etc during From f9f0c63c0e8e27537409bf4fcf5afddf10d72585 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 16:05:58 +0100 Subject: [PATCH 07/14] Give container a friendly name. --- .github/workflows/ci.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d2d3ed41dc..051ec408c3 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -108,6 +108,7 @@ jobs: 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 From 956bec24273057442b82180de992c50287fed033 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 16:07:28 +0100 Subject: [PATCH 08/14] Also print used metrics driver + coordination backend on service start up to make troubleshooting easier. --- st2common/st2common/service_setup.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/st2common/st2common/service_setup.py b/st2common/st2common/service_setup.py index 03f9bcdfa7..405bde515e 100644 --- a/st2common/st2common/service_setup.py +++ b/st2common/st2common/service_setup.py @@ -147,6 +147,9 @@ def setup( LOG.info("Using logging config: %s", logging_config_path) + LOG.info("Using coordination url: %s", cfg.CONF.coordination.url) + LOG.info("Using metrics driver: %s", cfg.CONF.metrics.driver) + is_debug_enabled = cfg.CONF.debug or cfg.CONF.system.debug try: From 565b136230dbddd853d1158b3fc25ecc8c5fcac8 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 16:14:21 +0100 Subject: [PATCH 09/14] Run the same task multiple times on CI to see if coordination backend indeed helps. --- .github/workflows/ci.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 051ec408c3..f9ea4fa0b6 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -259,7 +259,11 @@ jobs: - name: make # use: script -e -c to print colors run: | - script -e -c "make ${TASK}" + for (( i=1; i <= 6; i++ )) + do + echo "Test run: $i" + script -e -c "make ${TASK}" + done - name: Nightly # Run any additional nightly checks only as part of a nightly (cron) build if: "${{ env.IS_NIGHTLY_BUILD == 'yes' }}" From 540c1c3d6ac6bfd3b1f0106382cf76857f3eaad3 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 16:21:50 +0100 Subject: [PATCH 10/14] Update affected tests. --- .../test_service_setup_log_level_filtering.py | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/st2common/tests/integration/test_service_setup_log_level_filtering.py b/st2common/tests/integration/test_service_setup_log_level_filtering.py index 67479fc8e3..822140e61b 100644 --- a/st2common/tests/integration/test_service_setup_log_level_filtering.py +++ b/st2common/tests/integration/test_service_setup_log_level_filtering.py @@ -74,11 +74,13 @@ def test_audit_log_level_is_filtered_if_log_level_is_not_debug_or_audit(self): process.send_signal(signal.SIGKILL) # Verify first 4 environment related log messages - stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[:4]) + stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[:6]) self.assertIn("INFO [-] Using Python:", stdout) self.assertIn("INFO [-] Using fs encoding:", stdout) self.assertIn("INFO [-] Using config files:", stdout) self.assertIn("INFO [-] Using logging config:", stdout) + self.assertIn("INFO [-] Using coordination url:", stdout) + self.assertIn("INFO [-] Using metrics driver:", stdout) # 1. INFO log level - audit messages should not be included process = self._start_process(config_path=ST2_CONFIG_INFO_LL_PATH) @@ -88,8 +90,8 @@ def test_audit_log_level_is_filtered_if_log_level_is_not_debug_or_audit(self): eventlet.sleep(3) process.send_signal(signal.SIGKILL) - # First 4 log lines are debug messages about the environment which are always logged - stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[4:]) + # First 6 log lines are debug messages about the environment which are always logged + stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[6:]) self.assertIn("INFO [-]", stdout) self.assertNotIn("DEBUG [-]", stdout) @@ -103,8 +105,8 @@ def test_audit_log_level_is_filtered_if_log_level_is_not_debug_or_audit(self): eventlet.sleep(5) process.send_signal(signal.SIGKILL) - # First 4 log lines are debug messages about the environment which are always logged - stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[4:]) + # First 6 log lines are debug messages about the environment which are always logged + stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[6:]) self.assertIn("INFO [-]", stdout) self.assertIn("DEBUG [-]", stdout) @@ -118,8 +120,8 @@ def test_audit_log_level_is_filtered_if_log_level_is_not_debug_or_audit(self): eventlet.sleep(5) process.send_signal(signal.SIGKILL) - # First 4 log lines are debug messages about the environment which are always logged - stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[4:]) + # First 6 log lines are debug messages about the environment which are always logged + stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[6:]) self.assertNotIn("INFO [-]", stdout) self.assertNotIn("DEBUG [-]", stdout) @@ -133,8 +135,8 @@ def test_audit_log_level_is_filtered_if_log_level_is_not_debug_or_audit(self): eventlet.sleep(5) process.send_signal(signal.SIGKILL) - # First 4 log lines are debug messages about the environment which are always logged - stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[4:]) + # First 6 log lines are debug messages about the environment which are always logged + stdout = "\n".join(process.stdout.read().decode("utf-8").split("\n")[6:]) self.assertIn("INFO [-]", stdout) self.assertIn("DEBUG [-]", stdout) From 32ffc5963aabe41d00df9ad93d6fabb2d189cd12 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 18:04:05 +0100 Subject: [PATCH 11/14] Revert testing changes. --- .github/workflows/ci.yaml | 36 ++++++++++++--------------- scripts/github/prepare-integration.sh | 6 +++++ 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f9ea4fa0b6..1fe0aa15d7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -44,25 +44,25 @@ jobs: fail-fast: false matrix: include: - # - name: 'Lint Checks' - # task: 'ci-checks' - # python-version: '3.6' - # - name: 'Compile' - # task: 'ci-compile' - # python-version: '3.6' - # - name: 'Pack Tests' - # task: 'ci-packs-tests' - # python-version: '3.6' - # - name: 'Unit Tests' - # task: 'ci-unit' - # python-version: '3.6' + - name: 'Lint Checks' + task: 'ci-checks' + python-version: '3.6' + - name: 'Compile' + task: 'ci-compile' + python-version: '3.6' + - name: 'Pack Tests' + task: 'ci-packs-tests' + python-version: '3.6' + - name: 'Unit Tests' + task: 'ci-unit' + python-version: '3.6' + - name: 'Integration Tests' + task: 'ci-integration' + python-version: '3.6' # This job is slow so we only run in on a daily basis # - name: 'Micro Benchmarks' # task: 'micro-benchmarks' # python-version: '3.6' - - name: 'Integration Tests' - task: 'ci-integration' - python-version: '3.6' services: mongo: image: mongo:4.0 @@ -259,11 +259,7 @@ jobs: - name: make # use: script -e -c to print colors run: | - for (( i=1; i <= 6; i++ )) - do - echo "Test run: $i" - script -e -c "make ${TASK}" - done + script -e -c "make ${TASK}" - name: Nightly # Run any additional nightly checks only as part of a nightly (cron) build if: "${{ env.IS_NIGHTLY_BUILD == 'yes' }}" diff --git a/scripts/github/prepare-integration.sh b/scripts/github/prepare-integration.sh index fe04b6c518..35e138c8ea 100755 --- a/scripts/github/prepare-integration.sh +++ b/scripts/github/prepare-integration.sh @@ -17,8 +17,14 @@ sed -i "s#\#url = redis://localhost#url = redis://127.0.0.1#g" ./conf/st2.ci.con echo "Used config for the tests" echo "" +echo "st2.dev.conf" +echo "" cat conf/st2.dev.conf echo "" +echo "st2.ci.conf" +echo "" +cat conf/st2.ci.conf || true +echo "" # Needed by the coordination backend pip install "redis==3.5.3" From ae7f2e3dbf169dd6719f91068eb4007b76a6160e Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 18:39:18 +0100 Subject: [PATCH 12/14] Remove if check. --- .github/workflows/ci.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1fe0aa15d7..2879da7151 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -225,7 +225,6 @@ jobs: run: | ./scripts/ci/install-requirements.sh - name: Setup Integration Tests - if: "${{ env.TASK == 'ci-packs-tests' || env.TASK == 'ci-integration' }}" run: | # prep a ci-specific dev conf file that uses runner instead of stanley # this user is the username of the user in GitHub actions, used for SSH, etc during From c8cb76871f6a5fa9b9c35d2d94474a41fc63522d Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 18:46:18 +0100 Subject: [PATCH 13/14] Set cancel_others to true. --- .github/workflows/ci.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 2879da7151..cfaeff6b8e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -33,6 +33,7 @@ jobs: - id: skip_check uses: fkirc/skip-duplicate-actions@4c656bbdb6906310fa6213604828008bc28fe55d # v3.3.0 with: + cancel_others: 'true' github_token: ${{ github.token }} ci: From d9e6c5517316ef4ef7173cabe90eae7c8cd20692 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 25 Mar 2021 19:33:47 +0100 Subject: [PATCH 14/14] Only log the backend name and make sure we don't leak any credentials. --- st2common/st2common/service_setup.py | 3 ++- st2common/st2common/services/coordination.py | 13 +++++++++++++ .../test_service_setup_log_level_filtering.py | 2 +- .../tests/unit/services/test_synchronization.py | 12 ++++++++++++ 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/service_setup.py b/st2common/st2common/service_setup.py index 405bde515e..065ce98833 100644 --- a/st2common/st2common/service_setup.py +++ b/st2common/st2common/service_setup.py @@ -44,6 +44,7 @@ from st2common.services import coordination from st2common.logging.misc import add_global_filters_for_all_loggers from st2common.constants.error_messages import PYTHON2_DEPRECATION +from st2common.services.coordination import get_driver_name # Note: This is here for backward compatibility. # Function has been moved in a standalone module to avoid expensive in-direct @@ -147,7 +148,7 @@ def setup( LOG.info("Using logging config: %s", logging_config_path) - LOG.info("Using coordination url: %s", cfg.CONF.coordination.url) + LOG.info("Using coordination driver: %s", get_driver_name()) LOG.info("Using metrics driver: %s", cfg.CONF.metrics.driver) is_debug_enabled = cfg.CONF.debug or cfg.CONF.system.debug diff --git a/st2common/st2common/services/coordination.py b/st2common/st2common/services/coordination.py index 1a068632ab..fadfd2768f 100644 --- a/st2common/st2common/services/coordination.py +++ b/st2common/st2common/services/coordination.py @@ -175,6 +175,19 @@ def configured(): return backend_configured and not mock_backend +def get_driver_name() -> str: + """ + Return coordination driver name (aka protocol part from the URI / URL). + """ + url = cfg.CONF.coordination.url + + if not url: + return None + + driver_name = url.split("://")[0] + return driver_name + + def coordinator_setup(start_heart=True): """ Sets up the client for the coordination service. diff --git a/st2common/tests/integration/test_service_setup_log_level_filtering.py b/st2common/tests/integration/test_service_setup_log_level_filtering.py index 822140e61b..9062319808 100644 --- a/st2common/tests/integration/test_service_setup_log_level_filtering.py +++ b/st2common/tests/integration/test_service_setup_log_level_filtering.py @@ -79,7 +79,7 @@ def test_audit_log_level_is_filtered_if_log_level_is_not_debug_or_audit(self): self.assertIn("INFO [-] Using fs encoding:", stdout) self.assertIn("INFO [-] Using config files:", stdout) self.assertIn("INFO [-] Using logging config:", stdout) - self.assertIn("INFO [-] Using coordination url:", stdout) + self.assertIn("INFO [-] Using coordination driver:", stdout) self.assertIn("INFO [-] Using metrics driver:", stdout) # 1. INFO log level - audit messages should not be included diff --git a/st2common/tests/unit/services/test_synchronization.py b/st2common/tests/unit/services/test_synchronization.py index 991e6b9036..e0c42b50f4 100644 --- a/st2common/tests/unit/services/test_synchronization.py +++ b/st2common/tests/unit/services/test_synchronization.py @@ -39,16 +39,28 @@ def tearDownClass(cls): super(SynchronizationTest, cls).tearDownClass() def test_service_configured(self): + cfg.CONF.set_override(name="url", override=None, group="coordination") + self.assertEqual(coordination.get_driver_name(), None) + cfg.CONF.set_override( name="url", override="kazoo://127.0.0.1:2181", group="coordination" ) self.assertTrue(coordination.configured()) + self.assertEqual(coordination.get_driver_name(), "kazoo") cfg.CONF.set_override(name="url", override="file:///tmp", group="coordination") self.assertFalse(coordination.configured()) + self.assertEqual(coordination.get_driver_name(), "file") cfg.CONF.set_override(name="url", override="zake://", group="coordination") self.assertFalse(coordination.configured()) + self.assertEqual(coordination.get_driver_name(), "zake") + + cfg.CONF.set_override( + name="url", override="redis://foo:bar@127.0.0.1", group="coordination" + ) + self.assertTrue(coordination.configured()) + self.assertEqual(coordination.get_driver_name(), "redis") def test_lock(self): name = uuid.uuid4().hex