#1136 module scratch builds
Merged 4 months ago by jkaluza. Opened 5 months ago by merlinm.

file modified
+19 -6

@@ -18,7 +18,10 @@

  Change Log

  ==========

  

- For a detailed change log, see ``docs/CHANGELOG.rst``.

+ For a detailed change log, see |docs/CHANGELOG.rst|_.

+ 

+ .. |docs/CHANGELOG.rst| replace:: ``docs/CHANGELOG.rst``

+ .. _docs/CHANGELOG.rst: docs/CHANGELOG.rst

  

  Supported build systems

  =======================

@@ -102,9 +105,13 @@

    ``{'platform': ['f28', 'f29']}``.

  - ``require_overrides`` - the requires to override the modulemd with. The overrides must be to

    existing requires on the modulemd. The expected format is ``{'platform': ['f28', 'f29']}``.

+ - ``scratch`` - a boolean indicating if a scratch module build should be performed.

+   Only allowed to be ``True`` if the MBS setting ``MODULE_ALLOW_SCRATCH`` is ``True``.

  - ``yaml`` - a string of the input file when submitting a YAML file directly in a

    ``multipart/form-data`` request. The MBS setting ``YAML_SUBMIT_ALLOWED`` must be set to ``True``

    for this to be allowed.

+ - ``srpms`` - an optional list of Koji upload URLs of SRPMs to include in a module scratch build.

+   Only allowed if ``scratch`` is ``True``.

  - ``rebuild_strategy`` - a string of the desired rebuild strategy (affects what components get

    rebuilt). For the available options, please look at the "Rebuild Strategies" section below.

  

@@ -239,8 +246,8 @@

  - ``owner`` - the username of the owner or person who submitted the module build.

  - ``scmurl`` - the source control URL used to build the module.

  - ``state`` - the numerical state of the module build.

- - ``state_name`` - the named state of the module build. See the section called.

-   "Module Build States" for more information.

+ - ``state_name`` - the named state of the module build. See the section called

+   `Module Build States`_ for more information.

  - ``state_reason`` - the reason why the module build is in this state. This is useful

    when the build fails.

  - ``stream`` - the module's stream.

@@ -925,7 +932,10 @@

    TestConfiguration is used, otherwise...

  - if ``MODULE_BUILD_SERVICE_DEVELOPER_ENV`` is set to some reasonable

    value, DevConfiguration is forced and ``config.py`` is used directly from the

-   MBS's develop instance. For more information see ``docs/CONTRIBUTING.rst``.

+   MBS's develop instance. For more information see |docs/CONTRIBUTING.rst|_.

+ 

+ .. |docs/CONTRIBUTING.rst| replace:: ``docs/CONTRIBUTING.rst``

+ .. _docs/CONTRIBUTING.rst: docs/CONTRIBUTING.rst

  

  

  Setting Up Kerberos + LDAP Authentication

@@ -954,12 +964,15 @@

  Development

  ===========

  

- For help on setting up a development environment, see ``docs/CONTRIBUTING.rst``.

+ For help on setting up a development environment, see |docs/CONTRIBUTING.rst|_.

  

  License

  =======

  

- MBS is licensed under MIT license. See LICENSE file for details.

+ MBS is licensed under MIT license. See |LICENSE|_ file for details.

+ 

+ .. |LICENSE| replace:: ``LICENSE``

+ .. _LICENSE: LICENSE

  

  Parts of MBS are licensed under 3-clause BSD license from:

  https://github.com/projectatomic/atomic-reactor/blob/master/LICENSE

file modified
+2 -1

@@ -31,7 +31,7 @@

      ARCHES = ['i686', 'armv7hl', 'x86_64']

      ALLOW_ARCH_OVERRIDE = False

      KOJI_REPOSITORY_URL = 'https://kojipkgs.fedoraproject.org/repos'

-     KOJI_TAG_PREFIXES = ['module']

+     KOJI_TAG_PREFIXES = ['module', 'scrmod']

      KOJI_ENABLE_CONTENT_GENERATOR = True

      CHECK_FOR_EOL = False

      PDC_URL = 'https://pdc.fedoraproject.org/rest_api/v1'

@@ -57,6 +57,7 @@

  

      MODULES_DEFAULT_REPOSITORY = 'https://src.fedoraproject.org/modules/'

      MODULES_ALLOW_REPOSITORY = False

+     MODULES_ALLOW_SCRATCH = False

  

      ALLOWED_GROUPS = set([

          'packager',

@@ -11,13 +11,27 @@

  

  There is the MBS frontend, which provides a REST API (See ``views.py``). A user sends a POST request

  with JSON describing the module to build. There is mainly the URL to the git repository (called

- ``scmurl``) and branch name (called ``branch``). The ``scmurl`` points to the git repository containing

- the modulemd file defining the module.

+ ``scmurl`` |---| which points to the git repository containing the modulemd file defining the module)

+ and branch name (called ``branch``).

  

  This JSON data is handled by ``views.SCMHandler``, which validates the JSON and calls

  ``utils.submit.submit_module_build_from_scm(...)`` method. This goes down to

  ``submit_module_build(...)``.

  

+ Alternatively, if submitting a YAML modulemd file is allowed (MBS setting

+ ``YAML_SUBMIT_ALLOWED`` is ``True``), the user can send a ``multipart/form-data``

+ POST request directly including the contents of a YAML modulemd file

+ (called ``yaml``). In this case, the JSON data and YAML file are handled by

+ ``views.YAMLFileHandler``, which validates the data and calls the

+ ``utils.submit.submit_module_build_from_yaml(...)`` method which also goes down

+ to ``submit_module_build(...)``.

+ 

+ If module scratch builds are allowed (MBS setting ``MODULE_ALLOW_SCRATCH`` is

+ ``True``), the user can also upload one or more source RPMs uploaded to Koji

+ via calls to Koji's ``session.uploadWrapper(..)``, and supply the list of

+ upload links to MBS (called ``srpms``). Such custom SRPMs will be used to

+ override the git repository source for corresponding components.

+ 

  

  Module Stream Expansion (MSE)

  -----------------------------

@@ -183,3 +197,6 @@

  This method imports the module build into Koji using the ``KojiContentGenerator`` class.

  The module build in Koji points to the Koji tag with the module's components and also contains the

  final modulemd files generated for earch architecture the module is built for.

+ 

+ .. |---| unicode:: U+2014  .. em dash, trimming surrounding whitespace

+    :trim:

@@ -758,7 +758,11 @@

                  raise RuntimeError("Buildroot is not prep-ed")

  

              self._koji_whitelist_packages([artifact_name])

-             if '://' not in source:

+ 

+             if source.startswith('cli-build/'):

+                 # treat source as a custom srpm that has already been uploaded to koji

+                 pass

+             elif '://' not in source:

                  # treat source as an srpm and upload it

                  serverdir = _unique_path('cli-build')

                  callback = None

@@ -1201,9 +1205,10 @@

              return list(nvrs)

  

      def finalize(self):

-         # Only import to koji CG if the module is "build".

-         if self.config.koji_enable_content_generator and \

-                 self.module.state == models.BUILD_STATES['build']:

+         # Only import to koji CG if the module is "build" and not scratch.

+         if (not self.module.scratch and

+                 self.config.koji_enable_content_generator and

+                 self.module.state == models.BUILD_STATES['build']):

              cg = KojiContentGenerator(self.module, self.config)

              cg.koji_import()

              if conf.koji_cg_devel_module:

@@ -1227,7 +1232,8 @@

          tags = []

          koji_tags = session.listTags(rpm_md["build_id"])

          for t in koji_tags:

-             if not t["name"].endswith("-build") and t["name"].startswith("module-"):

+             if (not t["name"].endswith("-build") and

+                     t["name"].startswith(tuple(conf.koji_tag_prefixes))):

                  tags.append(t["name"])

  

          return tags

@@ -197,7 +197,7 @@

              'desc': 'Target to build "module-build-macros" RPM in.'},

          'koji_tag_prefixes': {

              'type': list,

-             'default': ['module'],

+             'default': ['module', 'scrmod'],

              'desc': 'List of allowed koji tag prefixes.'},

          'koji_target_delete_time': {

              'type': int,

@@ -102,11 +102,12 @@

  

  @manager.option('--stream', action='store', dest="stream")

  @manager.option('--file', action='store', dest="yaml_file")

+ @manager.option('--srpm', action='append', default=[], dest="srpms", metavar='SRPM')

  @manager.option('--skiptests', action='store_true', dest="skiptests")

  @manager.option('-l', '--add-local-build', action='append', default=None, dest='local_build_nsvs')

  @manager.option('-s', '--set-stream', action='append', default=[], dest='default_streams')

- def build_module_locally(local_build_nsvs=None, yaml_file=None, stream=None, skiptests=False,

-                          default_streams=None):

+ def build_module_locally(local_build_nsvs=None, yaml_file=None, srpms=None,

+                          stream=None, skiptests=False, default_streams=None):

      """ Performs local module build using Mock

      """

      if 'SERVER_NAME' not in app.config or not app.config['SERVER_NAME']:

@@ -139,6 +140,8 @@

          for ns in default_streams:

              name, stream = ns.split(":")

              optional_params["default_streams"][name] = stream

+         if srpms:

+             optional_params["srpms"] = srpms

  

          username = getpass.getuser()

          if not yaml_file or not yaml_file.endswith(".yaml"):

@@ -0,0 +1,25 @@

+ """Add columns for module scratch builds

+ 

+ Revision ID: 60c6093dde9e

+ Revises: 526fb7d445f7

+ Create Date: 2019-02-01 22:06:03.916296

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = '60c6093dde9e'

+ down_revision = '526fb7d445f7'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ 

+ def upgrade():

+     op.add_column('module_builds', sa.Column('scratch', sa.Boolean(), nullable=True))

+     op.add_column('module_builds', sa.Column('srpms', sa.String(), nullable=True))

+ 

+ 

+ def downgrade():

+     with op.batch_alter_table('module_builds') as b:

+         b.drop_column('srpms')

+         b.drop_column('scratch')

@@ -197,6 +197,9 @@

      # Koji tag to which tag the Content Generator Koji build.

      cg_build_koji_tag = db.Column(db.String)  # This gets set after wait

      scmurl = db.Column(db.String)

+     scratch = db.Column(db.Boolean, default=False)

+     # JSON encoded list of links of custom SRPMs uploaded to Koji

+     srpms = db.Column(db.String)

      owner = db.Column(db.String, nullable=False)

      time_submitted = db.Column(db.DateTime, nullable=False)

      time_modified = db.Column(db.DateTime)

@@ -485,7 +488,8 @@

  

      @classmethod

      def create(cls, session, conf, name, stream, version, modulemd, scmurl, username,

-                context=None, rebuild_strategy=None, publish_msg=True):

+                context=None, rebuild_strategy=None, scratch=False, srpms=None,

+                publish_msg=True, **kwargs):

          now = datetime.utcnow()

          module = cls(

              name=name,

@@ -499,7 +503,10 @@

              time_submitted=now,

              time_modified=now,

              # If the rebuild_strategy isn't specified, use the default

-             rebuild_strategy=rebuild_strategy or conf.rebuild_strategy

+             rebuild_strategy=rebuild_strategy or conf.rebuild_strategy,

+             scratch=scratch,

+             srpms=json.dumps(srpms or []),

+             **kwargs

          )

          # Add a state transition to "init"

          mbt = ModuleBuildTrace(state_time=now, state=module.state)

@@ -636,13 +643,15 @@

              buildrequires = xmd['mbs']['buildrequires']

          except KeyError:

              buildrequires = {}

-         json = self.short_json()

-         json.update({

+         rv = self.short_json()

+         rv.update({

              'component_builds': [build.id for build in self.component_builds],

              'koji_tag': self.koji_tag,

              'owner': self.owner,

              'rebuild_strategy': self.rebuild_strategy,

              'scmurl': self.scmurl,

+             'scratch': self.scratch,

+             'srpms': json.loads(self.srpms or '[]'),

              'siblings': self.siblings,

              'state_reason': self.state_reason,

              'time_completed': _utc_datetime_to_iso(self.time_completed),

@@ -651,8 +660,8 @@

              'buildrequires': buildrequires,

          })

          if show_tasks:

-             json['tasks'] = self.tasks()

-         return json

+             rv['tasks'] = self.tasks()

+         return rv

  

      def extended_json(self, show_state_url=False, api_version=1):

          """

@@ -662,12 +671,12 @@

          SQLAlchemy sessions.

          :kwarg api_version: the API version to use when building the state URL

          """

-         json = self.json(show_tasks=True)

+         rv = self.json(show_tasks=True)

          state_url = None

          if show_state_url:

              state_url = get_url_for('module_build', api_version=api_version, id=self.id)

  

-         json.update({

+         rv.update({

              'base_module_buildrequires': [br.short_json(True) for br in self.buildrequires],

              'build_context': self.build_context,

              'modulemd': self.modulemd,

@@ -684,7 +693,7 @@

              'state_url': state_url,

          })

  

-         return json

+         return rv

  

      def tasks(self):

          """

@@ -62,7 +62,6 @@

          done = (

              module_build_service.models.BUILD_STATES["failed"],

              module_build_service.models.BUILD_STATES["ready"],

-             # XXX should this one be removed?

              module_build_service.models.BUILD_STATES["done"],

          )

          result = module.state in done

@@ -116,6 +116,7 @@

      """Called whenever a module enters the 'done' state.

  

      We currently don't do anything useful, so moving to ready.

+     Except for scratch module builds, which remain in the done state.

      Otherwise the done -> ready state should happen when all

      dependent modules were re-built, at least that's the current plan.

      """

@@ -128,8 +129,10 @@

          # This is ok.. it's a race condition we can ignore.

          pass

  

-     build.transition(config, state="ready")

-     session.commit()

+     # Scratch builds stay in 'done' state, otherwise move to 'ready'

+     if not build.scratch:

+         build.transition(config, state="ready")

+         session.commit()

  

      build_logs.stop(build)

      module_build_service.builder.GenericBuilder.clear_cache(build)

@@ -183,7 +186,8 @@

      """

      log.info('Getting tag for %s:%s:%s', build.name, build.stream, build.version)

      if conf.system in ['koji', 'test']:

-         return generate_koji_tag(build.name, build.stream, build.version, build.context)

+         return generate_koji_tag(build.name, build.stream, build.version, build.context,

+                                  scratch=build.scratch, scratch_id=build.id)

      else:

          return '-'.join(['module', build.name, build.stream, build.version])

  

@@ -295,7 +299,10 @@

      log.debug("Assigning koji tag=%s to module build" % tag)

      build.koji_tag = tag

  

-     if conf.koji_cg_tag_build:

+     if build.scratch:

+         log.debug('Assigning Content Generator build koji tag is skipped for'

+                   ' scratch module build.')

+     elif conf.koji_cg_tag_build:

          cg_build_koji_tag = get_content_generator_build_koji_tag(build_deps)

          log.debug("Assigning Content Generator build koji tag=%s to module build",

                    cg_build_koji_tag)

@@ -105,7 +105,7 @@

      return state

  

  

- def generate_koji_tag(name, stream, version, context, max_length=256):

+ def generate_koji_tag(name, stream, version, context, max_length=256, scratch=False, scratch_id=0):

      """Generate a koji tag for a module

  

      Generally, a module's koji tag is in format ``module-N-S-V-C``. However, if

@@ -115,19 +115,29 @@

      :param str stream: a module's stream

      :param str version: a module's version

      :param str context: a module's context

-     :kwarg int max_length: the maximum length the Koji tag can be before

+     :param int max_length: the maximum length the Koji tag can be before

          falling back to the old format of "module-<hash>". Default is 256

          characters, which is the maximum length of a tag Koji accepts.

+     :param bool scratch: a flag indicating if the generated tag will be for

+         a scratch module build

+     :param int scratch_id: for scratch module builds, a unique build identifier

      :return: a Koji tag

      :rtype: str

      """

+     if scratch:

+         prefix = 'scrmod-'

+         # use unique suffix so same commit can be resubmitted

+         suffix = '+' + str(scratch_id)

+     else:

+         prefix = 'module-'

+         suffix = ''

      nsvc_list = [name, stream, str(version), context]

-     nsvc_tag = 'module-' + '-'.join(nsvc_list)

+     nsvc_tag = prefix + '-'.join(nsvc_list) + suffix

      if len(nsvc_tag) + len('-build') > max_length:

          # Fallback to the old format of 'module-<hash>' if the generated koji tag

          # name is longer than max_length

          nsvc_hash = hashlib.sha1('.'.join(nsvc_list).encode('utf-8')).hexdigest()[:16]

-         return 'module-' + nsvc_hash

+         return prefix + nsvc_hash + suffix

      return nsvc_tag

  

  

@@ -241,8 +251,11 @@

              log.warning('Module build {0} does not buildrequire a base module ({1})'

                          .format(module_build.id, ' or '.join(conf.base_module_names)))

  

+     # use alternate prefix for scratch module build components so they can be identified

+     prefix = ('scrmod+' if module_build.scratch else conf.default_dist_tag_prefix)

I'm wondering if it'd be better to make this configurable like conf.default_dist_tag_prefix or if it makes sense to just continue using conf.default_dist_tag_prefix but add "scr+" after it. What do you think @merlinm and @jkaluza?

I defer to the the subject matter experts here. The prefix just needs to be something short and easily identifiable for later garbage collection, right?

+ 

      return '{prefix}{base_module_stream}{index}+{dist_hash}'.format(

-         prefix=conf.default_dist_tag_prefix,

+         prefix=prefix,

          base_module_stream=base_module_stream,

          index=index,

          dist_hash=dist_hash,

@@ -93,7 +93,7 @@

      previous_module_build = session.query(models.ModuleBuild)\

          .filter_by(name=mmd.get_name())\

          .filter_by(stream=mmd.get_stream())\

-         .filter(models.ModuleBuild.state.in_([3, 5]))\

+         .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())

@@ -22,6 +22,7 @@

  # Written by Ralph Bean <rbean@redhat.com>

  #            Matt Prahl <mprahl@redhat.com>

  #            Jan Kaluza <jkaluza@redhat.com>

+ import json

  import re

  import time

  import shutil

@@ -313,6 +314,56 @@

      mmd.set_xmd(glib.dict_values(xmd))

  

  

+ def get_module_srpm_overrides(module):

+     """

+     Make necessary preparations to use any provided custom SRPMs.

+ 

+     :param module: ModuleBuild object representing the module being submitted.

+     :type module: :class:`models.ModuleBuild`

+     :return: mapping of package names to SRPM links for all packages which

+              have custom SRPM overrides specified

+     :rtype: dict[str, str]

+ 

+     """

+     overrides = {}

+ 

+     if not module.srpms:

+         return overrides

+ 

+     try:

+         # Make sure we can decode the custom SRPM list

+         srpms = json.loads(module.srpms)

+         assert isinstance(srpms, list)

+     except Exception:

+         raise ValueError("Invalid srpms list encountered: {}".format(module.srpms))

+ 

+     for source in srpms:

+         if source.startswith('cli-build/') and source.endswith('.src.rpm'):

+             # This is a custom srpm that has been uploaded to koji by rpkg

+             # using the package name as the basename suffixed with .src.rpm

+             rpm_name = os.path.basename(source)[:-len('.src.rpm')]

+         else:

+             # This should be a local custom srpm path

+             if not os.path.exists(source):

+                 raise IOError("Provided srpm is missing: {}".format(source))

+             # Get package name from rpm headers

+             try:

+                 rpm_hdr = kobo.rpmlib.get_rpm_header(source)

+                 rpm_name = kobo.rpmlib.get_header_field(rpm_hdr, 'name').decode('utf-8')

+             except Exception:

+                 raise ValueError("Provided srpm is invalid: {}".format(source))

+ 

+         if rpm_name in overrides:

+             log.warning('Encountered duplicate custom SRPM "{0}"'

+                         ' for package {1}'.format(source, rpm_name))

+             continue

+ 

+         log.debug('Using custom SRPM "{0}" for package {1}'.format(source, rpm_name))

+         overrides[rpm_name] = source

+ 

+     return overrides

+ 

+ 

  def record_component_builds(mmd, module, initial_batch=1,

                              previous_buildorder=None, main_mmd=None, session=None):

      # Imported here to allow import of utils in GenericBuilder.

@@ -349,6 +400,9 @@

      if not all_components:

          return

  

+     # Get map of packages that have SRPM overrides

+     srpm_overrides = get_module_srpm_overrides(module)

+ 

      rpm_weights = module_build_service.builder.GenericBuilder.get_build_weights(

          [c.get_name() for c in rpm_components])

      all_components.sort(key=lambda x: x.get_buildorder())

@@ -376,13 +430,20 @@

                                              previous_buildorder, main_mmd, session=session)

              continue

  

-         component_ref = mmd.get_xmd()['mbs']['rpms'][component.get_name()]['ref']

-         full_url = component.get_repository() + "?#" + component_ref

+         package = component.get_name()

+         if package in srpm_overrides:

+             component_ref = None

+             full_url = srpm_overrides[package]

+             log.info('Building custom SRPM "{0}"'

+                      ' for package {1}'.format(full_url, package))

+         else:

+             component_ref = mmd.get_xmd()['mbs']['rpms'][package]['ref']

+             full_url = component.get_repository() + "?#" + component_ref

  

          # Skip the ComponentBuild if it already exists in database. This can happen

          # in case of module build resubmition.

          existing_build = models.ComponentBuild.from_component_name(

-             db.session, component.get_name(), module.id)

+             db.session, package, module.id)

          if existing_build:

              # Check that the existing build has the same most important attributes.

              # This should never be a problem, but it's good to be defensive here so

@@ -396,12 +457,12 @@

  

          build = models.ComponentBuild(

              module_id=module.id,

-             package=component.get_name(),

+             package=package,

              format="rpms",

              scmurl=full_url,

              batch=batch,

              ref=component_ref,

-             weight=rpm_weights[component.get_name()]

+             weight=rpm_weights[package]

          )

          session.add(build)

  

file modified
+27 -12

@@ -37,7 +37,7 @@

      pagination_metadata, filter_module_builds, filter_component_builds,

      submit_module_build_from_scm, submit_module_build_from_yaml,

      get_scm_url_re, cors_header, validate_api_version, import_mmd,

-     get_mmd_from_scm)

+     get_mmd_from_scm, str_to_bool)

  from module_build_service.errors import (

      ValidationError, Forbidden, NotFound, ProgrammingError)

  from module_build_service.backports import jsonify

@@ -172,7 +172,7 @@

      # Additional POST and DELETE handlers for modules follow.

      @validate_api_version()

      def post(self, api_version):

-         if "multipart/form-data" in request.headers.get("Content-Type", ""):

+         if hasattr(request, 'files') and "yaml" in request.files:

              handler = YAMLFileHandler(request)

          else:

              handler = SCMHandler(request)

@@ -314,7 +314,31 @@

  class BaseHandler(object):

      def __init__(self, request):

          self.username, self.groups = module_build_service.auth.get_user(request)

-         self.data = None

+         if "multipart/form-data" in request.headers.get("Content-Type", ""):

+             self.data = request.form.to_dict()

+         else:

+             try:

+                 self.data = json.loads(request.get_data().decode("utf-8"))

+             except Exception:

+                 log.error('Invalid JSON submitted')

+                 raise ValidationError('Invalid JSON submitted')

+ 

+         # canonicalize and validate scratch option

+         if 'scratch' in self.data and str_to_bool(str(self.data['scratch'])):

+             self.data['scratch'] = True

+             if conf.modules_allow_scratch is not True:

+                 raise Forbidden('Scratch builds are not enabled')

+         else:

+             self.data['scratch'] = False

+ 

+         # canonicalize and validate srpms list

+         if 'srpms' in self.data and self.data['srpms']:

+             if not self.data['scratch']:

+                 raise Forbidden('srpms may only be specified for scratch builds')

+             if not isinstance(self.data['srpms'], list):

+                 raise ValidationError('srpms must be specified as a list')

+         else:

+             self.data['srpms'] = []

  

      @property

      def optional_params(self):

@@ -371,14 +395,6 @@

  

  

  class SCMHandler(BaseHandler):

-     def __init__(self, request):

-         super(SCMHandler, self).__init__(request)

-         try:

-             self.data = json.loads(request.get_data().decode("utf-8"))

-         except Exception:

-             log.error('Invalid JSON submitted')

-             raise ValidationError('Invalid JSON submitted')

- 

      def validate(self, skip_branch=False, skip_optional_params=False):

          if "scmurl" not in self.data:

              log.error('Missing scmurl')

@@ -415,7 +431,6 @@

          if not conf.yaml_submit_allowed:

              raise Forbidden("YAML submission is not enabled")

          super(YAMLFileHandler, self).__init__(request)

-         self.data = request.form.to_dict()

  

      def validate(self):

          if "yaml" not in request.files:

@@ -127,7 +127,7 @@

            RESOLVER = 'db'

  

            # This is a whitelist of prefixes of koji tags we're allowed to manipulate

-           KOJI_TAG_PREFIXES = ["module"]

+           KOJI_TAG_PREFIXES = ["module", "scrmod"]

  

            DEFAULT_DIST_TAG_PREFIX = 'module'

  

file modified
+20 -7

@@ -110,7 +110,7 @@

          import_mmd(db.session, mmd)

  

  

- def init_data(data_size=10, contexts=False, multiple_stream_versions=False):

+ def init_data(data_size=10, contexts=False, multiple_stream_versions=False, scratch=False):

      """

      Creates data_size * 3 modules in database in different states and

      with different component builds. See _populate_data for more info.

@@ -132,10 +132,10 @@

              mmd.set_stream(stream)

              import_mmd(db.session, mmd)

      with make_session(conf) as session:

-         _populate_data(session, data_size, contexts=contexts)

+         _populate_data(session, data_size, contexts=contexts, scratch=scratch)

  

  

- def _populate_data(session, data_size=10, contexts=False):

+ def _populate_data(session, data_size=10, contexts=False, scratch=False):

      num_contexts = 2 if contexts else 1

      for index in range(data_size):

          for context in range(num_contexts):

@@ -144,6 +144,7 @@

              build_one.stream = '1'

              build_one.version = 2 + index

              build_one.state = BUILD_STATES['ready']

+             build_one.scratch = scratch

              if contexts:

                  build_one.stream = str(index)

                  unique_hash = hashlib.sha1(("%s:%s:%d:%d" % (

@@ -155,7 +156,10 @@

                  combined_hashes = '{0}:{1}'.format(unique_hash, unique_hash)

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

              build_one.modulemd = read_staged_data('nginx_mmd')

-             build_one.koji_tag = 'module-nginx-1.2'

+             if scratch:

+                 build_one.koji_tag = 'scrmod-nginx-1.2'

+             else:

+                 build_one.koji_tag = 'module-nginx-1.2'

              build_one.scmurl = ('git://pkgs.domain.local/modules/nginx?'

                                  '#ba95886c7a443b36a9ce31abda1f9bef22f2f8c9')

              build_one.batch = 2

@@ -206,8 +210,12 @@

          build_two.stream = '1'

          build_two.version = 2 + index

          build_two.state = BUILD_STATES['done']

+         build_two.scratch = scratch

          build_two.modulemd = read_staged_data('testmodule')

-         build_two.koji_tag = 'module-postgressql-1.2'

+         if scratch:

+             build_two.koji_tag = 'scrmod-postgressql-1.2'

+         else:

+             build_two.koji_tag = 'module-postgressql-1.2'

          build_two.scmurl = ('git://pkgs.domain.local/modules/postgressql?'

                              '#aa95886c7a443b36a9ce31abda1f9bef22f2f8c9')

          build_two.batch = 2

@@ -257,6 +265,7 @@

          build_three.stream = '4.3.43'

          build_three.version = 6 + index

          build_three.state = BUILD_STATES['wait']

+         build_three.scratch = scratch

          build_three.modulemd = read_staged_data('testmodule')

          build_three.koji_tag = None

          build_three.scmurl = ('git://pkgs.domain.local/modules/testmodule?'

@@ -310,7 +319,7 @@

          session.commit()

  

  

- def scheduler_init_data(tangerine_state=None):

+ def scheduler_init_data(tangerine_state=None, scratch=False):

      """ Creates a testmodule in the building state with all the components in the same batch

      """

      clean_database()

@@ -329,10 +338,14 @@

      build_one.stream = 'master'

      build_one.version = 20170109091357

      build_one.state = BUILD_STATES['build']

+     build_one.scratch = scratch

      build_one.build_context = 'ac4de1c346dcf09ce77d38cd4e75094ec1c08eb0'

      build_one.runtime_context = 'ac4de1c346dcf09ce77d38cd4e75094ec1c08eb0'

      build_one.context = '7c29193d'

-     build_one.koji_tag = 'module-testmodule-master-20170109091357-7c29193d'

+     if scratch:

+         build_one.koji_tag = 'scrmod-testmodule-master-20170109091357-7c29193d'

+     else:

+         build_one.koji_tag = 'module-testmodule-master-20170109091357-7c29193d'

      build_one.scmurl = 'https://src.stg.fedoraproject.org/modules/testmodule.git?#ff1ea79'

      if tangerine_state:

          build_one.batch = 3

@@ -336,6 +336,21 @@

          release = module_build_service.utils.get_rpm_release(build_one)

          assert release == 'module+f28+2+814cfa39'

  

+     def test_get_rpm_release_mse_scratch(self):

+         init_data(contexts=True, scratch=True)

+         build_one = models.ModuleBuild.query.get(2)

+         build_two = models.ModuleBuild.query.get(3)

+         release_one = module_build_service.utils.get_rpm_release(build_one)

+         release_two = module_build_service.utils.get_rpm_release(build_two)

+         assert release_one == "scrmod+2+b8645bbb"

+         assert release_two == "scrmod+2+17e35784"

+ 

+     def test_get_rpm_release_platform_stream_scratch(self):

+         scheduler_init_data(1, scratch=True)

+         build_one = models.ModuleBuild.query.get(2)

+         release = module_build_service.utils.get_rpm_release(build_one)

+         assert release == 'scrmod+f28+2+814cfa39'

+ 

      @pytest.mark.parametrize('scmurl', [

          ('https://src.stg.fedoraproject.org/modules/testmodule.git'

           '?#620ec77321b2ea7b0d67d82992dda3e1d67055b4'),

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

  import module_build_service.scm

  

  from mock import patch, PropertyMock

+ from werkzeug.datastructures import FileStorage

  from shutil import copyfile

  from os import path, mkdir

  from os.path import dirname

@@ -129,6 +130,8 @@

          assert data['context'] == '00000000'

          assert data['name'] == 'nginx'

          assert data['owner'] == 'Moe Szyslak'

+         assert data['scratch'] is False

+         assert data['srpms'] == []

          assert data['stream'] == '1'

          assert data['siblings'] == []

          assert data['state'] == 5

@@ -190,6 +193,8 @@

          assert data['owner'] == 'Moe Szyslak'

          assert data['scmurl'] == ('git://pkgs.domain.local/modules/nginx'

                                    '?#ba95886c7a443b36a9ce31abda1f9bef22f2f8c9')

+         assert data['scratch'] is False

+         assert data['srpms'] == []

          assert data['siblings'] == []

          assert data['state'] == 5

          assert data['state_name'] == 'ready'

@@ -274,7 +279,9 @@

                  "rebuild_strategy": "changed-and-after",

                  "scmurl": ("git://pkgs.domain.local/modules/testmodule"

                             "?#ca95886c7a443b36a9ce31abda1f9bef22f2f8c9"),

+                 "scratch": False,

                  "siblings": [],

+                 "srpms": [],

                  "state": 1,

                  "state_name": "wait",

                  "state_reason": None,

@@ -311,7 +318,9 @@

                  "rebuild_strategy": "changed-and-after",

                  "scmurl": ("git://pkgs.domain.local/modules/postgressql"

                             "?#aa95886c7a443b36a9ce31abda1f9bef22f2f8c9"),

+                 "scratch": False,

                  "siblings": [],

+                 "srpms": [],

                  "state": 3,

                  "state_name": "done",

                  "state_reason": None,

@@ -358,7 +367,9 @@

                  "rebuild_strategy": "changed-and-after",

                  "scmurl": ("git://pkgs.domain.local/modules/nginx"

                             "?#ba95886c7a443b36a9ce31abda1f9bef22f2f8c9"),

+                 "scratch": False,

                  "siblings": [2],

+                 "srpms": [],

                  "state": 5,

                  "state_name": "ready",

                  "state_reason": None,

@@ -1666,3 +1677,180 @@

  

          assert br_modulea == buildrequires.get('modulea')

          assert br_moduleb == buildrequires.get('moduleb')

+ 

+     @pytest.mark.parametrize('api_version', [1, 2])

+     @patch('module_build_service.auth.get_user', return_value=user)

+     @patch('module_build_service.scm.SCM')

+     @patch('module_build_service.config.Config.modules_allow_scratch',

+            new_callable=PropertyMock, return_value=True)

+     def test_submit_scratch_build(self, mocked_allow_scratch, mocked_scm, mocked_get_user,

+                                   api_version):

+         FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml',

+                 '620ec77321b2ea7b0d67d82992dda3e1d67055b4')

+ 

+         post_url = '/module-build-service/{0}/module-builds/'.format(api_version)

+         post_data = {

+             'branch': 'master',

+             'scmurl': 'https://src.stg.fedoraproject.org/modules/'

+                       'testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49',

+             'scratch': True,

+         }

+         rv = self.client.post(post_url, data=json.dumps(post_data))

+         data = json.loads(rv.data)

+ 

+         if api_version >= 2:

+             assert isinstance(data, list)

+             assert len(data) == 1

+             data = data[0]

+ 

+         assert 'component_builds' in data, data

+         assert data['component_builds'] == []

+         assert data['name'] == 'testmodule'

+         assert data['scmurl'] == ('https://src.stg.fedoraproject.org/modules/testmodule.git'

+                                   '?#68931c90de214d9d13feefbd35246a81b6cb8d49')

+         assert data['scratch'] is True

+         assert data['version'] == '281'

+         assert data['time_submitted'] is not None

+         assert data['time_modified'] is not None

+         assert data['time_completed'] is None

+         assert data['stream'] == 'master'

+         assert data['owner'] == 'Homer J. Simpson'

+         assert data['id'] == 8

+         assert data['rebuild_strategy'] == 'changed-and-after'

+         assert data['state_name'] == 'init'

+         assert data['state_url'] == '/module-build-service/{0}/module-builds/8'.format(api_version)

+         assert len(data['state_trace']) == 1

+         assert data['state_trace'][0]['state'] == 0

+         assert data['tasks'] == {}

+         assert data['siblings'] == []

+         mmd = Modulemd.Module().new_from_string(data['modulemd'])

+         mmd.upgrade()

+ 

+         # Make sure the buildrequires entry was created

+         module = ModuleBuild.query.get(8)

+         assert len(module.buildrequires) == 1

+         assert module.buildrequires[0].name == 'platform'

+         assert module.buildrequires[0].stream == 'f28'

+         assert module.buildrequires[0].version == '3'

+         assert module.buildrequires[0].context == '00000000'

+         assert module.buildrequires[0].stream_version == 280000

+ 

+     @pytest.mark.parametrize('api_version', [1, 2])

+     @patch('module_build_service.auth.get_user', return_value=user)

+     @patch('module_build_service.scm.SCM')

+     @patch('module_build_service.config.Config.modules_allow_scratch',

+            new_callable=PropertyMock, return_value=False)

+     def test_submit_scratch_build_not_allowed(self, mocked_allow_scratch,

+                                               mocked_scm, mocked_get_user,

+                                               api_version):

+         FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml',

+                 '620ec77321b2ea7b0d67d82992dda3e1d67055b4')

+ 

+         post_url = '/module-build-service/{0}/module-builds/'.format(api_version)

+         post_data = {

+             'branch': 'master',

+             'scmurl': 'https://src.stg.fedoraproject.org/modules/'

+                       'testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49',

+             'scratch': True,

+         }

+         rv = self.client.post(post_url, data=json.dumps(post_data))

+         data = json.loads(rv.data)

+         expected_error = {

+             'error': 'Forbidden',

+             'message': 'Scratch builds are not enabled',

+             'status': 403

+         }

+         assert data == expected_error

+         assert rv.status_code == expected_error['status']

+ 

+     @pytest.mark.parametrize('api_version', [1, 2])

+     @patch('module_build_service.auth.get_user', return_value=user)

+     @patch('module_build_service.config.Config.modules_allow_scratch',

+            new_callable=PropertyMock, return_value=True)

+     @patch('module_build_service.config.Config.yaml_submit_allowed',

+            new_callable=PropertyMock, return_value=True)

+     def test_submit_scratch_build_with_mmd(self, mocked_allow_yaml,

+                                            mocked_allow_scratch,

+                                            mocked_get_user,

+                                            api_version):

+         base_dir = path.abspath(path.dirname(__file__))

+         mmd_path = path.join(base_dir, '..', 'staged_data', 'testmodule.yaml')

+         post_url = '/module-build-service/{0}/module-builds/'.format(api_version)

+         with open(mmd_path, 'rb') as f:

+             yaml_file = FileStorage(f)

+             post_data = {

+                 'branch': 'master',

+                 'scratch': True,

+                 'yaml': yaml_file,

+             }

+             rv = self.client.post(post_url, content_type='multipart/form-data', data=post_data)

+         data = json.loads(rv.data)

+ 

+         if api_version >= 2:

+             assert isinstance(data, list)

+             assert len(data) == 1

+             data = data[0]

+ 

+         assert 'component_builds' in data, data

+         assert data['component_builds'] == []

+         assert data['name'] == 'testmodule'

+         assert data['scmurl'] is None

+         # generated modulemd is nondeterministic, so just make sure it is set

+         assert data['modulemd'] is not None

+         assert data['scratch'] is True

+         # generated version is nondeterministic, so just make sure it is set

+         assert data['version'] is not None

+         assert data['time_submitted'] is not None

+         assert data['time_modified'] is not None

+         assert data['time_completed'] is None

+         assert data['stream'] == 'master'

+         assert data['owner'] == 'Homer J. Simpson'

+         assert data['id'] == 8

+         assert data['rebuild_strategy'] == 'changed-and-after'

+         assert data['state_name'] == 'init'

+         assert data['state_url'] == '/module-build-service/{0}/module-builds/8'.format(api_version)

+         assert len(data['state_trace']) == 1

+         assert data['state_trace'][0]['state'] == 0

+         assert data['tasks'] == {}

+         assert data['siblings'] == []

+         mmd = Modulemd.Module().new_from_string(data['modulemd'])

+         mmd.upgrade()

+ 

+         # Make sure the buildrequires entry was created

+         module = ModuleBuild.query.get(8)

+         assert len(module.buildrequires) == 1

+         assert module.buildrequires[0].name == 'platform'

+         assert module.buildrequires[0].stream == 'f28'

+         assert module.buildrequires[0].version == '3'

+         assert module.buildrequires[0].context == '00000000'

+         assert module.buildrequires[0].stream_version == 280000

+ 

+     @pytest.mark.parametrize('api_version', [1, 2])

+     @patch('module_build_service.auth.get_user', return_value=user)

+     @patch('module_build_service.config.Config.modules_allow_scratch',

+            new_callable=PropertyMock, return_value=True)

+     @patch('module_build_service.config.Config.yaml_submit_allowed',

+            new_callable=PropertyMock, return_value=False)

+     def test_submit_scratch_build_with_mmd_not_allowed(self, mocked_allow_yaml,

+                                                        mocked_allow_scratch,

+                                                        mocked_get_user,

+                                                        api_version):

+         base_dir = path.abspath(path.dirname(__file__))

+         mmd_path = path.join(base_dir, '..', 'staged_data', 'testmodule.yaml')

+         post_url = '/module-build-service/{0}/module-builds/'.format(api_version)

+         with open(mmd_path, 'rb') as f:

+             yaml_file = FileStorage(f)

+             post_data = {

+                 'branch': 'master',

+                 'scratch': True,

+                 'yaml': yaml_file,

+             }

+             rv = self.client.post(post_url, content_type='multipart/form-data', data=post_data)

+         data = json.loads(rv.data)

+         expected_error = {

+             'error': 'Forbidden',

+             'message': 'YAML submission is not enabled',

+             'status': 403

+         }

+         assert data == expected_error

+         assert rv.status_code == expected_error['status']

This is currently a work in progress--merge at your own risk.

This is the fm-orchestrator portion of the implementation of the Proposal for Module Scratch Builds as described at https://pagure.io/modularity/working-documents/blob/master/f/module-scratch-builds/module-scratch-build-proposal.md.

There are a few places in the code where I'm not sure how to proceed that I have indicated with comments containing "TODO scrmod:".

Feedback and guidance are appreciated.

Note: To run the docker based tests at the moment, the update of pip -- included in the docker/Dockerfile-tests change as part of PR #1133 -- is needed.

I've checked it very briefly before the Biathlon starts :). Will do more formal review on Monday. You did good job here.

From the code it looks like the scratch module can also end up in "ready" state as any other normal module. If that's the case, we need to ensure that scratch modules are not considered for Module Stream Expansion, or for component reuse and other things.

You can see places like that in a code by running:

$ grep BUILD_STATE -R .|grep ready
$ grep ModuleBuild.state -R .|grep 5

I think most of these queries need to check that ModuleBuild.scratch is False.

Or maybe the better way really would be to not move scratch module builds to "ready", but end them in "done" state. That way, it is clear that they cannot be used by other modules.

The corresponding rpkg work can be found at https://pagure.io/rpkg/pull-request/414.
The corresponding fedpkg work can be found at https://pagure.io/fedpkg/pull-request/309.

nice nice nice! looking forward to see land this!

I know @merlinm is on PTO, but since people are asking me what is the state of this.... The only blocker I see is what I've described in my previous comments about "ready" state.

It also needs to be rebased.

Otherwise it looks good to me. We will need to check this on staging, but the amount of changes are low and the biggest new code is mostly executed only in case of scratch-builds, so the chance this would break any current MBS behaviour is low.

I still have one question, @merlinm, how do you plan to remove the expired scratch-builds? Will that be done by koji-gc? This is not blocker for this PR, but I wonder what is the plan.

Instead of hard-coding this, can we please use config.koji_tag_prefixes?

I don't think we'll need to check for srcmod since I don't think we'd be doing local scratch builds. @jkaluza thoughts on this todo comment?

To support SQLite for local development, could you please change your downgrade method to the following?

with op.batch_alter_table('module_builds') as b:
    b.drop_column('srpms')
    b.drop_column('scratch')

Optional: I actually preferred json.dumps and json.loads. It make it easier to read, but it's also fine as is.

Can you use srpms=None since a default parameter shouldn't be an object that can be mutated?

Here is a better explanation:
https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

Optional: How about srpms=dumps(srpms or []), assuming srpms will be None?

Optional: I don't like how config.koji_tag_prefixes already has this information. I wonder if we should just remove this configuration option entirely and have a tag_prefixes class variable or method that takes scratch as a argument on the KojiModuleBuilder class.

Good point. Since the tag is based on the NSVC, and I assume you can resubmit the same commit for a scratch build, that you'll need something unique for each scratch build. Maybe the ID of the module build in the database? @jkaluza thoughts?

Yes, this would be the correct place for it. I assume something like scratch as a prefix/suffix would be good enough.

This isn't an exception that we handle in Flask in errors.py so if this is encountered, the user would get a 500 error assuming this is called in the Flask API and not the back-end. Either use an existing exception that is handled in errors.py or add IOError as one that is handled.

Edit: It seems this is called in the back-end, so this is ok.

This constructor should just be removed since it doesn't do anything the base class' constructor doesn't do.

Is cli-build/ a convention you expect user's to know about for custom SRPMs already uploaded to Koji. Perhaps this should be documented.

@merlinm looks great so far. Thanks for your contribution.

I think we do not to set anything in modulemd for custom SRPMs, because we will not ship these scratch builds and this part of code is mainly needed for modules we are going to ship.

I think rpkg will be configure to upload the custom SRPMs to this server-side directory, so users do not have to know about this as long as they are using rpkg.

@jkaluza @mprahl Thank you for the feedback. I'll respond as soon as I get a bit caught up after my PTO.

I would have preferred to keep it the original way, too. However, as soon as I added the call to json.loads() within the json() method of the ModuleBuild class, things exploded! I considered using import json as jsonlib or similar, but figured that would have been more confusing to the reader than simply importing the methods.

Maybe you could just rename the json variable in the json method.

cli-build/ is the same convention rpkg already uses when uploading SRPMs to Koji for package scratch-builds.

Hmmm. I thought it was the json() method name that was causing the name overloading problem. I'll check that.

Indeed, it was the json variable name that was causing the problem. Thanks. I'll change the variable name and revert the calls back to json.dumps and json.loads.

I still have one question, @merlinm, how do you plan to remove the expired scratch-builds? Will that be done by koji-gc? This is not blocker for this PR, but I wonder what is the plan.

@jkaluza, Yes, koji-gc is the plan.

9 new commits added

  • Cleanup extraneous comments.
  • Keep scratch module builds in the 'done' state.
  • Use alternate prefix for scratch module build components so they can
  • Revise koji tagging for scratch modules to make them unique so the same
  • Cleanup __init__ method no longer needed in SCMHandler.
  • Module scratch build data model cleanup per review feedback.
  • Module scratch build Koji tagging cleanup per review feedback.
  • Module scratch build database migration cleanup per review feedback.
  • Revise command line parsing for local module builds to require SRPMs to be
5 months ago

@jkaluza @mprahl I think I have addressed all of your feedback except rebasing. Would you care to review before I do that? Thanks!

rebased onto 303def7bbf9942eb4a754e3c81e016512d4097a1

5 months ago

@jkaluza @mprahl Rebasing to the latest is now complete.

@merlinm could you please fix the flake8 errors? It's causing the Jenkins job to fail.

I'm wondering if it'd be better to make this configurable like conf.default_dist_tag_prefix or if it makes sense to just continue using conf.default_dist_tag_prefix but add "scr+" after it. What do you think @merlinm and @jkaluza?

@merlinm the changes look good but you'll need to make some changes in module_build_service.utils.reuse.get_reusable_component so that components from scratch builds are not reused.

@merlinm you'll also need to modify content in module_build_service/resolvers so that dependency resolution does not take into account scratch builds.

@mprahl I'll take a closer look, but doesn't the fact that scratch builds never go to state="ready" prevent them from being considered?

I defer to the the subject matter experts here. The prefix just needs to be something short and easily identifiable for later garbage collection, right?

@mprahl I'll take a closer look, but doesn't the fact that scratch builds never go to state="ready" prevent them from being considered?

That should be the case but it's not. You can see that inget_reusable_module. It checks for modules in the 3 or 5 state? Could you modify this to just be 5 (i.e. ready)?

You are correct though that the resolver code should be fine.

1 new commit added

  • Correct flake8 errors.
5 months ago

1 new commit added

  • Prevent scratch build components from being reused.
5 months ago

@merlinm the changes look good but you'll need to make some changes in module_build_service.utils.reuse.get_reusable_component so that components from scratch builds are not reused.

@mprahl I adjusted get_reusable_module to only consider modules in the 'ready' state. With that, it appears that no changes are needed to get_reusable_component--since it will never try to use a scratch build as the previous build.

Can you add new kwargs to the end? I don't think there is a method calling .create() with all the args/kwargs, but still it's good habit :).

looks good to me otherwise.

Can you remove this or actually assert the version is correct?

@merlinm looks great. Could you please squash your "fixup" commits and address the minor comments before we merge it?

1 new commit added

  • Add kwargs to ModuleBuild.create
4 months ago

Can you add new kwargs to the end? I don't think there is a method calling .create() with all the args/kwargs, but still it's good habit :).

@jkaluza Is https://pagure.io/fork/merlinm/fm-orchestrator/c/5468e92666ad88a3041eb877d3369b79814b61ff what you had in mind?

1 new commit added

  • Revise comments regarding modulemd/version for scratch build test
4 months ago

Can you remove this or actually assert the version is correct?

@mprahl The generated modulemd and version values are nondeterministic, so I updated the comments to generically indicate that.

rebased onto fcac146

4 months ago

@mprahl @jkaluza I've squashed the fixup comments and rebased again for good measure.

+1, merging.

I will create new release later today and deploy this on internal staging for more tests.

Pull-Request has been merged by jkaluza

4 months ago

If there would be any guidance how we can start experimenting with this in CI, that would help, also without the official rhpkg support ...