Hi team,
we have received a few requests to run tasks on koji scratch builds. Unfortunately, it is not as straightforward as it would seem since "real" koji builds and scratch builds are not treated the same. That's why "real" builds are downloaded with "koji download-build" and scratch builds are downloaded with its own tool called "koji-download-scratch" which is part of the fedora-review package.
The way the "real" koji builds work is that they are referenced by NVRs since NVRs are unique (I think there can be multiple builds with the same NVR, many with "failed" status and just one with the "completed" status and only the completed ones are searched, but that's just my guess). In taskotron's koji tasks, we feed NVR's to libtaskotron's koji directive which downloads build's RPMs.
In the case of scratch builds, they are not referenced by NVR's but rather just with task ID's.
With that being said, we just can't write a trigger for listening for the scratch builds fedmsgs [1] and trigger a libtaskotron job because there's no NVR in the fedmsg and even if it was, we wouldn't be able to download the rpms.
During a discussion with Kamil, a few solutions were mentioned (none of them is pretty):
1. We can ask koji developers if there is a way to add a method that would return all koji scratch builds for a given NVR - we would then take the latest one and work with it.
2. We can use "koji download-task" which works for both. That would mean koji_build item type would eat koji task IDs instead of NVRs. This would lead to having koji task IDs in resultsdb instead of NVR's which kills readability. Unless libtaskotron finds the NVR from the koji task ID in the case of "real" build" and stores it in a field in resultsdb...Also, we need NVRs for fedmsgs. So add code to the fedmsg layer that would take care of somehow adding a NVR to fedmsg of completed scratch builds tasks...
3. We can add a new action to the koji directive "download_task". And then have for each koji task two formulas, one for scratch build (using "download_task") and one for "real" build" (using "download_build). This would require writing code for supporting multiple formulas for one task and would result in having two almost identical formulas...
Before proceeding with 1. and asking koji folks if they are able to help with that I wanted to check here if anyone has any thoughts on this.
Thanks! Martin
[1] https://apps.fedoraproject.org/datagrepper/id?id=2017-738c5d32-b2b9-40a3-bd1...
On Mon, 9 Jan 2017 08:09:52 -0500 (EST) Martin Krizek mkrizek@redhat.com wrote:
Hi team,
we have received a few requests to run tasks on koji scratch builds. Unfortunately, it is not as straightforward as it would seem since "real" koji builds and scratch builds are not treated the same. That's why "real" builds are downloaded with "koji download-build" and scratch builds are downloaded with its own tool called "koji-download-scratch" which is part of the fedora-review package.
The way the "real" koji builds work is that they are referenced by NVRs since NVRs are unique (I think there can be multiple builds with the same NVR, many with "failed" status and just one with the "completed" status and only the completed ones are searched, but that's just my guess). In taskotron's koji tasks, we feed NVR's to libtaskotron's koji directive which downloads build's RPMs.
In the case of scratch builds, they are not referenced by NVR's but rather just with task ID's.
With that being said, we just can't write a trigger for listening for the scratch builds fedmsgs [1] and trigger a libtaskotron job because there's no NVR in the fedmsg and even if it was, we wouldn't be able to download the rpms.
Couldn't we use the task ID as the primary identifier but use the srpm for readability sake since there is no "build" NVR for scratch builds? Either that or make the primary ID a tuple of (srpm name, koji task start time) either stored that way or rendered as a string e.g foo-1.2-3.fc99.src.rpm built at 201701092100 (YYYYMMDDHHMM) would become foo-1.2-3.fc99-201701092100.
During a discussion with Kamil, a few solutions were mentioned (none of them is pretty):
- We can ask koji developers if there is a way to add a method that
would return all koji scratch builds for a given NVR - we would then take the latest one and work with it.
How would we get the NVR if there's no build identifier on scratch builds? Are you talking about the SRPM name in the fedmsg?
- We can use "koji download-task" which works for both. That would
mean koji_build item type would eat koji task IDs instead of NVRs. This would lead to having koji task IDs in resultsdb instead of NVR's which kills readability. Unless libtaskotron finds the NVR from the koji task ID in the case of "real" build" and stores it in a field in resultsdb...Also, we need NVRs for fedmsgs. So add code to the fedmsg layer that would take care of somehow adding a NVR to fedmsg of completed scratch builds tasks...
Can't we tell the difference between an NVR and a task ID just by the form that the content takes? Wouldn't the following work:
1. attempt to parse input as NVR, if there are no issues, assume that it's a build ID 2. if the NVR parse fails, check to see if the input is an int. if assume that it's a koji task ID
It'd mean that our koji downloading code would get more complicated but it seems like nothing that more unit tests couldn't mitigate. Unless I'm mis-thinking here, the changes needed to pull this off in trigger would be even smaller.
- We can add a new action to the koji directive "download_task". And
then have for each koji task two formulas, one for scratch build (using "download_task") and one for "real" build" (using "download_build). This would require writing code for supporting multiple formulas for one task and would result in having two almost identical formulas...
I'm not a huge fan of 3 if there is any other reasonable way to do things. Code duplication is not the way to go.
Before proceeding with 1. and asking koji folks if they are able to help with that I wanted to check here if anyone has any thoughts on this.
If what I wrote above would work well enough, that sounds like a good way to go forward to me. If not, 2 sounds like a decent option.
Tim
Couldn't we use the task ID as the primary identifier but use the srpm for readability sake since there is no "build" NVR for scratch builds?
Systems like new hotness will need to query according to the task ID, though (to get reliable results). So we're talking about hacking just resultsdb *frontend* here, e.g. by having "item: task_id" and "item_display: nvr" in the results yaml. I don't like it much. Searching in the frontend would have to search both in item and item_display.
Or we could use our existing "note" field to put NEVR inside, and it would be easily visible in the frontend without any ugly hacks. People would have to know that if they want to search by NEVR for scratch builds, they need to put the NEVR in the note search field (we'd have to add it).
I assume you proposed using task id as primary identifier just to scratch builds (but not standard builds). If you also mean it for standard builds, then scheduling tasks locally starts to be quite inconvenient (for each build you want to run your task on, you need to find its task id first; it's hard to re-run something from shell history). We would also be changing something that's a de-facto standard in our infra (using NEVR as a unique identifier across projects).
Either that or make the primary ID a tuple of (srpm name, koji task start time) either stored that way or rendered as a string e.g foo-1.2-3.fc99.src.rpm built at 201701092100 (YYYYMMDDHHMM) would become foo-1.2-3.fc99-201701092100.
The same problem with inconvenient local execution. The command line starts to be hard to read (in tutorials, etc).
Also, the srpm name doesn't have to be anything reasonable, Koji will happily accept anything (it doesn't care, for scratch builds). So we can easily receive "1.srpm" or "my-new-patch.srpm" from the fedmsg (we actually tried this [1]). Deriving any useful info from this input is probably a mistake. We would have to download the srpm and look into the included spec file to reliably decide which package this is related to.
[1] https://apps.fedoraproject.org/datagrepper/id?id=2017-2fe001d2-32d4-434a-b3a...
During a discussion with Kamil, a few solutions were mentioned (none of them is pretty):
- We can ask koji developers if there is a way to add a method that
would return all koji scratch builds for a given NVR - we would then take the latest one and work with it.
How would we get the NVR if there's no build identifier on scratch builds? Are you talking about the SRPM name in the fedmsg?
Yes. Not from the fedmsg, but from the Koji task (the fedmsg gets this value from the srpm filename included in the Koji task, I believe). But see the problem described above with srpm file naming (can be arbitrary). So even better would be if Koji could search and return scratch build info containing the metadata of the srpm or from the spec file included.
But I'm honestly not sure it is a good idea even if they wanted to implement it. More on that below.
- We can use "koji download-task" which works for both. That would
mean koji_build item type would eat koji task IDs instead of NVRs. This would lead to having koji task IDs in resultsdb instead of NVR's which kills readability. Unless libtaskotron finds the NVR from the koji task ID in the case of "real" build" and stores it in a field in resultsdb...Also, we need NVRs for fedmsgs. So add code to the fedmsg layer that would take care of somehow adding a NVR to fedmsg of completed scratch builds tasks...
Can't we tell the difference between an NVR and a task ID just by the form that the content takes? Wouldn't the following work:
- attempt to parse input as NVR, if there are no issues, assume that it's a build ID
- if the NVR parse fails, check to see if the input is an int. if assume that it's a koji task ID
The reverse might be easier, but yes, that's of course possible. There are other caveats, though.
It'd mean that our koji downloading code would get more complicated but it seems like nothing that more unit tests couldn't mitigate. Unless I'm mis-thinking here, the changes needed to pull this off in trigger would be even smaller.
I'm afraid our code could get infested with if clauses pretty soon. This is not just about downloading rpms in the koji directive. We have more koji build-related functionality and we'll sure get more in the future. For example, we currently support "download_latest_stable", but that might be very tricky for scratch builds with "1.srpm" files (which package is that?). If we ever needed to work with package epoch, it is unavailable for scratch builds (since there is no build info object to be returned). Another example, we have dist-git directive. Without clear identification of the package you can't use dist-git directive. Even if we download 1.srpm and look into it, you can still find something that's not available in dist-git (because you can build something in Koji that is not packaged in Fedora).
What I'm trying to illustrate here is that this is not just a few if clauses in a single piece of code (koji utils). This can influence the whole workflow through the formula. Some directives might return different data based on whether it is a regular build or a scratch build. Some might completely fail to work (distgit directive). And you might want to have different standards and approaches for scratch builds.
As an example, you might want to use distgit directive to download rpmlint.config for that particular package when running rpmlint. If that git repo doesn't exist, you want to error out (should never happen). But for scratch builds, it's helpful if you figure out which package it is and the config file is used, but you don't mind if such a repo doesn't exist - the rpmlint output without any personalized rpmlint.config is still valuable. How do you implement that? Will we depend on heuristics (recognizing standard vs scratch build) and hard-code whether it errors out or not? We can, but will we be always to do the right thing? What if some tasks have different preferences? Will we not end up with monster code full of if-then-else? It seems to me that it's a better approach to allow the task to decide what it wants to do and fully inform it in advance whether this is a standard build or a scratch build. That means using different item_type and therefore different formula. The advantage is that you can easily implement something like "fail_on_error: True/False" option for a directive and the task can set what it needs. Or the task will know whether it's possible to retrieve epoch for a build and will act accordingly.
As a last example, different logic might be desirable for certain tasks even in the "task code" (not libtaskotron code) itself. With abicheck, we currently compare new koji build with the latest build in stable updates. But for scratch builds, different approach might be desired - e.g. comparing to the last standard/scratch build, regardless of the koji tag it is in. We actually talked about very this topic the last time I saw Sinny and Dodji. If we decided to merge standard builds and scratch builds into a single workflow, we don't allow tasks to take a different approach even if they wanted (unless they go into such extents like determining whether the current "item" is a standard or a scratch build, and then emulating e.g. a koji directive in their own code). Btw, what about tasks that want/need to run exclusively on just standard or just scratch builds?
- We can add a new action to the koji directive "download_task". And
then have for each koji task two formulas, one for scratch build (using "download_task") and one for "real" build" (using "download_build). This would require writing code for supporting multiple formulas for one task and would result in having two almost identical formulas...
I'm not a huge fan of 3 if there is any other reasonable way to do things. Code duplication is not the way to go.
I've been thinking about this and I actually like it the most. I already described much of the issues with compounding scratch+standard builds in the text above, but let me expand on that. I currently believe having a separate koji_build and koji_scratch_build type is the best and the most technically clean way to go. These two events look very similar on the first (and second and third) glance, but they are implemented very differently. There's a good reason that their fedmsgs look not alike at all. Scratch builds have no restrictions on NEVRs, no uniqueness constraints, no dist-git constraints (anything can be built), they do not have any API (except a completely generic "task" API), we can't get any structured metadata about them. They are very different beasts. Compare this to e.g. DNF repo compose, ostree compose, and docker compose. They are all "composes", but sufficiently different to get different item types and different handling in directives (or probably even some exclusive directives). I believe this is the same case.
The code duplication doesn't have to be a problem, I think. In package distgit, you'll have koji_build/mytest/runtask.yaml and koji_scratch_build/mytest/runtask.yaml. You can symlink any code between dirs, if your task is able to handle both. Or you can have separate code, if your task needs it (!). The formulas are duplicated, yes, but not really. First, they don't change much, so it's not really a problem, and second you might want to have a different behavior anyway (e.g. remove dist-git directive to download rpmlint.conf, since it's problematic for scratch builds).
For non-distgit (generic) tasks, there would be two formulas under different names in the same repo (say runtask.yaml and runtask_scratch.yaml). We would configure trigger to run the task with 'runtask.yaml' for koji builds and 'runtask_scratch.yaml' for scratch builds (that should be hopefully trivial). And this is in line with our future ideas how scheduling should work - users should be able to configure when to run their task in a web UI, and a formula name would be part of that. That makes it flexible and would allow them to do awesome things, like experiment with a new formula while keeping the old formula still working. In short, I believe we want to go this route (multiple formula support) anyway. Having it currently "hard-coded" in trigger is not necessarily a bad thing, it's a step forward towards having it in a database.
Ugh, such a long email. Bad kparal.
I don't have much to add, just that I agree with Kamil. I see some minor problems even in what he wrote, but that's well beyond what I think we should be solving now.
j.
On Tue, Jan 10, 2017 at 5:19 PM, Kamil Paral kparal@redhat.com wrote:
Couldn't we use the task ID as the primary identifier but use the srpm for readability sake since there is no "build" NVR for scratch builds?
Systems like new hotness will need to query according to the task ID, though (to get reliable results). So we're talking about hacking just resultsdb *frontend* here, e.g. by having "item: task_id" and "item_display: nvr" in the results yaml. I don't like it much. Searching in the frontend would have to search both in item and item_display.
Or we could use our existing "note" field to put NEVR inside, and it would be easily visible in the frontend without any ugly hacks. People would have to know that if they want to search by NEVR for scratch builds, they need to put the NEVR in the note search field (we'd have to add it).
I assume you proposed using task id as primary identifier just to scratch builds (but not standard builds). If you also mean it for standard builds, then scheduling tasks locally starts to be quite inconvenient (for each build you want to run your task on, you need to find its task id first; it's hard to re-run something from shell history). We would also be changing something that's a de-facto standard in our infra (using NEVR as a unique identifier across projects).
Either that or make the primary ID a tuple of (srpm name, koji task start time) either stored that way or rendered as a string e.g foo-1.2-3.fc99.src.rpm built at 201701092100 (YYYYMMDDHHMM) would become foo-1.2-3.fc99-201701092100.
The same problem with inconvenient local execution. The command line starts to be hard to read (in tutorials, etc).
Also, the srpm name doesn't have to be anything reasonable, Koji will happily accept anything (it doesn't care, for scratch builds). So we can easily receive "1.srpm" or "my-new-patch.srpm" from the fedmsg (we actually tried this [1]). Deriving any useful info from this input is probably a mistake. We would have to download the srpm and look into the included spec file to reliably decide which package this is related to.
[1] https://apps.fedoraproject.org/datagrepper/id?id=2017- 2fe001d2-32d4-434a-b3a9-29ab31eebbb0&is_raw=true&size=extra-large
During a discussion with Kamil, a few solutions were mentioned (none of them is pretty):
- We can ask koji developers if there is a way to add a method that
would return all koji scratch builds for a given NVR - we would then take the latest one and work with it.
How would we get the NVR if there's no build identifier on scratch builds? Are you talking about the SRPM name in the fedmsg?
Yes. Not from the fedmsg, but from the Koji task (the fedmsg gets this value from the srpm filename included in the Koji task, I believe). But see the problem described above with srpm file naming (can be arbitrary). So even better would be if Koji could search and return scratch build info containing the metadata of the srpm or from the spec file included.
But I'm honestly not sure it is a good idea even if they wanted to implement it. More on that below.
- We can use "koji download-task" which works for both. That would
mean koji_build item type would eat koji task IDs instead of NVRs. This would lead to having koji task IDs in resultsdb instead of NVR's which kills readability. Unless libtaskotron finds the NVR from the koji task ID in the case of "real" build" and stores it in a field in resultsdb...Also, we need NVRs for fedmsgs. So add code to the fedmsg layer that would take care of somehow adding a NVR to fedmsg of completed scratch builds tasks...
Can't we tell the difference between an NVR and a task ID just by the form that the content takes? Wouldn't the following work:
- attempt to parse input as NVR, if there are no issues, assume that it's a build ID
- if the NVR parse fails, check to see if the input is an int. if assume that it's a koji task ID
The reverse might be easier, but yes, that's of course possible. There are other caveats, though.
It'd mean that our koji downloading code would get more complicated but it seems like nothing that more unit tests couldn't mitigate. Unless I'm mis-thinking here, the changes needed to pull this off in trigger would be even smaller.
I'm afraid our code could get infested with if clauses pretty soon. This is not just about downloading rpms in the koji directive. We have more koji build-related functionality and we'll sure get more in the future. For example, we currently support "download_latest_stable", but that might be very tricky for scratch builds with "1.srpm" files (which package is that?). If we ever needed to work with package epoch, it is unavailable for scratch builds (since there is no build info object to be returned). Another example, we have dist-git directive. Without clear identification of the package you can't use dist-git directive. Even if we download 1.srpm and look into it, you can still find something that's not available in dist-git (because you can build something in Koji that is not packaged in Fedora).
What I'm trying to illustrate here is that this is not just a few if clauses in a single piece of code (koji utils). This can influence the whole workflow through the formula. Some directives might return different data based on whether it is a regular build or a scratch build. Some might completely fail to work (distgit directive). And you might want to have different standards and approaches for scratch builds.
As an example, you might want to use distgit directive to download rpmlint.config for that particular package when running rpmlint. If that git repo doesn't exist, you want to error out (should never happen). But for scratch builds, it's helpful if you figure out which package it is and the config file is used, but you don't mind if such a repo doesn't exist - the rpmlint output without any personalized rpmlint.config is still valuable. How do you implement that? Will we depend on heuristics (recognizing standard vs scratch build) and hard-code whether it errors out or not? We can, but will we be always to do the right thing? What if some tasks have different preferences? Will we not end up with monster code full of if-then-else? It seems to me that it's a better approach to allow the task to decide what it wants to do and fully inform it in advance whether this is a standard build or a scratch build. That means using different item_type and therefore different formula. The advantage is that you can easily implement something like "fail_on_error: True/False" option for a directive and the task can set what it needs. Or the task will know whether it's possible to retrieve epoch for a build and will act accordingly.
As a last example, different logic might be desirable for certain tasks even in the "task code" (not libtaskotron code) itself. With abicheck, we currently compare new koji build with the latest build in stable updates. But for scratch builds, different approach might be desired - e.g. comparing to the last standard/scratch build, regardless of the koji tag it is in. We actually talked about very this topic the last time I saw Sinny and Dodji. If we decided to merge standard builds and scratch builds into a single workflow, we don't allow tasks to take a different approach even if they wanted (unless they go into such extents like determining whether the current "item" is a standard or a scratch build, and then emulating e.g. a koji directive in their own code). Btw, what about tasks that want/need to run exclusively on just standard or just scratch builds?
- We can add a new action to the koji directive "download_task". And
then have for each koji task two formulas, one for scratch build (using "download_task") and one for "real" build" (using "download_build). This would require writing code for supporting multiple formulas for one task and would result in having two almost identical formulas...
I'm not a huge fan of 3 if there is any other reasonable way to do things. Code duplication is not the way to go.
I've been thinking about this and I actually like it the most. I already described much of the issues with compounding scratch+standard builds in the text above, but let me expand on that. I currently believe having a separate koji_build and koji_scratch_build type is the best and the most technically clean way to go. These two events look very similar on the first (and second and third) glance, but they are implemented very differently. There's a good reason that their fedmsgs look not alike at all. Scratch builds have no restrictions on NEVRs, no uniqueness constraints, no dist-git constraints (anything can be built), they do not have any API (except a completely generic "task" API), we can't get any structured metadata about them. They are very different beasts. Compare this to e.g. DNF repo compose, ostree compose, and docker compose. They are all "composes", but sufficiently different to get different item types and different handling in directives (or probably even some exclusive directives). I believe this is the same case.
The code duplication doesn't have to be a problem, I think. In package distgit, you'll have koji_build/mytest/runtask.yaml and koji_scratch_build/mytest/runtask.yaml. You can symlink any code between dirs, if your task is able to handle both. Or you can have separate code, if your task needs it (!). The formulas are duplicated, yes, but not really. First, they don't change much, so it's not really a problem, and second you might want to have a different behavior anyway (e.g. remove dist-git directive to download rpmlint.conf, since it's problematic for scratch builds).
For non-distgit (generic) tasks, there would be two formulas under different names in the same repo (say runtask.yaml and runtask_scratch.yaml). We would configure trigger to run the task with 'runtask.yaml' for koji builds and 'runtask_scratch.yaml' for scratch builds (that should be hopefully trivial). And this is in line with our future ideas how scheduling should work - users should be able to configure when to run their task in a web UI, and a formula name would be part of that. That makes it flexible and would allow them to do awesome things, like experiment with a new formula while keeping the old formula still working. In short, I believe we want to go this route (multiple formula support) anyway. Having it currently "hard-coded" in trigger is not necessarily a bad thing, it's a step forward towards having it in a database.
Ugh, such a long email. Bad kparal. _______________________________________________ qa-devel mailing list -- qa-devel@lists.fedoraproject.org To unsubscribe send an email to qa-devel-leave@lists.fedoraproject.org
qa-devel@lists.fedoraproject.org
-
Josef Skladanka -
Kamil Paral -
Martin Krizek -
Tim Flink