diff --git a/.github/rulesets/branches/default-branch-required.json b/.github/rulesets/branches/default-branch-required.json index ce23aa70..029d9056 100644 --- a/.github/rulesets/branches/default-branch-required.json +++ b/.github/rulesets/branches/default-branch-required.json @@ -40,10 +40,6 @@ "strict_required_status_checks_policy": true, "do_not_enforce_on_create": false, "required_status_checks": [ - { - "context": "unittest-complete", - "integration_id": 15368 - }, { "context": "crucible-ci-complete", "integration_id": 15368 diff --git a/.github/workflows/crucible-ci.yaml b/.github/workflows/crucible-ci.yaml index 055aa077..2d0924d3 100644 --- a/.github/workflows/crucible-ci.yaml +++ b/.github/workflows/crucible-ci.yaml @@ -29,7 +29,6 @@ jobs: .github/workflows/crucible-ci.yaml .github/workflows/fork-check.yaml .github/workflows/controller-build.yaml - .github/workflows/unittest.yaml docs/** engine/engine-script-library.py engine/engine-script.py @@ -37,9 +36,14 @@ jobs: - name: Display changes run: echo '${{ toJSON(steps.filter.outputs) }}' | jq . - call-real-core-release-crucible-ci: + call-unittest: needs: changes if: ${{ github.event.pull_request.head.repo.fork != true && (github.event_name == 'workflow_dispatch' || needs.changes.outputs.only-docs != 'true') }} + uses: ./.github/workflows/unittest.yaml + + call-real-core-release-crucible-ci: + needs: [ changes, call-unittest ] + if: ${{ github.event.pull_request.head.repo.fork != true && (github.event_name == 'workflow_dispatch' || needs.changes.outputs.only-docs != 'true') }} uses: perftool-incubator/crucible-ci/.github/workflows/core-release-crucible-ci.yaml@main with: ci_target: "rickshaw" @@ -55,7 +59,7 @@ jobs: uses: perftool-incubator/crucible-ci/.github/workflows/faux-core-release-crucible-ci.yaml@main crucible-ci-complete: - needs: [ call-real-core-release-crucible-ci, call-faux-core-release-crucible-ci ] + needs: [ call-unittest, call-real-core-release-crucible-ci, call-faux-core-release-crucible-ci ] if: always() runs-on: ubuntu-latest steps: diff --git a/.github/workflows/unittest.yaml b/.github/workflows/unittest.yaml index 9b378fae..a0884feb 100644 --- a/.github/workflows/unittest.yaml +++ b/.github/workflows/unittest.yaml @@ -1,46 +1,15 @@ name: unittest on: - pull_request: - branches: [ master ] + workflow_call: workflow_dispatch: -concurrency: - group: ${{ github.ref }}/unittest - cancel-in-progress: true +permissions: + contents: read jobs: - changes: - runs-on: ubuntu-latest - outputs: - only-docs: ${{ steps.filter.outputs.only_modified }} - steps: - - uses: actions/checkout@v4 - - id: filter - uses: tj-actions/changed-files@v47 - with: - files: | - LICENSE - *.md - **/*.md - .github/rulesets/** - .github/workflows/run-crucible-tracking.yaml - .github/workflows/crucible-merged.yaml - .github/workflows/crucible-ci.yaml - .github/workflows/fork-check.yaml - .github/workflows/controller-build.yaml - .github/workflows/unittest.yaml - docs/** - engine/engine-script-library.py - engine/engine-script.py - userenvs/rhel-ai-*.json - - name: Display changes - run: echo '${{ toJSON(steps.filter.outputs) }}' | jq . - blockbreaker: runs-on: ubuntu-latest - needs: changes - if: ${{ github.event.pull_request.head.repo.fork != true && (github.event_name == 'workflow_dispatch' || needs.changes.outputs.only-docs != 'true') }} steps: - uses: actions/checkout@v4 @@ -65,8 +34,6 @@ jobs: rickshaw-run-tests: runs-on: ubuntu-latest - needs: changes - if: ${{ github.event.pull_request.head.repo.fork != true && (github.event_name == 'workflow_dispatch' || needs.changes.outputs.only-docs != 'true') }} steps: - uses: actions/checkout@v4 @@ -86,22 +53,3 @@ jobs: with: name: rickshaw-run-tests-report path: report.html - - faux-unittest: - runs-on: ubuntu-latest - needs: changes - if: ${{ github.event.pull_request.head.repo.fork != true && github.event_name != 'workflow_dispatch' && needs.changes.outputs.only-docs == 'true' }} - steps: - - run: echo "faux-unittest-complete" - - unittest-complete: - runs-on: ubuntu-latest - needs: [ blockbreaker, rickshaw-run-tests, faux-unittest ] - if: always() - steps: - - name: Check Results - if: >- - contains(needs.*.result, 'failure') || - contains(needs.*.result, 'cancelled') - run: exit 1 - - run: echo "unittest-complete" diff --git a/endpoints/endpoints.py b/endpoints/endpoints.py index b9fc7cbd..c3fdaaee 100644 --- a/endpoints/endpoints.py +++ b/endpoints/endpoints.py @@ -40,6 +40,7 @@ save_received_messages as _tb_save_received_messages, ROADBLOCK_EXITS, ) +from toolbox.roadblock import do_roadblock as _tb_do_roadblock ROADBLOCK_HOME = os.environ.get('ROADBLOCK_HOME') if ROADBLOCK_HOME is None: @@ -53,7 +54,6 @@ print("ERROR: /roadblock.py ('%s') does not exist!" % (p)) exit(2) sys.path.append(str(Path(ROADBLOCK_HOME))) -from roadblock import roadblock from roadblock import VERBOSE_DEBUG_LEVEL roadblock_exits = ROADBLOCK_EXITS @@ -620,6 +620,11 @@ def do_roadblock(roadblock_id = None, label = None, timeout = None, messages = N """ Run a roadblock + Thin endpoint-specific wrapper around toolbox.roadblock.do_roadblock(): + adds the pre-connect ping diagnostic and message stream logging that + callers of this function rely on, then delegates the actual roadblock + mechanics to the shared implementation also used by engine_lib.py. + Args: roadblock_id (str): The base ID to use as part of the roadblock's name label (str): The name of the roadblock to participate in @@ -643,8 +648,6 @@ def do_roadblock(roadblock_id = None, label = None, timeout = None, messages = N raise ValueError("No roadblock label specified") logger.info("Processing roadblock '%s'" % (label), stacklevel = 2) - uuid = "%s:%s" % (roadblock_id, label) - logger.info("[%s] Roadblock uuid is '%s'" % (label, uuid)) if timeout is None: timeout = 300 @@ -652,28 +655,10 @@ def do_roadblock(roadblock_id = None, label = None, timeout = None, messages = N else: logger.info("[%s] Roadblock timeout set to %d" % (label, timeout)) - if messages is None: - logger.info("[%s] No roadblock messages to send" % (label)) - else: - logger.info("[%s] Sending roadblock messages %s" % (label, messages)) - - if wait_for is None: - logger.info("[%s] No roadblock wait-for" % (label)) - else: - wait_for_log = tempfile.mkstemp(suffix = "log") - os.close(wait_for_log[0]) - wait_for_log = wait_for_log[1] - logger.info("[%s] Going to run this wait-for command: %s" % (label, wait_for)) - logger.info("[%s] Going to log wait-for to this file: %s" % (label, wait_for_log)) - - if not abort is None and not abort is False: - logger.info("[%s] Going to send an abort" % (label)) - msgs_log_file = msgs_dir + "/" + label + ".json" logger.info("[%s] Logging messages to: %s" % (label, msgs_log_file)) redis_server = "localhost" - leader = "controller" result = run_local("ping -w 10 -c 4 " + redis_server) ping_log_msg = "[%s] Pinged redis server '%s' with return code %d:\nstdout:\n%sstderr:\n%s" % (label, redis_server, result.exited, result.stdout, result.stderr) @@ -682,44 +667,31 @@ def do_roadblock(roadblock_id = None, label = None, timeout = None, messages = N else: logger.info(ping_log_msg) - my_roadblock = roadblock(None, None) - my_roadblock.set_uuid(uuid) - my_roadblock.set_role("follower") - my_roadblock.set_follower_id(follower_id) - my_roadblock.set_leader_id(leader) - my_roadblock.set_timeout(timeout) - my_roadblock.set_redis_server(redis_server) - my_roadblock.set_redis_password(redis_password) - my_roadblock.set_abort(abort) - my_roadblock.set_message_log(msgs_log_file) - my_roadblock.set_user_messages(messages) - if not wait_for is None: - my_roadblock.set_wait_for_cmd(wait_for) - my_roadblock.set_wait_for_log(wait_for_log) - if connection_watchdog: - my_roadblock.set_connection_watchdog("enabled") - else: - my_roadblock.set_connection_watchdog("disabled") - - rc = my_roadblock.run_it() + rc, _ = _tb_do_roadblock(roadblock_id = roadblock_id, + label = label, + role = "follower", + follower_id = follower_id, + leader_id = "controller", + timeout = timeout, + redis_server = redis_server, + redis_password = redis_password, + messages = messages, + abort = abort, + connection_watchdog = connection_watchdog, + msgs_dir = msgs_dir, + wait_for = wait_for) result_log_msg = "[%s] Roadblock resulted in return code %d" % (label, rc) if rc != 0: logger.error(result_log_msg) else: logger.info(result_log_msg) - stream = "" - with open(msgs_log_file, "r", encoding = "ascii") as msgs_log_file_fp: - for line in msgs_log_file_fp: - stream += line - logger.info("[%s] Logged messages from roadblock:\n%s" % (label, stream)) - - if not wait_for is None: + if os.path.exists(msgs_log_file): stream = "" - with open(wait_for_log, "r", encoding = "ascii") as wait_for_log_fp: - for line in wait_for_log_fp: - stream += line - logger.info("[%s] Wait-for log from raodblock:\n%s" % (label, stream)) + with open(msgs_log_file, "r", encoding = "ascii") as msgs_log_file_fp: + for line in msgs_log_file_fp: + stream += line + logger.info("[%s] Logged messages from roadblock:\n%s" % (label, stream)) logger.info("[%s] Returning %d" % (label, rc)) return rc diff --git a/tests/test_endpoints_do_roadblock.py b/tests/test_endpoints_do_roadblock.py new file mode 100644 index 00000000..7029d867 --- /dev/null +++ b/tests/test_endpoints_do_roadblock.py @@ -0,0 +1,220 @@ +#!/usr/bin/env python3 +# -*- mode: python; indent-tabs-mode: nil; python-indent-level: 4 -*- +# vim: autoindent tabstop=4 shiftwidth=4 expandtab softtabstop=4 filetype=python + +"""Unit tests for endpoints.py's do_roadblock() thin wrapper around +toolbox.roadblock.do_roadblock() (PERFNFV-462). + +toolbox, roadblock, and the fabric/invoke/paramiko third-party deps are all +mocked out rather than required, since endpoints.py imports from them at +module scope and CI does not check any of them out or install them for this +test job. +""" + +import importlib.machinery +import importlib.util +import os +import sys +import tempfile +import types +import unittest +from unittest.mock import MagicMock + + +class FakeRunResult: + def __init__(self, exited=0, stdout="", stderr=""): + self.exited = exited + self.stdout = stdout + self.stderr = stderr + + +def import_endpoints(): + """Load endpoints.py as a module with all of its external deps mocked out.""" + mock_fabric = types.ModuleType("fabric") + mock_fabric.Connection = MagicMock + + mock_invoke = types.ModuleType("invoke") + mock_invoke.run = MagicMock() + + mock_ssh_exception = types.ModuleType("paramiko.ssh_exception") + mock_ssh_exception.AuthenticationException = Exception + mock_ssh_exception.NoValidConnectionsError = Exception + + mock_paramiko = types.ModuleType("paramiko") + mock_paramiko.ssh_exception = mock_ssh_exception + + mock_toolbox_json = types.ModuleType("toolbox.json") + + mock_toolbox_messages = types.ModuleType("toolbox.messages") + mock_toolbox_messages.create_roadblock_msg = lambda *a, **k: None + mock_toolbox_messages.prepare_user_msgs_file = lambda *a, **k: None + mock_toolbox_messages.evaluate_roadblock_result = lambda *a, **k: None + mock_toolbox_messages.save_received_messages = lambda *a, **k: None + mock_toolbox_messages.ROADBLOCK_EXITS = { + "success": 0, + "input": 2, + "timeout": 3, + "abort": 4, + "heartbeat_timeout": 5, + "abort_waiting": 6, + } + + mock_toolbox_roadblock = types.ModuleType("toolbox.roadblock") + mock_toolbox_roadblock.do_roadblock = MagicMock(return_value=(0, None)) + + mock_toolbox = types.ModuleType("toolbox") + mock_toolbox.json = mock_toolbox_json + mock_toolbox.messages = mock_toolbox_messages + mock_toolbox.roadblock = mock_toolbox_roadblock + + mock_roadblock_engine_mod = types.ModuleType("roadblock") + mock_roadblock_engine_mod.VERBOSE_DEBUG_LEVEL = 5 + + mod_name = "endpoints_under_test" + sys.modules.pop(mod_name, None) + + mocks = { + "fabric": mock_fabric, + "invoke": mock_invoke, + "paramiko": mock_paramiko, + "paramiko.ssh_exception": mock_ssh_exception, + "toolbox": mock_toolbox, + "toolbox.json": mock_toolbox_json, + "toolbox.messages": mock_toolbox_messages, + "toolbox.roadblock": mock_toolbox_roadblock, + "roadblock": mock_roadblock_engine_mod, + } + saved = {key: sys.modules.get(key) for key in mocks} + sys.modules.update(mocks) + + # endpoints.py's module-level TOOLBOX_HOME/ROADBLOCK_HOME guards check + # that these paths exist on disk before appending them to sys.path -- + # the actual imports are satisfied by the sys.modules mocks above, so + # these just need to pass the existence checks. + with tempfile.TemporaryDirectory() as tmp_home: + toolbox_python_dir = os.path.join(tmp_home, "python") + os.makedirs(toolbox_python_dir) + roadblock_dir = os.path.join(tmp_home, "roadblock") + os.makedirs(roadblock_dir) + open(os.path.join(roadblock_dir, "roadblock.py"), "w").close() + + saved_env = { + "TOOLBOX_HOME": os.environ.get("TOOLBOX_HOME"), + "ROADBLOCK_HOME": os.environ.get("ROADBLOCK_HOME"), + } + os.environ["TOOLBOX_HOME"] = tmp_home + os.environ["ROADBLOCK_HOME"] = roadblock_dir + + try: + script_path = os.path.join( + os.path.dirname(__file__), "..", "endpoints", "endpoints.py" + ) + loader = importlib.machinery.SourceFileLoader(mod_name, script_path) + spec = importlib.util.spec_from_loader(mod_name, loader) + mod = importlib.util.module_from_spec(spec) + sys.modules[mod_name] = mod + spec.loader.exec_module(mod) + finally: + for key, val in saved.items(): + if val is None: + sys.modules.pop(key, None) + else: + sys.modules[key] = val + for key, val in saved_env.items(): + if val is None: + os.environ.pop(key, None) + else: + os.environ[key] = val + + mod.run_local = MagicMock(return_value=FakeRunResult()) + return mod + + +class TestDoRoadblock(unittest.TestCase): + def setUp(self): + self.mod = import_endpoints() + self.tmpdir = tempfile.TemporaryDirectory() + self.msgs_dir = self.tmpdir.name + + def tearDown(self): + self.tmpdir.cleanup() + + def test_missing_label_raises_value_error(self): + with self.assertRaises(ValueError): + self.mod.do_roadblock(roadblock_id="run-1", msgs_dir=self.msgs_dir) + self.mod._tb_do_roadblock.assert_not_called() + + def test_delegates_to_toolbox_with_expected_args(self): + rc = self.mod.do_roadblock( + roadblock_id="run-1", + label="endpoint-deploy-begin", + timeout=120, + messages="/tmp/msgs.json", + wait_for="some-cmd", + abort=False, + follower_id="client-1", + redis_password="secret", + msgs_dir=self.msgs_dir, + connection_watchdog=True, + ) + self.assertEqual(rc, 0) + self.mod._tb_do_roadblock.assert_called_once_with( + roadblock_id="run-1", + label="endpoint-deploy-begin", + role="follower", + follower_id="client-1", + leader_id="controller", + timeout=120, + redis_server="localhost", + redis_password="secret", + messages="/tmp/msgs.json", + abort=False, + connection_watchdog=True, + msgs_dir=self.msgs_dir, + wait_for="some-cmd", + ) + + def test_none_timeout_defaults_to_300(self): + self.mod.do_roadblock( + roadblock_id="run-1", + label="endpoint-deploy-begin", + follower_id="client-1", + msgs_dir=self.msgs_dir, + ) + _, kwargs = self.mod._tb_do_roadblock.call_args + self.assertEqual(kwargs["timeout"], 300) + + def test_returns_only_rc_not_messages_data(self): + self.mod._tb_do_roadblock.return_value = (4, {"some": "messages"}) + rc = self.mod.do_roadblock( + roadblock_id="run-1", + label="client-start-begin", + follower_id="client-1", + msgs_dir=self.msgs_dir, + ) + self.assertEqual(rc, 4) + + def test_pings_redis_before_delegating(self): + self.mod.do_roadblock( + roadblock_id="run-1", + label="client-start-begin", + follower_id="client-1", + msgs_dir=self.msgs_dir, + ) + self.mod.run_local.assert_called_once_with("ping -w 10 -c 4 localhost") + + def test_no_crash_when_msgs_log_file_missing(self): + # toolbox.roadblock.do_roadblock's mock doesn't actually write the + # message log file to disk, so this exercises the path where the + # wrapper's post-call stream-logging must not assume it exists. + rc = self.mod.do_roadblock( + roadblock_id="run-1", + label="client-start-end", + follower_id="client-1", + msgs_dir=self.msgs_dir, + ) + self.assertEqual(rc, 0) + + +if __name__ == "__main__": + unittest.main()