diff --git a/jenkins/opensearch/distribution-build.jenkinsfile b/jenkins/opensearch/distribution-build.jenkinsfile index 3b433eccd0..eae0e9ed0c 100644 --- a/jenkins/opensearch/distribution-build.jenkinsfile +++ b/jenkins/opensearch/distribution-build.jenkinsfile @@ -138,7 +138,7 @@ pipeline { def snapshotBuild = build job: 'publish-opensearch-min-snapshots', propagate: false, - wait: false, + wait: false, parameters: [ string(name: 'INPUT_MANIFEST', value: "${INPUT_MANIFEST}"), ] @@ -849,4 +849,4 @@ def markStageUnstableIfPluginsFailedToBuild() { if (stageLogs.any{e -> e.contains('Failed plugins are')}) { unstable('Some plugins failed to build. See the ./build.sh step for logs and more details') } -} \ No newline at end of file +} diff --git a/src/build_workflow/README.md b/src/build_workflow/README.md index 6a4b02c751..b2cdc7749c 100644 --- a/src/build_workflow/README.md +++ b/src/build_workflow/README.md @@ -1,10 +1,11 @@ - [Building from Source](#building-from-source) - [OpenSearch](#opensearch) - [OpenSearch Dashboards](#opensearch-dashboards) - - [Build Paths](#build-paths) - - [Build.sh Options](#buildsh-options) - - [Custom Build Scripts](#custom-build-scripts) - - [Avoiding Rebuilds](#avoiding-rebuilds) + - [Build Paths](#build-paths) + - [Build.sh Options](#buildsh-options) + - [Custom Build Scripts](#custom-build-scripts) + - [Avoiding Rebuilds](#avoiding-rebuilds) + - [Incremental Build](#incremental-build) ## Building from Source @@ -101,3 +102,15 @@ fi ``` The [Jenkins workflows](../../jenkins) in this repository can use this mechanism to avoid rebuilding all of OpenSearch and OpenSearch Dashboards unnecessarily. + +### Incremental Build + +This functionality augments the existing build process by introducing the `--incremental` binary parameter. + +Sample command: `./build.sh manifests/2.12.0/opensearch-2.12.0.yml --incremental`. + +The build workflow will examine the build manifest from the previous build using path `{distribution}/builds/opensearch/manifest.yml` when this command is executed. +The build workflow will be executed in accordance with the comparison between the commits for each component in the preceding build manifest and the current input manifest. +It will contain every modified component, and every component that relies on these revised components based on the `depends_on` entry in the input manifest. + +Once build is finished, new built artifacts will override the previous artifacts and a new build manifest will be generated using the previous build manifest as a reference, ensuring that all non-modified components remain unchanged. diff --git a/src/build_workflow/build_args.py b/src/build_workflow/build_args.py index 2c1965b8a0..1ad2650325 100644 --- a/src/build_workflow/build_args.py +++ b/src/build_workflow/build_args.py @@ -51,14 +51,6 @@ def __init__(self) -> None: default=False, help="Build snapshot.", ) - parser.add_argument( - "-c", - "--component", - dest="components", - nargs='*', - type=str, - help="Rebuild one or more components." - ) parser.add_argument( "--keep", dest="keep", @@ -104,7 +96,16 @@ def __init__(self) -> None: action="store_true", help="Do not fail the distribution build on any plugin component failure.", ) - parser.add_argument( + group = parser.add_mutually_exclusive_group() + group.add_argument( + "-c", + "--component", + dest="components", + nargs='*', + type=str, + help="Rebuild one or more components." + ) + group.add_argument( "-i", "--incremental", dest="incremental", diff --git a/src/build_workflow/build_recorder.py b/src/build_workflow/build_recorder.py index 143614c693..4356fe4478 100644 --- a/src/build_workflow/build_recorder.py +++ b/src/build_workflow/build_recorder.py @@ -17,8 +17,8 @@ class BuildRecorder: - def __init__(self, target: BuildTarget) -> None: - self.build_manifest = self.BuildManifestBuilder(target) + def __init__(self, target: BuildTarget, build_manifest: BuildManifest = None) -> None: + self.build_manifest = self.BuildManifestBuilder(target, build_manifest) self.target = target self.name = target.name @@ -53,18 +53,24 @@ def write_manifest(self) -> None: logging.info(f"Created build manifest {manifest_path}") class BuildManifestBuilder: - def __init__(self, target: BuildTarget) -> None: + def __init__(self, target: BuildTarget, build_manifest: BuildManifest = None) -> None: self.data: Dict[str, Any] = {} - self.data["build"] = {} - self.data["build"]["id"] = target.build_id - self.data["build"]["name"] = target.name - self.data["build"]["version"] = target.opensearch_version - self.data["build"]["platform"] = target.platform - self.data["build"]["architecture"] = target.architecture - self.data["build"]["distribution"] = target.distribution if target.distribution else "tar" - self.data["schema-version"] = "1.2" self.components_hash: Dict[str, Dict[str, Any]] = {} + if build_manifest: + self.data = build_manifest.__to_dict__() + for component in build_manifest.components.select(): + self.components_hash[component.name] = component.__to_dict__() + else: + self.data["build"] = {} + self.data["build"]["id"] = target.build_id + self.data["build"]["name"] = target.name + self.data["build"]["version"] = target.opensearch_version + self.data["build"]["platform"] = target.platform + self.data["build"]["architecture"] = target.architecture + self.data["build"]["distribution"] = target.distribution if target.distribution else "tar" + self.data["schema-version"] = "1.2" + def append_component(self, name: str, version: str, repository_url: str, ref: str, commit_id: str) -> None: component = { "name": name, @@ -75,6 +81,7 @@ def append_component(self, name: str, version: str, repository_url: str, ref: st "version": version, } self.components_hash[name] = component + logging.info(f"Appended {name} component in build manifest.") def append_artifact(self, component: str, type: str, path: str) -> None: artifacts = self.components_hash[component]["artifacts"] diff --git a/src/run_build.py b/src/run_build.py index b49790c7af..cf4e590f21 100755 --- a/src/run_build.py +++ b/src/run_build.py @@ -15,6 +15,7 @@ from build_workflow.build_recorder import BuildRecorder from build_workflow.build_target import BuildTarget from build_workflow.builders import Builders +from manifests.build_manifest import BuildManifest from manifests.input_manifest import InputManifest from paths.build_output_dir import BuildOutputDir from system import console @@ -25,6 +26,8 @@ def main() -> int: args = BuildArgs() console.configure(level=args.logging_level) manifest = InputManifest.from_file(args.manifest) + build_manifest = None + components = args.components failed_plugins = [] if args.ref_manifest: @@ -45,8 +48,20 @@ def main() -> int: if args.incremental: buildIncremental = BuildIncremental(manifest, args.distribution) list_of_updated_plugins = buildIncremental.commits_diff(manifest) - logging.info(f"Plugins for incremental build: {buildIncremental.rebuild_plugins(list_of_updated_plugins, manifest)}") - return 0 + components = buildIncremental.rebuild_plugins(list_of_updated_plugins, manifest) + if not components: + logging.info("No commit difference found between any components. Skipping the build") + return 0 + + logging.info(f"Plugins for incremental build: {components}") + + build_manifest_path = os.path.join(args.distribution, "builds", manifest.build.filename, "manifest.yml") + if not os.path.exists(build_manifest_path): + logging.error(f"Previous build manifest missing at path: {build_manifest_path}") + + logging.info(f"Build {components} incrementally.") + + build_manifest = BuildManifest.from_path(build_manifest_path) with TemporaryDirectory(keep=args.keep, chdir=True) as work_dir: logging.info(f"Building in {work_dir.name}") @@ -63,11 +78,11 @@ def main() -> int: architecture=args.architecture or manifest.build.architecture, ) - build_recorder = BuildRecorder(target) + build_recorder = BuildRecorder(target, build_manifest) if args.incremental else BuildRecorder(target) logging.info(f"Building {manifest.build.name} ({target.architecture}) into {target.output_dir}") - for component in manifest.components.select(focus=args.components, platform=target.platform): + for component in manifest.components.select(focus=components, platform=target.platform): logging.info(f"Building {component.name}") builder = Builders.builder_from(component, target) diff --git a/tests/test_run_build.py b/tests/test_run_build.py index bdc862ad5f..92443f9618 100644 --- a/tests/test_run_build.py +++ b/tests/test_run_build.py @@ -9,10 +9,12 @@ import tempfile import unittest from typing import Any -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import MagicMock, Mock, call, patch import pytest +from build_workflow.build_incremental import BuildIncremental +from manifests.build_manifest import BuildManifest from manifests.input_manifest import InputManifest from run_build import main @@ -40,6 +42,17 @@ def test_usage(self) -> None: OPENSEARCH_MANIFEST_2_12 = os.path.realpath(os.path.join(MANIFESTS, "templates", "opensearch", "2.x", "os-template-2.12.0.yml")) NON_OPENSEARCH_MANIFEST = os.path.realpath(os.path.join(MANIFESTS, "templates", "opensearch", "1.x", "non-os-template-1.1.0.yml")) + INPUT_MANIFEST_PATH = os.path.realpath(os.path.join(os.path.dirname(__file__), "tests_build_workflow", "data", "opensearch-input-2.12.0.yml")) + INPUT_MANIFEST = InputManifest.from_path(INPUT_MANIFEST_PATH) + BUILD_MANIFEST = BuildManifest.from_path( + os.path.join(os.path.dirname(__file__), "tests_build_workflow", "data", "opensearch-build-tar-2.12.0.yml")) + BUILD_MANIFEST_PATH = os.path.join(os.path.dirname(__file__), "tests_build_workflow", "data", "opensearch-build-tar-2.12.0.yml") + INPUT_MANIFEST_DASHBOARDS = InputManifest.from_path( + os.path.join(os.path.dirname(__file__), "tests_build_workflow", "data", "opensearch-dashboards-input-2.12.0.yml")) + BUILD_MANIFEST_DASHBOARDS = BuildManifest.from_path( + os.path.join(os.path.dirname(__file__), "tests_build_workflow", "data", "opensearch-dashboards-build-tar-2.12.0.yml")) + buildIncremental = BuildIncremental(INPUT_MANIFEST, "tar") + @patch("argparse._sys.argv", ["run_build.py", OPENSEARCH_MANIFEST, "-p", "linux"]) @patch("run_build.Builders.builder_from", return_value=MagicMock()) @patch("run_build.BuildRecorder", return_value=MagicMock()) @@ -229,3 +242,162 @@ def test_failed_plugins_default(self, mock_logging_error: Mock, mock_temp: Mock, with pytest.raises(Exception, match="Error during build"): main() mock_logging_error.assert_called_with(f"Error building common-utils, retry with: run_build.py {self.NON_OPENSEARCH_MANIFEST} --component common-utils") + + @patch("argparse._sys.argv", ["run_build.py", OPENSEARCH_MANIFEST, "--incremental"]) + @patch("os.path.exists") + @patch("run_build.Builders.builder_from", return_value=MagicMock()) + @patch("run_build.BuildRecorder", return_value=MagicMock()) + @patch("manifests.build_manifest.BuildManifest.from_path") + @patch("run_build.TemporaryDirectory") + @patch("run_build.BuildIncremental") + def test_main_incremental(self, mock_build_incremental: MagicMock, mock_temp: MagicMock, + mock_build_manifest: MagicMock, *mocks: Any) -> None: + mock_temp.return_value.__enter__.return_value.name = tempfile.gettempdir() + mock_build_manifest.return_value = self.BUILD_MANIFEST + main() + self.assertEqual(mock_build_incremental.call_count, 1) + mock_build_incremental.return_value.commits_diff.assert_called() + mock_build_incremental.return_value.rebuild_plugins.assert_called() + + @patch("argparse._sys.argv", ["run_build.py", INPUT_MANIFEST_PATH, "--incremental", "-p", "linux"]) + @patch("run_build.BuildIncremental.commits_diff", return_value=MagicMock()) + @patch("run_build.BuildIncremental.rebuild_plugins", return_value=MagicMock()) + @patch("run_build.logging.info") + def test_build_incremental_no_change(self, mock_logging_info: MagicMock, + mock_build_incremental: MagicMock, *mocks: Any) -> None: + mock_build_incremental.return_value = [] + main() + mock_logging_info.assert_has_calls([ + call("No commit difference found between any components. Skipping the build") + ], any_order=True) + + @patch("argparse._sys.argv", ["run_build.py", INPUT_MANIFEST_PATH, "--incremental"]) + @patch("run_build.BuildIncremental", return_value=MagicMock()) + @patch("os.path.exists") + @patch("run_build.Builders.builder_from", return_value=MagicMock()) + @patch("run_build.BuildRecorder", return_value=MagicMock()) + @patch("run_build.TemporaryDirectory") + def test_build_incremental_no_prebuild_manifest(self, mock_temp: MagicMock, mock_recorder: MagicMock, + mock_builder: MagicMock, mock_path_exists: MagicMock, + *mocks: Any) -> None: + mock_temp.return_value.__enter__.return_value.name = tempfile.gettempdir() + mock_path_exists.return_value = False + try: + main() + self.assertRaises(FileNotFoundError) + except FileNotFoundError: + pass + + @patch("argparse._sys.argv", ["run_build.py", INPUT_MANIFEST_PATH, "--incremental", "-p", "linux"]) + @patch("run_build.BuildIncremental.commits_diff", return_value=MagicMock()) + @patch("run_build.BuildIncremental.rebuild_plugins", return_value=MagicMock()) + @patch("run_build.logging.info") + @patch("run_build.BuildOutputDir") + @patch("os.path.exists") + @patch("manifests.build_manifest.BuildManifest.from_path") + @patch("run_build.Builders.builder_from", return_value=MagicMock()) + @patch("run_build.BuildRecorder", return_value=MagicMock()) + @patch("run_build.TemporaryDirectory") + def test_build_incremental_with_prebuild_manifest(self, mock_temp: MagicMock, mock_recorder: MagicMock, + mock_builder: MagicMock, mock_build_manifest: MagicMock, + mock_path_exist: MagicMock, mock_build_output_dir: MagicMock, + mock_logging_info: MagicMock, mock_build_incremental: MagicMock, + *mocks: Any) -> None: + mock_temp.return_value.__enter__.return_value.name = tempfile.gettempdir() + mock_path_exist.return_value = True + mock_build_manifest.return_value = self.BUILD_MANIFEST + mock_build_incremental.return_value = ["common-utils", "opensearch-observability"] + main() + mock_build_manifest.assert_called_once() + mock_build_manifest.assert_called_with(os.path.join("tar", "builds", "opensearch", "manifest.yml")) + self.assertNotEqual(mock_builder.return_value.build.call_count, 0) + self.assertEqual(mock_builder.return_value.build.call_count, 2) + self.assertEqual(mock_builder.return_value.build.call_count, mock_builder.return_value.export_artifacts.call_count) + + mock_logging_info.assert_has_calls([ + call('Building common-utils'), + call('Building opensearch-observability'), + ], any_order=True) + + mock_recorder.assert_called_once() + mock_recorder.return_value.write_manifest.assert_called() + + @patch("argparse._sys.argv", ["run_build.py", INPUT_MANIFEST_PATH, "--incremental", "-p", "linux", "--continue-on-error"]) + @patch("run_build.BuildIncremental.commits_diff", return_value=MagicMock()) + @patch("run_build.BuildIncremental.rebuild_plugins", return_value=MagicMock()) + @patch("run_build.logging.error") + @patch("run_build.logging.info") + @patch("run_build.BuildOutputDir") + @patch("os.path.exists") + @patch("manifests.build_manifest.BuildManifest.from_path") + @patch("run_build.Builders.builder_from", return_value=MagicMock()) + @patch("run_build.BuildRecorder", return_value=MagicMock()) + @patch("run_build.TemporaryDirectory") + def test_build_incremental_continue_on_fail_core(self, mock_temp: MagicMock, mock_recorder: MagicMock, + mock_builder_from: MagicMock, mock_build_manifest: MagicMock, + mock_path_exist: MagicMock, mock_build_output_dir: MagicMock, + mock_logging_info: MagicMock, mock_logging_error: MagicMock, + mock_build_incremental: MagicMock, *mocks: Any) -> None: + mock_temp.return_value.__enter__.return_value.name = tempfile.gettempdir() + mock_path_exist.return_value = True + mock_build_manifest.return_value = self.BUILD_MANIFEST + mock_builder = MagicMock() + mock_builder.build.side_effect = Exception("Error building") + mock_builder_from.return_value = mock_builder + mock_build_incremental.return_value = ["common-utils", "opensearch-observability"] + + with pytest.raises(Exception, match="Error building"): + main() + + mock_logging_error.assert_called_with(f"Error building common-utils, retry with: run_build.py {self.INPUT_MANIFEST_PATH} --component common-utils") + mock_build_manifest.assert_called_once() + mock_build_manifest.assert_called_with(os.path.join("tar", "builds", "opensearch", "manifest.yml")) + self.assertNotEqual(mock_builder.build.call_count, 0) + self.assertEqual(mock_builder.build.call_count, 1) + + mock_logging_info.assert_has_calls([ + call('Building common-utils') + ], any_order=True) + + mock_recorder.assert_called_once() + mock_recorder.return_value.write_manifest.assert_not_called() + + @patch("argparse._sys.argv", ["run_build.py", INPUT_MANIFEST_PATH, "--incremental", "-p", "linux", "--continue-on-error"]) + @patch("run_build.BuildIncremental.commits_diff", return_value=MagicMock()) + @patch("run_build.BuildIncremental.rebuild_plugins", return_value=MagicMock()) + @patch("run_build.logging.error") + @patch("run_build.logging.info") + @patch("run_build.BuildOutputDir") + @patch("os.path.exists") + @patch("manifests.build_manifest.BuildManifest.from_path") + @patch("run_build.Builders.builder_from", return_value=MagicMock()) + @patch("run_build.BuildRecorder", return_value=MagicMock()) + @patch("run_build.TemporaryDirectory") + def test_build_incremental_continue_on_fail_plugin(self, mock_temp: MagicMock, mock_recorder: MagicMock, + mock_builder_from: MagicMock, mock_build_manifest: MagicMock, + mock_path_exist: MagicMock, mock_build_output_dir: MagicMock, + mock_logging_info: MagicMock, mock_logging_error: MagicMock, + mock_build_incremental: MagicMock, *mocks: Any) -> None: + mock_temp.return_value.__enter__.return_value.name = tempfile.gettempdir() + mock_path_exist.return_value = True + mock_build_manifest.return_value = self.BUILD_MANIFEST + mock_builder = MagicMock() + mock_builder.build.side_effect = Exception("Error build") + mock_builder_from.return_value = mock_builder + mock_build_incremental.return_value = ["ml-commons", "opensearch-observability"] + + main() + + mock_logging_error.assert_called_with("Failed plugins are ['ml-commons', 'opensearch-observability']") + mock_build_manifest.assert_called_once() + mock_build_manifest.assert_called_with(os.path.join("tar", "builds", "opensearch", "manifest.yml")) + self.assertNotEqual(mock_builder.build.call_count, 0) + self.assertEqual(mock_builder.build.call_count, 2) + + mock_logging_info.assert_has_calls([ + call('Building ml-commons'), + call('Building opensearch-observability') + ], any_order=True) + + mock_recorder.assert_called_once() + mock_recorder.return_value.write_manifest.assert_called() diff --git a/tests/tests_build_workflow/test_build_incremental.py b/tests/tests_build_workflow/test_build_incremental.py index 5994ce1c73..5a0686d872 100644 --- a/tests/tests_build_workflow/test_build_incremental.py +++ b/tests/tests_build_workflow/test_build_incremental.py @@ -20,6 +20,7 @@ class TestBuildIncremental(unittest.TestCase): os.path.join(os.path.dirname(__file__), "data", "opensearch-input-2.12.0.yml")) BUILD_MANIFEST = BuildManifest.from_path( os.path.join(os.path.dirname(__file__), "data", "opensearch-build-tar-2.12.0.yml")) + BUILD_MANIFEST_PATH = os.path.join(os.path.dirname(__file__), "data", "opensearch-build-tar-2.12.0.yml") INPUT_MANIFEST_DASHBOARDS = InputManifest.from_path( os.path.join(os.path.dirname(__file__), "data", "opensearch-dashboards-input-2.12.0.yml")) BUILD_MANIFEST_DASHBOARDS = BuildManifest.from_path( diff --git a/tests/tests_build_workflow/test_build_recorder.py b/tests/tests_build_workflow/test_build_recorder.py index 24011ce2e0..e411a1ceb5 100644 --- a/tests/tests_build_workflow/test_build_recorder.py +++ b/tests/tests_build_workflow/test_build_recorder.py @@ -33,6 +33,22 @@ def __mock(self, snapshot: bool = True) -> BuildRecorder: ) ) + def __mock_with_manifest(self, snapshot: bool = True) -> BuildRecorder: + build_manifest_path = os.path.join("tests", "tests_build_workflow", "data", "opensearch-build-tar-2.12.0.yml") + build_manifest = BuildManifest.from_path(build_manifest_path) + return BuildRecorder( + BuildTarget( + build_id="1", + output_dir="output_dir", + name="OpenSearch", + version="1.3.0", + platform="linux", + architecture="x64", + snapshot=snapshot, + ), + build_manifest + ) + def __mock_distribution(self, snapshot: bool = True) -> BuildRecorder: return BuildRecorder( BuildTarget( @@ -191,7 +207,8 @@ def test_write_manifest(self) -> None: @patch("shutil.copyfile") @patch("os.makedirs") @patch.object(BuildArtifactOpenSearchCheckPlugin, "check") - def test_record_artifact_check_plugin_version_properties(self, mock_plugin_check: Mock, mock_makedirs: Mock, mock_copyfile: Mock) -> None: + def test_record_artifact_check_plugin_version_properties(self, mock_plugin_check: Mock, mock_makedirs: Mock, + mock_copyfile: Mock) -> None: mock = self.__mock(snapshot=False) mock.record_component( "security", @@ -212,7 +229,8 @@ def test_record_artifact_check_plugin_version_properties(self, mock_plugin_check @patch("shutil.copyfile") @patch("os.makedirs") @patch.object(BuildArtifactOpenSearchCheckPlugin, "check") - def test_record_artifact_check_plugin_version_properties_snapshot(self, mock_plugin_check: Mock, mock_makedirs: Mock, mock_copyfile: Mock) -> None: + def test_record_artifact_check_plugin_version_properties_snapshot(self, mock_plugin_check: Mock, + mock_makedirs: Mock, mock_copyfile: Mock) -> None: mock = self.__mock(snapshot=True) mock.record_component( "security", @@ -229,3 +247,34 @@ def test_record_artifact_check_plugin_version_properties_snapshot(self, mock_plu mock_plugin_check.assert_called() mock_copyfile.assert_called() mock_makedirs.assert_called() + + def test_append_component_with_existing_manifest(self) -> None: + mock = self.__mock_with_manifest(snapshot=False) + + self.assertEqual(mock.build_manifest.components_hash.get("job-scheduler").get("commit_id"), + "aaf09b0211df15dd74ff2756f2590c360b03486b") + self.assertEqual(mock.build_manifest.components_hash.get("geospatial").get("commit_id"), + "8776900f2f26312b4d3a08e4343f3e3f7bdde536") + self.assertEqual(mock.build_manifest.components_hash.get("geospatial").get("repository"), + "https://github.com/opensearch-project/geospatial.git") + self.assertEqual(mock.build_manifest.components_hash.get("security").get("commit_id"), + "e3c8902dea26fd20f56a6f144042b2623f652e3e") + self.assertEqual(mock.build_manifest.components_hash.get("security").get("version"), "2.12.0.0") + + mock.record_component( + "job-scheduler", + MagicMock( + url="https://github.com/opensearch-project/job-scheduler.git", + ref="2.12", + sha="mockcommitid", + ), + ) + + self.assertEqual(mock.build_manifest.components_hash.get("job-scheduler").get("commit_id"), "mockcommitid") + self.assertEqual(mock.build_manifest.components_hash.get("job-scheduler").get("ref"), "2.12") + self.assertEqual(mock.build_manifest.components_hash.get("job-scheduler").get("repository"), + "https://github.com/opensearch-project/job-scheduler.git") + self.assertEqual(mock.build_manifest.components_hash.get("geospatial").get("commit_id"), + "8776900f2f26312b4d3a08e4343f3e3f7bdde536") + self.assertEqual(mock.build_manifest.components_hash.get("security").get("commit_id"), + "e3c8902dea26fd20f56a6f144042b2623f652e3e")