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

jobs,test: Fix TestTimer_ExitOnCloseFnCtx channel close panic #25211

Merged
merged 1 commit into from May 5, 2023
Merged

jobs,test: Fix TestTimer_ExitOnCloseFnCtx channel close panic #25211

merged 1 commit into from May 5, 2023

Conversation

dylandreimerink
Copy link
Member

This test enforces that a timer callback will not be called after the context has been canceled. However, it was written such that if the test fails a panic is thrown. This fixes both the panic-on-fail and resolves the edge case causing the flake.

Fixes: #25177

Fixed TestTimer_ExitOnCloseFnCtx channel close panic

@dylandreimerink
jobs,test: Fix TestTimer_ExitOnCloseFnCtx channel close panic
8b91ce5 
This test enforces that a timer callback will not be called after the
context has been canceled. However, it was written such that if the test
fails a panic is thrown. This fixes both the panic-on-fail and resolves
the edge case causing the flake.

Fixes: cilium#25177

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
@dylandreimerink dylandreimerink added the release-note/ci This PR makes changes to the CI. label Apr 29, 2023
@dylandreimerink dylandreimerink requested a review from a team as a code owner April 29, 2023 10:06
@dylandreimerink
Copy link
Member Author

dylandreimerink commented May 1, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside

Failure Output

FAIL: Can not connect to service "http://[fd04::12]:32684" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2007/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@dylandreimerink
Copy link
Member Author

Looks like we are hitting flake #25119. Re-running to confirm its a flake

@dylandreimerink
Copy link
Member Author

/test-1.26-net-next

@tommyp1ckles
Copy link
Contributor

tommyp1ckles commented May 2, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside

Failure Output

FAIL: Can not connect to service "http://[fd04::12]:32610" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2041/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@dylandreimerink
Copy link
Member Author

dylandreimerink commented May 5, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testserver-host-d55sk

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2088/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@dylandreimerink
Copy link
Member Author

Sigh, hit a new flake, its #15455 this time.

@dylandreimerink
Copy link
Member Author

/test-1.26-net-next

@joestringer joestringer mentioned this pull request May 5, 2023
Closed
@dylandreimerink dylandreimerink added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 5, 2023
@dylandreimerink
Copy link
Member Author

Marked ready-to-merge. ci-e2e seems to be bugged, likely due to the rename from ci-datapath to ci-e2e, we do have green on the old test so I think we are good to go.

@joestringer
Copy link
Member

/ci-e2e

@joestringer
Copy link
Member

OK yeah I guess ci-e2e doesn't trigger on branches based on older versions of main. LGTM.

@joestringer joestringer merged commit daa85a0 into cilium:main May 5, 2023
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pippolo84 pippolo84 pippolo84 approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
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: IntegrationTests: TestTimer_ExitOnCloseFnCtx: panic: close of closed channel
4 participants
@dylandreimerink @tommyp1ckles @joestringer @pippolo84