jobs-builder: add PR jobs for Fedora 35 #419

wainersm · 2022-01-24T18:54:52Z

This created the backbone to generate pull request and baseline jobs
with Jenkins Job Builder. Currently the yaml files are configured to
generate PR and baseline jobs for Fedora 35.

The following new jobs will be generated:

kata-containers-fedora-35-x86_64-CRIO_K8S-PR
kata-containers-fedora-35-x86_64-CRIO_K8S_MINIMAL-PR
kata-containers-main-fedora-35-x86_64-CRIO_K8S-baseline
kata-containers-main-fedora-35-x86_64-CRIO_K8S_MINIMAL-baseline
tests-fedora-35-x86_64-CRIO_K8S-PR
tests-fedora-35-x86_64-CRIO_K8S_MINIMAL-PR

Fixes #389
Signed-off-by: Wainer dos Santos Moschetta [email protected]

wainersm · 2022-01-24T19:32:59Z

@jodh-intel @fidencio this is a continuation of #406. I'm adding jobs for Fedora 35 but the most important thing are the two added templates which can be used to generate PR and baseline jobs. With this merged I expect that migrating the jobs ( #407) to be straightforward.

@GabyCT @devimc mind to review this PR? I basically copied the properties and configuration of the existing Ubuntu jobs (either PR and baseline). The only change that I made (and I am not so sure) is the use of the ci_entry_point.sh script on both baseline and PR jobs, but I've seen that script being used only on baselines. Does it work fine with PR jobs too?

I already published two jobs on our Jenkins just for the sake of testing. You can have a look how they will end up generated:
http://jenkins.katacontainers.io/job/kata-containers-main-fedora-35-x86_64-CRIO_K8S-baseline
http://jenkins.katacontainers.io/job/tests-fedora-35-x86_64-CRIO_K8S-PR/

devimc

thanks @wainersm - I've left some comments

jobs-builder/jobs/include/ci_entrypoint.sh.inc

devimc · 2022-01-24T21:07:36Z

jobs-builder/jobs/include/ci_entrypoint.sh.inc

+
+curl -OL https://raw.githubusercontent.com/kata-containers/tests/main/.ci/ci_entry_point.sh
+export DEBUG=true
+export CI_JOB="{ci_job}"


missing $

@devimc thanks for reviewing it! The Jenkins Job Builder will interpret a value enclosed in curly brackets as a template variable. Example, suppose I set ci_jo: foobar somewhere on the yaml file, that will result on the line export CI_JOB="{ci_job}".

If you don't want it to interpret the value as a template variable then you can scape (use double curly brackets). That answers your next comment; ${{GOPATH}} becomes ${GOPATH}.

devimc · 2022-01-24T21:09:17Z

jobs-builder/jobs/pr.yaml

+
+              export GOPATH=$WORKSPACE/go
+              export GOROOT="/usr/local/go"
+              export PATH="${{GOPATH}}/bin:/usr/local/go/bin:/usr/sbin:/usr/local/bin:${{PATH}}"


why double { here and only one { in the following line ?

jodh-intel

{l}-{g}-{t}-{m}{ }{!} { :-) } <= Not a fork bomb, honest! ;)

wainersm · 2022-01-25T17:36:23Z

{l}-{g}-{t}-{m}{ }{!} { :-) } <= Not a fork bomb, honest! ;)

Hehehehhe thanks for reviewing @jodh-intel !

This created the backbone to generate pull request and baseline jobs with Jenkins Job Builder. Currently the yaml files are configured to generate PR and baseline jobs for Fedora 35. The following new jobs will be generated: kata-containers-fedora-35-x86_64-CRIO_K8S-PR kata-containers-fedora-35-x86_64-CRIO_K8S_MINIMAL-PR kata-containers-main-fedora-35-x86_64-CRIO_K8S-baseline kata-containers-main-fedora-35-x86_64-CRIO_K8S_MINIMAL-baseline tests-fedora-35-x86_64-CRIO_K8S-PR tests-fedora-35-x86_64-CRIO_K8S_MINIMAL-PR Fixes kata-containers#389 Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

wainersm · 2022-01-25T17:55:41Z

Updates to this PR:

Moved the shebang to the first line (@devimc)
I realized the baseline jobs would be running 3x per day. Changed the timed value to 'H 0 * * *'

@jodh-intel @GabyCT maybe don't need to approve again.

jodh-intel

Thanks @wainersm. A few nits but overall,

lgtm

jodh-intel · 2022-01-26T11:13:03Z

jobs-builder/jobs/include/ci_entrypoint.sh.inc

+set -e
+set -x


Nit: Might be worth adding the usual header:

set -o errexit set -o nounset set -o pipefail [ -n "$BASH_VERSION" ] && set -o errtrace [ -n "${DEBUG:-}" ] && set -o xtrace

This script is just a thin wrapper to https://raw.githubusercontent.com/kata-containers/tests/main/.ci/ci_entry_point.sh , which in turn adds all those flags :)

jodh-intel · 2022-01-26T11:13:21Z

jobs-builder/jobs/pr.yaml

+              - log-text: .*
+                operator: OR
+            script: |
+              #!/bin/bash


Again, why not add the standard shell header here too?

Random thought: I wonder if we need a .ci/kata_header.sh to make this easy to source and minimise duplication.

Here is yet another thin wrapper, this time to https://raw.githubusercontent.com/kata-containers/tests/main/.ci/teardown.sh . I just realized that unlike ci_entry_point.sh, the teardown.sh script doesn't set those shell properties. Maybe it is by design because we don't want it to fail early? (it collects logs; one or more commands might fail and it is ok?)

Regarding adding the .ci/kata_header.sh; then people like me will forget to source it :)

jodh-intel · 2022-01-26T11:15:20Z

jobs-builder/jobs/pr.yaml

+      - '@snir911'
+      - '@c3d'


ooi, are you both in the same TZ? If so, maybe we could add a few more users: I'd say minimum of three, but even better would be to add a single GH team here.

We agreed that baseline jobs should be owners (#403). @snir911 and @c3d are going to be the owners of those Fedora 35 jobs. So it is fine.

What does having a user specified in maintainers here actually mean though? What if they are both unavailable? Is it simply for mail notification or similar?

I'm not sure I understood your first question. The maintainers variable is used on the description of the job, so on the Jenkins UI it will be listed the names on the jobs' description. An example here: http://jenkins.katacontainers.io/view/Daily%20baseline/job/kata-containers-main-fedora-35-x86_64-CRIO_K8S-baseline/

The idea is to have their names on the job's description for the case a developer need to speak with the owners (e.g. developer's PR cannot be merged because a job is failing and she/he notices the baseline has failed too). The owners are also listed in a table on the view here. Having their names on the job's description just increase visibility. Do you see any problem with that?

fidencio · 2022-01-26T12:45:55Z

A head's up that may affect your PR.

In the Architecture Committee meeting from January 25th, 2022, the Architecture Committee has agreed on using the "Dismiss stale pull request approvals when new commits are pushed" configuration from GitHub. It basically means that if your PR has been rebased or updated, the approvals given will be erased.

In order to minimize traumas due to the new approach, please, consider adding a note on the changes done before the rebase / force-push, and also pinging the reviewers for a subsequent round of reviews.

Thanks for your understanding!

Related issue: kata-containers/community#249

snir911

LGTM, thanks!
(for the record, as discussed with @wainersm) We just need to make sure it indeed override/replace the f35 jobs i created manually, and also once we see everything works as expected, remove the f32 jobs

snir911 · 2022-01-30T15:45:39Z

adding do-not-merge until Wainer's ack to merge

wainersm · 2022-01-31T19:51:59Z

@jodh-intel can I have you ack?

wainersm · 2022-02-01T13:09:13Z

LGTM, thanks! (for the record, as discussed with @wainersm) We just need to make sure it indeed override/replace the f35 jobs i created manually, and also once we see everything works as expected, remove the f32 jobs

I just renamed the Fedora 35 @snir911 created manually so on publishing they will be overwritten (and we keep the historic of executions).

GabyCT previously approved these changes Jan 24, 2022

View reviewed changes

devimc reviewed Jan 24, 2022

View reviewed changes

jodh-intel previously approved these changes Jan 25, 2022

View reviewed changes

wainersm dismissed stale reviews from jodh-intel and GabyCT via c546de5 January 25, 2022 17:52

wainersm force-pushed the fedora35_jobs branch from ba708d2 to c546de5 Compare January 25, 2022 17:52

jodh-intel approved these changes Jan 26, 2022

View reviewed changes

snir911 approved these changes Jan 30, 2022

View reviewed changes

snir911 added the do-not-merge PR has problems or depends on another label Jan 30, 2022

wainersm removed the do-not-merge PR has problems or depends on another label Feb 1, 2022

wainersm added no-backport-needed Changed do not need to be applied to an older branch / repository no-forward-port-needed Changed do not need to be applied to a newer branch / repository labels Feb 1, 2022

GabyCT approved these changes Feb 1, 2022

View reviewed changes

GabyCT merged commit 13e4ddf into kata-containers:main Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jobs-builder: add PR jobs for Fedora 35 #419

jobs-builder: add PR jobs for Fedora 35 #419

wainersm commented Jan 24, 2022

wainersm commented Jan 24, 2022

devimc left a comment

devimc Jan 24, 2022

wainersm Jan 25, 2022

devimc Jan 24, 2022

jodh-intel left a comment •

edited

Loading

wainersm commented Jan 25, 2022

wainersm commented Jan 25, 2022

jodh-intel left a comment

jodh-intel Jan 26, 2022

wainersm Jan 27, 2022

jodh-intel Jan 26, 2022

wainersm Jan 27, 2022

wainersm Jan 27, 2022

jodh-intel Jan 26, 2022

wainersm Jan 27, 2022

jodh-intel Jan 27, 2022

wainersm Jan 27, 2022

fidencio commented Jan 26, 2022

snir911 left a comment

snir911 commented Jan 30, 2022

wainersm commented Jan 31, 2022

wainersm commented Feb 1, 2022

jobs-builder: add PR jobs for Fedora 35 #419

jobs-builder: add PR jobs for Fedora 35 #419

Conversation

wainersm commented Jan 24, 2022

wainersm commented Jan 24, 2022

devimc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel left a comment • edited Loading

Choose a reason for hiding this comment

wainersm commented Jan 25, 2022

wainersm commented Jan 25, 2022

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fidencio commented Jan 26, 2022

snir911 left a comment

Choose a reason for hiding this comment

snir911 commented Jan 30, 2022

wainersm commented Jan 31, 2022

wainersm commented Feb 1, 2022

jodh-intel left a comment •

edited

Loading