I'm now wondering if someone would benefit if this behaviour is configurable - e.g. enabled by default but possible to disable completely and avoid long timeouts when network/repos are unreachable.

Also, shouldn't this mirror be configurable?

@jflorian Could there be something else? yum/createrepo uses only this one and I've not found any other DTD for repomd.

@tkopecek I'm sorry, I didn't look close enough at that. Short version: disregard my comment.

Long version: My thought was tainted by how I achieved this objective in my gojira tool where I checked the metadata timestamps of my local repo mirrors which upon a change would trigger a regen-repo command. So when I saw the "metadata" bit of that URL I mistook that just another repo. I don't work with DTDs directly, but I get the concept so I'm rather useless here unfortunately. Seems like a glaring central point of failure unless DNS provides some failover means. Outta my element here.

I would use this feature for my instance.

Few toughs.
There is maybe a need to explicitly opt-in this feature because some "external repo" might be under the control of the "koji hub admins". Thinking about Fedora EPEL case here. The regen-repo task might be run each time the base repos are updated, there is no detection needed here. Once that said it should be harmless. As regen repo tasks will quickly be newer than the last koji external repo update.

Right now, I have the following error in kojira.log when the patch is applied to koji 1.13 on el7.
sept. 08 13:40:59 koji01.online.rpmfusion.net kojira[6605]: for arch in self.tag_arches.get(tag, self.session.getTag(tag)['arches'].split()):
sept. 08 13:40:59 koji01.online.rpmfusion.net kojira[6605]: AttributeError: 'NoneType' object has no attribute 'split'

Thanks for your work on this.

I've changed the self.session.getTag(tag)['arches'].split()): to hardcode to 'x86_64' since this arch is built for every target. It seems to work for some cases, but it fails on others.

I need to investigate more deeply. (it could be caused by the fedora/fedora-secondary split).

This patch applied on our infra seems to work as expected.

But I think there is a race condition. This might be caused by the following events.
(not sure how to verify this).
- A new repo is created in fedora, but not yet published.
- A build job is created, and an override set for the target, so the regen-repo task is triggered
- Then the new fedora repo is published.
- A later buildroot creation will fail because of missing packages from the (old) fedora repos.

Here kojira might still consider the external repository as fresh because there is an event that has triggered a regen-repo task. But it doesn't mean this last regen-repo task was run against the latest repodata. Specially if the repodata was not yet published.

So I think it's a good path to rely on the xml timestamp value of the repomd.xml . But this probably need to be register as a property of the given external url (the later one for all arches of a given external_url seems enough).
Then this value can be compared to the previous one, no matter which event triggered a rebuild if the previous timestamp is older than the current one (and if a regen-repo task isn't already running).

@kwizart I've changed logic of url retrieval, so error shouldn't appear now.

Now checking race condition issue.

I've re-applied this patch against 1.14.0 and hit the following issue:
arch_url = url.replace('$arch', arch)
TypeError: expected a character buffer object

Changed to:
arch_url = url.replace('$arch', str(arch))
fixes the problem

Any tough about the race condition ?

I've added a fix for that. Reason is bad if condition. Your fix will create 'path/to/repo/None/os' url, which is wrong and will result in 404.

To race condition - I don't think it will happen, but maybe I'm wrong. Even if external repo will be created after kojira's one, its timestamp will be higher than last regen-repo, so regeneration will be triggered correctly as external repos are checked every time if tag's content has not changed.

What I'm still worried about is amount of potential network traffic - but maybe it could be solved by admin by using proper proxy and it's not kojira's thing to care about. I can add option to config if checking of external repos should happen at all for some network-critical deployments.

@mikem ideas?

Thx for fixing the arch issue.

About the race condition:
This is the reverse, I expect the race occurs only when the external repo is {{created}} "before" the kojira generated one, but {{published}} "after". When such repo is published, kojira expects it' is already new enough. But actually, it was run against a previous version of the external repo.

Fedora is particularly good at producing such external repository because with bodhi-backend there is a strict separation between the repository production tasks (mash, pungi, dist-repos, whatever) and the publication tasks (rsync, etc).

Hmm, I understand it now. Timestamp in this case wouldn't be a property of external repo, but of repo-external_repo relation as any repo can be regenerated in different times and in that time it will get fetched. Only place, where we have this relation in db is tag_external_repos table, which could be extended for this.

Hi there, I've experienced two issue with the current rebase on top of 16.1:
line 328 kojira
- if arch is None and '$arch' not in url:
+ if arch is None or '$arch' not in url:
But I'm not sure about why to iterate on a None Arch at all.

line 353 kojira
- for external_repo in session.getExternalRepoList(tag):
+ for external_repo in self.session.getExternalRepoList(tag):

After talking about this feature with Mike at CentOS Dojo Brussel 2019 this month.

This is a need to track the timestamp for each arch where an external repo is used, because if one arch is published late, it will not be seen by the script and only this arch may fail.

Another point is that if the timestamp ( by repo - by arch ) is updated during the regen-repo task, there is a need to discard the result and schedule a new regen-repo task.
So there is a need to compare the timestamp for earch arch of earch external repos for changes right before and right after the regen-repo task.

What I recall from the conversation was that we were worried about races where we could see inconsistent data in the merge.

The relatively simple check we came up with was to check the external repo before and after. If it changed during the merge, then we through out the results and re-run (or maybe just fail and let kojira re-run the jobs).

This check could either be done on a per-arch basis (weaker), or on a overall basis (stronger). I.e. the stronger case would mean that if any of the individual external arch repos changed during the entire run (over all arches) of the createpo tasks, then we start over.

The weaker case is likely much easier to implement, and would be a good start.

@mikem Does it make sense to plan it for 1.18? I would rewrite it (kojira changed a bit meanwhile, so even merge is impossible now).

I've rebased original PR due reappearing problem in #1443 and have following proposals:

1. Use this PR for kojira
   • start with this
   • add configuration option to disable checking of external repos
   • move external repo checking to separate thread, as it can be quite slow
2. Create new RFE/bug for newRepo task. This one should handle proposed problem with consistency.
   • check timestamps of external repos before and after merge, fail in case when any of these is different
   • store external repo timestamps in tag.extra and use it in kojira's code instead of event_ts in case it exists.

Comments?

I've gone ahead and modified current PR according to previous comment.

This seems like could break this out into a smaller "normalize_arch_url" method to centralize this "$arch substitution" logic.

As an admin, I don't understand what action I would perform to correct this warning message

@tkopecek what's missing here? Can we target it for 1.22?

It is already scheduled for 1.21. Needs some reviews and testing from other folks (@mikem, @julian8628) - otherwise it is complete.

I still don't understand much of the arch= [None] case...

I'm using that instead:

+    def checkExternalRepo(self, ts, repodata, tag):
+        """Determine which external repos are current"""
+        url = repodata['url']
+        if url not in self.external_repos:
+            newest_ts = 0
+            **arches = []** # placeholder for repos without $arch bit
+            try:
+                arches += self.session.getTag(tag)['arches'].split()
+            except AttributeError:
+                pass
+            for arch in arches:
+                **if '$arch' not in url:**
+                    arch_url = url
+                else:
+                    arch_url = url.replace('$arch', arch)

In others words , I don't get in which situation self.session.getTag(tag)['arches'].split() could output None or even noarch. So there are only two situations to handle IMO:

1/ repo has architecture harcoded (or implicit) because that koji instance only build for x86_64:
url=http://dl.fp.org/pub/linux/fedora/updates/x86_64
in this case, there is no need to use url.replace. whatever the arch is .

2/ repo use the $arch variable in url, then there is a need to use url.replace with an existing and valid architecture. Even in the case of building for noarch.
In such case, None (or noarch) will never ever be a valid architecture to use url.replace. (at least not in the fedora default URL context).

At the very least if a noarch case ever matter, then there is a need to use a "sane default" arch, but one cannot predict which sane default arch will be relevant for a particular koji instance.

I've not tested the current code, but older revision was producing 404 errors on the local mirror because of the use of
url=http://dl.fp.org/pub/linux/fedora/updates/None URLs.

Ok, I don't remember why I felt it is important. Your solution looks ok to me - added.

@tkopecek
I don't see the change implemented in the PR is it still in your local tree ?

@kwizart - it is here https://pagure.io/fork/tkopecek/koji/c/25f2612d38d4dc1c3d6d0550da56d7fcf857acd9

Okay, good for merge for me.

:thumbsup:

:thumbsup:

Commit 9639f78c fixes this pull-request

It took me a while to see that this feature was controlled by a new option (check_external_repos) that was disabled by default. Is this documented somewhere?

Yes, see https://docs.pagure.org/koji/utils/ - there is a potential problem with server timezone, so we've disabled it by default.