#516 Perform non-CVE builds
Merged 4 years ago by gnaponie. Opened 4 years ago by gnaponie.
gnaponie/freshmaker FACTORY-5837  into  master

@@ -0,0 +1,48 @@ 

+ #!/usr/bin/env python

+ """

+ Example usage:

+     ./test_async_rebuild.py BRANCH_NAME     CSV_IMAGES_LIST

+     ./test_async_rebuild.py extras-rhel-7.8 etcd-container,rhel-server-container

+ """

+ import os

+ import sys

+ from unittest.mock import patch

+ from logging.config import dictConfig

+ import fedmsg.config

+ 

+ # Allow imports from parent directory.

+ sys.path.insert(1, os.path.join(sys.path[0], '..'))

+ 

+ # Set the FRESHMAKER_DEVELOPER_ENV variable.

+ os.environ["FRESHMAKER_DEVELOPER_ENV"] = "1"

+ os.environ["FRESHMAKER_CONFIG_FILE"] = os.path.join(sys.path[0], "config.py")

+ os.environ["REQUESTS_CA_BUNDLE"] = "/etc/ssl/certs/ca-bundle.crt"

+ 

+ from freshmaker import db, app

+ from freshmaker.events import FreshmakerAsyncManualBuildEvent

+ from freshmaker.handlers.koji import RebuildImagesOnRPMAdvisoryChange, RebuildImagesOnAsyncManualBuild

+ 

+ fedmsg_config = fedmsg.config.load_config()

+ dictConfig(fedmsg_config.get('logging', {'version': 1}))

+ 

+ if len(sys.argv) < 3:

+     print("Usage: ./test_async_rebuild.py BRANCH CSV_LIST_IMAGES")

+     sys.exit(1)

+ 

+ app_context = app.app_context()

+ app_context.__enter__()

+ 

+ db.drop_all()

+ db.create_all()

+ db.session.commit()

+ 

+ branch = sys.argv[1]

+ images = sys.argv[2].split(',')

+ kwargs = {}

+ 

+ event = FreshmakerAsyncManualBuildEvent(msg_id='fake-msg', dist_git_branch=branch, container_images=images, dry_run=True)

+ 

+ handler = RebuildImagesOnAsyncManualBuild()

+ with patch("freshmaker.consumer.get_global_consumer"):

+     with patch("freshmaker.handlers.koji.RebuildImagesOnAsyncManualBuild.start_to_build_images"):

+         handler.handle(event)

file modified
+1 -1
@@ -160,7 +160,7 @@ 

          'parsers': {

              'type': list,

              'default': [

-                 'freshmaker.parsers.internal:FreshmakerAsyncManualbuildParser',

+                 'freshmaker.parsers.koji:FreshmakerAsyncManualbuildParser',

                  'freshmaker.parsers.internal:FreshmakerManualRebuildParser',

                  'freshmaker.parsers.brew:BrewTaskStateChangeParser',

                  'freshmaker.parsers.errata:ErrataAdvisoryStateChangedParser',

@@ -524,7 +524,7 @@ 

          compose_ids = []

          for relation in build.composes:

              compose_ids.append(relation.compose.odcs_compose_id)

-         if args["renewed_odcs_compose_ids"]:

+         if args.get("renewed_odcs_compose_ids"):

              compose_ids += args["renewed_odcs_compose_ids"]

  

          for compose_id in compose_ids:

@@ -23,4 +23,3 @@ 

  from .generate_advisory_signed_event_on_rpm_sign import GenerateAdvisorySignedEventOnRPMSign  # noqa

  from .update_db_on_odcs_compose_fail import UpdateDBOnODCSComposeFail  # noqa

  from .cancel_event_on_freshmaker_manage_request import CancelEventOnFreshmakerManageRequest  # noqa

- from .rebuild_images_on_async_manual_build import RebuildOnAsyncManualBuild  # noqa 

\ No newline at end of file

@@ -1,33 +0,0 @@ 

- # -*- coding: utf-8 -*-

- # Copyright (c) 2020  Red Hat, Inc.

- #

- # Permission is hereby granted, free of charge, to any person obtaining a copy

- # of this software and associated documentation files (the "Software"), to deal

- # in the Software without restriction, including without limitation the rights

- # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

- # copies of the Software, and to permit persons to whom the Software is

- # furnished to do so, subject to the following conditions:

- #

- # The above copyright notice and this permission notice shall be included in all

- # copies or substantial portions of the Software.

- #

- # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

- # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

- # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

- # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

- # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

- # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

- # SOFTWARE.

- 

- from freshmaker.handlers import BaseHandler

- from freshmaker.events import FreshmakerAsyncManualBuildEvent

- 

- 

- class RebuildOnAsyncManualBuild(BaseHandler):

-     """Rebuild images on async.manual.build"""

- 

-     def can_handle(self, event):

-         return isinstance(event, FreshmakerAsyncManualBuildEvent)

- 

-     def handle(self, event):

-         """ TODO: """

@@ -22,3 +22,4 @@ 

  from .rebuild_images_on_odcs_compose_done import RebuildImagesOnODCSComposeDone  # noqa

  from .rebuild_images_on_parent_image_build import RebuildImagesOnParentImageBuild  # noqa

  from .rebuild_images_on_rpm_advisory_change import RebuildImagesOnRPMAdvisoryChange  # noqa

+ from .rebuild_images_on_async_manual_build import RebuildImagesOnAsyncManualBuild  # noqa

@@ -0,0 +1,381 @@ 

+ # -*- coding: utf-8 -*-

+ # Copyright (c) 2020  Red Hat, Inc.

+ #

+ # Permission is hereby granted, free of charge, to any person obtaining a copy

+ # of this software and associated documentation files (the "Software"), to deal

+ # in the Software without restriction, including without limitation the rights

+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

+ # copies of the Software, and to permit persons to whom the Software is

+ # furnished to do so, subject to the following conditions:

+ #

+ # The above copyright notice and this permission notice shall be included in all

+ # copies or substantial portions of the Software.

+ #

+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

+ # SOFTWARE.

+ 

+ import koji

+ import json

+ 

+ from freshmaker import conf, db, log

+ from freshmaker.lightblue import LightBlue

+ from freshmaker.handlers import ContainerBuildHandler, fail_event_on_handler_exception

+ from freshmaker.events import FreshmakerAsyncManualBuildEvent

+ from freshmaker.types import EventState

+ from freshmaker.models import Event

+ from freshmaker.kojiservice import koji_service

+ from freshmaker.types import ArtifactBuildState, ArtifactType

+ from freshmaker.utils import sorted_by_nvr

+ 

+ 

+ class RebuildImagesOnAsyncManualBuild(ContainerBuildHandler):

+     """Rebuild images on async.manual.build"""

+ 

+     name = 'RebuildImagesOnAsyncManualBuild'

+ 

+     def can_handle(self, event):

+         return isinstance(event, FreshmakerAsyncManualBuildEvent)

+ 

+     @fail_event_on_handler_exception

+     def handle(self, event):

+         """

+         Rebuilds all container images requested by the user and the tree in between the

+         requested images, if relationships between them are found.

+         """

+ 

+         if event.dry_run:

+             self.force_dry_run()

+ 

+         self.event = event

+ 

+         db_event = Event.get_or_create_from_event(db.session, event)

+         self.set_context(db_event)

+ 

+         # Check if we are allowed to build this image.

+         if not self.event.is_allowed(self, ArtifactType.IMAGE):

+             msg = ("This image rebuild is not allowed by internal policy. "

+                    f"message_id: {event.msg_id}")

+             db_event.transition(EventState.SKIPPED, msg)

+             db.session.commit()

+             self.log_info(msg)

+             return []

+ 

+         lb = self.init_lightblue_instance()

+         # images contains at this point a list of images with all NVR for the same package

+         images = self._find_images_to_rebuild(lb)

+ 

+         # Since the input is an image name, and not an NVR, freshmaker won't be able to know

+         # exactly which one needs to be rebuilt. For this reason Freshmaker asked Lightblue

+         # all the NVRs that match that image name. Now we need to check which one has the

+         # dist_git_branch. If more than one is found Freshmaker will choose the one with the

+         # highest NVR.

+         images = self.filter_images_based_on_dist_git_branch(images, db_event)

+         if not images:

+             return []

+ 

+         # If the user requested to rebuild only one image, there's no need to find out all the tree

+         # it is just more efficient to return that single image

+         if len(images) == 1:

+             batches = [images]

+         else:

+             images_trees = self.find_images_trees_to_rebuild(images, lb)

+             to_rebuild = self.filter_out_unrelated_images(images_trees)

+             batches = self.generate_batches(to_rebuild, images, lb)

+ 

+         builds = self._record_batches(batches, db_event, lb)

+ 

+         if not builds:

+             msg = f"No container images to rebuild for event with message_id {event.msg_id}"

+             self.log_info(msg)

+             db_event.transition(EventState.SKIPPED, msg)

+             db.session.commit()

+             return []

+ 

+         if all([build.state == ArtifactBuildState.FAILED.value

+                 for build in builds.values()]):

+             db_event.transition(

+                 EventState.COMPLETE,

+                 "No container images to rebuild, all are in failed state.")

+             db.session.commit()

+             return []

+ 

+         self.start_to_build_images(

+             db_event.get_image_builds_in_first_batch(db.session))

+ 

+         msg = 'Rebuilding %d container images.' % (len(db_event.builds.all()))

+         db_event.transition(EventState.BUILDING, msg)

+ 

+         return []

+ 

+     def init_lightblue_instance(self):

+         return LightBlue(server_url=conf.lightblue_server_url,

+                          cert=conf.lightblue_certificate,

+                          private_key=conf.lightblue_private_key,

+                          event_id=self.event.msg_id)

+ 

+     def filter_out_unrelated_images(self, batches):

+         """

+         Filters out images that are unrelated to the requested images.

+         Example:

+         batches =

+             [

+                 [image_b, image_a, image_0],

+                 [image_d, image_a, image_0]

+             ]

+         Returns:

+         [

+             [image_b],

+             [image_d]

+         ]

+ 

+         :param batches: list of lists, the first item is the image requested and the

+             following items are its tree, up to the base image.

+         :return: container images that will actually be rebuilt, because the user requested them

+             or because they are part of the tree that needs to be rebuilt.

+         """

+         new_batches = []

+         for batch in batches:

+             # We expect the first item in the list to always be in the requested images

+             # if not, there must be something wrong... maybe we should return an error.

+             if batch[0]['brew']['package'] not in self.event.container_images:

+                 self.log_info('Unexpected error identifying images to rebuild.')

+                 return []

+             filtered_batch = []

+             maybe_batch = []

+             for image in batch:

+                 maybe_batch.append(image)

+                 if image['brew']['package'] in self.event.container_images:

+                     filtered_batch.extend(maybe_batch)

+                     maybe_batch = []

+             new_batches.append(filtered_batch)

+         return new_batches

+ 

+     def generate_batches(self, to_rebuild, images, lb):

+         # Get all the directly affected images so that any parents that are not marked as

+         # directly affected can be set in _images_to_rebuild_to_batches

+         directly_affected_nvrs = {

+             image.nvr for image in images if image.get("directly_affected")

+         }

+         # Now generate batches from deduplicated list and return it.

+         return lb._images_to_rebuild_to_batches(to_rebuild, directly_affected_nvrs)

+ 

+     def _find_images_to_rebuild(self, lb):

+         """

+         Since the input is an image name, and not an NVR, freshmaker won't be able to know

+         exactly which one needs to be rebuilt. For this reason Freshmaker will ask Lightblue

+         all the NVRs that match that image name. It will then check which one has the

+         dist_git_branch. If more than one is found Freshmaker will choose the one with the highest

+         NVRArtifactType.IMAGE.

+ 

+         :param lb LightBlue: LightBlue instance

+         :return: list of container images matching a certain name.

+         :rtype: list

+         """

+ 

+         return lb.get_images_by_brew_package(self.event.container_images)

+ 

+     def get_image_tree(self, lb, image, tree):

+         """

+         This method recursively finds the tree for given image, up to the base image.

+         At every recursive call it will add one element of the tree.

+         When the base image is reached, the tree will be completed.

+ 

+         :param lb LightBlue: LightBlue instance

+         :param image ContainerImage: image of which we want the tree.

+         :param tree list: the list of images that we found until now.

+         :return: list of images, in this order: [parent, grandparent, ..., baseimage]

+         :rtype: list

+         """

+         parent_nvr = lb.find_parent_brew_build_nvr_from_child(image)

+         if parent_nvr:

+             parent = lb.get_images_by_nvrs([parent_nvr], published=None)

+             if parent:

+                 parent = parent[0]

+                 parent.resolve(lb)

+                 image['parent'] = parent

+                 tree.append(parent)

+                 return self.get_image_tree(lb, parent, tree)

+         return tree

+ 

+     def filter_images_based_on_dist_git_branch(self, images, db_event):

+         """

+         Filter images based on the dist-git branch requested by the user. If the images were never

+         be built for that branch, let's skip the build.

+         The input images are all the images matching a specific name. In this method we also select

+         the images with higher NVR for the same name (package).

+ 

+         :param images list: images to rebuild.

+         :param db_event Event: event object in the db.

+         :return: list of images to rebuild. If the event gets skipped, return empty list.

+         :rtype: list

+         """

+         with koji_service(

+                 conf.koji_profile, log, dry_run=conf.dry_run,

+                 login=False) as session:

+ 

+             # Sort images by nvr

+             images = sorted_by_nvr(images, reverse=True)

+ 

+             # Use a dict to map a package (name) to the highest NVR image. For example:

+             # {"ubi8-container": ContainerImage<nvr=ubi8-container-8.1-100>,

+             # "nodejs-12-container": ContainerImage<nvr=nodejs12-container-1.0-101>)}

+             images_to_rebuild = {}

+ 

+             # In case the user requested to build ['s2i-core-container', 'cnv-libvirt-container']

+             # lightblue will return a bunch of NVRs for each name, example:

+             #   * s2i-core-container-1-127

+             #   * s2i-core-container-1-126

+             #   * ...

+             #   * cnv-libvirt-container-1.3-1

+             #   * cnv-libvirt-container-1.2-4

+             #   * ...

+             # Since `images` is a list of sorted NVRs, we just need to select the first NVR for

+             # each package (name).

+             for image in images:

+                 build = None

+                 git_branch = None

+ 

+                 package = image['brew']['package']

+                 # if package is already in images_to_rebuild we don't need to keep searching

+                 # since the images were sorted by NVR in the beginning

+                 if package not in images_to_rebuild:

+                     # Let's get the branch for this image

+                     build = session.get_build(image.nvr)

+                     task_id = build.get("extra", {}).get("container_koji_task_id")

+                     if task_id:

+                         task = session.get_task_request(task_id)

+                         # The task_info should always be in the 3rd element

+                         task_info = task[2]

+                         git_branch = task_info.get("git_branch") if len(task_info) else None

+ 

+                     if (build and task_id and git_branch and

+                             self.event.dist_git_branch == git_branch):

+                         images_to_rebuild[package] = image

+ 

+             if not images_to_rebuild or len(images_to_rebuild) < len(self.event.container_images):

+                 # If we didn't find images to rebuild, or we found less than what the user asked

+                 # it means that some of those images were never been built before for the requested

+                 # branch. In this case we need to throw an error because we won't build something

+                 # that was never built before.

+                 # We cannot return to the API with an error, because the request already completed

+                 # at this point. Let's mark this build as FAILED then.

+                 msg = ("One or more of the requested image was never built before for the "

+                        f"requested branch: {self.event.dist_git_branch}. "

+                        "Cannot build it, please change your request.")

+                 missing_images = set(self.event.container_images) - set(images_to_rebuild.keys())

+                 if missing_images:

+                     msg += f" Problematic images are {missing_images}"

+                 db_event.transition(EventState.FAILED, msg)

+                 db.session.commit()

+                 self.log_info(msg)

+                 return []

+ 

+             # The result needs to be a list

+             return list(images_to_rebuild.values())

+ 

+     def find_images_trees_to_rebuild(self, images_to_rebuild, lb):

+         """

+         At this point images_to_rebuilds contains the images to rebuild requested by the

+         user. We now need to find out if there's some dependency between these images.

+         Example: image A is parent image of B, B is parent image of C (A > B > C). The user

+         requests to build A and C. In this case we'll also have to build B, and we need to

+         build all of them in the right order (first A, then B, and C in the end).

+         Let's find out if those images are related to each other. To do that, we find check

+         all the hierarchy of these images until we don't reach the base image. Then we check

+         if these images are related (if A appears in the parent tree of C).

+ 

+         This method, for the given input images, return their trees, after deduplication.

+ 

+         :param images_to_rebuild list: list of images to rebuild.

+         :param lb LightBlue: LightBlue instance

+         :return: list of trees of images.

+         :rtype: list of lists

+         """

+         # images_trees will be a list of lists, where the first elements in the list will be

+         # the requested images and the following elements are the elements in the tree up to the

+         # base image.

+         images_trees = []

+         for image in images_to_rebuild:

+             images_trees.append([image] + self.get_image_tree(lb, image, []))

+ 

+         # Let's remove duplicated images which share the same name and version, but different

+         # release.

+         to_rebuild = lb._deduplicate_images_to_rebuild(images_trees)

+         return to_rebuild

+ 

+     def _record_batches(self, batches, db_event, lb):

+         """

+             Records the images from batches to the database.

+ 

+         :param batches list: Output of LightBlue._find_images_to_rebuild(...).

+         :param db_event: event to handle.

+         :param lb LightBlue: LightBlue instance

+         :return: a mapping between image build NVR and corresponding ArtifactBuild

+             object representing a future rebuild of that. It is extended by including

+             those images stored into database.

+         :rtype: dict

+         """

+         # builds tracks all the builds we register in db

+         builds = {}

+ 

+         for batch in batches:

+             for image in batch:

+                 # Reset context to db_event for each iteration before

+                 # the ArtifactBuild is created.

+                 self.set_context(db_event)

+ 

+                 nvr = image["brew"]["build"]

+ 

+                 self.log_debug("Recording %s", nvr)

+                 parent_nvr = image["parent"].nvr \

+                     if "parent" in image and image["parent"] else None

+ 

+                 if "error" in image and image["error"]:

+                     state_reason = image["error"]

+                     state = ArtifactBuildState.FAILED.value

+                 else:

+                     state_reason = ""

+                     state = ArtifactBuildState.PLANNED.value

+ 

+                 image_name = koji.parse_NVR(image["brew"]["build"])["name"]

+                 parent_nvr = image["parent"].nvr \

+                     if "parent" in image and image["parent"] else None

+                 dep_on = builds[parent_nvr] if parent_nvr in builds else None

+ 

+                 # We don't need to rebuild the nvr this time. The release value

+                 # will be automatically generated by OSBS.

+                 build = self.record_build(

+                     self.event, image_name, ArtifactType.IMAGE,

+                     dep_on=dep_on,

+                     state=ArtifactBuildState.PLANNED.value,

+                     original_nvr=nvr)

+ 

+                 # Set context to particular build so logging shows this build

+                 # in case of error.

+                 self.set_context(build)

+ 

+                 image.resolve(lb)

+                 build.transition(state, state_reason)

+                 build_args = {}

+                 build_args["repository"] = image['repository']

+                 build_args["commit"] = image["commit"]

+                 build_args["target"] = (

+                     self.event.brew_target if self.event.brew_target else image["target"])

@lucarval I believe (if I understood correctly what this parameter is for) that this should be enough to address the brew_target parameter.

+                 build_args["branch"] = image["git_branch"]

+                 build_args["original_parent"] = parent_nvr,

+                 build_args["arches"] = image["arches"]

+                 build.build_args = json.dumps(build_args)

+ 

+                 db.session.commit()

+ 

+                 builds[nvr] = build

+ 

+         # Reset context to db_event.

+         self.set_context(db_event)

+ 

+         return builds

file modified
+30
@@ -1034,6 +1034,36 @@ 

              images = self.filter_out_images_with_higher_srpm_nvr(images, srpm_name_to_nvrs)

          return images

  

+     def get_images_by_brew_package(self, names):

+         """

+         Query Lightblue to get all the images for a specific list of names.

+         :param names list: list of names we want to find images for.

+         :return: list of container images matching the requested names.

+         :rtype: list of ContainerImages

+         """

+ 

+         query = {

+             "objectType": "containerImage",

+             "query": {

+                 "$and": [

+                     {

+                         "field": "repositories.*.published",

+                         "op": "=",

+                         "rvalue": True

+                     },

+                     {

+                         "$or": [{

+                             "field": "brew.package",

+                             "op": "=",

+                             "rvalue": name,

+                         } for name in names]

+                     }

+                 ]

+             },

+             "projection": self._get_default_projection(include_rpm_manifest=False)

+         }

+         return self.find_container_images(query)

+ 

      def find_parent_brew_build_nvr_from_child(self, child_image):

          """

          Returns the parent brew build NVR of the input image. If the parent is not found it returns None.

@@ -21,4 +21,3 @@ 

  

  from .manual_rebuild import FreshmakerManualRebuildParser  # noqa

  from .freshmaker_manage_request import FreshmakerManageRequestParser  # noqa

- from .async_manual_build import FreshmakerAsyncManualbuildParser  # noqa

@@ -20,3 +20,4 @@ 

  # SOFTWARE.

  

  from .task_state_change import KojiTaskStateChangeParser  # noqa

+ from .async_manual_build import FreshmakerAsyncManualbuildParser  # noqa

freshmaker/parsers/koji/async_manual_build.py freshmaker/parsers/internal/async_manual_build.py
file renamed
+1 -3
@@ -44,7 +44,7 @@ 

          """

          msg_id = data.get('msg_id', "async_build_%s" % (str(time.time())))

  

-         event = FreshmakerAsyncManualBuildEvent(

+         return FreshmakerAsyncManualBuildEvent(

              msg_id, data.get('dist_git_branch'), data.get('container_images', []),

              freshmaker_event_id=data.get('freshmaker_event_id'),

              brew_target=data.get('brew_target'),
@@ -52,8 +52,6 @@ 

              requester=data.get('requester', None),

              requester_metadata_json=data.get("metadata", None))

  

-         return event

- 

      def parse(self, topic, msg):

          inner_msg = msg['msg']

          return self.parse_post_data(inner_msg)

file modified
+7 -1
@@ -41,7 +41,7 @@ 

  from freshmaker.api_utils import pagination_metadata

  from freshmaker.auth import login_required, requires_roles, require_scopes, user_has_role

  from freshmaker.parsers.internal.manual_rebuild import FreshmakerManualRebuildParser

- from freshmaker.parsers.internal.async_manual_build import FreshmakerAsyncManualbuildParser

+ from freshmaker.parsers.koji.async_manual_build import FreshmakerAsyncManualbuildParser

  from freshmaker.monitor import (

      monitor_api, freshmaker_build_api_latency, freshmaker_event_api_latency)

  from freshmaker.image_verifier import ImageVerifier
@@ -607,6 +607,12 @@ 

                      'event.'.format(data.get('freshmaker_event_id')),

                  )

  

+         # The '-container' string is optional, the user might have omitted it. But we need it to be

+         # there for our query. Let's check if it's there, and if it's not, let's add it.

+         for i, image in enumerate(data.get('container_images', [])):

+             if not image.endswith('-container'):

+                 data.get('container_images')[i] = f"{image}-container"

+ 

          # parse the POST data and generate FreshmakerAsyncManualBuildEvent

          parser = FreshmakerAsyncManualbuildParser()

          db_event = _create_rebuild_event_from_request(db.session, parser, request)

@@ -1,34 +0,0 @@ 

- # -*- coding: utf-8 -*-

- # Copyright (c) 2020  Red Hat, Inc.

- #

- # Permission is hereby granted, free of charge, to any person obtaining a copy

- # of this software and associated documentation files (the "Software"), to deal

- # in the Software without restriction, including without limitation the rights

- # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

- # copies of the Software, and to permit persons to whom the Software is

- # furnished to do so, subject to the following conditions:

- #

- # The above copyright notice and this permission notice shall be included in

- # all copies or substantial portions of the Software.

- #

- # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

- # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

- # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

- # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

- # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

- # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

- # SOFTWARE.

- 

- from unittest import TestCase

- 

- from freshmaker.handlers.internal import RebuildOnAsyncManualBuild

- from freshmaker.events import FreshmakerAsyncManualBuildEvent

- 

- 

- class TestRebuildOnAsyncManualBuild(TestCase):

- 

-     def test_can_handle_event(self):

-         event = FreshmakerAsyncManualBuildEvent(

-             'msg-id-01', 'repo-branch', ['image1', 'image2'])

-         handler = RebuildOnAsyncManualBuild()

-         self.assertTrue(handler.can_handle(event))

@@ -0,0 +1,389 @@ 

+ # -*- coding: utf-8 -*-

+ # Copyright (c) 2020  Red Hat, Inc.

+ #

+ # Permission is hereby granted, free of charge, to any person obtaining a copy

+ # of this software and associated documentation files (the "Software"), to deal

+ # in the Software without restriction, including without limitation the rights

+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

+ # copies of the Software, and to permit persons to whom the Software is

+ # furnished to do so, subject to the following conditions:

+ #

+ # The above copyright notice and this permission notice shall be included in

+ # all copies or substantial portions of the Software.

+ #

+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

+ # SOFTWARE.

+ 

+ from freshmaker import db

+ from freshmaker.handlers.koji import RebuildImagesOnAsyncManualBuild

+ from freshmaker.events import FreshmakerAsyncManualBuildEvent

+ from freshmaker.types import EventState

+ from freshmaker.models import Event

+ from freshmaker.lightblue import ContainerImage

+ from tests import helpers

+ 

+ 

+ class TestRebuildImagesOnAsyncManualBuild(helpers.ModelsTestCase):

+ 

+     def setUp(self):

+         super(TestRebuildImagesOnAsyncManualBuild, self).setUp()

+ 

+         self.patcher = helpers.Patcher(

+             'freshmaker.handlers.koji.RebuildImagesOnAsyncManualBuild.')

+         # We do not want to send messages to message bus while running tests

+         self.mock_messaging_publish = self.patcher.patch(

+             'freshmaker.messaging.publish')

+ 

+         # Mocking koji

+         self.mock_get_build = self.patcher.patch(

+             'freshmaker.kojiservice.KojiService.get_build',

+             return_value={'build_id': 123456, 'extra': {'container_koji_task_id': 21938204}})

+         self.mock_get_task_request = self.patcher.patch(

+             'freshmaker.kojiservice.KojiService.get_task_request', return_value=[

+                 'git://example.com/rpms/repo-1#commit_hash1',

+                 'test-target',

+                 {'compose_ids': None,

+                  'git_branch': 'test_branch',

+                  'scratch': False,

+                  'signing_intent': None,

+                  'yum_repourls': [('fake-url.repo')]}])

+ 

+         self.mock_allow_build = self.patcher.patch('allow_build', return_value=True)

+ 

+         # Mocking Lightblue

+         self.mock_find_images_to_rebuild = self.patcher.patch('_find_images_to_rebuild')

+         self.mock_lightblue = self.patcher.patch('init_lightblue_instance')

+ 

+         self.mock_start_to_build_images = self.patcher.patch('start_to_build_images')

+         self.mock_get_image_builds_in_first_batch = self.patcher.patch(

+             'freshmaker.models.Event.get_image_builds_in_first_batch')

+ 

+         # Structure of the images used for testing:

+         #       image_0

+         #         +

+         #         |

+         #         +

+         #      image_a

+         #         +

+         #         |

+         #    +----+----+

+         #    |         |

+         #    +         +

+         # image_b    image_d

+         #              +

+         #              |

+         #              +

+         #            image_e

+ 

+         # image_c is unrelated.

+ 

+         # image_0 is a base image, with no parent

+         self.image_0 = ContainerImage({

+             'repository': 'repo_1',

+             'commit': '1234567',

+             'target': 'container-candidate',

+             'git_branch': 'test_branch',

+             'content_sets': ['image_0_content_set_1', 'image_0_content_set_2'],

+             'arches': 'x86_64',

+             'brew': {

+                 'build': 'image-container-1.0-2',

+                 'package': 'image-container',

+             },

+             'parent': None,

+             'parsed_data': {

+                 'layers': [

+                     'sha512:7890',

+                     'sha512:5678',

+                 ]

+             },

+             'published': False,

+         })

+ 

+         self.image_a = ContainerImage({

+             'repository': 'repo_1',

+             'commit': '1234567',

+             'target': 'container-candidate',

+             'git_branch': 'test_branch',

+             'content_sets': ['image_a_content_set_1', 'image_a_content_set_2'],

+             'arches': 'x86_64',

+             'brew': {

+                 'build': 'image-a-container-1.0-2',

+                 'package': 'image-a-container',

+             },

+             'parent': self.image_0,

+             'parsed_data': {

+                 'layers': [

+                     'sha512:7890',

+                     'sha512:5678',

+                 ]

+             },

+             'published': False,

+         })

+ 

+         # image_b is a child image of image_a

+         self.image_b = ContainerImage({

+             'repository': 'repo_2',

+             'commit': '5678901',

+             'target': 'container-candidate',

+             'git_branch': 'test_branch',

+             'content_sets': ['image_b_content_set_1', 'image_b_content_set_2'],

+             'arches': 'x86_64',

+             'brew': {

+                 'build': 'image-b-container-2.14-1',

+                 'package': 'image-b-container'

+             },

+             'parent': self.image_a,

+             'parsed_data': {

+                 'layers': [

+                     'sha512:f109',

+                     'sha512:7890',

+                     'sha512:5678',

+                 ]

+             },

+             'published': False,

+         })

+ 

+         # image_c is an image unrelated to image_a and image_b

+         # it also has no parent image.

+         # image_c has the same name of image_a, that's why it has this name

+         self.image_c = ContainerImage({

+             'repository': 'repo_1',

+             'commit': '1234569',

+             'target': 'container-candidate',

+             'git_branch': 'test_branch',

+             'content_sets': ['image_a_content_set_1', 'image_a_content_set_2'],

+             'arches': 'x86_64',

+             'brew': {

+                 'build': 'image-a-container-1.0-3',

+                 'package': 'image-a-container',

+             },

+             'parent': None,

+             'parsed_data': {

+                 'layers': [

+                     'sha512:7890',

+                     'sha512:5678',

+                 ]

+             },

+             'published': False,

+         })

+ 

+         # image_d is a child image of image_a, same as image_b

+         # so image_d and image_b are unrelated, since they are sibilings

+         self.image_d = ContainerImage({

+             'repository': 'repo_2',

+             'commit': '5678906',

+             'target': 'container-candidate',

+             'git_branch': 'test_branch',

+             'content_sets': ['image_d_content_set_1', 'image_d_content_set_2'],

+             'arches': 'x86_64',

+             'brew': {

+                 'build': 'image-d-container-3.3-1',

+                 'package': 'image-d-container'

+             },

+             'parent': self.image_a,

+             'parsed_data': {

+                 'layers': [

+                     'sha512:f109',

+                 ]

+             },

+             'published': False,

+         })

+ 

+         # image_e is a child image of image_d

+         self.image_e = ContainerImage({

+             'repository': 'repo_2',

+             'commit': '5678906',

+             'target': 'container-candidate',

+             'git_branch': 'test_branch',

+             'content_sets': ['image_e_content_set_1', 'image_e_content_set_2'],

+             'arches': 'x86_64',

+             'brew': {

+                 'build': 'image-e-container-3.3-1',

+                 'package': 'image-e-container'

+             },

+             'parent': self.image_d,

+             'parsed_data': {

+                 'layers': [

+                     'sha512:f109',

+                 ]

+             },

+             'published': False,

+         })

+ 

+     def test_can_handle_event(self):

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-01', 'repo-branch', ['image1', 'image2'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         self.assertTrue(handler.can_handle(event))

+ 

+     def test_building_single_image(self):

+         """

+         This tests the successful build of a single image

+         """

+         self.mock_find_images_to_rebuild.return_value = [self.image_a]

+         self.mock_find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[[self.image_a]])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'test_branch', ['image-a-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+ 

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.BUILDING.value, db_event.state)

+         self.mock_get_image_builds_in_first_batch.assert_called_once_with(db.session)

+         self.assertEqual(len(db_event.builds.all()), 1)

+         self.mock_start_to_build_images.assert_called_once()

+ 

+     def test_building_related_images_correct_order(self):

+         """

+         This tests the successful build of 2 related images in the correct order.

+         """

+         self.mock_find_images_to_rebuild.return_value = [self.image_a, self.image_b]

+         self.mock_find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[[self.image_a, self.image_b]])

+         self.mock_generate_batches = self.patcher.patch('generate_batches', return_value=[

+             [self.image_a],

+             [self.image_b]

+         ])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'test_branch', ['image-b-container', 'image-a-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+ 

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.BUILDING.value, db_event.state)

+         self.mock_get_image_builds_in_first_batch.assert_called_once_with(db.session)

+         self.assertEqual(len(db_event.builds.all()), 2)

+         self.mock_start_to_build_images.assert_called_once()

+ 

+     def test_failed_to_build_images_never_built_before(self):

+         """

+         This test checks that trying to build an image that was never built before (for that

+         branch) will make the build fail.

+         """

+         self.mock_find_images_to_rebuild.return_value = [self.image_a]

+         self.mock_find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[[self.image_a]])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'another-branch', ['image-a-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.FAILED.value, db_event.state)

+ 

+     def test_multiple_nvrs_for_the_same_name(self):

+         """

+         This test checks that when for one name more nvrs are returned by lightblue, Freshmaker

+         will pick the one with higher nvr.

+         """

+         self.mock_find_images_to_rebuild.return_value = [self.image_a, self.image_c]

+         self.mock_find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[[self.image_c]])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'test_branch', ['image-a-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+ 

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.BUILDING.value, db_event.state)

+         self.mock_get_image_builds_in_first_batch.assert_called_once_with(db.session)

+         self.mock_start_to_build_images.assert_called_once()

+         self.assertEqual(len(db_event.builds.all()), 1)

+         self.assertEqual(db_event.builds.one().original_nvr, 'image-a-container-1.0-3')

+ 

+     def test_building_sibilings(self):

+         """

+         This test checks that when the users requests to rebuild 2 images that are sibilings

+         (or other unrelated images) Freshmaker will rebuild them separately, without the need

+         of rebuilding the parent.

+         """

+         self.mock_find_images_to_rebuild.return_value = [self.image_b, self.image_d]

+         self.find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[

+                 [self.image_b, self.image_a, self.image_0],

+                 [self.image_d, self.image_a, self.image_0]])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'test_branch', ['image-b-container', 'image-d-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+ 

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.BUILDING.value, db_event.state)

+         self.mock_get_image_builds_in_first_batch.assert_called_once_with(db.session)

+         self.assertEqual(len(db_event.builds.all()), 2)

+         self.mock_start_to_build_images.assert_called_once()

+ 

+     def test_building_images_with_disconnected_tree(self):

+         self.mock_find_images_to_rebuild.return_value = [self.image_b, self.image_d, self.image_e]

+         self.find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[

+                 [self.image_b, self.image_a, self.image_0],

+                 [self.image_d, self.image_a, self.image_0],

+                 [self.image_e, self.image_d, self.image_a, self.image_0]])

+         self.mock_generate_batches = self.patcher.patch('generate_batches', return_value=[

+             [self.image_b, self.image_d],

+             [self.image_e]

+         ])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'test_branch',

+             ['image-b-container', 'image-d-container', 'image-e-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+ 

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.BUILDING.value, db_event.state)

+         self.mock_get_image_builds_in_first_batch.assert_called_once_with(db.session)

+         self.assertEqual(len(db_event.builds.all()), 3)

+         self.mock_start_to_build_images.assert_called_once()

+ 

+     def test_intermediate_images_are_build(self):

+         self.mock_find_images_to_rebuild.return_value = [self.image_b, self.image_d, self.image_0]

+         self.find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[

+                 [self.image_0],

+                 [self.image_b, self.image_a, self.image_0],

+                 [self.image_d, self.image_a, self.image_0]])

+         self.mock_generate_batches = self.patcher.patch('generate_batches', return_value=[

+             [self.image_0],

+             [self.image_a],

+             [self.image_b, self.image_d]

+         ])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'test_branch',

+             ['image-container', 'image-b-container', 'image-d-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+ 

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.BUILDING.value, db_event.state)

+         self.mock_get_image_builds_in_first_batch.assert_called_once_with(db.session)

+         self.assertEqual(len(db_event.builds.all()), 4)

+         self.mock_start_to_build_images.assert_called_once()

+ 

+     def test_related_images_are_built(self):

+         self.mock_find_images_to_rebuild.return_value = [self.image_b, self.image_d, self.image_a]

+         self.find_images_trees_to_rebuild = self.patcher.patch(

+             'find_images_trees_to_rebuild', return_value=[

+                 [self.image_a, self.image_0],

+                 [self.image_b, self.image_a, self.image_0],

+                 [self.image_d, self.image_a, self.image_0]])

+         self.mock_generate_batches = self.patcher.patch('generate_batches', return_value=[

+             [self.image_a],

+             [self.image_b, self.image_d]

+         ])

+         event = FreshmakerAsyncManualBuildEvent(

+             'msg-id-123', 'test_branch',

+             ['image-a-container', 'image-b-container', 'image-d-container'])

+         handler = RebuildImagesOnAsyncManualBuild()

+         handler.handle(event)

+ 

+         db_event = Event.get(db.session, 'msg-id-123')

+         self.assertEqual(EventState.BUILDING.value, db_event.state)

+         self.mock_get_image_builds_in_first_batch.assert_called_once_with(db.session)

+         self.assertEqual(len(db_event.builds.all()), 3)

+         self.mock_start_to_build_images.assert_called_once()

file modified
+6 -6
@@ -981,7 +981,7 @@ 

          self.client = app.test_client()

  

      @patch('freshmaker.messaging.publish')

-     @patch('freshmaker.parsers.internal.async_manual_build.time.time')

+     @patch('freshmaker.parsers.koji.async_manual_build.time.time')

      def test_async_build(self, time, publish):

          time.return_value = 123

          with patch('freshmaker.models.datetime') as datetime_patch:
@@ -1007,7 +1007,7 @@ 

              u'event_type_id': 14,

              u'id': 1,

              u'message_id': 'async_build_123',

-             u'requested_rebuilds': ['foo-1-1', 'bar-1-1'],

+             u'requested_rebuilds': ['foo-1-1-container', 'bar-1-1-container'],

              u'requester': 'root',

              u'requester_metadata': {},

              u'search_key': 'async_build_123',
@@ -1023,12 +1023,12 @@ 

              {

                  'msg_id': 'async_build_123',

                  'dist_git_branch': 'master',

-                 'container_images': ['foo-1-1', 'bar-1-1'],

-                 'requester': 'root',

+                 'container_images': ['foo-1-1-container', 'bar-1-1-container'],

+                 'requester': 'root'

              })

  

      @patch('freshmaker.messaging.publish')

-     @patch('freshmaker.parsers.internal.async_manual_build.time.time')

+     @patch('freshmaker.parsers.koji.async_manual_build.time.time')

      def test_async_build_dry_run(self, time, publish):

          time.return_value = 123

  
@@ -1049,7 +1049,7 @@ 

              {

                  'msg_id': 'async_build_123',

                  'dist_git_branch': 'master',

-                 'container_images': ['foo-1-1', 'bar-1-1'],

+                 'container_images': ['foo-1-1-container', 'bar-1-1-container'],

                  'dry_run': True,

                  'requester': 'root',

              })

Freshmaker now will be able to perform non-CVE builds, when triggered
by a user. It will take care of building the requested images in the
correct order, and to fill possible gaps between images.

ref: FACTORY-5837

WIP: new tests are missing

Signed-off-by: Giulia Naponiello gnaponie@redhat.com

Can we use brew.package for an exact match?

Consider removing this object. I'm not sure what the purpose of this match is, but it seems to not have any impact.

Do we want to restrict the query to just published content?

Can we use brew.package for an exact match?

If that's the case, consider renaming this method to get_images_by_brew_package.

if not image.endswith('-container'):

You may want to apply this modification as early as possible. This would mean, when the API request is received.

Consider always reaching for the 3rd element. The first one is the git source URL, the second the koji build target, and the third are the options.

What if 3 images are requested, but only 2 are found? We want to raise an error as well right?

I think we can make this processing a little bit more efficient.

Consider sorting the images in reverse before iterating over them. This will allow skipping a bunch of calls to Koji in most cases.

# Use a dict to map a package to the highest NVR image with matching branch. For example:
#  {"ubi8-container": ContainerImage<nvr=ubi8-container-8.1-100>, "nodejs-12-container": ContainerImage<nvr=nodejs12-container-1.0-101>)}
images_to_rebuild = {}

# Sort images by nvr
images = sorted_by_nvr(images, reverse=True)

for image in images:
  package = ... # get package from image
  if package in images_to_rebuild:
    if image.nvr <= images_to_rebuild[package].nvr:
      # We have already found a better match, skip this image.
      continue
  image_branch = ... # get branch for the image
  if image_branch == expected_branch:
    images_to_rebuild[package] = image

What if 3 images are requested, but only 2 are found? We want to raise an error as well right?

Mmm, yes, I guess. Especially if the requested images are related to each other. Let's change that.

Can we use brew.package for an exact match?

I'm addressing this comment. Because of that, I removed the *, because at this point I believe it should be an exact match. Let me know if you think otherwise.

Can you name the handler class according to filename - RebuildImagesOnAsyncManualBuild?
I think this handler should be in the ./freshmaker/handlers/koji/ directory, because it uses Koji for rebuilds. Maybe we should put some README in some other PR here to store what has been decided long ago in Proposed solution of https://docs.google.com/document/d/1dx6cuFblaZlf0BZtPU6kv-ZzNJgf2JHWQ-VfSDuHVyM/edit#heading=h.x6yj4z1j38x.

I've added some item in the parking lot ideas we have for the guild. If necessary I can file a card.

rebased onto 27b557059c7b6f1118e8ec9defeea2178ff573f6

4 years ago

rebased onto 47119ab4c6b559023899dcda56d3ddc8238716c0

4 years ago

@lucarval @jkaluza comments addressed. It is ready for another review.

if not image.endswith('-container'):

You may want to apply this modification as early as possible. This would mean, when the API request is received.

@gnaponie, this was not addressed. It would make things much simpler now and in the future if the suffix was added as needed in the initial processing of the API request.

Will self.event.dist_git_branch ever be None? If not, then this if statement could be significantly reduced to if self.event.dist_git_branch == git_branch):

This doesn't looks right. Sure the API request has already completed, but we shouldn't set the event as skipped. I think if we raise an exception here, then it should mark the even as failed due to the usage of the fail_event_on_handler_exception decorator.

Images in multi-stream repos may never be tagged with "latest" tag. Instead they will have a per version "latest" tag, e.g. "v3.4", "v3.5". Removing this filter is probably the right thing, but you may get a much larger result set.

This doesn't looks right. Sure the API request has already completed, but we shouldn't set the event as skipped. I think if we raise an exception here, then it should mark the even as failed due to the usage of the fail_event_on_handler_exception decorator.

Yeah we can also mark it as FAILED. I thought SKIPPED maybe was better. But I'm ok also with marking it as FAILED. It seems more consistent maybe with other cases we already mark as FAILED

Will self.event.dist_git_branch ever be None? If not, then this if statement could be significantly reduced to if self.event.dist_git_branch == git_branch):

It is a mandatory field. But we could have build or task_id or git_branch == None, we might have encourred errors retrieving them. So I believe this check would be necessary.

rebased onto 4d1ce28b536faafb09141cd2cc2e32d76f5947c0

4 years ago

rebased onto 469a338

4 years ago

rebased onto 677023e76696ea0497bc225fcce2e4abe0719646

4 years ago

rebased onto 4a89996a7af76ab4a55547b45799abd1890ab907

4 years ago

rebased onto 3fe44328a5db356bdaedf965cddeeddf63941a15

4 years ago

@jkaluza @lucarval it should be complete now. Can you review again?

include_rpms is not a valid parameter

You should use an exact match, not regex.

I would've expected batches to container batches, not images. Should this be:

for batch in batches:
  for image in batch:
    ...

It looks like the image hasn't been resolved at this point. So this information won't be available.

I modified dev_scripts/find_images_to_rebuild.py to test this out. After modifying the call to _get_default_projection to use the correct parameter include_rpm_manifest, the code executes.

Changing the LightBlue query to not use a regex makes the execution much faster. Less matches to process.

I was able to get it to execute until _record_batches then I hit issues related to the image not being resolved.

Consider using dev_scripts/find_images_to_rebuild.py to continue development on this. Here are the modifications I did:

diff --git a/dev_scripts/find_images_to_rebuild.py b/dev_scripts/find_images_to_rebuild.py
index 8f1f186..11e686d 100755
--- a/dev_scripts/find_images_to_rebuild.py
+++ b/dev_scripts/find_images_to_rebuild.py
@@ -18,8 +18,8 @@ os.environ["REQUESTS_CA_BUNDLE"] = "/etc/ssl/certs/ca-bundle.crt"
 from freshmaker import db, app
 from freshmaker.errata import Errata, ErrataAdvisory
 from freshmaker.events import (
-    ErrataAdvisoryStateChangedEvent, ManualRebuildWithAdvisoryEvent)
-from freshmaker.handlers.koji import RebuildImagesOnRPMAdvisoryChange
+    ErrataAdvisoryStateChangedEvent, ManualRebuildWithAdvisoryEvent, FreshmakerAsyncManualBuildEvent)
+from freshmaker.handlers.koji import RebuildImagesOnRPMAdvisoryChange, RebuildImagesOnAsyncManualBuild

 fedmsg_config = fedmsg.config.load_config()
 dictConfig(fedmsg_config.get('logging', {'version': 1}))
@@ -39,16 +39,25 @@ db.create_all()
 db.session.commit()

 errata = Errata()
-kwargs = {}
-if container_images:
-    EventClass = ManualRebuildWithAdvisoryEvent
-    kwargs['container_images'] = container_images
-else:
-    EventClass = ErrataAdvisoryStateChangedEvent
-event = EventClass(
-    "fake_message", ErrataAdvisory.from_advisory_id(errata, sys.argv[1]),
-    dry_run=True, **kwargs)
-
-handler = RebuildImagesOnRPMAdvisoryChange()
+
+branch = sys.argv[1]
+images = 'etcd'
+kwargs = {
+}
+
+# if container_images:
+#     EventClass = ManualRebuildWithAdvisoryEvent
+#     kwargs['container_images'] = container_images
+# else:
+#     EventClass = ErrataAdvisoryStateChangedEvent
+# event = EventClass(
+#     "fake_message", ErrataAdvisory.from_advisory_id(errata, sys.argv[1]),
+#     dry_run=True, **kwargs)
+
+event = FreshmakerAsyncManualBuildEvent(msg_id='fake-msg', dist_git_branch=branch, container_images=images, dry_run=True)
+
+# handler = RebuildImagesOnRPMAdvisoryChange()
+handler = RebuildImagesOnAsyncManualBuild()
 with patch("freshmaker.consumer.get_global_consumer"):
     handler.handle(event)
diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py
index 79825fd..6d56b6e 100644
--- a/freshmaker/lightblue.py
+++ b/freshmaker/lightblue.py
@@ -1039,7 +1039,7 @@ class LightBlue(object):
                     }
                 ]
             },
-            "projection": self._get_default_projection(include_rpms=False)
+            "projection": self._get_default_projection(include_rpm_manifest=False)
         }
         return self.find_container_images(query)

Executed as: ./find_images_to_rebuild.py extras-rhel-7.8

I used the certs and config from the shared secrets repo.

rebased onto 25c8d0290592f412f7d2024ed491d59dcdb8876f

4 years ago

@lucarval thank you for the review! I've addressed all the comments and uploaded a script similar to your patch (I did change a couple of things, like the fact that the parameter should be a list and the name should contain "-container" because the check gets done by the earlier in the API) that works fine on my machine.

@gnaponie, nice! When I ran your script it does complete successfully. I did notice that the dep_on_id attribute for the etcd-container is null. Should this be pointing to the rhel-server-container build?

🐚  sqlite3 freshmaker.db 'SELECT `id`, `name`, `dep_on_id` FROM artifact_builds;'
1|rhel-server-container|
2|etcd-container|

I think the output should've been:

1|rhel-server-container|
2|etcd-container|1

Debugging _record_batches I can see that it does create two batches as expected. So maybe the dependency between builds is just not being recorded properly.

rebased onto 1c96ec6

4 years ago

@lucarval there were a couple of lines missing. It should be there now.

rebased onto f702e90

4 years ago

1 new commit added

  • Fix flake8 complains
4 years ago

2 new commits added

  • Fix flake8 complains
  • Perform non-CVE builds
4 years ago

Added a commit for some unrelated flake8 errors that suddenly started failing.

Let's remove this hard coded list and accept a list of images as a parameter.

Oh! Actually, just remove this line and use container_images from above :)

@gnaponie, if I run this with branch rhel-8.2.0 and images ubi8-container and python-38-container, then I get the following:

🐚  sqlite3 freshmaker.db 'SELECT `id`, `name`, `dep_on_id` FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

This is exactly what's expected! Yay!

Now, if I run with branch rhel-8.2.0 and just the image python-38-container, then I get the same output:

🐚  sqlite3 freshmaker.db 'SELECT `id`, `name`, `dep_on_id` FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

Is that expected? Shouldn't just python-38-container be built?

```

It seems quite odd that this check is buried here. What scenario would trigger this anyways?

BTW, I think the logging logic is not quite right, I'm seeing this in my logs:

Found container images to rebuild in following order:
   Batch 0:
      - containers/ubi8#2f70d2e5b96ea67813c0f06b74a23775a118e7db (based on [None])
      - containers/s2i-core#9f55e017a7daebc2b3aeb7538863b2ad7923a8ff (based on None)
      - containers/s2i-base#f2c337e3945859c323e03acf857f7d397766ec81 (based on None)
      - containers/python-38#46ca3db6ca6d20b02618cde32142dcc98965e45d (based on None)

I think the order is correct, but each one should be built as a separate batch since each image is based on the previous image in the list.

Suggestion: Remove this method completely, and just use freshmaker-cli to get this sort of information if needed.

It would be useful to list which images are problematic.

missing_images = set(self.event.container_images) - set(images_to_rebuild.keys())
if missing_images:
   ...

@gnaponie, if I run this with branch rhel-8.2.0 and images ubi8-container and python-38-container, then I get the following:
🐚 sqlite3 freshmaker.db 'SELECT id, name, dep_on_id FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

This is exactly what's expected! Yay!
Now, if I run with branch rhel-8.2.0 and just the image python-38-container, then I get the same output:
🐚 sqlite3 freshmaker.db 'SELECT id, name, dep_on_id FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

Is that expected? Shouldn't just python-38-container be built?
```

I think you are right. I removed some lines (it was about directly building that one image if only that one was requested), I don't recall now why I removed it.... I'll put it back.

It seems quite odd that this check is buried here. What scenario would trigger this anyways?
BTW, I think the logging logic is not quite right, I'm seeing this in my logs:
Found container images to rebuild in following order:
Batch 0:
- containers/ubi8#2f70d2e5b96ea67813c0f06b74a23775a118e7db (based on [None])
- containers/s2i-core#9f55e017a7daebc2b3aeb7538863b2ad7923a8ff (based on None)
- containers/s2i-base#f2c337e3945859c323e03acf857f7d397766ec81 (based on None)
- containers/python-38#46ca3db6ca6d20b02618cde32142dcc98965e45d (based on None)

I think the order is correct, but each one should be built as a separate batch since each image is based on the previous image in the list.
Suggestion: Remove this method completely, and just use freshmaker-cli to get this sort of information if needed.

Yeah, this is here only for debugging I'd say, it's the same method we have in other handles. Perhaps it is a good idea to remove it completely.

rebased onto f5541ee

4 years ago

@lucarval I hope this is the last time :'(

rebased onto 0a7d964

4 years ago

2 new commits added

  • Fix flake8 complains
  • Perform non-CVE builds
4 years ago

rebased onto be6722a

4 years ago

rebased onto 9d4ecd1

4 years ago

rebased onto e0c4758

4 years ago

rebased onto 3d88567

4 years ago

1 new commit added

  • Perform non-CVE builds
4 years ago

rebased onto ddfae4b

4 years ago

@lucarval I believe (if I understood correctly what this parameter is for) that this should be enough to address the brew_target parameter.

This should be 3. I only see at most sys.argv[2] being used. The example usage docstring also needs updating.

This doesn't seem to be used.

No need to use a f-string here. Just use name directly: "rvalue": name

I left a couple of very minor comments. This lgtm once those are addressed :thumbsup:

rebased onto ee823a4

4 years ago

Comments addressed. @lucarval I'll just double check with you in chat, if that's ok, I'd merge it.

Pull-Request has been merged by gnaponie

4 years ago
Metadata
Flags
jenkins
success (100%)
Build #1065 successful (commit: ee823a4e)
4 years ago
jenkins
success (100%)
Build #1053 successful (commit: ddfae4b9)
4 years ago
jenkins
failure
Build #1052 failed (commit: 3d88567e)
4 years ago
jenkins
failure
Build #1051 failed (commit: aa04e31a)
4 years ago
jenkins
failure
Build #1050 failed (commit: 27ca78f3)
4 years ago
jenkins
failure
Build #1049 failed (commit: 8c1eeba5)
4 years ago
jenkins
success (100%)
Build #1039 successful (commit: 089b8eea)
4 years ago
jenkins
success (100%)
Build #1036 successful (commit: a5e84a8a)
4 years ago
jenkins
failure
Build #1034 failed (commit: da8300d9)
4 years ago
jenkins
success (100%)
Build #1026 successful (commit: de3addaa)
4 years ago
jenkins
success (100%)
Build #1013 successful (commit: db9f89c6)
4 years ago
jenkins
failure
Build #1012 failed (commit: db20dfd4)
4 years ago
jenkins
failure
Build #1011 failed (commit: f702e90c)
4 years ago
jenkins
failure
Build #1009 failed (commit: 1c96ec6f)
4 years ago
jenkins
success (100%)
Build #1005 successful (commit: 25c8d029)
4 years ago
jenkins
success (100%)
Build #1002 successful (commit: 3fe44328)
4 years ago
jenkins
failure
Build #1001 failed (commit: 4a89996a)
4 years ago
jenkins
failure
Build #1000 failed (commit: 677023e7)
4 years ago
jenkins
success (100%)
Build #944 successful (commit: 469a3388)
4 years ago
jenkins
failure
Build #943 failed (commit: 4d1ce28b)
4 years ago
jenkins
success (100%)
Build #930 successful (commit: 47119ab4)
4 years ago
jenkins
failure
Build #929 failed (commit: 27b55705)
4 years ago
jenkins
success (100%)
Build #919 successful (commit: 2b022896)
4 years ago