Merge pull request ClickHouse#33751 from ClickHouse/fix-docker-dependencies

Fix docker dependencies

Author: Felixoid

.github/workflows/master.yml

@@ -9,6 +9,18 @@ on: # yamllint disable-line rule:truthy
     branches:
       - 'master'
 jobs:
+  PythonUnitTests:
+    runs-on: [self-hosted, style-checker]
+    steps:
+      - name: Clear repository
+        run: |
+          sudo rm -fr "$GITHUB_WORKSPACE" && mkdir "$GITHUB_WORKSPACE"
+      - name: Check out repository code
+        uses: actions/checkout@v2
+      - name: Python unit tests
+        run: |
+          cd "$GITHUB_WORKSPACE/tests/ci"
+          python3 -m unittest discover -s . -p '*_test.py'
   DockerHubPushAarch64:
     runs-on: [self-hosted, func-tester-aarch64]
     steps:
@@ -44,7 +56,7 @@ jobs:
           name: changed_images_amd64
           path: ${{ runner.temp }}/docker_images_check/changed_images_amd64.json
   DockerHubPush:
-    needs: [DockerHubPushAmd64, DockerHubPushAarch64]
+    needs: [DockerHubPushAmd64, DockerHubPushAarch64, PythonUnitTests]
     runs-on: [self-hosted, style-checker]
     steps:
       - name: Clear repository

.github/workflows/pull_request.yml

@@ -31,6 +31,19 @@ jobs:
         run: |
           cd "$GITHUB_WORKSPACE/tests/ci"
           python3 run_check.py
+  PythonUnitTests:
+    needs: CheckLabels
+    runs-on: [self-hosted, style-checker]
+    steps:
+      - name: Clear repository
+        run: |
+          sudo rm -fr "$GITHUB_WORKSPACE" && mkdir "$GITHUB_WORKSPACE"
+      - name: Check out repository code
+        uses: actions/checkout@v2
+      - name: Python unit tests
+        run: |
+          cd "$GITHUB_WORKSPACE/tests/ci"
+          python3 -m unittest discover -s . -p '*_test.py'
   DockerHubPushAarch64:
     needs: CheckLabels
     runs-on: [self-hosted, func-tester-aarch64]
@@ -68,7 +81,7 @@ jobs:
           name: changed_images_amd64
           path: ${{ runner.temp }}/docker_images_check/changed_images_amd64.json
   DockerHubPush:
-    needs: [DockerHubPushAmd64, DockerHubPushAarch64]
+    needs: [DockerHubPushAmd64, DockerHubPushAarch64, PythonUnitTests]
     runs-on: [self-hosted, style-checker]
     steps:
       - name: Clear repository

tests/ci/docker_images_check.py

@@ -6,7 +6,7 @@
 import shutil
 import subprocess
 import time
-from typing import List, Tuple
+from typing import List, Optional, Set, Tuple, Union
 
 from github import Github
 
@@ -24,21 +24,49 @@
 TEMP_PATH = os.path.join(RUNNER_TEMP, "docker_images_check")
 
 
+class DockerImage:
+    def __init__(
+        self,
+        path: str,
+        repo: str,
+        parent: Optional["DockerImage"] = None,
+        gh_repo_path: str = GITHUB_WORKSPACE,
+    ):
+        self.path = path
+        self.full_path = os.path.join(gh_repo_path, path)
+        self.repo = repo
+        self.parent = parent
+        self.built = False
+
+    def __eq__(self, other):
+        """Is used to check if DockerImage is in a set or not"""
+        return self.path == other.path
+
+    def __hash__(self):
+        return hash(self.path)
+
+    def __str__(self):
+        return self.repo
+
+    def __repr__(self):
+        return f"DockerImage(path={self.path},path={self.path},parent={self.parent})"
+
+
 def get_changed_docker_images(
     pr_info: PRInfo, repo_path: str, image_file_path: str
-) -> List[Tuple[str, str]]:
+) -> Set[DockerImage]:
     images_dict = {}
     path_to_images_file = os.path.join(repo_path, image_file_path)
     if os.path.exists(path_to_images_file):
-        with open(path_to_images_file, "r") as dict_file:
+        with open(path_to_images_file, "rb") as dict_file:
             images_dict = json.load(dict_file)
     else:
         logging.info(
             "Image file %s doesnt exists in repo %s", image_file_path, repo_path
         )
 
     if not images_dict:
-        return []
+        return set()
 
     files_changed = pr_info.changed_files
 
@@ -54,49 +82,40 @@ def get_changed_docker_images(
     for dockerfile_dir, image_description in images_dict.items():
         for f in files_changed:
             if f.startswith(dockerfile_dir):
+                name = image_description["name"]
                 logging.info(
                     "Found changed file '%s' which affects "
                     "docker image '%s' with path '%s'",
                     f,
-                    image_description["name"],
+                    name,
                     dockerfile_dir,
                 )
-                changed_images.append(dockerfile_dir)
+                changed_images.append(DockerImage(dockerfile_dir, name))
                 break
 
     # The order is important: dependents should go later than bases, so that
     # they are built with updated base versions.
     index = 0
     while index < len(changed_images):
         image = changed_images[index]
-        for dependent in images_dict[image]["dependent"]:
+        for dependent in images_dict[image.path]["dependent"]:
             logging.info(
                 "Marking docker image '%s' as changed because it "
                 "depends on changed docker image '%s'",
                 dependent,
                 image,
             )
-            changed_images.append(dependent)
+            changed_images.append(DockerImage(dependent, image.repo, image))
         index += 1
         if index > 5 * len(images_dict):
             # Sanity check to prevent infinite loop.
             raise RuntimeError(
                 f"Too many changed docker images, this is a bug. {changed_images}"
             )
 
-    # If a dependent image was already in the list because its own files
-    # changed, but then it was added as a dependent of a changed base, we
-    # must remove the earlier entry so that it doesn't go earlier than its
-    # base. This way, the dependent will be rebuilt later than the base, and
-    # will correctly use the updated version of the base.
-    seen = set()
-    no_dups_reversed = []
-    for x in reversed(changed_images):
-        if x not in seen:
-            seen.add(x)
-            no_dups_reversed.append(x)
-
-    result = [(x, images_dict[x]["name"]) for x in reversed(no_dups_reversed)]
+    # With reversed changed_images set will use images with parents first, and
+    # images without parents then
+    result = set(reversed(changed_images))
     logging.info(
         "Changed docker images for PR %s @ %s: '%s'",
         pr_info.number,
@@ -106,66 +125,109 @@ def get_changed_docker_images(
     return result
 
 
+def gen_versions(
+    pr_info: PRInfo, suffix: Optional[str]
+) -> Tuple[List[str], Union[str, List[str]]]:
+    pr_commit_version = str(pr_info.number) + "-" + pr_info.sha
+    # The order is important, PR number is used as cache during the build
+    versions = [str(pr_info.number), pr_commit_version]
+    result_version = pr_commit_version
+    if pr_info.number == 0:
+        # First get the latest for cache
+        versions.insert(0, "latest")
+
+    if suffix:
+        # We should build architecture specific images separately and merge a
+        # manifest lately in a different script
+        versions = [f"{v}-{suffix}" for v in versions]
+        # changed_images_{suffix}.json should contain all changed images
+        result_version = versions
+
+    return versions, result_version
+
+
 def build_and_push_one_image(
-    path_to_dockerfile_folder: str, image_name: str, version_string: str, push: bool
+    image: DockerImage,
+    version_string: str,
+    push: bool,
+    child: bool,
 ) -> Tuple[bool, str]:
-    path = path_to_dockerfile_folder
     logging.info(
         "Building docker image %s with version %s from path %s",
-        image_name,
+        image.repo,
         version_string,
-        path,
+        image.full_path,
     )
     build_log = os.path.join(
-        TEMP_PATH,
-        "build_and_push_log_{}_{}".format(
-            str(image_name).replace("/", "_"), version_string
-        ),
+        TEMP_PATH, f"build_and_push_log_{image.repo.replace('/', '_')}_{version_string}"
     )
     push_arg = ""
     if push:
         push_arg = "--push "
 
-    with open(build_log, "w") as bl:
+    from_tag_arg = ""
+    if child:
+        from_tag_arg = f"--build-arg FROM_TAG={version_string} "
+
+    with open(build_log, "wb") as bl:
         cmd = (
             "docker buildx build --builder default "
-            f"--build-arg FROM_TAG={version_string} "
+            f"{from_tag_arg}"
             f"--build-arg BUILDKIT_INLINE_CACHE=1 "
-            f"--tag {image_name}:{version_string} "
-            f"--cache-from type=registry,ref={image_name}:{version_string} "
+            f"--tag {image.repo}:{version_string} "
+            f"--cache-from type=registry,ref={image.repo}:{version_string} "
             f"{push_arg}"
-            f"--progress plain {path}"
+            f"--progress plain {image.full_path}"
         )
         logging.info("Docker command to run: %s", cmd)
-        retcode = subprocess.Popen(cmd, shell=True, stderr=bl, stdout=bl).wait()
+        with subprocess.Popen(cmd, shell=True, stderr=bl, stdout=bl) as proc:
+            retcode = proc.wait()
+
         if retcode != 0:
             return False, build_log
 
-    logging.info("Processing of %s successfully finished", image_name)
+    logging.info("Processing of %s successfully finished", image.repo)
     return True, build_log
 
 
 def process_single_image(
-    versions: List[str], path_to_dockerfile_folder: str, image_name: str, push: bool
+    image: DockerImage,
+    versions: List[str],
+    push: bool,
+    child: bool,
 ) -> List[Tuple[str, str, str]]:
     logging.info("Image will be pushed with versions %s", ", ".join(versions))
     result = []
     for ver in versions:
         for i in range(5):
-            success, build_log = build_and_push_one_image(
-                path_to_dockerfile_folder, image_name, ver, push
-            )
+            success, build_log = build_and_push_one_image(image, ver, push, child)
             if success:
-                result.append((image_name + ":" + ver, build_log, "OK"))
+                result.append((image.repo + ":" + ver, build_log, "OK"))
                 break
             logging.info(
                 "Got error will retry %s time and sleep for %s seconds", i, i * 5
             )
             time.sleep(i * 5)
         else:
-            result.append((image_name + ":" + ver, build_log, "FAIL"))
+            result.append((image.repo + ":" + ver, build_log, "FAIL"))
 
     logging.info("Processing finished")
+    image.built = True
+    return result
+
+
+def process_image_with_parents(
+    image: DockerImage, versions: List[str], push: bool, child: bool = False
+) -> List[Tuple[str, str, str]]:
+    result = []  # type: List[Tuple[str,str,str]]
+    if image.built:
+        return result
+
+    if image.parent is not None:
+        result += process_image_with_parents(image.parent, versions, push, False)
+        child = True
+
+    result += process_single_image(image, versions, push, child)
     return result
 
 
@@ -182,7 +244,7 @@ def process_test_results(
             build_url = s3_client.upload_test_report_to_s3(
                 build_log, s3_path_prefix + "/" + os.path.basename(build_log)
             )
-            url_part += '<a href="{}">build_log</a>'.format(build_url)
+            url_part += f'<a href="{build_url}">build_log</a>'
         if url_part:
             test_name = image + " (" + url_part + ")"
         else:
@@ -255,8 +317,6 @@ def main():
             shell=True,
         )
 
-    repo_path = GITHUB_WORKSPACE
-
     if os.path.exists(TEMP_PATH):
         shutil.rmtree(TEMP_PATH)
     os.makedirs(TEMP_PATH)
@@ -267,43 +327,30 @@ def main():
     else:
         pr_info = PRInfo(need_changed_files=True)
 
-    changed_images = get_changed_docker_images(pr_info, repo_path, "docker/images.json")
-    logging.info(
-        "Has changed images %s", ", ".join([str(image[0]) for image in changed_images])
+    changed_images = get_changed_docker_images(
+        pr_info, GITHUB_WORKSPACE, "docker/images.json"
     )
-    pr_commit_version = str(pr_info.number) + "-" + pr_info.sha
-    # The order is important, PR number is used as cache during the build
-    versions = [str(pr_info.number), pr_commit_version]
-    result_version = pr_commit_version
-    if pr_info.number == 0:
-        # First get the latest for cache
-        versions.insert(0, "latest")
+    logging.info("Has changed images %s", ", ".join([im.path for im in changed_images]))
 
-    if args.suffix:
-        # We should build architecture specific images separately and merge a
-        # manifest lately in a different script
-        versions = [f"{v}-{args.suffix}" for v in versions]
-        # changed_images_{suffix}.json should contain all changed images
-        result_version = versions
+    image_versions, result_version = gen_versions(pr_info, args.suffix)
 
     result_images = {}
     images_processing_result = []
-    for rel_path, image_name in changed_images:
-        full_path = os.path.join(repo_path, rel_path)
-        images_processing_result += process_single_image(
-            versions, full_path, image_name, push
+    for image in changed_images:
+        images_processing_result += process_image_with_parents(
+            image, image_versions, push
         )
-        result_images[image_name] = result_version
+        result_images[image.repo] = result_version
 
     if changed_images:
-        description = "Updated " + ",".join([im[1] for im in changed_images])
+        description = "Updated " + ",".join([im.repo for im in changed_images])
     else:
         description = "Nothing to update"
 
     if len(description) >= 140:
         description = description[:136] + "..."
 
-    with open(changed_json, "w") as images_file:
+    with open(changed_json, "w", encoding="utf-8") as images_file:
         json.dump(result_images, images_file)
 
     s3_helper = S3Helper("https://s3.amazonaws.com")
@@ -317,8 +364,8 @@ def main():
 
     url = upload_results(s3_helper, pr_info.number, pr_info.sha, test_results, [], NAME)
 
-    print("::notice ::Report url: {}".format(url))
-    print('::set-output name=url_output::"{}"'.format(url))
+    print(f"::notice ::Report url: {url}")
+    print(f'::set-output name=url_output::"{url}"')
 
     if args.no_reports:
         return