Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build incremental components through build workflow with loading previous build manifest #4289

Merged
merged 13 commits into from
Jan 4, 2024
Merged

Build incremental components through build workflow with loading previous build manifest #4289

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
254809a
Update build incremental with tests
zelinh Dec 13, 2023
87fddd6
Rebase from main
zelinh Dec 14, 2023
cbad22f
More tests for run_build class
zelinh Dec 14, 2023
a8d01d0
More tests for continue on error
zelinh Dec 15, 2023
984e79c
Merge remote-tracking branch 'upstream/main' into build_incremental
zelinh Dec 15, 2023
20b74d4
Merge remote-tracking branch 'upstream/main' into build_incremental
zelinh Jan 2, 2024
225b0ec
Refractoring the incremental building process
zelinh Jan 3, 2024
9534288
Rename data to build manifest
zelinh Jan 3, 2024
ec61ce5
Add documentations
zelinh Jan 3, 2024
142480f
Remove unnecessary variable
zelinh Jan 3, 2024
cb0050c
Update wording
zelinh Jan 3, 2024
0c9da6b
Make components and incremental mutually exclusive
zelinh Jan 4, 2024
c442dab
Add more tests
zelinh Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions jenkins/opensearch/distribution-build.jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}"),
]
Expand Down Expand Up @@ -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')
}
}
}
30 changes: 19 additions & 11 deletions src/build_workflow/build_recorder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -53,18 +53,25 @@ 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:
build_manifest_data = build_manifest.__to_dict__()
self.data = build_manifest_data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you should be able to iterate through components without turning the build manifest into a dictionary first, which would remove the need for the build_manifest_data variable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would require to change the logics in other sections for example to_manifest from build_recorder since it generates the build manifest based on dictionary. Do you think it's necessary or it may be overcomplicated.

Copy link
Member

@dblock dblock Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever you need to add to build_manifest is a good idea. It should look like this here:

self.data = dict(build_manifest)
for component in build_manifest.components:
    self.components_hash[component.name] = dict(component)

or even

self.data = dict(build_manifest)
self.components_hash = dict(build_manifest.components)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean! Updated in the latest commit. Thanks for suggestion!

for components_data in build_manifest_data.get("components"):
self.components_hash[components_data["name"]] = components_data
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,
Expand All @@ -75,6 +82,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 input manifest.")
gaiksaya marked this conversation as resolved.
Show resolved Hide resolved

def append_artifact(self, component: str, type: str, path: str) -> None:
artifacts = self.components_hash[component]["artifacts"]
Expand Down
19 changes: 15 additions & 4 deletions src/run_build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -45,8 +48,16 @@ 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)
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}")
Expand All @@ -63,11 +74,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):
gaiksaya marked this conversation as resolved.
Show resolved Hide resolved
zelinh marked this conversation as resolved.
Show resolved Hide resolved
logging.info(f"Building {component.name}")

builder = Builders.builder_from(component, target)
Expand Down
162 changes: 161 additions & 1 deletion tests/test_run_build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -229,3 +242,150 @@ 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"])
@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()
1 change: 1 addition & 0 deletions tests/tests_build_workflow/test_build_incremental.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading
Loading