#1593 Add the ability to limit arches
Merged 2 months ago by mprahl. Opened 3 months ago by breilly.
breilly/fm-orchestrator arches-5737  into  master

@@ -23,13 +23,19 @@ 

  

      :param mmd: Module MetaData

      :param config: config (module_build_service.common.config.Config instance)

-     :return list of architectures

+     :return: list of architectures

      """

      # Imported here to allow import of utils in GenericBuilder.

      from module_build_service.builder import GenericBuilder

  

      nsvc = mmd.get_nsvc()

  

+     def _conditional_log(msg, arches, new_arches):

+         # Checks if the arch list returned by _check_buildopts_arches is the same one passed to it

+         # If it is, it outputs the message

+         if arches is new_arches:

+             log.info(msg)

+ 

      # At first, handle BASE_MODULE_ARCHES - this overrides any other option.

      # Find out the base modules in buildrequires section of XMD and

      # set the Koji tag arches according to it.

@@ -38,17 +44,21 @@ 

              ns = ":".join([req_name, req_data["stream"]])

              if ns in config.base_module_arches:

                  arches = config.base_module_arches[ns]

-                 log.info("Setting build arches of %s to %r based on the BASE_MODULE_ARCHES." % (

-                     nsvc, arches))

-                 return arches

+                 new_arches = _check_buildopts_arches(mmd, arches)

+                 msg = "Setting build arches of %s to %r based on the BASE_MODULE_ARCHES." % (

+                     nsvc, new_arches)

+                 _conditional_log(msg, arches, new_arches)

+                 return new_arches

  

      # Check whether the module contains the `koji_tag_arches`. This is used only

      # by special modules defining the layered products.

      try:

          arches = mmd.get_xmd()["mbs"]["koji_tag_arches"]

-         log.info("Setting build arches of %s to %r based on the koji_tag_arches." % (

-             nsvc, arches))

-         return arches

+         new_arches = _check_buildopts_arches(mmd, arches)

+         msg = "Setting build arches of %s to %r based on the koji_tag_arches." % (

+             nsvc, new_arches)

+         _conditional_log(msg, arches, new_arches)

+         return new_arches

      except KeyError:

          pass

  

@@ -81,13 +91,45 @@ 

                  continue

              arches = GenericBuilder.get_module_build_arches(module_obj)

              if arches:

-                 log.info("Setting build arches of %s to %r based on the buildrequired "

-                          "module %r." % (nsvc, arches, module_obj))

-                 return arches

+                 new_arches = _check_buildopts_arches(mmd, arches)

+                 msg = "Setting build arches of %s to %r based on the buildrequired module %r." % (

+                     nsvc, new_arches, module_obj)

+                 _conditional_log(msg, arches, new_arches)

+                 return new_arches

  

      # As a last resort, return just the preconfigured list of arches.

      arches = config.arches

-     log.info("Setting build arches of %s to %r based on default ARCHES." % (nsvc, arches))

+     new_arches = _check_buildopts_arches(mmd, arches)

+     msg = "Setting build arches of %s to %r based on default ARCHES." % (nsvc, new_arches)

+     _conditional_log(msg, arches, new_arches)

+     return new_arches

+ 

+ 

+ def _check_buildopts_arches(mmd, arches):

+     """

+     Returns buildopts arches if valid, or otherwise the arches provided.

+ 

+     :param mmd: Module MetaData

+     :param arches: list of architectures

+     :return: list of architectures

+     """

+     buildopts = mmd.get_buildopts()

+     if not buildopts:

+         return arches

+     try:

+         buildopts_arches = buildopts.get_arches()

+     except AttributeError:

+         # libmodulemd version < 2.8.3

+         return arches

+     # Must be a subset of the input module arches

+     unsupported_arches = set(buildopts_arches) - set(arches)

+     if unsupported_arches:

+         raise ValidationError("The following buildopts arches are not supported with these "

+                               "buildrequires: %r" % unsupported_arches)

+     if buildopts_arches:

+         log.info("Setting build arches of %s to %r based on the buildopts arches." % (

+             mmd.get_nsvc(), buildopts_arches))

+         return buildopts_arches

      return arches

  

  

@@ -9,6 +9,7 @@ 

  from module_build_service import app

  from module_build_service.common import conf, models

  from module_build_service.common.errors import UnprocessableEntity

+ from module_build_service.common.modulemd import Modulemd

  from module_build_service.common.utils import load_mmd, load_mmd_file, mmd_to_str

  from module_build_service.scheduler.db_session import db_session

  import module_build_service.scheduler.handlers.components

@@ -76,6 +77,26 @@ 

          r = get_build_arches(mmd, conf)

          assert r == ["x86_64", "i686"]

  

+     @mock.patch.object(conf, "base_module_arches", new={"platform:xx": ["x86_64", "i686"]})

+     def test_get_build_arches_set_in_mmd(self):

+         mmd = load_mmd(read_staged_data("formatted_testmodule"))

+         xmd = mmd.get_xmd()

+         mbs_options = xmd.get("mbs", {})

+         mbs_options["buildrequires"] = {"platform": {"stream": "xx"}}

+         xmd["mbs"] = mbs_options

+         mmd.set_xmd(xmd)

+         try:

+             opts = Modulemd.Buildopts()

+             opts.add_arch("x86_64")

+             mmd.set_buildopts(opts)

+             expected_result = ["x86_64"]

+         except AttributeError:

+             # libmodulemd version < 2.8.3

+             expected_result = ["x86_64", "i686"]

+ 

+         r = get_build_arches(mmd, conf)

+         assert r == expected_result

+ 

      @mock.patch("module_build_service.scheduler.submit.get_build_arches")

      def test_record_module_build_arches(self, get_build_arches):

          get_build_arches.return_value = ["x86_64", "i686"]

no initial comment

In the new modulemd case, you are catching 'arches.get()' - self.arches is a list, and doesn't have a get method. You want to keep this kind of try/except really tight.

try:
    arches = buildopt.get_arches()
except AttributeError:
   arches  = None

In addition to the above, a test case addition would likely be a good idea as well. I just checked, and using mock.patch("gi.repository.Modulemd.Buildopts.get_arches", side_effect=AttributeError) should work if you want to test both branches with a new libmodulemd.

@breilly could you please add a unit test for this?

rebased onto 4513b38d238ebb3d51e6c959d803b500e6c15384

2 months ago

rebased onto 75d7b503673dcdd470d78f124ac71f662580237c

2 months ago

Could you please move this code to the get_build_arches function? That way the architectures associated with the module build in the database are influenced by the modulemd file.

Edit: The correct function is get_build_arches in submit.py.

How about buildopts_arches = [] here instead? Then you can remove buildopts_arches = None and if buildopts_arches:.

rebased onto 915cbe9663d67a0cf9f7b54b8ea45536a19b49ae

2 months ago

This section of the code is for when arches are configured for a specific base module in the MBS configuration. An example of when this is used is when a new version of Fedora drops one or more architectures. This happened for Fedora 31 when i686 was dropped.

My suggestion is to change this function so that when a set of architectures are found, you then check to see if the modulemd of the module being built has architectures set. If it does, you need to verify that the architectures on the modulemd are a subset of the architectures that were just found. If they are, return the architectures on the modulemd file. If they are not a subset, then you need to raise a ValidationError explaining the issue which will cause the module build to fail. If the modulemd doesn't have the architectures set, then just return the found architectures like the existing behavior.

Could you move this to under the if branch since the variable is only used if buildopts is truthy? You could also just replace pass with buildopts_arches = [].

Could you use set theory here instead of a for loop? For instance:

unsupported_arches = set(buildopts_arches) - set(arches)
if unsupported_arches:
    # Raise some error that lets the user know which arches aren't supported

rebased onto 2d138221f220cf689eeb7b730db52ad9c4117320

2 months ago

Optional: You could do the following to avoid the long if branch:

if not buildopts:
   return arches

It'd be nice to have a docstring, especially explaining arches.

Instead of passing in nsvc, I believe you can just call mmd.get_nsvc().

Why is checking the length relevant here?

I would think that if buildopts_arches is set and compatible, then it should be returned

Edit: It looks like you are trying to see if the arches are different before logging. In that case, just do:

if set(arches) != set(buildopts_arches):

Edit 2: On second thought, I don't think should be checking this at all. If buildopts_arches is set and compatible, it should be used, even if its the same. It'd be confusing otherwise since the log messages would imply that buildopts_arches was not consulted when it's the same as what the arches would have been.

You could probably just return arches here

I wanted to check if the restricted arches were the same as the ones gotten from the original means, for the sake of a correct log message (since it would essentially be the same thing as setting based on the base module arches for instance). Length was just a quick way to do that.

Checking the length is not a good way to determine if the arches have changed. Just do:

if set(arches) == set(new_arches):

Alternatively, you can check to see if arches was returned as is by _check_buildopts_arches:

if arches is new_arches:

How about something like this?
test_get_build_arches_limited => test_get_build_arches_set_in_mmd

Optional: There was an effort to keep the imports in alphabetical order

We'd still want to avoid a situation in which both strings from _check_buildopts_arches and get_build_arches are printed, right? Which would happen in the case of the limited arch set being the same as the original set, if the check in _check_buildopts_arches is removed. Is there a better solution here?

rebased onto 79e71463b603e6ff2f6194b4ec7cb945e9cdf585

2 months ago

Thanks, updated.

A couple ways come to mind to avoid the above situation: 1) pass the log message into the function or 2) return a boolean value in a tuple. Both of those seem very inelegant, so I haven't implemented either. Open to suggestions on this.

How about?

if arches is new_arches:

This would check to see if arches and new_arches point to the same memory address. If so, then you know _check_buildopts_arches didn't return the arches from the modulemd.

the platform module arches => the input module arches

Optional: Since this logic is repeated, you could create a small function inside this function:

def _conditional_log(msg, arches, new_arches):
    if arches is new_arches:
        log.info(msg)

rebased onto af58453efce04e44b46e5b12a57ace9582c876a8

2 months ago

Ah, right, checking the memory address was exactly what was needed. Thanks!
Updated.

It'd be nice to include a comment explaining this :smile:

Optional: Instead of str(unsupported_arches), I think you can just use unsupported_arches directly but instead of %s, you use %r.

How about something like?

raise ValueError("The following buildopts arches are not supported with these buildrequires: %r" % unsupported_arches)

The main thing is that we should avoid saying platform since it's customary to use that name for the a base module, but it's not hard coded logic in MBS. Additionally, things other than the buildrequired platform module can influence the build. This is true for privileged modules that are buildrequired (used for layered product module builds).

This should also raise a ValidationError instead of ValueError. This is because in the modules.py::init function, it will only use the message from certain exception types for the module's state_reason when it's transitioned to the failed state.

@breilly sorry I missed a few other things in the last review. Once these comments are addressed, I think this will be good to merge.

rebased onto c4799afcbbb29e0afb887fcc8eab9aee959e41e9

2 months ago

Could you just return buildopts_arches here?

In general, I find it to be a bad practice to overwrite function parameters since it makes code harder to read. This forces you to have to read the whole function to see where the variable could change, and it might not be expected by the reader if they are reading a small section within the function.

rebased onto ea3261aa3afa0cd2e9912e019c9649ae8502cb77

2 months ago

Optional: This is more readable:

mbs_options = xmd.get("mbs", {})

Optional: It'd be nice if this was a single commit. There's no use having a separate commit for the unit test. It just makes the git history more cluttered and the change harder to revert.

rebased onto c929aeed961728d800f85344bd15d05ccc766e64

2 months ago

@breilly this should be:

mbs_options = xmd.get("mbs", {})

rebased onto 686aec791c4cae9085bf22eb085d6131675c254a

2 months ago

rebased onto 25cdfbb

2 months ago

Pull-Request has been merged by mprahl

2 months ago