From b2d3af712fdf5da1b5b2627aa22d07809979b535 Mon Sep 17 00:00:00 2001 From: Andrei Paplauski Date: Apr 21 2020 08:52:48 +0000 Subject: [PATCH 1/3] Remove git-archive calles and use Lightblue instead Now git-archive calls to git-dist will not be used to get info about Container Images like content-sets. This info already must be in Lightblue so we use it instead. RESOLVES: FACTORY-5860 and FACTORY-5861 --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index ed35f81..c5d963b 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -23,7 +23,6 @@ # Jan Kaluza # Ralph Bean -import yaml import json import os import re @@ -37,7 +36,7 @@ from itertools import groupby from freshmaker import log, conf from freshmaker.kojiservice import koji_service -from freshmaker.utils import sorted_by_nvr, get_distgit_files, get_distgit_url +from freshmaker.utils import sorted_by_nvr import koji @@ -204,6 +203,7 @@ class ContainerImage(dict): "arches": None, "odcs_compose_ids": None, "parent_image_builds": None, + "generate_pulp_repos": True, } @region.cache_on_arguments() @@ -295,87 +295,6 @@ class ContainerImage(dict): for archive in archives if archive['btype'] == 'image'] return ' '.join(sorted(arches)) - @region.cache_on_arguments() - def _get_additional_data_from_distgit(self, repository, branch, commit): - """ - Finds out information about this image in distgit and returns a dict - with following keys: - - - "generate_pulp_repos" - True when Freshmaker needs to generate Pulp - repos using ODCS itself (it means it is not done by OSBS). - - "content_sets" - List of x86_64 content_sets as defined in - content_sets.yml. We care only about x86_64, because to build - non-x86_64 image, OSBS will generate the Pulp repos and therefore - we don't need content_sets in Freshmaker. - """ - nvr = self.nvr - data = {"generate_pulp_repos": False, - "content_sets": []} - - if not repository or not branch or not commit: - log.warning("%s: Cannot get additional data from distgit.", nvr) - return data - if "/" in repository: - namespace, name = repository.split("/") - else: - namespace = "rpms" - name = repository - - try: - repo_url = get_distgit_url(namespace, name, ssh=False) - files = get_distgit_files( - repo_url, commit, ["content_sets.yml", "container.yaml"], - logger=log) - except OSError as e: - self.log_error("Error while fetching dist-git repo files: %s" % e) - return data - - content_sets_data = files["content_sets.yml"] - container_data = files["container.yaml"] - - if content_sets_data is None: - log.debug("%s: Should generate Pulp repo, content_sets.yml does " - "not exist.", nvr) - data["generate_pulp_repos"] = True - return data - - try: - content_sets_yaml = yaml.safe_load(content_sets_data) - except Exception as err: - log.exception(err) - data["generate_pulp_repos"] = True - return data - - if not content_sets_yaml: - log.warning("%s: Should generate Pulp repo, content_sets.yml is " - "empty" % nvr) - data["generate_pulp_repos"] = True - return data - - for content_sets in content_sets_yaml.values(): - data["content_sets"] += content_sets - - if container_data is None: - log.debug("%s: Should generate Pulp repo, container.yaml does not " - "exist.", nvr) - data["generate_pulp_repos"] = True - return data - - container_yaml = yaml.safe_load(container_data) - - if (not container_yaml or "compose" not in container_yaml or - "pulp_repos" not in container_yaml["compose"] or - not container_yaml["compose"]["pulp_repos"]): - log.debug("%s: Should generate Pulp repo, pulp_repos not " - "enabled in containery.yaml.", nvr) - data["generate_pulp_repos"] = True - return data - - # This is workaround until OSBS-5919 is fixed. - if "arches" in self and self["arches"] == "x86_64": - data["generate_pulp_repos"] = True - - return data def resolve_commit(self): """ @@ -406,18 +325,6 @@ class ContainerImage(dict): content_sets from in case this container image is unpublished and therefore without "content_sets" set. """ - data = self._get_additional_data_from_distgit( - self["repository"], self["git_branch"], self["commit"]) - self["generate_pulp_repos"] = data["generate_pulp_repos"] - - # Prefer content_sets from content_sets.yml, because it contains - # content_sets for all architectures. - if data["content_sets"]: - self["content_sets"] = data["content_sets"] - self["content_sets_source"] = "distgit" - log.info("Container image %s uses following content sets: %r", - self.nvr, data["content_sets"]) - return # ContainerImage now has content_sets field, so use it if available. if "content_sets" in self and self["content_sets"]: @@ -427,9 +334,8 @@ class ContainerImage(dict): self["content_sets_source"] = "lightblue_container_image" return - # In case content_sets cannot be get from content_sets.yml and also - # are not set directly in this ContainerImage, try to get them from - # children image. + # In case content_sets are not set directly in this ContainerImage, + # try to get them from children image. self["content_sets_source"] = "child_image" if not children: log.warning("Container image %s does not have 'content_sets' set " @@ -1279,8 +1185,20 @@ class LightBlue(object): # image list. sorted_images = sorted_by_nvr(images, reverse=True) images = [] - for k, v in groupby(sorted_images, key=lambda item: item.nvr): - images.append(next(v)) + + # We must combine content_sets with same image NVR + # but different architectures into one content_sets field + for k, temp_images in groupby(sorted_images, key=lambda item: item.nvr): + temp_images = list(temp_images) + img = temp_images[0] + if 'content_sets' in img and len(temp_images) > 1: + new_content_sets = set(img.get('content_sets')) + for i in temp_images[1:]: + new_content_sets.update(i.get('content_sets', [])) + img["content_sets"] = list(new_content_sets) + images.append(img) + + # In case we query for unpublished images, we need to return just # the latest NVR for given name-version, otherwise images would diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index b60acac..d7e7ee5 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -34,7 +34,7 @@ from freshmaker.lightblue import ContainerRepository from freshmaker.lightblue import LightBlue from freshmaker.lightblue import LightBlueRequestError from freshmaker.lightblue import LightBlueSystemError -from freshmaker.utils import sorted_by_nvr, get_distgit_url +from freshmaker.utils import sorted_by_nvr from freshmaker import log from tests import helpers @@ -140,142 +140,6 @@ description JBWEB000121: This request requires HTTP authentication. repr(self.e)) -class TestGetAdditionalDataFromDistGit(helpers.FreshmakerTestCase): - - def setUp(self): - super(TestGetAdditionalDataFromDistGit, self).setUp() - - self.patcher = helpers.Patcher( - "freshmaker.lightblue.") - self.get_distgit_files = self.patcher.patch("get_distgit_files") - self.get_distgit_files.return_value = { - "content_sets.yml": None, - "container.yaml": None, - } - - def tearDown(self): - super(TestGetAdditionalDataFromDistGit, self).tearDown() - self.patcher.unpatch_all() - - def test_generate(self): - self.get_distgit_files.return_value = { - "content_sets.yml": "x86_64:\n - content_set", - "container.yaml": "compose:\n pulp_repos: True", - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], False) - - repo_url = get_distgit_url('rpms', 'foo-docker', ssh=False) - self.get_distgit_files.assert_called_once_with( - repo_url, "commit", - ["content_sets.yml", "container.yaml"], logger=log) - - def test_generate_os_error(self): - self.get_distgit_files.side_effect = OSError( - "Got an error (128) from git: fatal: reference is not a tree: " - "4d42e2009cec70d871c65de821396cd750d523f1") - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], False) - - self.assertEqual( - image["error"], - "Error while fetching dist-git repo files: Got an error (128) from git: " - "fatal: reference is not a tree: 4d42e2009cec70d871c65de821396cd750d523f1") - - repo_url = get_distgit_url('rpms', 'foo-docker', ssh=False) - self.get_distgit_files.assert_called_once_with( - repo_url, 'commit', - ["content_sets.yml", "container.yaml"], logger=log) - - def test_generate_no_namespace(self): - self.get_distgit_files.return_value = { - "content_sets.yml": "x86_64:\n - content_set", - "container.yaml": "compose:\n pulp_repos: True", - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], False) - - repo_url = get_distgit_url('rpms', 'foo-docker', ssh=False) - self.get_distgit_files.assert_called_once_with( - repo_url, "commit", - ["content_sets.yml", "container.yaml"], logger=log) - - def test_generate_no_pulp_repos(self): - self.get_distgit_files.return_value = { - "content_sets.yml": "x86_64:\n - content_set", - "container.yaml": "compose:\n pulp_repos_x: True", - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], True) - - def test_generate_pulp_repos_false(self): - self.get_distgit_files.return_value = { - "content_sets.yml": "x86_64:\n - content_set", - "container.yaml": "compose:\n pulp_repos: False", - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], True) - - def test_generate_no_content_sets_yml(self): - self.get_distgit_files.return_value = { - "content_sets.yml": None, - "container.yaml": "compose:\n pulp_repos: False", - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], True) - - def test_generate_no_container_yaml(self): - self.get_distgit_files.return_value = { - "content_sets.yml": "x86_64:\n - content_set", - "container.yaml": None, - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], True) - - def test_generate_content_sets_yml_empty(self): - self.get_distgit_files.return_value = { - "content_sets.yml": "", - "container.yaml": "compose:\n pulp_repos: False", - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], True) - - def test_generate_container_yaml_empty(self): - self.get_distgit_files.return_value = { - "content_sets.yml": "x86_64:\n - content_set", - "container.yaml": "", - } - - image = ContainerImage.create({"brew": {"build": "nvr"}}) - ret = image._get_additional_data_from_distgit( - "rpms/foo-docker", "branch", "commit") - self.assertEqual(ret["generate_pulp_repos"], True) - - class TestContainerImageObject(helpers.FreshmakerTestCase): def setUp(self): @@ -287,11 +151,6 @@ class TestContainerImageObject(helpers.FreshmakerTestCase): self.patcher = helpers.Patcher( 'freshmaker.lightblue.') - self.get_distgit_files = self.patcher.patch("get_distgit_files") - self.get_distgit_files.return_value = { - "content_sets.yml": None, - "container.yaml": None, - } self.dummy_image = ContainerImage.create({ '_id': '1233829', @@ -420,6 +279,7 @@ class TestContainerImageObject(helpers.FreshmakerTestCase): self.assertEqual(self.dummy_image["commit"], "commit_hash1") self.assertEqual(self.dummy_image["target"], "target1") self.assertEqual(self.dummy_image["odcs_compose_ids"], [7300, 7301]) + self.assertTrue(self.dummy_image["generate_pulp_repos"]) @patch('freshmaker.kojiservice.KojiService.get_build') @patch('freshmaker.kojiservice.KojiService.get_task_request') @@ -665,11 +525,6 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): self.patcher = helpers.Patcher( 'freshmaker.lightblue.') - self.get_distgit_files = self.patcher.patch("get_distgit_files") - self.get_distgit_files.return_value = { - "content_sets.yml": None, - "container.yaml": None, - } self.fake_server_url = 'lightblue.localhost' self.fake_cert_file = 'path/to/cert' @@ -2047,6 +1902,77 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): {'field': 'rpm_manifest.*.rpms.*.srpm_name', 'include': True, 'recursive': True}], 'objectType': 'containerImage'}) + @patch('freshmaker.lightblue.LightBlue.find_container_repositories') + @patch('freshmaker.lightblue.LightBlue.find_container_images') + def test_content_sets_of_multiarch_images_to_rebuild( + self, find_images, find_repos): + new_images = [ + { + 'brew': { + 'completion_date': u'20170421T04:27:51.000-0400', + 'build': 'build1-name-1.1', + 'package': 'package-name-3' + }, + "content_sets": ["content-set-1", + "content-set-2"], + 'parent_brew_build': 'some-original-nvr-7.6-252.1561619826', + 'repositories': [ + {'repository': 'product1/repo1', 'published': True, + 'tags': [{"name": "latest"}]} + ], + 'rpm_manifest': [{ + 'rpms': [ + { + "srpm_name": "openssl", + "srpm_nevra": "openssl-0:1.2.3-1.src" + } + ] + }], + 'architecture': 'amd64' + }, + { + 'brew': { + 'completion_date': u'20170421T04:27:51.000-0400', + 'build': 'build1-name-1.1', + 'package': 'package-name-4' + }, + "content_sets": ["content-set-2", + "content-set-3"], + 'parent_brew_build': 'some-original-nvr-7.6-252.1561619826', + 'repositories': [ + {'repository': 'product1/repo1', 'published': True, + 'tags': [{"name": "latest"}]} + ], + 'rpm_manifest': [{ + 'rpms': [ + { + "srpm_name": "openssl", + "srpm_nevra": "openssl-0:1.2.3-1.src" + } + ] + }], + 'architecture': 's390x' + } + ] + new_images = [ContainerImage.create(i) for i in new_images] + find_repos.return_value = self.fake_repositories_with_content_sets + find_images.return_value = self.fake_container_images + new_images + right_content_sets = [["dummy-content-set-1"], + ["dummy-content-set-1", "dummy-content-set-2"], + ["content-set-1", "content-set-2", "content-set-3"]] + with patch('os.path.exists'): + lb = LightBlue(server_url=self.fake_server_url, + cert=self.fake_cert_file, + private_key=self.fake_private_key) + ret = lb.find_images_with_packages_from_content_set( + set(["openssl-1.2.3-3"]), + ["content-set-1", "content-set-2", "content-set-3"], + leaf_container_images=['placeholder']) + + self.assertEqual(3, len(ret)) + images_content_sets = [sorted(i.get('content_sets', ['!'])) for i in ret] + self.assertEqual(images_content_sets, right_content_sets) + @patch('freshmaker.lightblue.LightBlue.find_container_images') @patch('os.path.exists') def test_images_with_modular_container_image( From f74a6681224dd1f2b768c3e91bf52f93535a2524 Mon Sep 17 00:00:00 2001 From: Andrei Paplauski Date: Apr 21 2020 08:52:48 +0000 Subject: [PATCH 2/3] Delete unused functions to work with git dist Now we don't need to call git archive so it can be removed from code base, as well as tests for it. RESOLVES: FACTORY-5860 and FACTORY-5861 --- diff --git a/freshmaker/utils.py b/freshmaker/utils.py index 53cbced..6df9468 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -163,77 +163,6 @@ def retry(timeout=conf.net_timeout, interval=conf.net_retry_interval, wait_on=Ex return inner return wrapper - -def get_distgit_url(namespace, name, ssh=True, user=None): - """ - Returns the dist-git repository URL. - - :param str namespace: Namespace in which the repository is located, for - example "rpms", "containers", "modules", ... - :param str name: Name of the repository inside the namespace. - :param bool ssh: indicate whether SSH auth will be used when fetching the - files. Default is True. - :param str user: If set, overrides the default user for SSH auth. - Otherwise, username specified in config ``git_user`` will be used. - :return: The dist-git repository URL. - :rtype: str - """ - if ssh: - if user is None: - if hasattr(conf, 'git_user'): - user = conf.git_user - else: - user = getpass.getuser() - repo_url = conf.git_ssh_base_url % user - else: - repo_url = conf.git_base_url - - repo_url = os.path.join(repo_url, namespace, name) - return repo_url - - -@retry(logger=log) -def get_distgit_files(repo_url, commit_or_branch, files, logger=None): - """ - Fetches the `files` from dist-git repository defined by `namespace`, - `name` and `commit_or_branch` and returns them. - - This is much faster than cloning the dist-git repository and should be - preferred method to get the files from dist-git in case the full clone - of repository is not needed. - - :param str repo_url: the repository URL. - :param str commit_or_branch: Commit hash or branch name. - :param list[str] files: List of files to fetch. - :param freshmaker.log logger: Logger instance. - :return: Dictionary with file name as key and file content as value. - If the file does not exist in a dist-git repo, None is used as value. - :rtype: dict[str, str or None] - """ - # Use the "git archive" to get the files in tarball and then extract - # them and return in dict. We need to go file by file, because the - # "git archive" would fail completely in case any file does not exist - # in the git repo. - ret = {} - for f in files: - try: - cmd = ['git', 'archive', '--remote=%s' % repo_url, - commit_or_branch, f] - tar_data = _run_command(cmd, logger=logger, log_output=False) - with io.BytesIO(tar_data.encode()) as tar_bytes: - with tarfile.open(fileobj=tar_bytes) as tar: - for member in tar.getmembers(): - with tar.extractfile(member) as fd: - ret[member.name] = fd.read().decode() - except OSError as e: - if "path not found" in str(e): - ret[os.path.basename(f)] = None - else: - raise - - return ret - - def _run_command(command, logger=None, rundir=None, output=subprocess.PIPE, error=subprocess.PIPE, env=None, log_output=True): """Run a command, return output. Error out if command exit with non-zero code.""" diff --git a/tests/test_utils.py b/tests/test_utils.py index 4c43133..e4dd8b0 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -29,7 +29,7 @@ import pytest from freshmaker import conf from freshmaker.models import ArtifactType -from freshmaker.utils import get_rebuilt_nvr, sorted_by_nvr, get_distgit_files +from freshmaker.utils import get_rebuilt_nvr, sorted_by_nvr from tests import helpers @@ -74,53 +74,3 @@ class TestSortedByNVR(helpers.FreshmakerTestCase): expected = ["bar-1-2", "foo-1-1", "foo-1-10"] ret = sorted_by_nvr(lst, reverse=True) self.assertEqual(ret, list(reversed(expected))) - - -class TestGetDistGitFiles(object): - """Test get_distgit_files""" - - @classmethod - def setup_class(cls): - import os - import subprocess - cls.repo_dir = tempfile.mkdtemp() - with open(os.path.join(cls.repo_dir, 'a.txt'), 'w') as f: - f.write('hello') - with open(os.path.join(cls.repo_dir, 'b.txt'), 'w') as f: - f.write('world') - git_cmds = [ - ['git', 'init'], - ['git', 'add', 'a.txt', 'b.txt'], - ['git', 'config', 'user.name', 'tester'], - ['git', 'config', 'user.email', 'tester@localhost'], - ['git', 'commit', '-m', 'initial commit for test'], - ] - for cmd in git_cmds: - subprocess.check_call(cmd, cwd=cls.repo_dir) - - cls.repo_url = 'file://' + cls.repo_dir - - @classmethod - def teardown_class(cls): - shutil.rmtree(cls.repo_dir) - - @pytest.mark.parametrize('files,expected', [ - [['a.txt'], {'a.txt': 'hello'}], - [['a.txt', 'b.txt'], {'a.txt': 'hello', 'b.txt': 'world'}], - ]) - def test_get_files(self, files, expected): - result = get_distgit_files(self.repo_url, 'master', files) - assert expected == result - - @patch('freshmaker.utils._run_command') - def test_error_path_not_found(self, run_command): - run_command.side_effect = OSError('path not found') - result = get_distgit_files(self.repo_url, 'master', ['a.txt']) - assert {'a.txt': None} == result - - @patch('freshmaker.utils._run_command') - @patch('time.sleep') - def test_unhandled_error_occurs(self, sleep, run_command): - run_command.side_effect = ValueError - with pytest.raises(ValueError): - get_distgit_files(self.repo_url, 'master', ['a.txt']) From ea9d82a968087740429da82855e633a36827d591 Mon Sep 17 00:00:00 2001 From: Andrei Paplauski Date: Apr 21 2020 08:52:48 +0000 Subject: [PATCH 3/3] Code style fixes + remove unsused imports RESOLVES: FACTORY-5860 and FACTORY-5861 --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index c5d963b..3e78ed5 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -295,7 +295,6 @@ class ContainerImage(dict): for archive in archives if archive['btype'] == 'image'] return ' '.join(sorted(arches)) - def resolve_commit(self): """ Uses the ContainerImage data to resolve the information about @@ -1198,8 +1197,6 @@ class LightBlue(object): img["content_sets"] = list(new_content_sets) images.append(img) - - # In case we query for unpublished images, we need to return just # the latest NVR for given name-version, otherwise images would # contain all the versions which ever containing the srpm_name. diff --git a/freshmaker/utils.py b/freshmaker/utils.py index 6df9468..0c524bc 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -21,16 +21,12 @@ # import functools -import getpass -import os import subprocess import sys import tempfile import time import koji import kobo.rpmlib -import tarfile -import io from freshmaker import conf, app, log from freshmaker.types import ArtifactType @@ -163,6 +159,7 @@ def retry(timeout=conf.net_timeout, interval=conf.net_retry_interval, wait_on=Ex return inner return wrapper + def _run_command(command, logger=None, rundir=None, output=subprocess.PIPE, error=subprocess.PIPE, env=None, log_output=True): """Run a command, return output. Error out if command exit with non-zero code.""" diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index d7e7ee5..576070f 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -35,7 +35,6 @@ from freshmaker.lightblue import LightBlue from freshmaker.lightblue import LightBlueRequestError from freshmaker.lightblue import LightBlueSystemError from freshmaker.utils import sorted_by_nvr -from freshmaker import log from tests import helpers diff --git a/tests/test_utils.py b/tests/test_utils.py index e4dd8b0..b052863 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -20,9 +20,6 @@ # # Written by Jan Kaluza -import shutil -import tempfile - from unittest.mock import patch import pytest