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

Add CI jobs running through Zuul #7245

Closed
wants to merge 26 commits into from
Closed

Add CI jobs running through Zuul #7245

wants to merge 26 commits into from

Conversation

@mnaser
Copy link
Contributor

@mnaser mnaser commented Jun 11, 2021

This PR adds jobs to be running via Zuul instead of Travis CI.

Depends-On: #7248

@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 11, 2021

It seems that Zuul has complained here because there is a conflict on .travis.yml. I'll fix that shortly.

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Jun 11, 2021

Congratulations 🎉. DeepCode analyzed your code in 0.996 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 11, 2021

recheck

@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 11, 2021

Just had to play with the configs a little bit, the jobs are now running, let's see how they go.

Note, with Zuul, you can simply type recheck which will retrigger the jobs, or alternatively, simply use the GitHub UI with the "Re-run" button.

@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 12, 2021

Everything seems to be passing so far, except for the ngtcp2 job which is failing with this:

  [ 13%] Built target ssl
  CMakeFiles/Makefile2:80: recipe for target 'CMakeFiles/bssl.dir/rule' failed
  Makefile:118: recipe for target 'bssl' failed

  --- stderr
  clang: error: unsupported argument '-g' to option 'Wa,'
  make[3]: *** [CMakeFiles/crypto.dir/linux-x86_64/crypto/chacha/chacha-x86_64.S.o] Error 1
  make[2]: *** [CMakeFiles/crypto.dir/all] Error 2
  make[2]: *** Waiting for unfinished jobs....
  make[1]: *** [CMakeFiles/bssl.dir/rule] Error 2
  make: *** [bssl] Error 2
  thread 'main' panicked at '
  command did not execute successfully, got: exit code: 2

  build script failed, must exit now', /home/zuul/.cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.45/src/lib.rs:894:5
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Has this job worked before, maybe the OVERRIDE_CC and OVERRIDE_GCC stuff is actually put in use (never saw any references to it in the codebase)? Perhaps @albinvass could help out.

zuul.d/jobs.yaml Outdated Show resolved Hide resolved
@bagder
Copy link
Member

@bagder bagder commented Jun 12, 2021

Sure, the ngtcp2 jobs worked on travis. See a recent round. It also works locally when I try it. It doesn't need clang, I usually build it (and its dependencies) with gcc on my dev machine.

@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 12, 2021

So now we've got only scan-build jobs failing:

scan-build: 3 bugs found.
scan-build: Run 'scan-view /tmp/scan-build-2021-06-12-142328-18158-1' to examine bug reports.

The main reason is that with Zuul, it actually checks out the main branch, puts the PR on top of it and tests it (so we're really testing what will happen once we merge). This probably also means that the main branch had a change that merged with scan-build not passing, or that we did something wrong, but I don't think we touched the code. :)

@bagder
Copy link
Member

@bagder bagder commented Jun 12, 2021

I've added a commit that might work.

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

I checked out the scan build warnings and unfortunately the other ones you get in the zuul builds are false positives because of a very old scan-build version! disappointed You have version 6 there, I use version 11 on my debian sid install.

Well since it's a python package we should be able to easily install a newer version with pip.

@bagder
Copy link
Member

@bagder bagder commented Jun 12, 2021

scan-build is based on clang, so it'll carry with it a lot of non-python things surely?

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

Apparently it's only intercepting calls to gcc/clang and it's only prerequisite is that clang is already installed (except for python):
https://github.com/rizsotto/scan-build

and at the top the repository is telling people to install the tool with pip

@bagder
Copy link
Member

@bagder bagder commented Jun 12, 2021

Apparently it's only intercepting calls to gcc/clang and it's only prerequisite

I think that's wrong scan-build. Mine comes from the clang package:

$ dpkg -S /usr/bin/scan-build-11
clang-tools-11: /usr/bin/scan-build-11
@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

I think you're right about that. I'll do some digging and see if we can install a newer version without too much effort.

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

Looks like there are two options. We can either continue using ubuntu-bionic and install clang-tools-10 and see if that works, or we can run tests on ubuntu-focal instead that has clang-tools-11 available.

@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 12, 2021

Looks like there are two options. We can either continue using ubuntu-bionic and install clang-tools-10 and see if that works, or we can run tests on ubuntu-focal instead that has clang-tools-11 available.

Looks like it's working with 10! 😀

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

Looks like it's working with 10! grinning

I'm not sure, @bagder also introduced a workaround in the dependency so we haven't really tested it yet.

@bagder
Copy link
Member

@bagder bagder commented Jun 12, 2021

I presume it will only show up as a single job in the list on GitHub and not one per job? I think it would benefit from getting a more distinct name than curl/check. How about zuul/builds ?

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

I presume it will only show up as a single job in the list on GitHub and not one per job? I think it would benefit from getting a more distinct name than curl/check. How about zuul/builds ?

Yes, zuul reports one github check for the name of the pipeline, and there are reasons for that explained in the design decisions https://zuul-ci.org/docs/zuul/discussion/github-checks-api.html#design-decisions.

If you want to we can of course change the name of the pipeline since the name 'check' is only a convention.

Normally in zuul you'd also have something called a 'gate' pipeline where changes are added to a queue to be merged in order while they're also tested in parallel. This is to ensure that no two conflicting changes lands on the target branch which would leave the repository in a broken state.
You can read about how that works here:
https://zuul-ci.org/docs/zuul/discussion/gating.html#project-gating
or see it in action at openstack:
https://zuul.opendev.org/t/openstack/status

@bagder
Copy link
Member

@bagder bagder commented Jun 12, 2021

Normally in zuul you'd also have something called a 'gate' pipeline where changes are added to a queue to be merged in order while they're also tested in parallel

I bet that's very cool but as we're using a more CI systems than I have fingers on one hand, we can't easily use fancy features specific to one of the services.

@bagder
Copy link
Member

@bagder bagder commented Jun 12, 2021

we can of course change the name of the pipeline since the name 'check' is only a convention.

As you can see we're on 50+ something names in that list.Having 'curl' mentioned in there is completely pointless. It would be better to say what service it is. Saying 'check' is similarly pointless, and it could perhaps say "XX-jobs" or something where XX could be the total number of jobs so that a failure on that line could be easier to figure out that it could be one failure or up to XX ones.

(Many of the other exiuting names are also really crappy right now but I'm planning on working on them too going forward.)

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

As you can see we're on 50+ something names in that list.Having 'curl' mentioned in there is completely pointless. It would be better to say what service it is. Saying 'check' is similarly pointless, and it could perhaps say "XX-jobs" or something where XX could be the total number of jobs so that a failure on that line could be easier to figure out that it could be one failure or up to XX ones.

One possibility is to create one pipeline per job and let the name of the pipeline be the same as the job running in it. It's a bit more effort to maintain, but if the jobs don't change very often I don't think it should be too cumbersome. You'd still need two clicks to get to the logs but it would be possible to see exactly what job failed. If we're not looking at gating anyway I don't think there should be any issues.
@mnaser what do you think about doing something like that?

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 12, 2021

Normally in zuul you'd also have something called a 'gate' pipeline where changes are added to a queue to be merged in order while they're also tested in parallel

I bet that's very cool but as we're using a more CI systems than I have fingers on one hand, we can't easily use fancy features specific to one of the services.

Just to be clear, I think letting zuul gate the project would work well along other systems and it would give the project an extra bit of stability by making sure tests run before merging and not just when a PR is proposed. The biggest difference for a maintainer would be that instead of approving and merging a PR, they would instead only approve the PR. That would in turn trigger a second round of tests and zuul would only merge the PR if the tests are successful.

I'm not going to push this of course, just pointing out that it's an option that exists and something I'd recommend.

@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 12, 2021

As you can see we're on 50+ something names in that list.Having 'curl' mentioned in there is completely pointless. It would be better to say what service it is. Saying 'check' is similarly pointless, and it could perhaps say "XX-jobs" or something where XX could be the total number of jobs so that a failure on that line could be easier to figure out that it could be one failure or up to XX ones.

One possibility is to create one pipeline per job and let the name of the pipeline be the same as the job running in it. It's a bit more effort to maintain, but if the jobs don't change very often I don't think it should be too cumbersome. You'd still need two clicks to get to the logs but it would be possible to see exactly what job failed. If we're not looking at gating anyway I don't think there should be any issues.
@mnaser what do you think about doing something like that?

Sure, there's a few options we have. We can either rename the pipeline which would would allow us to rename/change the part after curl, so we can have something like

curl/vexxhost-ci

or some other name that we want to come up with? Alternatively, I'm not sure if there's a way of changing the entire name of the pipeline name being reported without changing the tenant or pipeline name.

/me digs into Zuul's source code

@jay jay added the CI label Jun 13, 2021
@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 13, 2021

I presume it will only show up as a single job in the list on GitHub and not one per job?

This is obviously a recurring issue with zuuls github ux. I have an idea how we can improve the way zuul reports results so each job is visible as a check while still allowing the same behavior, I'll see if I can find some time to write up a proposal for the project.

That can take a while for me to get to so should we go with the option of renaming our one existing pipeline, or create a pipeline per job so they all show up as a github check? The second option hangs a bit more on how often new jobs are added.

@bagder
Copy link
Member

@bagder bagder commented Jun 13, 2021

I'm getting ready to merge this. There are but two things I'd like to see first:

  1. How about putting the files from platbooks/ into the zuul.d directory so there's one dir for all Zuul configs?
  2. We want to restrict the CI jobs to only run for PRs or commits to master, not any other branches.
@mnaser
Copy link
Contributor Author

@mnaser mnaser commented Jun 13, 2021

I'm getting ready to merge this. There are but two things I'd like to see first:

  1. How about putting the files from platbooks/ into the zuul.d directory so there's one dir for all Zuul configs?

That's done!

  1. We want to restrict the CI jobs to only run for PRs or commits to master, not any other branches.

By default, Zuul only runs against PRs and merges against protected branches inside GitHub (which I believe master is!)

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 14, 2021

I'm getting ready to merge this. There are but two things I'd like to see first:

  1. How about putting the files from platbooks/ into the zuul.d directory so there's one dir for all Zuul configs?

That's done!

There needs to be a .zuul.ignore in zuul.d/playbooks so zuul doesn't load it as configuration. I'll push a fix :)

By default, Zuul only runs against PRs and merges against protected branches inside GitHub (which I believe master is!)

We can however further limit that so it does not trigger on PRs to any other protected branches.

@bagder
Copy link
Member

@bagder bagder commented Jun 14, 2021

Awesome, thanks for all this. I suppose this is now ready to merge, right?

@albinvass
Copy link
Contributor

@albinvass albinvass commented Jun 14, 2021

Awesome, thanks for all this. I suppose this is now ready to merge, right?

I think so. Any other concerns should be able to be solved in a separate PR. I'll continue supporting this for a while when I got time over, but if you experience any issues you can always connect to #zuul on OFTC.

We've had some other ideas how to improve package management etc for the different test environments while working on this so we may propose some PRs to follow up on that too :)

@bagder
Copy link
Member

@bagder bagder commented Jun 14, 2021

Thanks!

@bagder bagder closed this in 63583a0 Jun 14, 2021
bagder added a commit that referenced this pull request Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants