#1351 Check dependencies in reuse
Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin.
vmaljulin/fm-orchestrator FACTORY-4585-3  into  master

file modified
+63 -26
@@ -30,7 +30,7 @@ 

  import hashlib

  import json

  import re

- from collections import OrderedDict

+ from collections import OrderedDict, namedtuple

  from datetime import datetime

  

  import sqlalchemy
@@ -85,6 +85,9 @@ 

  FAILED_STATES = (BUILD_STATES["failed"], BUILD_STATES["garbage"])

  

  

+ Contexts = namedtuple("Contexts", "ref_build_context build_context runtime_context context")

+ 

+ 

  def _utc_datetime_to_iso(datetime_object):

      """

      Takes a UTC datetime object and returns an ISO formatted string
@@ -619,8 +622,8 @@ 

          else:

              raise ValueError("%r is not a module message." % type(event).__name__)

  

-     @staticmethod

-     def contexts_from_mmd(mmd_str):

+     @classmethod

+     def contexts_from_mmd(cls, mmd_str):

          """

          Returns tuple (ref_build_context, build_context, runtime_context, context)

          with hashes:
@@ -630,8 +633,8 @@ 

              - context - Hash of combined hashes of build_context and runtime_context.

  

          :param str mmd_str: String with Modulemd metadata.

-         :rtype: tuple of strings

-         :return: Tuple with build_context, strem_build_context, runtime_context and

+         :rtype: Contexts

+         :return: Named tuple with build_context, strem_build_context, runtime_context and

                   context hashes.

          """

          from module_build_service.utils.general import load_mmd
@@ -640,47 +643,81 @@ 

              mmd = load_mmd(mmd_str)

          except UnprocessableEntity:

              raise ValueError("Invalid modulemd")

-         mbs_xmd = mmd.get_xmd().get("mbs", {})

-         rv = []

+         mbs_xmd_buildrequires = mmd.get_xmd()["mbs"]["buildrequires"]

+         mmd_deps = mmd.get_dependencies()

+ 

+         build_context = cls.calculate_build_context(mbs_xmd_buildrequires)

+         runtime_context = cls.calculate_runtime_context(mmd_deps)

  

+         return Contexts(

+             cls.calculate_ref_build_context(mbs_xmd_buildrequires),

+             build_context,

+             runtime_context,

+             cls.calculate_module_context(build_context, runtime_context)

+         )

+ 

+     @staticmethod

+     def calculate_ref_build_context(mbs_xmd_buildrequires):

+         """

+         Returns the hash of commit hashes of expanded buildrequires.

+         :param mbs_xmd_buildrequires: xmd["mbs"]["buildrequires"] from Modulemd

+         :rtype: str

+         :return: ref_build_context hash

+         """

          # Get the buildrequires from the XMD section, because it contains

          # all the buildrequires as we resolved them using dependency resolver.

-         if "buildrequires" not in mbs_xmd:

-             raise ValueError("The module's modulemd hasn't been formatted by MBS")

          mmd_formatted_buildrequires = {

-             dep: info["ref"] for dep, info in mbs_xmd["buildrequires"].items()

+             dep: info["ref"] for dep, info in mbs_xmd_buildrequires.items()

          }

          property_json = json.dumps(OrderedDict(sorted(mmd_formatted_buildrequires.items())))

-         rv.append(hashlib.sha1(property_json.encode("utf-8")).hexdigest())

+         return hashlib.sha1(property_json.encode("utf-8")).hexdigest()

  

-         # Get the streams of buildrequires and hash it.

+     @staticmethod

+     def calculate_build_context(mbs_xmd_buildrequires):

+         """

+         Returns the hash of stream names of expanded buildrequires

+         :param mbs_xmd_buildrequires: xmd["mbs"]["buildrequires"] from Modulemd

+         :rtype: str

+         :return: build_context hash

+         """

          mmd_formatted_buildrequires = {

-             dep: info["stream"] for dep, info in mbs_xmd["buildrequires"].items()

+             dep: info["stream"] for dep, info in mbs_xmd_buildrequires.items()

          }

          property_json = json.dumps(OrderedDict(sorted(mmd_formatted_buildrequires.items())))

-         build_context = hashlib.sha1(property_json.encode("utf-8")).hexdigest()

-         rv.append(build_context)

+         return hashlib.sha1(property_json.encode("utf-8")).hexdigest()

  

-         # Get the requires from the real "dependencies" section in MMD.

+     @staticmethod

+     def calculate_runtime_context(mmd_dependencies):

+         """

+         Returns the hash of stream names of expanded runtime requires

+         :param mmd_dependencies: dependencies from Modulemd

+         :rtype: str

+         :return: runtime_context hash

+         """

          mmd_requires = {}

-         for deps in mmd.get_dependencies():

+         for deps in mmd_dependencies:

              for name in deps.get_runtime_modules():

                  streams = deps.get_runtime_streams(name)

-                 if name not in mmd_requires:

-                     mmd_requires[name] = set()

-                 mmd_requires[name] = mmd_requires[name].union(streams)

+                 mmd_requires[name] = mmd_requires.get(name, set()).union(streams)

  

          # Sort the streams for each module name and also sort the module names.

          mmd_requires = {dep: sorted(list(streams)) for dep, streams in mmd_requires.items()}

          property_json = json.dumps(OrderedDict(sorted(mmd_requires.items())))

-         runtime_context = hashlib.sha1(property_json.encode("utf-8")).hexdigest()

-         rv.append(runtime_context)

+         return hashlib.sha1(property_json.encode("utf-8")).hexdigest()

  

+     @staticmethod

+     def calculate_module_context(build_context, runtime_context):

+         """

+         Returns the hash of combined hashes of build_context and runtime_context

+         :param build_context: hash returned by calculate_build_context

+         :type build_context: str

+         :param runtime_context: hash returned by calculate_runtime_context

+         :type runtime_context: str

+         :rtype: str

+         :return: module context hash

+         """

          combined_hashes = "{0}:{1}".format(build_context, runtime_context)

-         context = hashlib.sha1(combined_hashes.encode("utf-8")).hexdigest()[:8]

-         rv.append(context)

- 

-         return tuple(rv)

+         return hashlib.sha1(combined_hashes.encode("utf-8")).hexdigest()[:8]

  

      def siblings(self, db_session):

          query = db_session.query(ModuleBuild).filter(

@@ -207,7 +207,7 @@ 

      return mmds

  

  

- def _get_base_module_mmds(db_session, mmd):

+ def get_base_module_mmds(db_session, mmd):

      """

      Returns list of MMDs of base modules buildrequired by `mmd` including the compatible

      old versions of the base module based on the stream version.
@@ -329,7 +329,7 @@ 

      mmds = {}

  

      # Get the MMDs of all compatible base modules based on the buildrequires.

-     base_module_mmds = _get_base_module_mmds(db_session, mmd)

+     base_module_mmds = get_base_module_mmds(db_session, mmd)

      if not base_module_mmds["ready"]:

          base_module_choices = " or ".join(conf.base_module_names)

          raise UnprocessableEntity(
@@ -526,7 +526,7 @@ 

          mmd_copy.set_xmd(xmd)

  

          # Now we have all the info to actually compute context of this module.

-         _, _, _, context = models.ModuleBuild.contexts_from_mmd(mmd_to_str(mmd_copy))

+         context = models.ModuleBuild.contexts_from_mmd(mmd_to_str(mmd_copy)).context

          mmd_copy.set_context(context)

  

          mmds.append(mmd_copy)

@@ -26,6 +26,7 @@ 

  

  import module_build_service.messaging

  from module_build_service import log, models, conf

+ from module_build_service.utils.mse import get_base_module_mmds

  

  

  def reuse_component(component, previous_component_build, change_state_now=False):
@@ -90,24 +91,40 @@ 

          return module.reused_module

  

      mmd = module.mmd()

-     # Find the latest module that is in the done or ready state

-     previous_module_build = (

-         db_session.query(models.ModuleBuild)

-         .filter_by(name=mmd.get_module_name())

-         .filter_by(stream=mmd.get_stream_name())

-         .filter_by(state=models.BUILD_STATES["ready"])

-         .filter(models.ModuleBuild.scmurl.isnot(None))

-         .filter_by(build_context=module.build_context)

-         .order_by(models.ModuleBuild.time_completed.desc())

-     )

-     # If we are rebuilding with the "changed-and-after" option, then we can't reuse

-     # components from modules that were built more liberally

-     if module.rebuild_strategy == "changed-and-after":

-         previous_module_build = previous_module_build.filter(

-             models.ModuleBuild.rebuild_strategy.in_(["all", "changed-and-after"]))

-         previous_module_build = previous_module_build.filter_by(

-             ref_build_context=module.ref_build_context)

-     previous_module_build = previous_module_build.first()

+     previous_module_build = None

+ 

+     base_mmds = get_base_module_mmds(db_session, mmd)["ready"]

+     for base_mmd in base_mmds:

+         mbs_xmd = mmd.get_xmd()["mbs"]

+         if base_mmd.get_module_name() not in mbs_xmd["buildrequires"]:

+             continue

+         mbs_xmd["buildrequires"][base_mmd.get_module_name()]["stream"] \

+             = base_mmd.get_stream_name()

+         build_context = module.calculate_build_context(mbs_xmd["buildrequires"])

+         # Find the latest module that is in the ready state

+         previous_module_build = (

+             db_session.query(models.ModuleBuild)

+                       .filter_by(name=mmd.get_module_name())

+                       .filter_by(stream=mmd.get_stream_name())

+                       .filter_by(state=models.BUILD_STATES["ready"])

+                       .filter(models.ModuleBuild.scmurl.isnot(None))

+                       .filter_by(build_context=build_context)

+                       .order_by(models.ModuleBuild.time_completed.desc()))

+ 

+         # If we are rebuilding with the "changed-and-after" option, then we can't reuse

+         # components from modules that were built more liberally

+         if module.rebuild_strategy == "changed-and-after":

+             previous_module_build = previous_module_build.filter(

+                 models.ModuleBuild.rebuild_strategy.in_(["all", "changed-and-after"])

+             )

+             previous_module_build = previous_module_build.filter_by(

+                 ref_build_context=module.ref_build_context

The ref_build_context should have been recalculated for the base module. After talking with contyk and jkaluza, we decided to just remove this concept in PR #1365. If we decide to not go through with that PR. We will need to fix this.

+             )

+         previous_module_build = previous_module_build.first()

+ 

+         if previous_module_build:

+             break

+ 

      # The component can't be reused if there isn't a previous build in the done

      # or ready state

      if not previous_module_build:

file modified
+24
@@ -129,6 +129,9 @@ 

      xmd["mbs"]["commit"] = "ff1ea79fc952143efeed1851aa0aa006559239ba"

      mmd.set_xmd(xmd)

      build_one.modulemd = mmd_to_str(mmd)

+     build_one.build_context = module_build_service.models.ModuleBuild.contexts_from_mmd(

+         build_one.modulemd

+     ).build_context

  

      db_session.add(build_one)

      db_session.commit()
@@ -226,6 +229,9 @@ 

      xmd["mbs"]["commit"] = "55f4a0a2e6cc255c88712a905157ab39315b8fd8"

      mmd.set_xmd(xmd)

      build_two.modulemd = mmd_to_str(mmd)

+     build_two.build_context = module_build_service.models.ModuleBuild.contexts_from_mmd(

+         build_two.modulemd

+     ).build_context

  

      db_session.add(build_two)

      db_session.commit()
@@ -305,6 +311,15 @@ 

          rebuild_strategy="changed-and-after",

      )

  

+     xmd = mmd.get_xmd()

+     xmd["mbs"]["scmurl"] = module_build.scmurl

+     xmd["mbs"]["commit"] = "55f4a0a2e6cc255c88712a905157ab39315b8fd8"

+     mmd.set_xmd(xmd)

+     module_build.modulemd = mmd_to_str(mmd)

+     module_build.build_context = module_build_service.models.ModuleBuild.contexts_from_mmd(

+         module_build.modulemd

+     ).build_context

+ 

      components = [

          mmd.get_rpm_component(rpm)

          for rpm in mmd.get_rpm_component_names()
@@ -358,6 +373,15 @@ 

          rebuild_strategy="changed-and-after",

      )

  

+     xmd = mmd2.get_xmd()

+     xmd["mbs"]["scmurl"] = module_build.scmurl

+     xmd["mbs"]["commit"] = "55f4a0a2e6cc255c88712a905157ab39315b8fd8"

+     mmd2.set_xmd(xmd)

+     module_build.modulemd = mmd_to_str(mmd2)

+     module_build.build_context = module_build_service.models.ModuleBuild.contexts_from_mmd(

+         module_build.modulemd

+     ).build_context

+ 

      components2 = [

          mmd2.get_rpm_component(rpm)

          for rpm in mmd2.get_rpm_component_names()

file modified
+23 -12
@@ -233,9 +233,18 @@ 

          mmd = second_module_build.mmd()

          xmd = mmd.get_xmd()

          xmd["mbs"]["buildrequires"]["platform"]["stream"] = "different"

+         deps = Modulemd.Dependencies()

+         deps.add_buildtime_stream("platform", "different")

+         deps.add_runtime_stream("platform", "different")

+         mmd.clear_dependencies()

+         mmd.add_dependencies(deps)

+ 

          mmd.set_xmd(xmd)

          second_module_build.modulemd = mmd_to_str(mmd)

-         second_module_build.build_context = "37c6c57bedf4305ef41249c1794760b5cb8fad17"

+         second_module_build.build_context = \

+             module_build_service.models.ModuleBuild.contexts_from_mmd(

+                 second_module_build.modulemd

+             ).build_context

          second_module_build.rebuild_strategy = rebuild_strategy

          db_session.commit()

  
@@ -614,8 +623,11 @@ 

          current `new_module`. In this case, reuse code should still be able to

          reuse the components.

          """

+         old_module = models.ModuleBuild.get_by_id(db_session, 2)

          new_module = models.ModuleBuild.get_by_id(db_session, 3)

-         rv = module_build_service.utils.get_reusable_component(db_session, new_module, "llvm")

+         rv = module_build_service.utils.get_reusable_component(

+             db_session, new_module, "llvm", previous_module_build=old_module

+         )

          assert rv.package == "llvm"

  

      def test_validate_koji_tag_wrong_tag_arg_during_programming(self):
@@ -1499,17 +1511,14 @@ 

              assert module_build

  

  

+ @pytest.mark.usefixtures("reuse_component_init_data")

  class TestUtilsModuleReuse:

  

-     def setup_method(self, test_method):

-         init_data()

- 

-     def teardown_method(self, test_method):

-         clean_database()

- 

      def test_get_reusable_module_when_reused_module_not_set(self, db_session):

-         module = db_session.query(models.ModuleBuild).filter_by(

-             name="nginx").order_by(models.ModuleBuild.id.desc()).first()

+         module = db_session.query(models.ModuleBuild)\

+                            .filter_by(name="testmodule")\

+                            .order_by(models.ModuleBuild.id.desc())\

+                            .first()

          module.state = models.BUILD_STATES["build"]

          db_session.commit()

  
@@ -1522,8 +1531,10 @@ 

          assert reusable_module.id == module.reused_module_id

  

      def test_get_reusable_module_when_reused_module_already_set(self, db_session):

-         modules = db_session.query(models.ModuleBuild).filter_by(

-             name="nginx").order_by(models.ModuleBuild.id.desc()).limit(2).all()

+         modules = db_session.query(models.ModuleBuild)\

+                             .filter_by(name="testmodule")\

+                             .order_by(models.ModuleBuild.id.desc())\

+                             .limit(2).all()

          build_module = modules[0]

          reused_module = modules[1]

          build_module.state = models.BUILD_STATES["build"]

@@ -503,7 +503,7 @@ 

          mmd.remove_dependencies(deps)

          mmd.add_dependencies(new_deps)

  

-         mmds = module_build_service.utils.mse._get_base_module_mmds(db_session, mmd)

+         mmds = module_build_service.utils.mse.get_base_module_mmds(db_session, mmd)

          expected = set(["platform:f29.0.0", "platform:f29.1.0", "platform:f29.2.0"])

          # Verify no duplicates were returned before doing set operations

          assert len(mmds["ready"]) == len(expected)
@@ -530,7 +530,7 @@ 

              "platform:lp29.1.1:12:c11",

              db_session=db_session, virtual_streams=virtual_streams)

  

-         mmds = module_build_service.utils.mse._get_base_module_mmds(db_session, mmd)

+         mmds = module_build_service.utils.mse.get_base_module_mmds(db_session, mmd)

          if virtual_streams == ["f29"]:

              expected = set(

                  ["platform:f29.0.0", "platform:f29.1.0", "platform:f29.2.0", "platform:lp29.1.1"])
@@ -568,7 +568,7 @@ 

          mmd.remove_dependencies(deps)

          mmd.add_dependencies(new_deps)

  

-         mmds = module_build_service.utils.mse._get_base_module_mmds(db_session, mmd)

+         mmds = module_build_service.utils.mse.get_base_module_mmds(db_session, mmd)

          expected = {}

          expected["ready"] = set(["platform:foo29", "platform:foo30"])

          expected["garbage"] = set(["platform:foo28"])

no initial comment

Build #279 failed (commit: 828c205ce7295ef4de8fe5d7cd31dce7403375ca).
Rebase or make new commits to rebuild.

Build #280 failed (commit: 828c205ce7295ef4de8fe5d7cd31dce7403375ca).
Rebase or make new commits to rebuild.

rebased onto 642679fd9d75c47a29650935d4fa0a82c5e853d8

4 years ago

rebased onto 6fb7f3dee5670859f9c4dd61a276f7acdec7c861

4 years ago

rebased onto 4f3f4b5357b5c92001726f7efdaced05c90a5c9a

4 years ago

rebased onto 93af22c64e7b520ad0567f7646ecf39a5b79fb54

4 years ago

Build #288 failed (commit: 93af22c64e7b520ad0567f7646ecf39a5b79fb54).
Rebase or make new commits to rebuild.

rebased onto 5d1cde8854df7528732e1e9fb11909d58c9162bb

4 years ago

Build #295 failed (commit: 5d1cde8854df7528732e1e9fb11909d58c9162bb).
Rebase or make new commits to rebuild.

rebased onto 26dbe9454947847a338682d86325186c8e028be9

4 years ago

Build #296 failed (commit: 26dbe9454947847a338682d86325186c8e028be9).
Rebase or make new commits to rebuild.

rebased onto 0ef6af53fce71dd8bf8e3201cf4b2a51f2d64f72

4 years ago

Build #297 failed (commit: 0ef6af53fce71dd8bf8e3201cf4b2a51f2d64f72).
Rebase or make new commits to rebuild.

rebased onto 7a05ade7d3a267429ab959f313f2e7eef616d954

4 years ago

rebased onto 8a4f20b1dced925570f835f612d77d3da01c92eb

4 years ago

Rebased and could now be reviewed

I think you can get xmd["mbs"]["buildrequires"] outside the for-loop to avoid getting it repeatedly for each base_mmd.

Probably we could fix this comment here. Only modules in ready state are selected in the code below.

Why change stream here even if the xmd is not set back to modulemd? It looks confused too much.

Base modules returned from get_base_module_mmds are just those defined in conf.base_module_names and which are buildrequires of the mmd. Is this check necessary again?

Original contexts_from_mmd calculates ref_build_context and build_context fro xmd["mbs"]["buildrequires"], and runtime_context from runtime modules got from mmd.get_dependencies. This change makes the xmd["mbs"] and runtime dependent modules optional. Instead of making this change to context_fro_mmd, I suggest to take this opportunity to split contexts_from_mmd into small functions to calculate each context separately. In this way, it would be clear and straightforward to pass optional xms["mbs"] and mmd_deps for different calculation.

Separated functions would be:

  • calculate_ref_build_context(xmd_mbs_buildrequires)
  • calculate_build_context(xmd_mbs_buildrequires)
  • calculate_runtime_context(mmd_dependencies)
  • calculate_module_context(build_context, runtime_context)

On explicit benefit is for the call below build_context = module.contexts_from_mmd("", xmd["mbs"], mmd_deps)[1]. That is, just as mentioned above, mmd_deps is not required for calculating build_context, but developers have to pass mmd_deps.

I think +1 to what @cqi said. I would still keep the contexts_from_mmd here and just call those 4 smaller methods there to get the result.

I would keep this condition there, because get_base_module_mmds returns compatible modules based on the mmd.get_buildtime_streams, while here we are using mmd["xmd"]["buildrequires"]. These two don't have to match, although it would be strange situation. It's better to handle it nicely here.

Because he passes it to contexts_from_mmd to compute the new context with the updated stream.

+1, the comment should mention only "ready".

1 new commit added

  • fixup! Check dependencies in reuse
4 years ago

Comments were addressed

pretty please pagure-ci rebuild

4 years ago

+1, but let's wait until the tests finishes.

@vmaljulin Thanks for making this update.

Can you make a further change to just accept buildrequiers rather than passing the whole xmd/mbs structure? As what my previous comment mentioned, this function and calculate_build_context only requires xmd/mbs/buildrequires. Or, in other word, they just require a mapping type dict[str, dict] and the inner dict must have key/value of ref/xxx, for example {module_1: {"ref": "1234"}, module_2: {"ref": "4567"}, ...}.

Just a new thought when reading these lines this time, a namedtuple could make it much easier to read the code, that is to give every context a name in the tuple. So, in other place in the code base, we can drop code like _, _, _, context = contexts_from_mmd(...) or context = contexts_from_mmd(...)[2]. Instead, we can write the code like contexts_from_mmd(...).ref_build_context or

context = contexts_from_mmd(...)
module_build.build_context = context.build_context
module_build.runtime_context = context.runtime_context
...

@vmaljulin Could you please squash these two commits finally?

@vmaljulin Could you please squash these two commits finally?

Of course, I will. I'm making fixup commits so reviewers could easily check what has been changed since the last review.

1 new commit added

  • fixup! fixup! Check dependencies in reuse
4 years ago

cqi's comments were addressed

3 new commits added

  • fixup! fixup! Check dependencies in reuse
  • fixup! Check dependencies in reuse
  • Check dependencies in reuse
4 years ago

rebased onto 5fb6a2f

4 years ago

All fixup commits were autosquashed

Of course, I will. I'm making fixup commits so reviewers could easily check what has been changed since the last review.

@vmaljulin I also think this is a good way. :thumbsup:

Pull-Request has been merged by vmaljulin

4 years ago

The ref_build_context should have been recalculated for the base module. After talking with contyk and jkaluza, we decided to just remove this concept in PR #1365. If we decide to not go through with that PR. We will need to fix this.

Metadata
Flags
jenkins
success (100%)
Build #937 successful (commit: 5fb6a2f2)
4 years ago
c3i-jenkins
success (100%)
Build #304 successful (commit: 5fb6a2f2)
4 years ago
jenkins
success (100%)
Build #936 successful (commit: 5fb6a2f2)
4 years ago
jenkins
success (100%)
Build #935 successful (commit: 0c0225e6)
4 years ago
c3i-jenkins
success (100%)
Build #303 successful (commit: 0c0225e6)
4 years ago
jenkins
success (100%)
Build #933 successful (commit: 3ffc4ec1)
4 years ago
jenkins
success (100%)
Build #931 successful (commit: 3ffc4ec1)
4 years ago
c3i-jenkins
success (100%)
Build #301 successful (commit: 3ffc4ec1)
4 years ago
jenkins
success (100%)
Build #928 successful (commit: 8a4f20b1)
4 years ago
c3i-jenkins
success (100%)
Build #299 successful (commit: 8a4f20b1)
4 years ago
jenkins
error
Build #927 aborted (commit: 7a05ade7)
4 years ago
c3i-jenkins
success (100%)
Build #298 successful (commit: 7a05ade7)
4 years ago
jenkins
error
Build #926 aborted (commit: 0ef6af53)
4 years ago
c3i-jenkins
failure
Build #297 failed (commit: 0ef6af53)
4 years ago
jenkins
failure
Build #925 failed (commit: 26dbe945)
4 years ago
c3i-jenkins
failure
Build #296 failed (commit: 26dbe945)
4 years ago
jenkins
failure
Build #924 failed (commit: 5d1cde88)
4 years ago
c3i-jenkins
failure
Build #295 failed (commit: 5d1cde88)
4 years ago
c3i-jenkins
failure
Build #288 failed (commit: 93af22c6)
4 years ago
jenkins
error
Build #918 aborted (commit: 93af22c6)
4 years ago
jenkins
failure
Build #917 failed (commit: 4f3f4b53)
4 years ago
jenkins
failure
Build #916 failed (commit: 6fb7f3de)
4 years ago
jenkins
failure
Build #915 failed (commit: 642679fd)
4 years ago
jenkins
error
Build #904 aborted (commit: 828c205c)
4 years ago
c3i-jenkins
failure
Build #280 failed (commit: 828c205c)
4 years ago