New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query resultsdb for the results of the Atomic CI pipeline and display them #1847

Merged
merged 1 commit into from Oct 9, 2017

Conversation

Projects
None yet
7 participants
@pypingou
Member

pypingou commented Oct 2, 2017

The way bodhi was querying resultsdb for test results was relying on the
specific structure of taskotron's results which lead to the results from
the Atomic CI pipeline to not be included.
With this commit we're querying for the specific structure of the Atomic
CI results so they are included in the "Automated tests" tab of the
update page.

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

@bowlofeggs

This comment has been minimized.

Show comment
Hide comment
@bowlofeggs

bowlofeggs Oct 2, 2017

Member

The change looks OK to me, though I don't know a lot about this area of the code so I'd appreciate feedback from @kparal, @AdamWill, or @ryanlerch.

@pypingou, do you know of an update that has these kinds of tests that we can use to see how it renders? I looked at some recent kernel updates and they didn't seem to show these tests.

Member

bowlofeggs commented Oct 2, 2017

The change looks OK to me, though I don't know a lot about this area of the code so I'd appreciate feedback from @kparal, @AdamWill, or @ryanlerch.

@pypingou, do you know of an update that has these kinds of tests that we can use to see how it renders? I looked at some recent kernel updates and they didn't seem to show these tests.

@@ -341,6 +347,8 @@
$("#tab-automatedtests").append(" <span class='fa fa-spinner fa-spin'></span>");
for (index = 0, len = builds.length; index < len; ++index) {
request_results(base_url+"results/latest?original_spec_nvr="
+builds[index]+"&testcases=org.centos.prod.ci.pipeline.complete");

This comment has been minimized.

@kparal

kparal Oct 3, 2017

Contributor

This will not work, because in Koji the builds are named e.g. cockpit-151-2.fc26 while centos pipeline results use cockpit-151-2.f26 (notice the difference).

@kparal

kparal Oct 3, 2017

Contributor

This will not work, because in Koji the builds are named e.g. cockpit-151-2.fc26 while centos pipeline results use cockpit-151-2.f26 (notice the difference).

This comment has been minimized.

@pypingou

pypingou Oct 3, 2017

Member

The pipeline is being fixed in CentOS-PaaS-SIG/ci-pipeline#400

@pypingou

pypingou Oct 3, 2017

Member

The pipeline is being fixed in CentOS-PaaS-SIG/ci-pipeline#400

Show outdated Hide outdated bodhi/server/templates/update.html
Show outdated Hide outdated bodhi/server/templates/update.html
@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou Oct 3, 2017

Member

@pypingou, do you know of an update that has these kinds of tests that we can use to see how it renders?

I was testing this with an update for criu-3.3-5.fc26 and I had this change as well to test it:

+      request_results(base_url+"results/latest?original_spec_nvr="
+        +builds[index].replace('.f', '.fc')+"&testcases=org.centos.prod.ci.pipeline.complete");
Member

pypingou commented Oct 3, 2017

@pypingou, do you know of an update that has these kinds of tests that we can use to see how it renders?

I was testing this with an update for criu-3.3-5.fc26 and I had this change as well to test it:

+      request_results(base_url+"results/latest?original_spec_nvr="
+        +builds[index].replace('.f', '.fc')+"&testcases=org.centos.prod.ci.pipeline.complete");
@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou Oct 3, 2017

Member
Member

pypingou commented Oct 3, 2017

@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou Oct 3, 2017

Member

Small screenshot to give an idea of what it looks now:

screenshot from 2017-10-03 15-41-09

Member

pypingou commented Oct 3, 2017

Small screenshot to give an idea of what it looks now:

screenshot from 2017-10-03 15-41-09

@centos-ci

This comment has been minimized.

Show comment
Hide comment
@centos-ci

centos-ci Oct 3, 2017

Collaborator

This pull request fails CI testing. Please review the Jenkins job.

Collaborator

centos-ci commented Oct 3, 2017

This pull request fails CI testing. Please review the Jenkins job.

@centos-ci

This comment has been minimized.

Show comment
Hide comment
@centos-ci

centos-ci Oct 3, 2017

Collaborator

This pull request fails CI testing. Please review the Jenkins job.

Collaborator

centos-ci commented Oct 3, 2017

This pull request fails CI testing. Please review the Jenkins job.

@kparal

If I understand this correctly from the screenshot, for every NVR section there will be also NVR#hash section containing a single centosci testcase (if some result is present).

This makes the bodhi result page a bit longer, but on the other hand it's more correct - the specific commit was tested using internal centosci build, and not the regular koji build. So I think it's even better this way.

Show outdated Hide outdated bodhi/server/templates/update.html
Show outdated Hide outdated bodhi/server/templates/update.html
@kparal

This comment has been minimized.

Show comment
Hide comment
@kparal

kparal Oct 3, 2017

Contributor

One further note. If you add the item+type to results soon, please don't forget to adjust this code. The queries should be faster with item than with original_spec_nvr, I believe, because that's the common case we've optimized for.

Contributor

kparal commented Oct 3, 2017

One further note. If you add the item+type to results soon, please don't forget to adjust this code. The queries should be faster with item than with original_spec_nvr, I believe, because that's the common case we've optimized for.

@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou Oct 3, 2017

Member

One further note. If you add the item+type to results soon, please don't forget to adjust this code

I'll wait for @tflink to comment on this before I change the current code, he definitely knows more than me so I suspect there might have been a good reason for this design choice.

Member

pypingou commented Oct 3, 2017

One further note. If you add the item+type to results soon, please don't forget to adjust this code

I'll wait for @tflink to comment on this before I change the current code, he definitely knows more than me so I suspect there might have been a good reason for this design choice.

@kparal

kparal approved these changes Oct 3, 2017

Ack from me for the current code

@centos-ci

This comment has been minimized.

Show comment
Hide comment
@centos-ci

centos-ci Oct 3, 2017

Collaborator

This pull request fails CI testing. Please review the Jenkins job.

Collaborator

centos-ci commented Oct 3, 2017

This pull request fails CI testing. Please review the Jenkins job.

@tflink

This comment has been minimized.

Show comment
Hide comment
@tflink

tflink Oct 3, 2017

item+type is a taskotron-ism and the reporter code wasn't written with taskotron in mind. Unless @kparal knows something I don't, there is no difference in query optimization for any of the key-value pair fields (item, item_type, original_spec_nvr etc.).

I don't really see a reason why item+item_type is needed but I don't think it'd actively hurt anything if there's a good reason to make that change.

tflink commented Oct 3, 2017

item+type is a taskotron-ism and the reporter code wasn't written with taskotron in mind. Unless @kparal knows something I don't, there is no difference in query optimization for any of the key-value pair fields (item, item_type, original_spec_nvr etc.).

I don't really see a reason why item+item_type is needed but I don't think it'd actively hurt anything if there's a good reason to make that change.

@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou Oct 3, 2017

Member

I was thinking about this a little more:

type=dist_git_commit
item=modules/dhcp#5c6331592b6c92bf7c52a2ac735f92cb8fc11304

Seems like the best actual item+type combination except that if we uses this, bodhi won't be able to query for it anyway since it doesn't know the hash.

Tbh, at this stage, I'm tempted to let the structure as is unless there is a real benefit to change it (such as optimization or so).

Member

pypingou commented Oct 3, 2017

I was thinking about this a little more:

type=dist_git_commit
item=modules/dhcp#5c6331592b6c92bf7c52a2ac735f92cb8fc11304

Seems like the best actual item+type combination except that if we uses this, bodhi won't be able to query for it anyway since it doesn't know the hash.

Tbh, at this stage, I'm tempted to let the structure as is unless there is a real benefit to change it (such as optimization or so).

@rajcze

This comment has been minimized.

Show comment
Hide comment
@rajcze

rajcze Oct 4, 2017

There are no optimizations for item/type searches in resultsdb (or at least I don't know of any). The only reason to have those would be consistency (aka taskotronism), but if the item is unique anyway, I don't see any added benefit in doing that.

rajcze commented Oct 4, 2017

There are no optimizations for item/type searches in resultsdb (or at least I don't know of any). The only reason to have those would be consistency (aka taskotronism), but if the item is unique anyway, I don't see any added benefit in doing that.

@@ -341,6 +344,8 @@
$("#tab-automatedtests").append(" <span class='fa fa-spinner fa-spin'></span>");
for (index = 0, len = builds.length; index < len; ++index) {
request_results(base_url+"results/latest?original_spec_nvr="
+builds[index]+"&testcases=org.centos.prod.ci.pipeline.complete");
request_results(base_url+"results/latest?item="+builds[index]+"&type=koji_build&testcases:like=dist.*");

This comment has been minimized.

@rajcze

rajcze Oct 4, 2017

Just a small thing - please add since= parameter in ISO8601 format (UTC without tz) to limit the search for the request_results. Bodhi IIRC knows when the update was created (or something like that), so set it to that (or any other reasonable) value, so resultsdb does not need to crawl the whole db in order to find that "for some testcases, there is no result". Thanks!

@rajcze

rajcze Oct 4, 2017

Just a small thing - please add since= parameter in ISO8601 format (UTC without tz) to limit the search for the request_results. Bodhi IIRC knows when the update was created (or something like that), so set it to that (or any other reasonable) value, so resultsdb does not need to crawl the whole db in order to find that "for some testcases, there is no result". Thanks!

This comment has been minimized.

@kparal

kparal Oct 4, 2017

Contributor

The builds were tested in centosci before the update was created, so I'm afraid there's no easy value to pass to since=. We could limit showing results to builds tested in the past 2 weeks, or a month, but it might be... limiting. Probably best to introduce that if we have performance issues.

We use since= when requesting results for type=bodhi_update, but we don't use it when requesting results for type=koji_build (see, another nice example of why conventions are good). And this is the same case as koji builds, I believe.

@kparal

kparal Oct 4, 2017

Contributor

The builds were tested in centosci before the update was created, so I'm afraid there's no easy value to pass to since=. We could limit showing results to builds tested in the past 2 weeks, or a month, but it might be... limiting. Probably best to introduce that if we have performance issues.

We use since= when requesting results for type=bodhi_update, but we don't use it when requesting results for type=koji_build (see, another nice example of why conventions are good). And this is the same case as koji builds, I believe.

This comment has been minimized.

@rajcze

rajcze Oct 4, 2017

Sure, that's where "something reasonable" goes. We already do have perf issues, and I'll be adding a "data since last month" defualt constraint to resultsdb anyway real-time-soon-now.

But if you don't mind sometimes waiting for a couple of minutes, before the data loads (if it loads), we could go with the unlimited approach.

@rajcze

rajcze Oct 4, 2017

Sure, that's where "something reasonable" goes. We already do have perf issues, and I'll be adding a "data since last month" defualt constraint to resultsdb anyway real-time-soon-now.

But if you don't mind sometimes waiting for a couple of minutes, before the data loads (if it loads), we could go with the unlimited approach.

This comment has been minimized.

@pypingou

pypingou Oct 4, 2017

Member

Doesn't using the /latest endpoint limit the results by default? (which should mean it's not scrolling the entire DB)

@pypingou

pypingou Oct 4, 2017

Member

Doesn't using the /latest endpoint limit the results by default? (which should mean it's not scrolling the entire DB)

This comment has been minimized.

@rajcze

rajcze Oct 4, 2017

@pypingou I understand the confusion, but no. /latest limits the data output, not the database contents. Say you query for ?item=foo&testcases=moo,bar. It does something like select * from results where item=foo and testcase=moo + select * from results where item=foo and testcase=bar (disclaimer: this is far more complicated in fact, but on a high level is what is happening).
In this example, if the foo/bar pair is not present, the only way (for any db) to say "there is none" is to go through all the data (or a subset of data limited by index).
So the queries are limited in some way, but between the amount of data in resultsdb, the insufficient hardware the db has, and the postgres' query planner, looking for sparse data is quite slow. Thus narrowing the interval can help.
This is not needed, nor required, but take it as a tip from an insider :)

@rajcze

rajcze Oct 4, 2017

@pypingou I understand the confusion, but no. /latest limits the data output, not the database contents. Say you query for ?item=foo&testcases=moo,bar. It does something like select * from results where item=foo and testcase=moo + select * from results where item=foo and testcase=bar (disclaimer: this is far more complicated in fact, but on a high level is what is happening).
In this example, if the foo/bar pair is not present, the only way (for any db) to say "there is none" is to go through all the data (or a subset of data limited by index).
So the queries are limited in some way, but between the amount of data in resultsdb, the insufficient hardware the db has, and the postgres' query planner, looking for sparse data is quite slow. Thus narrowing the interval can help.
This is not needed, nor required, but take it as a tip from an insider :)

This comment has been minimized.

@kparal

kparal Oct 4, 2017

Contributor

@rajcze So what do you think it's the reasonable value here? Is it better to fall back to the default value you'll be soon adding (so e.g. a month old bodhi updates will stop displaying results), or is it better to use a custom since= with a timestamp e.g. 2-4 weeks prior to the update creation? (The builds might be available and tested easily 2 weeks before the maintainer decides to submit them as an update).

Btw, we should use the same approach for type=koji_build query, not just this centosci one.

@kparal

kparal Oct 4, 2017

Contributor

@rajcze So what do you think it's the reasonable value here? Is it better to fall back to the default value you'll be soon adding (so e.g. a month old bodhi updates will stop displaying results), or is it better to use a custom since= with a timestamp e.g. 2-4 weeks prior to the update creation? (The builds might be available and tested easily 2 weeks before the maintainer decides to submit them as an update).

Btw, we should use the same approach for type=koji_build query, not just this centosci one.

This comment has been minimized.

@pypingou

pypingou Oct 4, 2017

Member

While I think this is a good question and a chance we should look into (indeed not just for the Atomic CI but likely for other), do we want to bring this in another PR?

Also, the since = 2 weeks before the update means that in a year, resultsdb may still have to go through 1 year + 2 weeks of data. Would an upper bound help?
Something like since <update - 2 weeks> until <update + 1 week> ?

@pypingou

pypingou Oct 4, 2017

Member

While I think this is a good question and a chance we should look into (indeed not just for the Atomic CI but likely for other), do we want to bring this in another PR?

Also, the since = 2 weeks before the update means that in a year, resultsdb may still have to go through 1 year + 2 weeks of data. Would an upper bound help?
Something like since <update - 2 weeks> until <update + 1 week> ?

@kparal

This comment has been minimized.

Show comment
Hide comment
@kparal

kparal Oct 4, 2017

Contributor

Unless @kparal knows something I don't, there is no difference in query optimization for any of the key-value pair fields (item, item_type, original_spec_nvr etc.).

I was wrong, jskladan replied above.

I don't really see a reason why item+item_type is needed but I don't think it'd actively hurt anything if there's a good reason to make that change.

The reason is that the results consumer then needs to know peculiarities of each and every testcase, instead of relying on conventions common for Fedora results. Compare this approach:

results/latest?item=NVR&type=koji_build&testcases:like=dist.*

with this approach:

results/latest?testcases:like=dist.rpmlint&nvr=NVR
results/latest?testcases:like=dist.rpmgrill&item=NVR
results/latest?testcases:like=dist.abicheck&spec_nvr=NVR
results/latest?testcases:like=dist.rpmdeplint&tested=NVR
results/latest?testcases:like=dist.upgradepath&subject=NVR
...

This is harder to maintain for consumer tools, and slower (I won't speculate about DB load again, but slower it is). That's why I think Bodhi should insist on keeping the Fedora conventions for results that should be shown in Bodhi, just to keep the code sane.

However, all my arguments are moot at the moment, because:

Seems like the best actual item+type combination except that if we uses this, bodhi won't be able to query for it anyway since it doesn't know the hash.

More or less. It would improve things for any consumer wanting to show dist_git_commit type results (and it would allow maintainers to use our current resultsdb frontend to see per-commit results for their packages), and it wouldn't do any harm for Bodhi (you would still query by original_spec_nvr, or by using item prefix, e.g. item:like=rpms/dhcp#*), but it doesn't improve things for Bodhi either, for this moment. But once https://pagure.io/koji/issue/550 is completed, you could easily switch to querying by item=rpms/name#hash and receive exactly the one result you want. But it doesn't have to be done now, yes.

Contributor

kparal commented Oct 4, 2017

Unless @kparal knows something I don't, there is no difference in query optimization for any of the key-value pair fields (item, item_type, original_spec_nvr etc.).

I was wrong, jskladan replied above.

I don't really see a reason why item+item_type is needed but I don't think it'd actively hurt anything if there's a good reason to make that change.

The reason is that the results consumer then needs to know peculiarities of each and every testcase, instead of relying on conventions common for Fedora results. Compare this approach:

results/latest?item=NVR&type=koji_build&testcases:like=dist.*

with this approach:

results/latest?testcases:like=dist.rpmlint&nvr=NVR
results/latest?testcases:like=dist.rpmgrill&item=NVR
results/latest?testcases:like=dist.abicheck&spec_nvr=NVR
results/latest?testcases:like=dist.rpmdeplint&tested=NVR
results/latest?testcases:like=dist.upgradepath&subject=NVR
...

This is harder to maintain for consumer tools, and slower (I won't speculate about DB load again, but slower it is). That's why I think Bodhi should insist on keeping the Fedora conventions for results that should be shown in Bodhi, just to keep the code sane.

However, all my arguments are moot at the moment, because:

Seems like the best actual item+type combination except that if we uses this, bodhi won't be able to query for it anyway since it doesn't know the hash.

More or less. It would improve things for any consumer wanting to show dist_git_commit type results (and it would allow maintainers to use our current resultsdb frontend to see per-commit results for their packages), and it wouldn't do any harm for Bodhi (you would still query by original_spec_nvr, or by using item prefix, e.g. item:like=rpms/dhcp#*), but it doesn't improve things for Bodhi either, for this moment. But once https://pagure.io/koji/issue/550 is completed, you could easily switch to querying by item=rpms/name#hash and receive exactly the one result you want. But it doesn't have to be done now, yes.

@bowlofeggs

This comment has been minimized.

Show comment
Hide comment
@bowlofeggs

bowlofeggs Oct 4, 2017

Member
Member

bowlofeggs commented Oct 4, 2017

@TomasTomecek

This comment has been minimized.

Show comment
Hide comment
@TomasTomecek

TomasTomecek Oct 9, 2017

I think the best solution is to do the gating in Pagure

Do you suggest that pull request workflow would be then mandatory?

TomasTomecek commented Oct 9, 2017

I think the best solution is to do the gating in Pagure

Do you suggest that pull request workflow would be then mandatory?

@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou Oct 9, 2017

Member

I think the best solution is to do the gating in Pagure rather than in Bodhi.

Well this PR isn't about gating but showing the test results :)

It doesn't make sense to me to invest engineering effort for Bodhi to know the commit hashes when the end result is a system that still isn't ideal.

And we're not doing that presently :)

Member

pypingou commented Oct 9, 2017

I think the best solution is to do the gating in Pagure rather than in Bodhi.

Well this PR isn't about gating but showing the test results :)

It doesn't make sense to me to invest engineering effort for Bodhi to know the commit hashes when the end result is a system that still isn't ideal.

And we're not doing that presently :)

@bowlofeggs

This comment has been minimized.

Show comment
Hide comment
@bowlofeggs

bowlofeggs Oct 9, 2017

Member

@TomasTomecek I don't suggest making the PR process mandatory. Consider how projects on GitHub work, such as Bodhi. I can just make direct commits on the develop branch but I don't do that because I want my CI system to tell me if my change passes tests. (Technically it is against fedora-infra policy too, because we do actually require PR review, but that's tangential to my point - we don't require it for CI purposes.) The CI system itself is motivation for me to use the PRs. I think the same would be true for pull requests in Pagure.

Member

bowlofeggs commented Oct 9, 2017

@TomasTomecek I don't suggest making the PR process mandatory. Consider how projects on GitHub work, such as Bodhi. I can just make direct commits on the develop branch but I don't do that because I want my CI system to tell me if my change passes tests. (Technically it is against fedora-infra policy too, because we do actually require PR review, but that's tangential to my point - we don't require it for CI purposes.) The CI system itself is motivation for me to use the PRs. I think the same would be true for pull requests in Pagure.

@bowlofeggs

This comment has been minimized.

Show comment
Hide comment
@bowlofeggs

bowlofeggs Oct 9, 2017

Member

@pypingou Glad that we aren't planning to get commit hashes into Bodhi ☺

Member

bowlofeggs commented Oct 9, 2017

@pypingou Glad that we aren't planning to get commit hashes into Bodhi ☺

@bowlofeggs

I don't personally see a problem with this PR though I also am not particularly familiar with this code. Since @kparal is a +1 that satisfies me, as he is pretty familiar with this code. Thanks @pypingou, @kparal and @rajcze!

@bowlofeggs

This comment has been minimized.

Show comment
Hide comment
@bowlofeggs

bowlofeggs Oct 9, 2017

Member

I'm going to rebase this, as I think the test failure has been fixed (and is unrelated anyway).

Member

bowlofeggs commented Oct 9, 2017

I'm going to rebase this, as I think the test failure has been fixed (and is unrelated anyway).

Query resultsdb for the results of the Atomic CI pipeline and display…
… them

The way bodhi was querying resultsdb for test results was relying on the
specific structure of taskotron's results which lead to the results from
the Atomic CI pipeline to not be included.
With this commit we're querying for the specific structure of the Atomic
CI results so they are included in the "Automated tests" tab of the
update page.

Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>
@bowlofeggs

This comment has been minimized.

Show comment
Hide comment
@bowlofeggs
Member

bowlofeggs commented Oct 9, 2017

Thanks @pypingou!

@bowlofeggs bowlofeggs merged commit 8ad5414 into develop Oct 9, 2017

2 checks passed

DCO All commits have a DCO sign-off from the author
default Build finished.
Details

@bowlofeggs bowlofeggs deleted the atomic_ci_results branch Oct 9, 2017

@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou Oct 10, 2017

Member

Thank you :)

Member

pypingou commented Oct 10, 2017

Thank you :)

@bowlofeggs

This comment has been minimized.

Show comment
Hide comment
@bowlofeggs

bowlofeggs Oct 10, 2017

Member

This has been cherry-picked to the 2.12 branch as c67d4ce, and will be included in the upcoming 2.12.0 release.

Member

bowlofeggs commented Oct 10, 2017

This has been cherry-picked to the 2.12 branch as c67d4ce, and will be included in the upcoming 2.12.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment