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.

Sign up for GitHub

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

Jump to bottom

ci: use separate Jenkins jobs for daily master tests + CI documentation overhaul #14997

Merged
merged 2 commits into from
Mar 1, 2021
Merged

ci: use separate Jenkins jobs for daily master tests + CI documentation overhaul #14997

merged 2 commits into from
Mar 1, 2021

Conversation

nbusseneau
Copy link
Member

Following today's cartography and multiple fixes, documentation has been updated accordingly.

Context

When splitting ginkgo-kubernetes-all.Jenkinsfile in #14861, we offloaded the actual work to PR jobs. However, this means daily cilium-master-K8s-all-triggered job results were mixed in with PR-triggered job results. For tracking purposes, separate jobs needed to be setup.

We took this oppurtunity to review, document, and accordingly fix a big part of the CI jobs, notably:

  • What to test automatically at regular intervals.
  • Which jobs are required for which PRs.
  • Which phrases trigger which jobs.

Detailed recap of changes made so far

  • Creation and publication of The Table of Truth:tm:: https://docs.google.com/spreadsheets/d/1TThkqvVZxaqLR-Ela4ZrcJ0lrTJByCqrbdCjnI32_X0
  • Master jobs
    • Automated testing jobs running every 4 hours or daily have been setup according to the Table of Truth:tm:.
    • Most jobs have been renamed to make it clear which K8s version and Kernel version they're running.
    • cilium-master-K8s-all, now superseded by individual jobs, has been disabled and moved to the Obsolete view.
  • PR jobs
    • All jobs marked as required for a PR in the Table of Truth:tm: have been setup accordingly on GitHub.
    • All the triggers in required PR jobs have been reviewed, fixed according to the Table of Truth:tm:, and documented below the Table of Truth:tm:.
    • Cilium-PR-Ginkgo-Tests-Kernel (job for K8s 1.19 / Kernel 4.19) edited to correctly use K8s version 1.19 (and not 1.18, which seems to be a leftover).

Note: unlike 1.8 and 1.9, test-backport-1.7 does not trigger individual jobs. It still triggers the old Cilium-PR-Ginkgo-Tests-K8s,
because we were unsure of the robustness of ginkgo-kernel.Jenkinsfile on 1.7. We may backport the updated ginkgo-kernel.Jenkinsfile to 1.7 in a future PR.

Partial revert of #14861

The previous changes to ginkgo-kubernetes-all.Jenkinsfile in #14861 are now unneeded. These changes were reverted as part of this PR to help with the transition, since another job is still dependent upon ginkgo-kubernetes-all.Jenkinsfile: https://jenkins.cilium.io/view/Quarantine%20Pipelines/job/cilium-master-K8s-all-Quarantine/

@nbusseneau nbusseneau added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/CI Continuous Integration testing issue or flake area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. sig/contributing Impacts contribution workflow, guidelines, and tools. labels Feb 16, 2021
@nbusseneau nbusseneau requested a review from a team as a code owner February 16, 2021 18:35
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 16, 2021
@nbusseneau nbusseneau mentioned this pull request Feb 16, 2021
Closed
@nbusseneau
Copy link
Member Author

Note: ideally we would also fix all the jobs in Jobs Overview, as they are completely outdated. I propose we do that in a separate PR in order to get the links to the CI matrix and updated trigger phrases out in the wild ASAP for contributors to use. Updated intel re: jobs can come in later in my opinion, please feel free to give your POV.

@nbusseneau
Copy link
Member Author

Also: I deliberately removed test-missed-k8s from the documentation, even though it still is kinda working. In details, it basically is useless as test-older-k8s supersedes it on master, and should not be used on older branches. The only PRs it could still be used in are backports to 1.7, but it triggers the same Cilium-PR-Ginkgo-Tests-K8s as test-backport-1.7, so it is redundant...

@aanm aanm removed their assignment Feb 17, 2021
@nbusseneau nbusseneau force-pushed the pr/k8s-all-separate-master-jobs-v2 branch from 8ae0598 to 9f444de Compare February 17, 2021 18:00
@nbusseneau
Copy link
Member Author

test-missed-k8s

@nbusseneau nbusseneau force-pushed the pr/k8s-all-separate-master-jobs-v2 branch from 9f444de to 47463dc Compare February 19, 2021 15:37
@nbusseneau nbusseneau force-pushed the pr/k8s-all-separate-master-jobs-v2 branch from 47463dc to 89451d0 Compare February 19, 2021 15:47
@nbusseneau
Copy link
Member Author

Added shorthands to the list of valid words with Documentation/update-spelling_wordlist.sh shorthands (seriously, that's a valid plural! https://en.wiktionary.org/wiki/shorthand)

joestringer
joestringer requested changes Feb 22, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more question about the other table.

Documentation/contributing/testing/ci.rst Outdated Show resolved Hide resolved
@nbusseneau nbusseneau force-pushed the pr/k8s-all-separate-master-jobs-v2 branch from 89451d0 to 25bbecb Compare February 23, 2021 16:33
@nbusseneau
Revert "Offload baremetal "K8s all" builds to sub-jobs"
36f24ed 
This reverts commit f3ff8be following
decision to split K8s all job directly via Jenkins jobs triggers, rather
than rely on the older `ginkgo-kubernetes-all.Jenkinsfile` infrastructure.

The revert itself is there to help with the transition, since another
job is still dependent upon `ginkgo-kubernetes-all.Jenkinsfile`:
https://jenkins.cilium.io/view/Quarantine%20Pipelines/job/cilium-master-K8s-all-Quarantine/

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau nbusseneau force-pushed the pr/k8s-all-separate-master-jobs-v2 branch 2 times, most recently from bac0f02 to 49a3183 Compare February 23, 2021 16:40
@nbusseneau
Update PR triggers and k8s/kernel pairs
a9c6afe 
Following today's cartography and multiple fixes, documentation has been
updated accordingly.

Context

When splitting `ginkgo-kubernetes-all.Jenkinsfile` in #14861, we
offloaded the actual work to PR jobs. However, this means daily
`cilium-master-K8s-all`-triggered job results were mixed in with
PR-triggered job results. For tracking purposes, separate jobs needed to
be setup.

We took this oppurtunity to review, document, and accordingly fix a big
part of the CI jobs, notably:
- What to test automatically at regular intervals.
- Which jobs are required for which PRs.
- Which phrases trigger which jobs.

Detailed recap of changes made so far:
- Creation and publication of The Table of Truth™️
  https://docs.google.com/spreadsheets/d/1TThkqvVZxaqLR-Ela4ZrcJ0lrTJByCqrbdCjnI32_X0
- Master jobs
  - Automated testing jobs running every 4 hours or daily have been
    setup according to the Table of Truth™️.
  - Most jobs have been renamed to make it clear which K8s version and
    Kernel version they're running.
  - `cilium-master-K8s-all`, now superseded by individual jobs, has been
    disabled and moved to the `Obsolete` view.
- PR jobs
  - All jobs marked as required for a PR in the Table of Truth™️ have
    been setup accordingly on GitHub.
  - All the triggers in required PR jobs have been reviewed, fixed
    according to the Table of Truth™️, and documented below the Table of
    Truth™️.
  - `Cilium-PR-Ginkgo-Tests-Kernel` (job for K8s 1.19 / Kernel 4.19)
    edited to correctly use K8s version 1.19 (and not 1.18, which seems
    to be a leftover).

Note: unlike 1.8 and 1.9, `test-backport-1.7` does not trigger
individual jobs. It still triggers the old `Cilium-PR-Ginkgo-Tests-K8s`,
because we were unsure of the robustness of `ginkgo-kernel.Jenkinsfile`
on 1.7. We may backport the updated `ginkgo-kernel.Jenkinsfile` to 1.7
in a future PR.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau nbusseneau force-pushed the pr/k8s-all-separate-master-jobs-v2 branch from 49a3183 to a9c6afe Compare February 23, 2021 16:51
@pchaigno
Copy link
Member

I was going to trigger the k8s-all jobs so we can merge this (it's the only jobs that should be affected AFAICT), but I actually can't find the information on how to do that anywhere (apart from looking at the Jenkins configuration). Did we miss some documentation?

@pchaigno
Copy link
Member

test-missed-k8s

@nbusseneau
Copy link
Member Author

I was going to trigger the k8s-all jobs so we can merge this (it's the only jobs that should be affected AFAICT), but I actually can't find the information on how to do that anywhere (apart from looking at the Jenkins configuration). Did we miss some documentation?

That's because you are not supposed to trigger it anymore. If you insist, you can do it with test-backport-1.7-k8s though ;)

@pchaigno
Copy link
Member

pchaigno commented Feb 26, 2021 •
edited

I was going to trigger the k8s-all jobs so we can merge this (it's the only jobs that should be affected AFAICT), but I actually can't find the information on how to do that anywhere (apart from looking at the Jenkins configuration). Did we miss some documentation?

That's because you are not supposed to trigger it anymore. If you insist, you can do it with test-backport-1.7-k8s though ;)

What if we made changes to the Jenkinsfile and want to verify all works as expected (e.g., present PR)? Or if we fixed a flake that affected those jobs and want to check the fix on master?

Does that also mean test-missed-k8s, which I triggered above, will be removed?

@nbusseneau
Copy link
Member Author

nbusseneau commented Feb 26, 2021 •
edited

What if we made changes to the Jenkinsfile and want to verify all works as expected (e.g., present PR)? Or if we fixed a flake that affected those jobs and want to check the fix on master?

Cilium-PR-Ginkgo-Tests-K8s is obsolete and only used by backports for 1.7 still, because we didn't want to backport other changes required for using separate jobs. Thus there is no point in making changes on master. Should fixes be made, I expect they should be made directly on 1.7 rather than master, since "upstream first" rule only applies for fixes that are relevant to both master and backports, not for backports-only fixes.

But still, we can test it manually on master with test-backports-1.7-k8s (you're not intended to, but the job does not care where it is triggered from, so it will work).

Does that also mean test-missed-k8s, which I triggered above, will be removed?

test-missed-k8s will not be removed: it has been recycled for triggering all non-required K8s versions on Kernel 4.9, as per our discussion above: #14997 (comment) 😇

@pchaigno
Copy link
Member

test-missed-k8s will not be removed: it has been recycled for triggering all non-required K8s versions on Kernel 4.9, as per our discussion above: #14997 (comment) innocent

Ok, I wasn't clear above. That's ☝️ what I want. I thought with this PR, test-missed-k8s would not be documented anywhere anymore, but I just saw we'll still have:

For master PRs: on top of test-me-please, one may use test-missed-k8s to trigger all non-required K8s versions on Kernel 4.9 as per the Cilium CI matrix.

in the docs. Probably good enough for such rarely used trigger phrase 👍

@pchaigno
Copy link
Member

Considering the above and given all reviews are in, I think this is good to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 26, 2021
@aanm aanm merged commit 1d8fd5b into master Mar 1, 2021
1.10.0 automation moved this from In progress to Done Mar 1, 2021
@aanm aanm deleted the pr/k8s-all-separate-master-jobs-v2 branch March 1, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkourt kkourt kkourt left review comments

@joestringer joestringer joestringer approved these changes

@nebril nebril nebril approved these changes

@pchaigno pchaigno pchaigno approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake area/CI-improvement Topic or proposal to improve the Continuous Integration workflow area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI. sig/contributing Impacts contribution workflow, guidelines, and tools.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nbusseneau @pchaigno @kkourt @joestringer @nebril @aanm