see also: #4027

This is feeling tantalizingly close, which probably means it's about half done ;)

Also the merge on my branch seems to have confused pagure. The diff displayed is incorrect.

squashed and rebased. pagure seems less confused now

This is nearly complete

--request-repo?

--request-repo?

This is also not accurate. The build handler (and any other handler that calls self.getRepo()) will always end up using the repo.request call (which will either return a matching active repo or an active request entry).

This flag controls affects the parameters of the repo we request. If given, we pass min_event = "last", which means the build requests a repo that is current as of that point in time. Without it, we get the default behavior, which uses a min_event value that is slightly older (based on RepoLag setting).

The default behavior is effectively similar to what we have now, where the build gets whatever the latest repo for the tag is, which may or may not be current depending on timing and kojira backlog.

So, perhaps something like --current-repo?

Hmm, thinking about it a little more, --wait-repo is probably fine if we adjust the help text, plus it matches --wait-build.

Rebased. Minor conflicts with PR#4097

Two more updates (migration + cleanup): https://pagure.io/fork/tkopecek/koji/commits/pr4033a

Maybe (not as a part of this PR) we could merge TaskWatcher and RepoWatcher into some baseclass. Also not sure if they shouldn't be in the same location. Probably both have some use outside of CLI.

Not sure if repo_queue shouldn't contain some priority field. But maybe it is not needed. I was thinking about some situation when we need to run some repo immediately, but we can spawn newRepo tasks without consulting the queue at all.

Two more updates (migration + cleanup)

I pulled in the migration one, though the script also needed a fix because of this:

ERROR:  CREATE INDEX CONCURRENTLY cannot run inside a transaction block

queue cleanup is actually already handled in the hub code. See clean_repo_queue.

Maybe (not as a part of this PR) we could merge TaskWatcher and RepoWatcher into some baseclass.

They're quite different code, based on different queries. RepoWatcher never directly queries the task, but uses the small bit of task info that can be returned by repo.checkRequest. There might be some small unification that could be done, but I agree it should probably be separate from this PR.

Not sure if repo_queue shouldn't contain some priority field. But maybe it is not needed. I was thinking about some situation when we need to run some repo immediately, but we can spawn newRepo tasks without consulting the queue at all.

I keep debating adjusting this. For the most part, I think first in first out is the best plan, but you have a good point about manual override, and it would be nice to deprioritize the auto regens.

My hope is that the queue will stay small or empty with on-demand, but it's hard to be sure how that will play out without actually running this with a real production load.

Casual querying in prod suggests that on-demand would trigger less than 25 regens per hour (assuming default RepoLag setting of 1 hour). It's unclear to me how many tags we'd need to set up for auto-regen, though I figure at least all the '*-repo' targets (currently there are 55 distinct build tags there). It's hard to know how often those would actually need to regen, but default RepoAutoLag is 2 hours, so worst case is about 28/hr.

Compared to our frequent spikes of 300+ regens/hour, I'd expect the queue to stay small.

I suppose the x-factor would be user abuse of the request api.

Relevant queries

select count(id), date_trunc('hour', create_time) as bin, count(distinct cast(xpath('//param[2]/value/string/text()', request::xml) as text[]))from task where method in ('build', 'maven', 'chainmaven') and create_time > now() - '1 day'::interval group by bin;
select count(id), date_trunc('hour', create_time) as bin, count(distinct cast(xpath('//param[4]/value/string/text()', request::xml) as text[]))from task where method in ('image', 'appliance', 'livecd', 'livemedia') and create_time > now() - '1 day'::interval group by bin;
select count(id), date_trunc('hour', create_time) as bin, count(distinct cast(xpath('//param[1]/value/string/text()', request::xml) as text[]))from task where method = 'newRepo' and create_time > now() - '1 day'::interval group by bin;

That brings repos.py to full coverage

kojihub/repos.py                        483      0   100%

rebased onto d68a927

rebased onto current master and squashed commits again. If anyone is interested, pre-squash branch is saved here. No merge conflicts.

Here is the current result of check_api as compared to current master:

Signature changed for koji.tasks.WaitrepoTask.handler:
  old: (self, tag, newer_than=None, nvrs=None)
  new: (self, tag, newer_than=None, nvrs=None, min_event=None)
Args extended for koji.tasks.WaitrepoTask.handler: ['min_event']
Added module global: koji.util.RepoWatcher
Added RPC call: repo.autoRequests
Added RPC call: repo.checkQueue
Added RPC call: repo.checkRequest
Added RPC call: repo.get
Added RPC call: repo.getExternalRepoData
Added RPC call: repo.query
Added RPC call: repo.queryQueue
Added RPC call: repo.references
Added RPC call: repo.request
Added RPC call: repo.setExternalRepoData
Added RPC call: repo.setRequestPriority
Added RPC call: repo.setState
Added RPC call: repo.updateEndEvents
Added RPC call: tagFirstChangeEvent
Added RPC call: tagLastChangeEvent
Signature changed for rpc call getEvent:
  old: (id)
  new: (id, strict=True)
Args extended for rpc call getEvent: ['strict']
Signature changed for rpc call getRepo:
  old: (tag, state=None, event=None, dist=False)
  new: (tag, state=None, event=None, dist=False, min_event=None)
Args extended for rpc call getRepo: ['min_event']
Signature changed for rpc call host.repoInit:
  old: (tag, task_id=None, with_src=False, with_debuginfo=False, event=None, with_separate_src=False)
  new: (tag, task_id=None, event=None, opts=None)
ERROR: Arg count reduced for rpc call host.repoInit
Got 23 warnings and 1 errors

@tkopecek please check the change for the sidetag plugin. It's not strictly required, but probably better to use the requests interface here. The main risk is if some client or script is assuming that the task_id field will be not None.

We could also drop the automatic initial regen option entirely, since the later builds will just make a request, but there is probably some value in triggering it early.

I've checked rpkg usage and it is safe in regard to their usage. I believe it is safe to update it.

@tkopecek a few more questions for review

• Do you think the queue cleanup in the hub code is ok, or should we consolidate into the cleanup script?
• Any concerns about the host.repoInit api change?
• Do you think we should require a permission to request a repo?
• repo init callback args changed a bit. Won't affect protonmsg plugin, but should we worry about anything else?
• Do the docs read ok?
• does the overall structure of the cli changes make sense?

• Do you think the queue cleanup in the hub code is ok, or should we consolidate into the cleanup script?

I was thinking about cleanup script since we've introduced scheduler planning. For now I would leave it here. In future I was imagining some hook which could call different cron-like tasks (scheduler planning, cleaning sessions, cleaning repo queue, ...). Hook could know what is the async needed and trigger them, so they don't cause big bursts when they hit each other. Wild idea would be to run async operations really async, so connection to user can be closed and these subtasks can run after that. But that is another major intervention we've not capacity to do now.

• Any concerns about the host.repoInit api change?

No, I believe it is not used by anyone except koji itself.

• Do you think we should require a permission to request a repo?

I would start without and see what is the real usage. I don't think it would be bigger load than is with current kojira.

There is now not possibility to request repo with overriding repo.opts via request_repo. But not sure if it has any real-world usage, so it is probably fine (I can still spawn newRepo manually).

• repo init callback args changed a bit. Won't affect protonmsg plugin, but should we worry about anything else?

I don't know about anything using it.

• Do the docs read ok?

Yep, I would only expand repo.auto and kojira interaction for people who migrate from older koji. It is probably not clear what kojira do now and that it still needs to be running.

• does the overall structure of the cli changes make sense?

Yes, only thing I'm not sure if koji build --wait-repo shouldn't file a request automatically.

Not sure if we want to update tag2distrepo to using queue. As it is just spawning the task and not waiting for anything, it could be better approach?

koji wait-repo-request <id> fails on KeyError: 'score'.

Yes, only thing I'm not sure if koji build --wait-repo shouldn't file a request automatically.

Hmm, interesting. I guess it could...

This seems like a relatively small optimization, as in most cases the build task will start reasonably quickly and make the request almost immediately. In cases where that is not true, it might actually be better to defer the load.

Definitely something to think about, but I'm not sure I'd want to implement this optimization in the client. Of course, a user could always just make a separate request-repo call if they feel the need.

Not sure if we want to update tag2distrepo to using queue

dist repos are not covered by the requests feature. They could be eventually, but I think it will be tricky to get it right.

koji wait-repo-request fails on KeyError: 'score'

Ah, I missed that field ref. Will fix!

Commit f2da4b8e fixes this pull-request

Some testing notes

overview

Since there is a lot of churn here, it is probably best to focus on higher level behavior. I.e.

• cli behavior
• kojira behavior
• ensuring that builds are triggering regenerations as needed

cli

The cli has two new commands: request-repo and wait-repo-request. These should function as expected.

The existing wait-repo and regen-repo commands have been modified in light of the repo requests. Both should work in compat mode with older hubs.

Additionally, taginfo and list-history have some small changes. Both commands should work normally.

Additionally, while the build command itself hasn't really changed, the interpretation of the --wait-repo option has. When this option is given, the system should use a current repo for the build tag. That doesn't always mean a regen, as there might already be a current repo, but the repo that is used match the last change event for the build tag. For example, if you make a change to the build tag (or one of its ancestors in the inheritance), and then trigger a build with this option, you should see the task fire off an appropriate waitrepo subtask.

api

There are a few calls that have been updated and several entirely new calls. Most of the new calls are used by kojira or the cli commands.

The repoInfo, getRepo calls have been rewritten. They should work as they did before.

The getEvent call gained a strict option (default True). The new option should be honored and the call should otherwise work as before.

The newRepo call can be disabled by the AllowNewRepo hub setting. It should otherwise work as before.

The tagLastChangeEvent and tagFirstChangeEvent are fairly technical calls used by the repo code. Both should work for any tag and event combo. In particular

• after making changes to a tag (or its ancestors), tagLastChange event should correctly report the most recent change
• tagLastChangeEvent should report None when querying before the first change event for the tag
• tagFirstChangeEvent should report None when querying after last change event for the tag

All of the repo.* calls are new. Almost all of them are exercised by either the new cli commands or kojira. The exceptions are -- repo.query, repo.setRequestPriority, and repo.queryQueue