From ded8248e52b0acbd5ac932b01d0bc6ce3c40e671 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Jun 27 2018 00:51:34 +0000 Subject: [PATCH 1/4] Get and set architectures. This fixes a bug we ran into today as a part of multi-arch migrations. First, describing the bug: When building an image, OSBS looks at the architectures set in koji on the 'build' tag associated with the koji target of the requested build. This is fine, normally. When a set of builds is ready to "go multi-arch", we just start adding architectures to their build tag and suddenly, new builds produce multiarch images. When freshmaker comes along, it tries to rebuild based on the last shipped image and reuses the parent image and the koji target of the last shipped image. Now, the last shipped image's parent is single-arch (from a few months ago), but the koji target now has a koji tag that specifies multiple architectures. OSBS fails our build because it cannot build a multiarch build on a single arch parent. The fix here is to look up the architectures produced by the last-shipped build and simply copy those into our request for the new build (just like we do with the scm url and koji target). The architectures of the last-shipped build are pulled from the manifest list api in the registry associated with koji. There is a corresponding change in OSBS that allows Freshmaker to supply the list of architectures which should be used for the new build here: https://github.com/release-engineering/koji-containerbuild/pull/99 We're going from 1 to N architectures now, but in the future we'll make moves again to go from N to N+1 architectures which warrants the complexity introduced in this change. --- diff --git a/freshmaker/config.py b/freshmaker/config.py index 2ab7e6c..fdcee6f 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -205,6 +205,10 @@ class Config(object): 'type': bool, 'default': False, 'desc': 'Whether to make a scratch build to rebuild the image.'}, + 'manifest_v2_arch_map': { + 'type': dict, + 'default': {'amd64': 'x86_64'}, + 'desc': 'A map of manifest api v2 architectures to brew architectures.'}, 'dry_run': { 'type': bool, 'default': False, diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py index 028660c..5c2f5d7 100644 --- a/freshmaker/handlers/__init__.py +++ b/freshmaker/handlers/__init__.py @@ -391,7 +391,8 @@ class ContainerBuildHandler(BaseHandler): def build_container(self, scm_url, branch, target, repo_urls=None, isolated=False, - release=None, koji_parent_build=None): + release=None, koji_parent_build=None, + arch_override=None): """ Build a container in Koji. @@ -407,8 +408,8 @@ class ContainerBuildHandler(BaseHandler): profile=conf.koji_profile, logger=log, dry_run=self.dry_run) as service: log.info('Building container from source: %s, ' - 'release=%r, parent=%r, target=%r', - scm_url, release, koji_parent_build, target) + 'release=%r, parent=%r, target=%r, arch=%r', + scm_url, release, koji_parent_build, target, arch_override) return service.build_container(scm_url, branch, @@ -417,6 +418,7 @@ class ContainerBuildHandler(BaseHandler): isolated=isolated, release=release, koji_parent_build=koji_parent_build, + arch_override=arch_override, scratch=conf.koji_container_scratch_build) @fail_artifact_build_on_handler_exception @@ -447,6 +449,10 @@ class ContainerBuildHandler(BaseHandler): target = args["target"] parent = args["parent"] + # If set to None, then OSBS defaults to using the arches + # of the build tag associated with the target. + arches = args.get("arches") + if not build.rebuilt_nvr and build.original_nvr: build.rebuilt_nvr = get_rebuilt_nvr( build.type, build.original_nvr) @@ -461,7 +467,8 @@ class ContainerBuildHandler(BaseHandler): return self.build_container( scm_url, branch, target, repo_urls=repo_urls, - isolated=True, release=release, koji_parent_build=parent) + isolated=True, release=release, koji_parent_build=parent, + arch_override=arches) def odcs_get_compose(self, compose_id): """ diff --git a/freshmaker/kojiservice.py b/freshmaker/kojiservice.py index 8d11ae0..2d17d01 100644 --- a/freshmaker/kojiservice.py +++ b/freshmaker/kojiservice.py @@ -139,7 +139,8 @@ class KojiService(object): def build_container(self, source_url, branch, target, scratch=None, repo_urls=None, isolated=False, - release=None, koji_parent_build=None): + release=None, koji_parent_build=None, + arch_override=None): """Build container by buildContainer""" build_target = target @@ -154,6 +155,8 @@ class KojiService(object): build_opts['isolated'] = True if koji_parent_build: build_opts['koji_parent_build'] = koji_parent_build + if arch_override: + build_opts['arch_override'] = arch_override if release: build_opts['release'] = release diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index cf9af57..b7ba117 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -21,6 +21,7 @@ # # Written by Chenxiong Qi # Jan Kaluza +# Ralph Bean import yaml import json @@ -171,8 +172,14 @@ class ContainerImage(dict): return dockerfile[0] def _get_default_additional_data(self): - return {"repository": None, "commit": None, "target": None, - "git_branch": None, "error": None} + return { + "repository": None, + "commit": None, + "target": None, + "git_branch": None, + "error": None, + "arches": None, + } @region.cache_on_arguments() def _get_additional_data_from_koji(self, nvr): @@ -240,8 +247,70 @@ class ContainerImage(dict): raise KojiLookupError( "Cannot find valid source of Koji build %r" % build) + data['arches'] = self._get_architectures_from_registry(build) + return data + def _get_architectures_from_registry(self, build): + """ Determine the architectures of the build by reading the manifest """ + + # If the image doesn't have the digest metadata we need, then we can + # assume it is an old single-arch build. But, if it does have a digest then carefully + # query the registry for it to extract the list of arches produced last time. + if 'extra' not in build: + return 'x86_64' + if 'image' not in build['extra']: + return 'x86_64' + if 'index' not in build['extra']['index']: + return 'x86_64' + + index = build['extra']['image']['index'] + manifest_list = 'application/vnd.docker.distribution.manifest.list.v2+json' + digest = index.get('digests', {}).get(manifest_list) + + if not digest: + return 'x86_64' + + # If it has a digest, then it must have a pull url. + registry_urls = [url for url in index['pull'] if digest in url] + if not registry_urls: + raise KojiLookupError( + "Could not find pull url for Koji build %r %r" % ( + build, digest)) + + url = registry_urls[0].split(digest)[0].strip('@') + response = requests.get(url, headers=dict(Accept=manifest_list)) + if not response.ok: + raise KojiLookupError( + "Could not pull manifest list from %s for %r: %r" % ( + url, build, response)) + + try: + data = response.json() + except ValueError as e: + raise KojiLookupError( + "Manifest list response for %r was not json: %r %s" % ( + build, e, url)) + + if 'manifests' not in data: + raise KojiLookupError( + "Manifest list response for %r was malformed: %s" % ( + build, url)) + + # Extract the list of arches, as written + manifests = data['manifests'] + arches = [ + manifest['platform']['architecture'] + for manifest in manifests + if 'platform' in manifest and 'architecture' in manifest['platform'] + ] + # But! Convert some arch values into ones familiar to Brew. + # Notably, turn amd64 into x86_64. + arches = [conf.manifest_v2_arch_map.get(arch, arch) for arch in arches] + + # Finally, return the list, joined. + return ','.join(arches) + @region.cache_on_arguments() def _get_additional_data_from_distgit(self, repository, branch, commit): """ diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index fcea0da..c5aaf69 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -964,6 +964,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): "target": "target2", "git_branch": "mybranch", "error": None, + "arches": "x86_64", "brew": { "completion_date": u"20170421T04:27:51.000-0400", "build": "package-name-2-4-12.10", From e1337c9b6e817ad1239b8e2bb5eda363543cc8dc Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Jun 27 2018 12:14:41 +0000 Subject: [PATCH 2/4] Add arm64. --- diff --git a/freshmaker/config.py b/freshmaker/config.py index fdcee6f..8c07e77 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -207,7 +207,11 @@ class Config(object): 'desc': 'Whether to make a scratch build to rebuild the image.'}, 'manifest_v2_arch_map': { 'type': dict, - 'default': {'amd64': 'x86_64'}, + 'default': { + # Someday, somebody please tell me why these names are different. + 'amd64': 'x86_64', + 'arm64': 'aarch64', + }, 'desc': 'A map of manifest api v2 architectures to brew architectures.'}, 'dry_run': { 'type': bool, From 3e9a70df01c6466db2223e863294a97ecd0fef5d Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Jun 27 2018 12:23:18 +0000 Subject: [PATCH 3/4] Feature flag: disable arch overrides by default. --- diff --git a/freshmaker/config.py b/freshmaker/config.py index 8c07e77..e47e47d 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -205,6 +205,11 @@ class Config(object): 'type': bool, 'default': False, 'desc': 'Whether to make a scratch build to rebuild the image.'}, + 'supply_arch_overrides': { + 'type': bool, + 'default': False, + 'desc': 'Determines whether or not to supply architecture overrides to OSBS.', + }, 'manifest_v2_arch_map': { 'type': dict, 'default': { diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index b7ba117..1b7ae9e 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -254,6 +254,12 @@ class ContainerImage(dict): def _get_architectures_from_registry(self, build): """ Determine the architectures of the build by reading the manifest """ + # First thing, check our feature flag. This feature won't work if the OSBS instance we're + # working with doesn't support supply arch_overrides, so, if our configuration says "don't + # supply arch overrides to OSBS" then return None. + if not conf.supply_arch_overrides: + return None + # If the image doesn't have the digest metadata we need, then we can # assume it is an old single-arch build. But, if it does have a digest then carefully # query the registry for it to extract the list of arches produced last time. diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index c5aaf69..6c426be 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -964,7 +964,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): "target": "target2", "git_branch": "mybranch", "error": None, - "arches": "x86_64", + "arches": None, "brew": { "completion_date": u"20170421T04:27:51.000-0400", "build": "package-name-2-4-12.10", From bbfefc61645fe053ff5aac60f47e9fedb83b6ff2 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Jun 27 2018 13:04:49 +0000 Subject: [PATCH 4/4] Add tests for architecture discovery. --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 1b7ae9e..732119b 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -247,11 +247,11 @@ class ContainerImage(dict): raise KojiLookupError( "Cannot find valid source of Koji build %r" % build) - data['arches'] = self._get_architectures_from_registry(build) + data['arches'] = self._get_architectures_from_registry(nvr, build) return data - def _get_architectures_from_registry(self, build): + def _get_architectures_from_registry(self, nvr, build): """ Determine the architectures of the build by reading the manifest """ # First thing, check our feature flag. This feature won't work if the OSBS instance we're @@ -267,7 +267,7 @@ class ContainerImage(dict): return 'x86_64' if 'image' not in build['extra']: return 'x86_64' - if 'index' not in build['extra']['index']: + if 'index' not in build['extra']['image']: return 'x86_64' index = build['extra']['image']['index'] @@ -281,27 +281,23 @@ class ContainerImage(dict): registry_urls = [url for url in index['pull'] if digest in url] if not registry_urls: raise KojiLookupError( - "Could not find pull url for Koji build %r %r" % ( - build, digest)) + "Could not find pull url for Koji build %r %r" % (nvr, digest)) url = registry_urls[0].split(digest)[0].strip('@') response = requests.get(url, headers=dict(Accept=manifest_list)) if not response.ok: raise KojiLookupError( - "Could not pull manifest list from %s for %r: %r" % ( - url, build, response)) + "Could not pull manifest list from %s for %r: %r" % (url, nvr, response)) try: data = response.json() except ValueError as e: raise KojiLookupError( - "Manifest list response for %r was not json: %r %s" % ( - build, e, url)) + "Manifest list response for %r was not json: %r %s" % (nvr, e, url)) if 'manifests' not in data: raise KojiLookupError( - "Manifest list response for %r was malformed: %s" % ( - build, url)) + "Manifest list response for %r was malformed: %s" % (nvr, url, data)) # Extract the list of arches, as written manifests = data['manifests'] diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 6c426be..1d8232e 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -26,8 +26,11 @@ import six from mock import call, patch, Mock, mock_open from six.moves import http_client +import freshmaker + from freshmaker.lightblue import ContainerImage from freshmaker.lightblue import ContainerRepository +from freshmaker.lightblue import KojiLookupError from freshmaker.lightblue import LightBlue from freshmaker.lightblue import LightBlueRequestError from freshmaker.lightblue import LightBlueSystemError @@ -1634,3 +1637,80 @@ class TestDeduplicateImagesToRebuild(helpers.FreshmakerTestCase): # We expect each image to be rebuilt just once. expected = [[deps[3]], [deps[2]], [deps[1]], [deps[0]]] self.assertEqual(batches, expected) + + +class TestArchitecturesFromRegistry(helpers.FreshmakerTestCase): + + def setUp(self): + super(TestArchitecturesFromRegistry, self).setUp() + self.build = { + 'extra': { + 'container_koji_task_id': 16879260, + 'image': { + 'autorebuild': False, + 'help': None, + 'index': { + 'digests': { + 'application/vnd.docker.distribution.manifest.list.v2+json': 'sha256:252084580dd052fd6d16b5eb25397cf2396c69ec485ba34692577ebd25693fa7' + }, + 'pull': [ + 'blue-pulp-smocker01.sledmat.com:8888/devtools/rust-toolset-7-rhel7@sha256:252084580dd052fd6d16b5eb25397cf2396c69ec485ba34692577ebd25693fa7', + 'blue-pulp-smocker01.sledmat.com:8888/devtools/rust-toolset-7-rhel7:1.26.2-4', + ], + 'tags': ['latest', '1.26.2', '1.26.2-4'] + }, + 'isolated': False, + 'media_types': [ + 'application/json', + 'application/vnd.docker.distribution.manifest.list.v2+json', + 'application/vnd.docker.distribution.manifest.v1+json', + 'application/vnd.docker.distribution.manifest.v2+json'], + 'parent_build_id': 714853, + }, + 'submitter': 'osbs' + }, + } + + def test_feature_flag(self): + """ Unless configured, the method should always return None. """ + image = ContainerImage.create({"brew": {"build": "nvr"}}) + result = image._get_architectures_from_registry('foo', 'whatever') + self.assertEqual(result, None) + + @patch.object(freshmaker.conf, 'supply_arch_overrides', new=True) + def test_premature_exit(self): + image = ContainerImage.create({"brew": {"build": "nvr"}}) + result = image._get_architectures_from_registry('foo', 'whatever') + self.assertEqual(result, 'x86_64') + + @patch.object(freshmaker.conf, 'supply_arch_overrides', new=True) + @patch('freshmaker.lightblue.requests') + def test_happy_path(self, requests): + image = ContainerImage.create({"brew": {"build": "nvr"}}) + requests.get.return_value.json.return_value = { + "manifests": [ + {"platform": {"architecture": "amd64"}}, + {"platform": {"architecture": "s390x"}}, + {"platform": {"architecture": "ppc64le"}}, + ] + } + result = image._get_architectures_from_registry("foo", self.build) + self.assertEqual(result, 'x86_64,s390x,ppc64le') + requests.get.assert_called_once_with( + 'blue-pulp-smocker01.sledmat.com:8888/devtools/rust-toolset-7-rhel7', + headers={'Accept': 'application/vnd.docker.distribution.manifest.list.v2+json'}, + ) + + @patch.object(freshmaker.conf, 'supply_arch_overrides', new=True) + @patch('freshmaker.lightblue.requests') + def test_invalid_json(self, requests): + image = ContainerImage.create({"brew": {"build": "nvr"}}) + requests.get.return_value.json.side_effect = ValueError + self.assertRaises(KojiLookupError, image._get_architectures_from_registry, "foo", self.build) + + @patch.object(freshmaker.conf, 'supply_arch_overrides', new=True) + @patch('freshmaker.lightblue.requests') + def test_bad_status(self, requests): + image = ContainerImage.create({"brew": {"build": "nvr"}}) + requests.get.return_value.ok = False + self.assertRaises(KojiLookupError, image._get_architectures_from_registry, "foo", self.build)