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

Enable previously disabled encryption tests on GKE #24603

Merged
merged 1 commit into from Apr 4, 2023
Merged

Enable previously disabled encryption tests on GKE #24603

merged 1 commit into from Apr 4, 2023

Conversation

brlbil
Copy link
Contributor

@brlbil brlbil commented Mar 28, 2023

Encryption connectivity tests were failing on new versions of GKE clusters.
Now the tests are passing and it is not clear how it is fixed.
The issue might be gone due to some changes on cilium or changes done on the cloud provider side.

Fixes: #22808

@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 Mar 28, 2023
@brlbil
Copy link
Contributor Author

brlbil commented Mar 28, 2023

Tests are passing see below.

https://github.com/cilium/cilium/actions/runs/4542154957/jobs/8006809084
https://github.com/cilium/cilium/actions/runs/4542154962/jobs/8007762078
https://github.com/cilium/cilium/actions/runs/4542154978/jobs/8005216727

@brlbil brlbil marked this pull request as ready for review March 28, 2023 13:40
@brlbil brlbil requested review from a team as code owners March 28, 2023 13:40
@brlbil brlbil requested review from aanm and tklauser March 28, 2023 13:40
@brlbil brlbil force-pushed the pr/brlbil/ci-re-enable-encryption-test branch from 257f33d to 697ea74 Compare March 28, 2023 13:41
@aanm aanm requested a review from pchaigno March 28, 2023 18:31
@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Mar 28, 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 Mar 28, 2023
pchaigno
pchaigno requested changes Mar 28, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I'm a bit undecided on whether reenabling this right now is a good idea.

On one hand, the longer we leave tests disabled, the more likely they are to regress. On the other, (1) we don't have a good explanation of why this may not be failing anymore, (2) it seems you only executed it once, and (3) the GKE workflow isn't really super stable at the moment (because of #22368 mostly). So I'm worried we'll just be adding to the flakiness noise of the GKE workflow is this ends up not being fixed.

Having written this down, I think I've shifted my opinion from Comment review to Request changes review 😅 Sorry.

Could we at least run this workflow ~10 times to ensure the original flake looks fixed? Or what the original issue not a flake but a complete test breakage?

@pchaigno pchaigno added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/ci This PR makes changes to the CI. and removed release-note/misc This PR makes changes that have no direct user impact. labels Mar 28, 2023
@brlbil
Copy link
Contributor Author

brlbil commented Mar 29, 2023

@pchaigno It also bugs me that we do not why the issue seems to be fixed.
It certainly would not hurt if we run the tests more times.

@brlbil brlbil force-pushed the pr/brlbil/ci-re-enable-encryption-test branch from e5de381 to 74ed2cd Compare March 29, 2023 21:48
@brlbil
Copy link
Contributor Author

brlbil commented Mar 29, 2023

@pchaigno I have run the tests many times, and there were some errors but none of them were about encryption.
I have seen more than 10 successful tests.

pchaigno
pchaigno approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for checking!

Let's merge then. We can always rerevert if we see it fail.

@brlbil
gh/workflow: enable encryption connectivity tests
b3bae50 
This commit enables previously disabled encryption tests.
Encryption connectivity tests were failing on new versions of GKE clusters.
Now the tests are passing and  it is not clear how it is fixed.
The issue might be gone due to some changes on cilium or
changes done on cloud provider side.

Signed-off-by: Birol Bilgin <birol@cilium.io>
@brlbil brlbil force-pushed the pr/brlbil/ci-re-enable-encryption-test branch from 74ed2cd to b3bae50 Compare April 3, 2023 13:56
@brlbil brlbil added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 3, 2023
@squeed squeed merged commit 3ca74d2 into master Apr 4, 2023
43 checks passed
@squeed squeed deleted the pr/brlbil/ci-re-enable-encryption-test branch April 4, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@aanm aanm aanm approved these changes

@tklauser tklauser Awaiting requested review from tklauser tklauser was automatically assigned from cilium/ci-structure

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. 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.

CI: Investigate GKE + IPsec failure with 1.24.7-gke and remove workaround
4 participants
@brlbil @pchaigno @aanm @squeed