From 152419f376a71e1e776dbbfdd537e1fc23b624f0 Mon Sep 17 00:00:00 2001 From: Merlin Mathesius Date: Fri, 1 Mar 2019 10:22:37 -0600 Subject: [PATCH] Module scratch build fixups per PR review feedback: - Keep scratch module builds in the 'done' state. - Make koji tagging for scratch modules unique so the same commit can be resubmitted. - Use alternate prefix for scratch module build components so they can be identified later. - Prevent scratch build components from being reused. - Assorted code and comment cleanup. Signed-off-by: Merlin Mathesius --- .../builder/KojiModuleBuilder.py | 3 +- .../builder/MockModuleBuilder.py | 1 - .../migrations/versions/60c6093dde9e_.py | 9 ++---- module_build_service/models.py | 30 ++++++++++--------- module_build_service/scheduler/__init__.py | 1 - .../scheduler/handlers/modules.py | 9 ++++-- module_build_service/utils/general.py | 24 +++++++++------ module_build_service/utils/reuse.py | 2 +- module_build_service/utils/submit.py | 6 ++-- module_build_service/views.py | 3 -- tests/__init__.py | 27 ++++++++++++----- tests/test_utils/test_utils.py | 15 ++++++++++ tests/test_views/test_views.py | 4 +-- 13 files changed, 82 insertions(+), 52 deletions(-) diff --git a/module_build_service/builder/KojiModuleBuilder.py b/module_build_service/builder/KojiModuleBuilder.py index 9936d7e9..5d7c60d7 100644 --- a/module_build_service/builder/KojiModuleBuilder.py +++ b/module_build_service/builder/KojiModuleBuilder.py @@ -1232,7 +1232,8 @@ chmod 644 %buildroot/etc/rpm/macros.zz-modules 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-", "scrmod-")): + if (not t["name"].endswith("-build") and + t["name"].startswith(tuple(conf.koji_tag_prefixes))): tags.append(t["name"]) return tags diff --git a/module_build_service/builder/MockModuleBuilder.py b/module_build_service/builder/MockModuleBuilder.py index 92f20899..a9534d75 100644 --- a/module_build_service/builder/MockModuleBuilder.py +++ b/module_build_service/builder/MockModuleBuilder.py @@ -318,7 +318,6 @@ class MockModuleBuilder(GenericBuilder): # module build repository. if tag.startswith(conf.mock_resultsdir): repo_name = os.path.basename(tag) - # TODO scrmod: is a check also needed for scratch module tag prefix? if repo_name.startswith("module-"): repo_name = repo_name[7:] repo_dir = tag diff --git a/module_build_service/migrations/versions/60c6093dde9e_.py b/module_build_service/migrations/versions/60c6093dde9e_.py index adad2671..2ecafbd6 100644 --- a/module_build_service/migrations/versions/60c6093dde9e_.py +++ b/module_build_service/migrations/versions/60c6093dde9e_.py @@ -15,14 +15,11 @@ import sqlalchemy as sa def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.add_column('module_builds', sa.Column('scratch', sa.Boolean(), nullable=True)) op.add_column('module_builds', sa.Column('srpms', sa.String(), nullable=True)) - # ### end Alembic commands ### def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.drop_column('module_builds', 'srpms') - op.drop_column('module_builds', 'scratch') - # ### end Alembic commands ### + with op.batch_alter_table('module_builds') as b: + b.drop_column('srpms') + b.drop_column('scratch') diff --git a/module_build_service/models.py b/module_build_service/models.py index fa09b064..5da2dfa0 100644 --- a/module_build_service/models.py +++ b/module_build_service/models.py @@ -27,7 +27,7 @@ """ import contextlib -from json import dumps, loads +import json from collections import OrderedDict from datetime import datetime import hashlib @@ -448,13 +448,13 @@ class ModuleBuild(MBSBase): 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()} - property_json = dumps(OrderedDict(sorted(mmd_formatted_buildrequires.items()))) + property_json = json.dumps(OrderedDict(sorted(mmd_formatted_buildrequires.items()))) rv.append(hashlib.sha1(property_json.encode('utf-8')).hexdigest()) # Get the streams of buildrequires and hash it. mmd_formatted_buildrequires = { dep: info['stream'] for dep, info in mbs_xmd["buildrequires"].items()} - property_json = dumps(OrderedDict(sorted(mmd_formatted_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) @@ -469,7 +469,7 @@ class ModuleBuild(MBSBase): # 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 = dumps(OrderedDict(sorted(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) @@ -488,7 +488,8 @@ class ModuleBuild(MBSBase): @classmethod def create(cls, session, conf, name, stream, version, modulemd, scmurl, username, - context=None, rebuild_strategy=None, scratch=False, srpms=[], publish_msg=True): + context=None, rebuild_strategy=None, scratch=False, srpms=None, + publish_msg=True, **kwargs): now = datetime.utcnow() module = cls( name=name, @@ -504,7 +505,8 @@ class ModuleBuild(MBSBase): # If the rebuild_strategy isn't specified, use the default rebuild_strategy=rebuild_strategy or conf.rebuild_strategy, scratch=scratch, - srpms=dumps(srpms) if srpms else '[]', + srpms=json.dumps(srpms or []), + **kwargs ) # Add a state transition to "init" mbt = ModuleBuildTrace(state_time=now, state=module.state) @@ -641,15 +643,15 @@ class ModuleBuild(MBSBase): 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': loads(self.srpms) if self.srpms else [], + 'srpms': json.loads(self.srpms or '[]'), 'siblings': self.siblings, 'state_reason': self.state_reason, 'time_completed': _utc_datetime_to_iso(self.time_completed), @@ -658,8 +660,8 @@ class ModuleBuild(MBSBase): '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): """ @@ -669,12 +671,12 @@ class ModuleBuild(MBSBase): 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, @@ -691,7 +693,7 @@ class ModuleBuild(MBSBase): 'state_url': state_url, }) - return json + return rv def tasks(self): """ diff --git a/module_build_service/scheduler/__init__.py b/module_build_service/scheduler/__init__.py index bfbf7592..e19f46c4 100644 --- a/module_build_service/scheduler/__init__.py +++ b/module_build_service/scheduler/__init__.py @@ -62,7 +62,6 @@ def make_simple_stop_condition(session): 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 diff --git a/module_build_service/scheduler/handlers/modules.py b/module_build_service/scheduler/handlers/modules.py index 68e5cd12..11b31e50 100644 --- a/module_build_service/scheduler/handlers/modules.py +++ b/module_build_service/scheduler/handlers/modules.py @@ -116,6 +116,7 @@ def done(config, session, msg): """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 @@ def done(config, session, msg): # 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) @@ -184,7 +187,7 @@ def generate_module_build_koji_tag(build): 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, - scratch=build.scratch) + scratch=build.scratch, scratch_id=build.id) else: return '-'.join(['module', build.name, build.stream, build.version]) diff --git a/module_build_service/utils/general.py b/module_build_service/utils/general.py index 24dac085..a048272e 100644 --- a/module_build_service/utils/general.py +++ b/module_build_service/utils/general.py @@ -105,7 +105,7 @@ def module_build_state_from_msg(msg): return state -def generate_koji_tag(name, stream, version, context, max_length=256, scratch=False): +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 @@ -120,18 +120,24 @@ def generate_koji_tag(name, stream, version, context, max_length=256, scratch=Fa 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 """ - prefix = ('scrmod' if scratch else 'module') - # TODO scrmod: is a unique _suffix_ needed here, too? + 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 = '-'.join([prefix] + nsvc_list) + nsvc_tag = prefix + '-'.join(nsvc_list) + suffix if len(nsvc_tag) + len('-build') > max_length: # Fallback to the old format of 'module-' if the generated koji tag # name is longer than max_length nsvc_hash = hashlib.sha1('.'.join(nsvc_list).encode('utf-8')).hexdigest()[:16] - return prefix + '-' + nsvc_hash + return prefix + nsvc_hash + suffix return nsvc_tag @@ -211,9 +217,6 @@ def validate_koji_tag(tag_arg_names, pre='', post='-', dict_key='name'): return validation_decorator -# TODO scrmod: scratch module build components need a unique dist tag prefix/suffix -# to distinguish them and make it possible to build the same NSVC multiple times, -# but what should it be? Is this the place to set that? def get_rpm_release(module_build): """ Generates the dist tag for the specified module @@ -248,8 +251,11 @@ def get_rpm_release(module_build): 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) + 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, diff --git a/module_build_service/utils/reuse.py b/module_build_service/utils/reuse.py index da63d406..6fa05ba5 100644 --- a/module_build_service/utils/reuse.py +++ b/module_build_service/utils/reuse.py @@ -93,7 +93,7 @@ def get_reusable_module(session, module): 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()) diff --git a/module_build_service/utils/submit.py b/module_build_service/utils/submit.py index 904a9e64..5eb4308d 100644 --- a/module_build_service/utils/submit.py +++ b/module_build_service/utils/submit.py @@ -163,10 +163,6 @@ def format_mmd(mmd, scmurl, module=None, session=None): if 'rpms' not in xmd['mbs']: xmd['mbs']['rpms'] = {} # Add missing data in RPM components - # TODO scrmod: Does something need to be done here for RPMs that are - # overridden by custom SRPMs in addition to what is currently being - # done in record_component_builds()? Should repository and cache - # properties be set? If so, to what? for pkgname, pkg in mmd.get_rpm_components().items(): # In case of resubmit of existing module which have been # cancelled/failed during the init state, the package @@ -317,6 +313,7 @@ def merge_included_mmd(mmd, included_mmd): # Set the modified xmd back to the modulemd mmd.set_xmd(glib.dict_values(xmd)) + def get_module_srpm_overrides(module): """ Make necessary preparations to use any provided custom SRPMs. @@ -366,6 +363,7 @@ def get_module_srpm_overrides(module): 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. diff --git a/module_build_service/views.py b/module_build_service/views.py index 59031c01..22ff45e7 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -395,9 +395,6 @@ class BaseHandler(object): class SCMHandler(BaseHandler): - def __init__(self, request): - super(SCMHandler, self).__init__(request) - def validate(self, skip_branch=False, skip_optional_params=False): if "scmurl" not in self.data: log.error('Missing scmurl') diff --git a/tests/__init__.py b/tests/__init__.py index ffaeb297..8352a2e9 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -110,7 +110,7 @@ def clean_database(add_platform_module=True): 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 @@ def init_data(data_size=10, contexts=False, multiple_stream_versions=False): 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 @@ def _populate_data(session, data_size=10, contexts=False): 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 @@ def _populate_data(session, data_size=10, contexts=False): 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 @@ def _populate_data(session, data_size=10, contexts=False): 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 @@ def _populate_data(session, data_size=10, contexts=False): 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 @@ def _populate_data(session, data_size=10, contexts=False): 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 @@ def scheduler_init_data(tangerine_state=None): 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 diff --git a/tests/test_utils/test_utils.py b/tests/test_utils/test_utils.py index 213c9365..e428020d 100644 --- a/tests/test_utils/test_utils.py +++ b/tests/test_utils/test_utils.py @@ -336,6 +336,21 @@ class TestUtils: 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'), diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index 250c1c86..99b1832b 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -1795,10 +1795,10 @@ class TestViews: assert data['component_builds'] == [] assert data['name'] == 'testmodule' assert data['scmurl'] is None -# TODO scrmod: assert data['modulemd'] == ??? + # generated modulemd is nondeterministic, so just make sure it is set assert data['modulemd'] is not None assert data['scratch'] is True -# TODO scrmod: assert data['version'] == ??? + # 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