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: Move IPsec CI jobs into separate pipelines #26730

Merged
merged 1 commit into from Aug 1, 2023
Merged

CI: Move IPsec CI jobs into separate pipelines #26730

merged 1 commit into from Aug 1, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Jul 10, 2023 •
edited

This PR moves IPsec test cases from conformance-e2e.yaml to conformance-ipsec-e2e.yaml. It would be beneficial to separate the IPsec test from normal ones as we will have more test specific for IPsec (e.g. #26350 ) and more IPsec features.

Signed-off-by: Zhichuan Liang gray.liang@isovalent.com

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 10, 2023
@jschwinger233
Copy link
Member Author

/test-ci-aks

@jschwinger233
Copy link
Member Author

/ci-aks

@jschwinger233 jschwinger233 force-pushed the gray/split-ipsec-ci branch 2 times, most recently from 91e8ba2 to 2f1c931 Compare July 12, 2023 08:57
@jschwinger233 jschwinger233 added the release-note/ci This PR makes changes to the CI. label Jul 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 12, 2023
@jschwinger233 jschwinger233 added the area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. label Jul 12, 2023
@jschwinger233 jschwinger233 changed the title [WIP] Split IPsec CI CI: Separate IPsec CI jobs into separate pipelines Jul 12, 2023
@jschwinger233
Copy link
Member Author

/test

@jschwinger233 jschwinger233 marked this pull request as ready for review July 12, 2023 11:05
@jschwinger233 jschwinger233 requested review from a team as code owners July 12, 2023 11:05
@jschwinger233 jschwinger233 changed the title CI: Separate IPsec CI jobs into separate pipelines CI: Move IPsec CI jobs into separate pipelines Jul 13, 2023
@aanm aanm requested a review from brb July 13, 2023 10:44
@giorio94
Copy link
Member

I'm personally against moving the IPSec tests away from the clustermesh workflow, unless there's a strong reason, as it increases the overall maintenance burden and likelihood of drifts. I mean, I perfectly understand the rationale behind doing this change for other workflows, but the goal of the clustermesh ones is to verify that cross-cluster communication works in a variety of scenarios, for different control-plane and datapath configurations. Detailed datapath tests are expected to be performed in other workflows (such as conformance E2E) as at the end of the day remote nodes are treated pretty much in the same way as the local ones.

@pchaigno
Copy link
Member

@jschwinger233 Did you test the two changed workflows? You might want to reopen this with a branch on cilium/cilium if you haven't already.

brb
brb reviewed Jul 17, 2023
View reviewed changes
.github/workflows/conformance-ipsec-e2e.yaml
- name: Set Environment Variables
uses: ./.github/actions/set-env-variables

- name: Set up job variables
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could benefit from https://docs.github.com/en/actions/using-workflows/reusing-workflows to avoid the same code in ci-e2e, ci-ipsec and upcoming ci-upgrade?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should block an improvement on making it a bigger improvement. Let's ship this and iterate after.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with Paul. Let me figure out how to make good use of reusing-workflows for #26983 after.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can do it as a follow-up.

christarazi
christarazi approved these changes Jul 27, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Pending Martynas's suggestion

@jschwinger233
ci: split ci-e2e into non-ipsec and ipsec pipelines
e89f149 
IPsec cases are moved from conformance-e2e.yaml to
conformance-ipsec-e2e.yaml, we can trigger the latter one using
/ci-ipsec-e2e.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233
Copy link
Member Author

Two effected workflows (ci-ipsec-e2e and ci-e2e) has been tested from #27145, both have passed:
ci-ipsec-e2e: https://github.com/cilium/cilium/actions/runs/5715863236
ci-e2e: https://github.com/cilium/cilium/actions/runs/5711043643

@jschwinger233 jschwinger233 removed the release-note/ci This PR makes changes to the CI. label Aug 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 1, 2023
@jschwinger233 jschwinger233 added the release-note/ci This PR makes changes to the CI. label Aug 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 1, 2023
@jschwinger233 jschwinger233 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 1, 2023
@jschwinger233
Copy link
Member Author

Only ci-e2e and ci-ipsec-e2e matter. Since both tests are green, I labelled this PR as ready-to-merge.

@dylandreimerink dylandreimerink merged commit 4c85662 into cilium:main Aug 1, 2023
147 checks passed
@jschwinger233 jschwinger233 mentioned this pull request Aug 1, 2023
Closed
This was referenced Sep 12, 2023
Merged
Merged
Merged
@jschwinger233 jschwinger233 added needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Sep 15, 2023
@jschwinger233 jschwinger233 deleted the gray/split-ipsec-ci branch September 15, 2023 03:34
@julianwiedmann julianwiedmann added backport-pending/1.12 backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Sep 15, 2023
@github-actions github-actions bot added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Oct 4, 2023
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Oct 5, 2023
This was referenced Oct 17, 2023
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb left review comments

@pchaigno pchaigno pchaigno left review comments

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jschwinger233 @giorio94 @pchaigno @brb @christarazi @aanm @dylandreimerink @julianwiedmann