Skip to content
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

[CI:BUILD] Packit: add copr_build task #15549

Closed
wants to merge 1 commit into from

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Aug 30, 2022

The production_build task can only be triggered when users with write
access submit PRs or add slash-packit production_build to PRs from
external contributors. This is because of koji's restriction to build
only verified code.

This could mean quite some manual intervention but would be really useful
for release PRs, cherry-picks and any other PRs where we could use extra
confirmation.

This commit adds a new copr_build task which gets triggered
automatically regardless of the PR submitter's privs.

Signed-off-by: Lokesh Mandvekar [email protected]

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Aug 30, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2022
@lsm5 lsm5 force-pushed the packit-copr-build-task branch from 4e9d6ff to 80f458c Compare August 30, 2022 15:27
@packit-as-a-service
Copy link

Failed to load packit config file:

Cannot load package config '.packit.yaml'. found undefined alias 'production_dist_targets'
  in "<unicode string>", line 35, column 14:
        targets: *production_dist_targets
                 ^

For more info, please check out the documentation or contact the Packit team.

@lsm5
Copy link
Member Author

lsm5 commented Aug 30, 2022

The wait time for copr-build could become an issue. I'm checking with the packit people about it.

@lsm5 lsm5 marked this pull request as ready for review August 30, 2022 17:43
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2022
@lsm5
Copy link
Member Author

lsm5 commented Aug 30, 2022

@edsantiago @cevich alright, so copr_build tasks work. They take a while but not longer than the Cirrus tests. So, I'll say this is good to go. PTAL

@lsm5 lsm5 changed the title Packit: add copr_build task [CI:BUILD] Packit: add copr_build task Aug 30, 2022
@cevich
Copy link
Member

cevich commented Aug 30, 2022

They take a while

Not a blocker, but something to consider:

Is that going to hold up (for example) a [CI:DOCS] build? If so, that suggests we probably don't want to make the checks mandatory for PR-merge. Which is kind of unfortunate since it dulls the "teeth" of having the check in the first place 😞

Ref. details: Currently (IIRC) PRs are blocked from merging based on the Total Success task. That's needed as an "status accumulator" since we cannot mark a check with a changing name as "required" for PR merge. i.e. rpm-build:fedora-37-x86_64 is going to change names with every new release.

So if we want these to be mandatory-passing for a PR merge, one idea is: Add a script to the Total success task such that:

  1. Query the event source, if it wasn't a PR or has the [CI:DOCS] in the title, exit(0).
  2. Otherwise, query the github status API for all rpm-build:* checks, blocking with a timeout until they report results.
  3. Exit non-zero if timeout or if any rpm-build:* check fails.

@lsm5
Copy link
Member Author

lsm5 commented Aug 30, 2022

Right now, none of the packit tasks are blockers. And, I'm thinking we should let this bake a while until they become blockers.

Not a blocker, but something to consider:

Is that going to hold up (for example) a [CI:DOCS] build? If so, that suggests we probably don't want to make the checks mandatory for PR-merge. Which is kind of unfortunate since it dulls the "teeth" of having the check in the first place disappointed

@cevich hmm, it will not block the success task. Btw, are manpage updates tagged [CI:DOCS]? If so, I think it's still worth having these jobs run on docs PRs too, but if people are in a rush to get them merged, we can ignore the tasks.

@cevich
Copy link
Member

cevich commented Aug 30, 2022

SGTM

@cevich
Copy link
Member

cevich commented Aug 30, 2022

Clearification: When total success goes green, that's the signal github uses to make the "Merge Pull Request" button green.

@edsantiago
Copy link
Member

What problem are we trying to solve here? And, is it worth this large a hammer? The size of the CI window is making me deeply upset. Even with my colorizer, the list is now unmanageably long, really hard to scroll through (which, yes, has to be done sometimes). And I don't even want to think about flakes on a list this big. What sort of problems is this likely to catch, and is it really worth imposing this kind of burden on every developer? Could/should this be done nightly instead?

@lsm5 lsm5 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2022
@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

What problem are we trying to solve here? And, is it worth this large a hammer? The size of the CI window is making me deeply upset. Even with my colorizer, the list is now unmanageably long, really hard to scroll through (which, yes, has to be done sometimes). And I don't even want to think about flakes on a list this big. What sort of problems is this likely to catch, and is it really worth imposing this kind of burden on every developer? Could/should this be done nightly instead?

See: #15537 (comment)

I didn't realize the write permissions issue when I added the production-build task earlier. The copr-build is a better option if we want builds to run automatically for every PR submitter. So, if we want to avoid dup tasks and also avoid manual intervention to trigger builds, we should only keep copr-build for every PR.

The older production-build task trigger can be changed to release instead of pull-request so it only gets triggered on new release tags, or it can be removed altogether.

@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

Also, while the copr-build may seem like a huge tasklist, it's effectively the same as the builds done by the production-build task. For copr-build, each architecture needs to specified or else it will just assume only x86_64.

@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

Clearification: When total success goes green, that's the signal github uses to make the "Merge Pull Request" button green.

So, that doesn't depend on any packit task right now, correct?

@lsm5 lsm5 marked this pull request as draft August 31, 2022 14:07
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2022
@lsm5 lsm5 force-pushed the packit-copr-build-task branch from 2936aac to e8c5460 Compare August 31, 2022 15:17
@packit-as-a-service
Copy link

Failed to load packit config file:

Cannot parse package config: ValidationError({'jobs': {0: {'env': ['Not a valid mapping type.']}, 1: {'env': ['Not a valid mapping type.']}, 2: {'env': ['Not a valid mapping type.']}}}).

For more info, please check out the documentation or contact the Packit team.

@lsm5 lsm5 force-pushed the packit-copr-build-task branch from e8c5460 to 8efac22 Compare August 31, 2022 15:21
@packit-as-a-service
Copy link

Failed to load packit config file:

Cannot parse package config: ValidationError({'jobs': {2: {'env': ['Not a valid mapping type.']}}}).

For more info, please check out the documentation or contact the Packit team.

The production_build task can only be triggered when users with write
access submit PRs or add `slash-packit production_build` to PRs from
external contributors. This is because of koji's restriction to build
only `verified code`.

This could mean quite some manual intervention but would be really useful
for release PRs, cherry-picks and any other PRs where we could use extra
confirmation.

This commit adds a new `copr_build` task which gets triggered
automatically regardless of the PR submitter's privs.

[NO NEW TESTS NEEDED]

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the packit-copr-build-task branch from 8efac22 to 77a3e66 Compare August 31, 2022 15:22
@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

@edsantiago @cevich : with the new changes, every fedora version task will build with the spec file from its own branch. Also, I've disabled production_build until we find a good use case for it.

It should not add any more burden than what we had with production_build.

@lsm5 lsm5 marked this pull request as ready for review August 31, 2022 15:40
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2022
@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

rpm-build:fedora-36-1386 has actually completed though something seems to be off with the final status reporting. I've pinged the packit people to take a look.

@edsantiago
Copy link
Member

Thanks. I've been wondering about that. I'm still really uncomfortable with all the unknown flakes and problems that this new system is going to impose.

@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

Thanks. I've been wondering about that. I'm still really uncomfortable with all the unknown flakes and problems that this new system is going to impose.

@edsantiago do you mean packit in general or specifically with the copr_build task (v/s the former production_build task) ?

@edsantiago
Copy link
Member

I mean all of this. It's new, and complicated, and lots of uncertainties.

Like, the duplication. I'm trying to find a way to remove all the duplication here, and my focus has been on using the fedora-development and fedora-latest-stable aliases, but am about to give it up as hopeless: (1) there doesn't seem to be a way to variableize the versions in a way that can be injected into the specfile curl, and (2) I can't figure out if those aliases include all arches.

My hunch is that this will very-infrequently highlight real problems with a PR, but very-frequently cause flakes and hassles and delays for everyone. I may be wrong, and I'm not going to block it, but I'm not going to be the one to lgtm it either. Sorry. I still think this all would be better as a nightly cron job.

@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

I mean all of this. It's new, and complicated, and lots of uncertainties.

If you're having reservations with this PR, maybe it's best I do another PR to comment out the existing production_build tasks.

Like, the duplication. I'm trying to find a way to remove all the duplication here, and my focus has been on using the fedora-development and fedora-latest-stable aliases, but am about to give it up as hopeless: (1) there doesn't seem to be a way to variableize the versions in a way that can be injected into the specfile curl, and (2) I can't figure out if those aliases include all arches.

Yes, I tried a few things, and none worked well enough that's why I listed them explicitly. fedora-all didn't list fedora-37 at least as of yesterday. And I don't think there are aliases for arches, so unless those are mentioned, it will assume x86_64.

My hunch is that this will very-infrequently highlight real problems with a PR, but very-frequently cause flakes and hassles and delays for everyone. I may be wrong, and I'm not going to block it, but I'm not going to be the one to lgtm it either. Sorry. I still think this all would be better as a nightly cron job.

Would you be cool with this first getting into a lower-traffic repo, maybe netavark / aardvark-dns or some other?

RE: cron job I couldn't find anything in the docs, but I'll check with the packit team tomorrow. Though I would still prefer this be run on every PR so we don't have to go bisecting for failures.

@edsantiago
Copy link
Member

I think a lower-traffic repo would be wise. But I really want to step back here and let others weigh in, because I think this is boiling down to a question of priorities. AIUI your priority is to get this rolling immediately, and stop specfile-problems from making their way to you at the last minute. My priority is minimizing hassle to the world of contributors. Not only hassles like the i386 job that has been hung for two hours, hassles that we can't even imagine yet because we have no experience with any of this new system.

I think this system is promising, and has the potential for good. I share your desire to catch specfile problems early.

@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

@edsantiago ok, so I'm thinking:

  1. Comment out the existing production_build tasks from podman so they don't get run at all.

  2. Defer this PR and do a new aardvark-dns and netavark PRs with copr_build tasks and see how things go, then revisit this current PR, or step up to skopeo and buildah and then finally get to podman.

Are you cool with these 2 steps?

  1. [STRETCH] I think there might also be value in adding packit jobs to c/image, c/storage etc to check if any PR can be vendored into skopeo / buildah / podman resulting in a successful rpm build. But that can be for later.

lsm5 added a commit to lsm5/podman that referenced this pull request Aug 31, 2022
There are concerns with Packit causing flakes and delays on Podman so
let's have Packit prove itself in other repos and only then make its way
into Podman.

See: containers#15549 (comment)

[NO NEW TESTS NEEDED]

Signed-off-by: Lokesh Mandvekar <[email protected]>
@cevich
Copy link
Member

cevich commented Aug 31, 2022

stop specfile-problems from making their way to you at the last minute

I wonder if a daily-job would cover that "good enough".

RE: cron job I couldn't find anything in the docs, but I'll check with the packit team tomorrow.

Cirrus-CI (and many/most CI systems) have some kind of API. Frequently they support triggering jobs, and very often this can be done with a fairly simple curl command. Arranging for the daily cirrus-cron tasks to call a curl command for packet or copr should be trivial.

If there's also a job-status API, that could be added so as to represent any failures into the cirrus-task. That then would automatically show up on the podman-monitor mails.

Just sayin'

@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

@cevich na, let's get this done in other smaller repos first and then resume this. I'm not sure how to do cron natively in packit yet and then if we have cirrus depend on packit prematurely, that might cause other troubles.

@lsm5
Copy link
Member Author

lsm5 commented Aug 31, 2022

Closing for now..

@lsm5 lsm5 closed this Aug 31, 2022
@lsm5 lsm5 deleted the packit-copr-build-task branch March 16, 2023 11:25
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants