From 11512866f9b19f6c5b6fa8f6336c59e7ae3ab692 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 19 2023 10:13:47 +0000 Subject: [PATCH 1/7] Don't spawn createrepo if not needed Related: https://pagure.io/koji/issue/3808 --- diff --git a/builder/kojid b/builder/kojid index f774943..e3db830 100755 --- a/builder/kojid +++ b/builder/kojid @@ -24,6 +24,7 @@ from __future__ import absolute_import, division import copy +import filecmp import glob import grp import io @@ -5557,6 +5558,84 @@ class NewRepoTask(BaseTaskHandler): Methods = ['newRepo'] _taskWeight = 0.1 + def copy_repo(self, src_repo_id, src_repo_path, repo_id, arch): + """Copy repodata, return False if it fails""" + try: + # copy repodata + dst_repodata = f'{self.workdir}/{arch}/repodata' + #dst_repo_path = koji.pathinfo.repo(repo_id, taginfo['name']) + src_repodata = f'{src_repo_path}/{arch}/repodata' + #dst_repodata = f'{dst_repo_path}/{arch}/repodata' + self.logger.debug(f'Copying repodata {src_repodata} to {dst_repodata}') + if os.path.exists(src_repodata): + shutil.copytree(src_repodata, dst_repodata) + with open(f'{dst_repodata}/repo.json', 'wt') as fp: + json.dump({'cloned_from_repo_id': src_repo_id}, fp, indent=2) + uploadpath = self.getUploadDir() + files = [] + for f in os.listdir(dst_repodata): + files.append(f) + self.session.uploadWrapper('%s/%s' % (dst_repodata, f), uploadpath, f) + return [uploadpath, files] + except Exception as ex: + self.logger.warning(f"Copying repo {src_repo_id} to {repo_id} failed. {ex}") + return False + + def check_repo(self, src_repo_path, dst_repo_path, src_repo, dst_repo, opts): + """Check if oldrepo is reusable as is and can be directly copied""" + # with_src, debuginfo, pkglist, blocklist, grouplist + if not os.path.exists(src_repo_path): + self.logger.debug(f"Source repo doesn't exist {src_repo_path}") + return False + try: + repo_json = json.load(open(f'{src_repo_path}/repo.json')) + for key in ('with_debuginfo', 'with_src', 'with_separate_src'): + if repo_json.get(key, False) != opts.get(key, False): + print(key, repo_json.get(key), opts.get(key)) + return False + except IOError: + self.logger.debug("Can't open repo.json for repo {repo_info['id']}") + return False + + # compare comps if they exist + src_comps_path = f'{src_repo_path}/groups/comps.xml' + dst_comps_path = f'{dst_repo_path}/groups/comps.xml' + src_exists = os.path.exists(src_comps_path) + if src_exists != os.path.exists(dst_comps_path): + self.logger.debug("Comps exists only in one repo") + return False + if src_exists and not filecmp.cmp(src_comps_path, dst_comps_path, shallow=False): + self.logger.debug("Comps differs") + return False + + # if there is any external repo, don't trust the repodata + if self.session.getExternalRepoList(src_repo['tag_id'], event=src_repo['create_event']): + self.logger.debug("Source repo use external repos") + return False + if self.session.getExternalRepoList(dst_repo['tag_id'], event=dst_repo['create_event']): + self.logger.debug("Destination repo use external repos") + return False + + self.logger.debug('Repo test passed') + return True + + def check_arch_repo(self, src_repo_path, dst_repo_path, arch): + """More checks based on architecture content""" + for fname in ('blocklist', 'pkglist'): + src_file = f'{src_repo_path}/{arch}/{fname}' + dst_file = f'{dst_repo_path}/{arch}/{fname}' + # both must non/exist + src_exists = os.path.exists(src_file) + if src_exists != os.path.exists(dst_file): + self.logger.debug(f'{fname} exists only in one repo') + return False + # if they exist, content must be same + if src_exists and not filecmp.cmp(src_file, dst_file, shallow=False): + self.logger.debug(f'{fname} differs') + return False + self.logger.debug(f'Arch repo test passed {arch}') + return True + def handler(self, tag, event=None, src=False, debuginfo=False, separate_src=False): tinfo = self.session.getTag(tag, strict=True, event=event) kwargs = {} @@ -5585,6 +5664,9 @@ class NewRepoTask(BaseTaskHandler): else: oldrepo_state = koji.REPO_READY oldrepo = self.session.getRepo(tinfo['id'], state=oldrepo_state) + if oldrepo: + oldrepo_path = koji.pathinfo.repo(oldrepo['id'], tinfo['name']) + oldrepo['tag_id'] = tinfo['id'] # If there is no old repo, try to find first usable repo in # inheritance chain and use it as a source. oldrepo is not used if # createrepo_update is not set, so don't waste call in such case. @@ -5595,23 +5677,38 @@ class NewRepoTask(BaseTaskHandler): for tag in sorted(tags, key=lambda x: x['currdepth']): oldrepo = self.session.getRepo(tag['parent_id'], state=oldrepo_state) if oldrepo: + parenttag = self.session.getTag(tag['parent_id']) + oldrepo_path = koji.pathinfo.repo(oldrepo['id'], parenttag['name']) + oldrepo['tag_id'] = parenttag['id'] break + newrepo_path = koji.pathinfo.repo(repo_id, tinfo['name']) + newrepo = {'tag_id': tinfo['id'], 'create_event': event_id} + if self.options.copy_old_repodata: + possibly_clonable = self.check_repo(oldrepo_path, newrepo_path, oldrepo, newrepo, kwargs) + else: + possibly_clonable = False subtasks = {} + data = {} for arch in arches: + if possibly_clonable and self.check_arch_repo(oldrepo_path, newrepo_path, arch): + result = self.copy_repo(oldrepo['id'], oldrepo_path, repo_id, arch) + if result: + data[arch] = result + continue + # if we can't copy old repo directly, trigger normal createrepo arglist = [repo_id, arch, oldrepo] subtasks[arch] = self.session.host.subtask(method='createrepo', arglist=arglist, label=arch, parent=self.id, arch='noarch') - # gather subtask results - data = {} if subtasks: results = self.wait(to_list(subtasks.values()), all=True, failany=True) for (arch, task_id) in six.iteritems(subtasks): data[arch] = results[task_id] - self.logger.debug("DEBUG: %r : %r " % (arch, data[arch],)) + + self.logger.debug("DEBUG: %r : %r " % (arch, data[arch],)) # finalize kwargs = {} @@ -6477,6 +6574,7 @@ def get_options(): 'createrepo_skip_stat': True, 'createrepo_update': True, 'distrepo_skip_stat': False, + 'copy_old_repodata': False, 'mock_bootstrap_image': False, 'pkgurl': None, 'allowed_scms': '', @@ -6513,7 +6611,7 @@ def get_options(): 'build_arch_can_fail', 'no_ssl_verify', 'log_timestamps', 'allow_noverifyssl', 'allowed_scms_use_config', 'allowed_scms_use_policy', 'allow_password_in_scm_url', - 'distrepo_skip_stat']: + 'distrepo_skip_stat', 'copy_old_repodata']: defaults[name] = config.getboolean('kojid', name) elif name in ['plugin', 'plugins']: defaults['plugin'] = value.split() diff --git a/builder/kojid.conf b/builder/kojid.conf index a905e3f..ae46248 100644 --- a/builder/kojid.conf +++ b/builder/kojid.conf @@ -73,6 +73,9 @@ topurl=http://hub.example.com/kojifiles ; be always run in same way. Not recommended ; distrepo_skip_stat=False +; copy old repodata if there is no apparent change +; copy_old_repodata = False + ; A space-separated list of tuples from which kojid is allowed to checkout. ; The format of those tuples is: ; diff --git a/docs/source/kojid_conf.rst b/docs/source/kojid_conf.rst index 01160bd..dbc6747 100644 --- a/docs/source/kojid_conf.rst +++ b/docs/source/kojid_conf.rst @@ -135,6 +135,13 @@ Building createrepo_update=True Recycle old repodata (if they exist) in createrepo. + copy_old_repodata=False + ``newRepo`` task can copy old repodata if they exist and there is no + apparent change in the content. It should be generally safe to turn on + and it would lower number of ``createrepo`` tasks in normal environment. + Note, that some cases (especially tags with external repos) will render + this as no-op as we can't be sure that content hasn't changed meanwhile. + failed_buildroot_lifetime=14400 Failed tasks leave buildroot content on disk for debugging purposes. They are removed after 4 hours by default. This value is specified From 45eb08866e03cbc24f17a4f8bec97e4555f8f8c3 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 19 2023 10:13:47 +0000 Subject: [PATCH 2/7] additional fixes --- diff --git a/builder/kojid b/builder/kojid index e3db830..be33c42 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5558,19 +5558,19 @@ class NewRepoTask(BaseTaskHandler): Methods = ['newRepo'] _taskWeight = 0.1 - def copy_repo(self, src_repo_id, src_repo_path, repo_id, arch): + def copy_arch_repo(self, src_repo_id, src_repo_path, repo_id, arch): """Copy repodata, return False if it fails""" + dst_repodata = f'{self.workdir}/{arch}/repodata' + src_repodata = f'{src_repo_path}/{arch}/repodata' try: # copy repodata - dst_repodata = f'{self.workdir}/{arch}/repodata' - #dst_repo_path = koji.pathinfo.repo(repo_id, taginfo['name']) - src_repodata = f'{src_repo_path}/{arch}/repodata' - #dst_repodata = f'{dst_repo_path}/{arch}/repodata' self.logger.debug(f'Copying repodata {src_repodata} to {dst_repodata}') if os.path.exists(src_repodata): + # symlink=True is not needed as they are no part of arch repodir shutil.copytree(src_repodata, dst_repodata) - with open(f'{dst_repodata}/repo.json', 'wt') as fp: - json.dump({'cloned_from_repo_id': src_repo_id}, fp, indent=2) + repo_json = koji.load_json(f'{src_repodata}/repo.json') + repo_json['cloned_from_repo_id'] = src_repo_id + koji.dump_json(f'{dst_repodata}/repo.json', repo_json, indent=2) uploadpath = self.getUploadDir() files = [] for f in os.listdir(dst_repodata): @@ -5579,16 +5579,20 @@ class NewRepoTask(BaseTaskHandler): return [uploadpath, files] except Exception as ex: self.logger.warning(f"Copying repo {src_repo_id} to {repo_id} failed. {ex}") + # Try to remove potential leftovers and fail if there is some problem + koji.util.rmtree(dst_repodata, self.logger) return False def check_repo(self, src_repo_path, dst_repo_path, src_repo, dst_repo, opts): """Check if oldrepo is reusable as is and can be directly copied""" # with_src, debuginfo, pkglist, blocklist, grouplist - if not os.path.exists(src_repo_path): + # We're ignoring maven support here. It is handled in repo_init which is called + # always, so it doesn't affect efficiency of pre-cloning rpm repos. + if not os.path.isdir(src_repo_path): self.logger.debug(f"Source repo doesn't exist {src_repo_path}") return False try: - repo_json = json.load(open(f'{src_repo_path}/repo.json')) + repo_json = koji.load_json(f'{src_repo_path}/repo.json') for key in ('with_debuginfo', 'with_src', 'with_separate_src'): if repo_json.get(key, False) != opts.get(key, False): print(key, repo_json.get(key), opts.get(key)) @@ -5625,12 +5629,11 @@ class NewRepoTask(BaseTaskHandler): src_file = f'{src_repo_path}/{arch}/{fname}' dst_file = f'{dst_repo_path}/{arch}/{fname}' # both must non/exist - src_exists = os.path.exists(src_file) - if src_exists != os.path.exists(dst_file): - self.logger.debug(f'{fname} exists only in one repo') + if not os.path.exists(src_file) or not os.path.exists(dst_file): + self.logger.debug(f"{fname} doesn't exit in one of the repos") return False - # if they exist, content must be same - if src_exists and not filecmp.cmp(src_file, dst_file, shallow=False): + # content must be same + if not filecmp.cmp(src_file, dst_file, shallow=False): self.logger.debug(f'{fname} differs') return False self.logger.debug(f'Arch repo test passed {arch}') @@ -5691,7 +5694,7 @@ class NewRepoTask(BaseTaskHandler): data = {} for arch in arches: if possibly_clonable and self.check_arch_repo(oldrepo_path, newrepo_path, arch): - result = self.copy_repo(oldrepo['id'], oldrepo_path, repo_id, arch) + result = self.copy_arch_repo(oldrepo['id'], oldrepo_path, repo_id, arch) if result: data[arch] = result continue @@ -5708,8 +5711,6 @@ class NewRepoTask(BaseTaskHandler): for (arch, task_id) in six.iteritems(subtasks): data[arch] = results[task_id] - self.logger.debug("DEBUG: %r : %r " % (arch, data[arch],)) - # finalize kwargs = {} if event is not None: From 57a5fa426210e7208c92a1cf518cec970415e54d Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 19 2023 10:13:47 +0000 Subject: [PATCH 3/7] use correct repo.json --- diff --git a/builder/kojid b/builder/kojid index be33c42..7334606 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5568,7 +5568,7 @@ class NewRepoTask(BaseTaskHandler): if os.path.exists(src_repodata): # symlink=True is not needed as they are no part of arch repodir shutil.copytree(src_repodata, dst_repodata) - repo_json = koji.load_json(f'{src_repodata}/repo.json') + repo_json = koji.load_json(f'{src_repo_path}/repo.json') repo_json['cloned_from_repo_id'] = src_repo_id koji.dump_json(f'{dst_repodata}/repo.json', repo_json, indent=2) uploadpath = self.getUploadDir() @@ -5588,6 +5588,9 @@ class NewRepoTask(BaseTaskHandler): # with_src, debuginfo, pkglist, blocklist, grouplist # We're ignoring maven support here. It is handled in repo_init which is called # always, so it doesn't affect efficiency of pre-cloning rpm repos. + if not src_repo_path: + self.logger.debug("Source repo wasn't found") + return False if not os.path.isdir(src_repo_path): self.logger.debug(f"Source repo doesn't exist {src_repo_path}") return False @@ -5667,6 +5670,7 @@ class NewRepoTask(BaseTaskHandler): else: oldrepo_state = koji.REPO_READY oldrepo = self.session.getRepo(tinfo['id'], state=oldrepo_state) + oldrepo_path = None if oldrepo: oldrepo_path = koji.pathinfo.repo(oldrepo['id'], tinfo['name']) oldrepo['tag_id'] = tinfo['id'] @@ -5687,7 +5691,8 @@ class NewRepoTask(BaseTaskHandler): newrepo_path = koji.pathinfo.repo(repo_id, tinfo['name']) newrepo = {'tag_id': tinfo['id'], 'create_event': event_id} if self.options.copy_old_repodata: - possibly_clonable = self.check_repo(oldrepo_path, newrepo_path, oldrepo, newrepo, kwargs) + possibly_clonable = self.check_repo(oldrepo_path, newrepo_path, + oldrepo, newrepo, kwargs) else: possibly_clonable = False subtasks = {} From fb4701f6a9c66ecf746ad83402e050b03fdcb742 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 19 2023 10:13:47 +0000 Subject: [PATCH 4/7] fix path --- diff --git a/builder/kojid b/builder/kojid index 7334606..b2a1d11 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5570,7 +5570,7 @@ class NewRepoTask(BaseTaskHandler): shutil.copytree(src_repodata, dst_repodata) repo_json = koji.load_json(f'{src_repo_path}/repo.json') repo_json['cloned_from_repo_id'] = src_repo_id - koji.dump_json(f'{dst_repodata}/repo.json', repo_json, indent=2) + koji.dump_json(f'{self.workdir}/{arch}/repo.json', repo_json, indent=2) uploadpath = self.getUploadDir() files = [] for f in os.listdir(dst_repodata): From 7f0e7d21264ce73a1b21449e01e99e0c7fd3773d Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 19 2023 10:13:47 +0000 Subject: [PATCH 5/7] update top-level repo.json --- diff --git a/builder/kojid b/builder/kojid index b2a1d11..05ca267 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5568,9 +5568,6 @@ class NewRepoTask(BaseTaskHandler): if os.path.exists(src_repodata): # symlink=True is not needed as they are no part of arch repodir shutil.copytree(src_repodata, dst_repodata) - repo_json = koji.load_json(f'{src_repo_path}/repo.json') - repo_json['cloned_from_repo_id'] = src_repo_id - koji.dump_json(f'{self.workdir}/{arch}/repo.json', repo_json, indent=2) uploadpath = self.getUploadDir() files = [] for f in os.listdir(dst_repodata): @@ -5697,11 +5694,13 @@ class NewRepoTask(BaseTaskHandler): possibly_clonable = False subtasks = {} data = {} + cloned_archs = [] for arch in arches: if possibly_clonable and self.check_arch_repo(oldrepo_path, newrepo_path, arch): result = self.copy_arch_repo(oldrepo['id'], oldrepo_path, repo_id, arch) if result: data[arch] = result + cloned_archs.append(arch) continue # if we can't copy old repo directly, trigger normal createrepo arglist = [repo_id, arch, oldrepo] @@ -5720,6 +5719,11 @@ class NewRepoTask(BaseTaskHandler): kwargs = {} if event is not None: kwargs['expire'] = True + if cloned_archs: + kwargs['repo_json_updates'] = { + 'cloned_from_repo_id': oldrepo['id'], + 'cloned_archs': cloned_archs, + } self.session.host.repoDone(repo_id, data, **kwargs) return repo_id, event_id diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 20e72fb..cc266f3 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -15272,13 +15272,15 @@ class HostExports(object): return repo_init(tag, task_id=task_id, with_src=with_src, with_debuginfo=with_debuginfo, event=event, with_separate_src=with_separate_src) - def repoDone(self, repo_id, data, expire=False): + def repoDone(self, repo_id, data, expire=False, repo_json_updates=None): """Finalize a repo repo_id: the id of the repo data: a dictionary of repo files in the form: { arch: [uploadpath, [file1, file2, ...]], ...} expire: if set to true, mark the repo expired immediately [*] + repo_json_updates: dict - if provided it will be shallow copied + into repo.json file Actions: * Move uploaded repo files into place @@ -15300,6 +15302,10 @@ class HostExports(object): raise koji.GenericError("Repo %(id)s not in INIT state (got %(state)s)" % rinfo) repodir = koji.pathinfo.repo(repo_id, rinfo['tag_name']) workdir = koji.pathinfo.work() + if repo_json_updates: + repo_json = koji.load_json(f'{repodir}/repo.json') + repo_json.update(repo_json_updates) + koji.dump_json(f'{repodir}/repo.json', repo_json, indent=2) if not rinfo['dist']: for arch, (uploadpath, files) in data.items(): archdir = "%s/%s" % (repodir, koji.canonArch(arch)) From 67aa192e2dfbc576e9ab762bdd588d3c01db39eb Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 19 2023 10:13:48 +0000 Subject: [PATCH 6/7] remove debug print --- diff --git a/builder/kojid b/builder/kojid index 05ca267..e90bb23 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5595,7 +5595,6 @@ class NewRepoTask(BaseTaskHandler): repo_json = koji.load_json(f'{src_repo_path}/repo.json') for key in ('with_debuginfo', 'with_src', 'with_separate_src'): if repo_json.get(key, False) != opts.get(key, False): - print(key, repo_json.get(key), opts.get(key)) return False except IOError: self.logger.debug("Can't open repo.json for repo {repo_info['id']}") From 9df0ba623d0b1d92f79e2a7c4af01274f0d0cc11 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 02 2023 07:19:42 +0000 Subject: [PATCH 7/7] replace f-strings with older syntax --- diff --git a/builder/kojid b/builder/kojid index e90bb23..b4e72b5 100755 --- a/builder/kojid +++ b/builder/kojid @@ -81,7 +81,15 @@ from koji.tasks import ( ServerExit, ServerRestart ) -from koji.util import dslice, dslice_ex, isSuccess, parseStatus, to_list, format_shell_cmd +from koji.util import ( + dslice, + dslice_ex, + format_shell_cmd, + isSuccess, + joinpath, + parseStatus, + to_list, +) try: import requests_gssapi as reqgssapi @@ -5560,11 +5568,11 @@ class NewRepoTask(BaseTaskHandler): def copy_arch_repo(self, src_repo_id, src_repo_path, repo_id, arch): """Copy repodata, return False if it fails""" - dst_repodata = f'{self.workdir}/{arch}/repodata' - src_repodata = f'{src_repo_path}/{arch}/repodata' + dst_repodata = joinpath(self.workdir, arch, 'repodata') + src_repodata = joinpath(src_repo_path, arch, 'repodata') try: # copy repodata - self.logger.debug(f'Copying repodata {src_repodata} to {dst_repodata}') + self.logger.debug('Copying repodata %s to %s' % (src_repodata, dst_repodata)) if os.path.exists(src_repodata): # symlink=True is not needed as they are no part of arch repodir shutil.copytree(src_repodata, dst_repodata) @@ -5575,7 +5583,7 @@ class NewRepoTask(BaseTaskHandler): self.session.uploadWrapper('%s/%s' % (dst_repodata, f), uploadpath, f) return [uploadpath, files] except Exception as ex: - self.logger.warning(f"Copying repo {src_repo_id} to {repo_id} failed. {ex}") + self.logger.warning("Copying repo %i to %i failed. %r" % (src_repo_id, repo_id, ex)) # Try to remove potential leftovers and fail if there is some problem koji.util.rmtree(dst_repodata, self.logger) return False @@ -5589,10 +5597,10 @@ class NewRepoTask(BaseTaskHandler): self.logger.debug("Source repo wasn't found") return False if not os.path.isdir(src_repo_path): - self.logger.debug(f"Source repo doesn't exist {src_repo_path}") + self.logger.debug("Source repo doesn't exist %s" % src_repo_path) return False try: - repo_json = koji.load_json(f'{src_repo_path}/repo.json') + repo_json = koji.load_json(joinpath(src_repo_path, 'repo.json')) for key in ('with_debuginfo', 'with_src', 'with_separate_src'): if repo_json.get(key, False) != opts.get(key, False): return False @@ -5601,8 +5609,8 @@ class NewRepoTask(BaseTaskHandler): return False # compare comps if they exist - src_comps_path = f'{src_repo_path}/groups/comps.xml' - dst_comps_path = f'{dst_repo_path}/groups/comps.xml' + src_comps_path = joinpath(src_repo_path, 'groups/comps.xml') + dst_comps_path = joinpath(dst_repo_path, 'groups/comps.xml') src_exists = os.path.exists(src_comps_path) if src_exists != os.path.exists(dst_comps_path): self.logger.debug("Comps exists only in one repo") @@ -5625,17 +5633,17 @@ class NewRepoTask(BaseTaskHandler): def check_arch_repo(self, src_repo_path, dst_repo_path, arch): """More checks based on architecture content""" for fname in ('blocklist', 'pkglist'): - src_file = f'{src_repo_path}/{arch}/{fname}' - dst_file = f'{dst_repo_path}/{arch}/{fname}' + src_file = joinpath(src_repo_path, arch, fname) + dst_file = joinpath(dst_repo_path, arch, fname) # both must non/exist if not os.path.exists(src_file) or not os.path.exists(dst_file): - self.logger.debug(f"{fname} doesn't exit in one of the repos") + self.logger.debug("%s doesn't exit in one of the repos" % fname) return False # content must be same if not filecmp.cmp(src_file, dst_file, shallow=False): - self.logger.debug(f'{fname} differs') + self.logger.debug('%s differs' % fname) return False - self.logger.debug(f'Arch repo test passed {arch}') + self.logger.debug('Arch repo test passed %s' % arch) return True def handler(self, tag, event=None, src=False, debuginfo=False, separate_src=False):