Skip to content

ebpf unit testing -- snat_v4_process test #17147

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
Closed
wants to merge 1 commit into from
Closed

ebpf unit testing -- snat_v4_process test #17147

wants to merge 1 commit into from

Conversation

xinyuanzzz
Copy link
Contributor

@xinyuanzzz xinyuanzzz commented Aug 12, 2021 •
edited
Loading

As discussed with @brb, I wrote a thorough test on func snat_v4_process based on our ebpf unit testing framework #16862. The test covers every line of func snat_v4_process.

Signed-off-by: Xinyuan Zhang zhangxinyuan@google.com

@xinyuanzzz xinyuanzzz requested review from a team and jrfastab August 12, 2021 04:42
@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 12, 2021
@Weil0ng Weil0ng requested a review from brb August 12, 2021 05:18
@Weil0ng Weil0ng added release-note/ci This PR makes changes to the CI. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 12, 2021
@xinyuanzzz xinyuanzzz force-pushed the ingress-egress-tests branch 18 times, most recently from 60ccd96 to 0006073 Compare August 17, 2021 03:57
@xinyuanzzz xinyuanzzz requested a review from a team as a code owner August 17, 2021 03:57
@xinyuanzzz xinyuanzzz force-pushed the ingress-egress-tests branch 3 times, most recently from 0c6f00b to dfb3ddf Compare August 17, 2021 04:48
jrfastab
jrfastab reviewed Aug 30, 2021
View reviewed changes
@xinyuanzzz xinyuanzzz force-pushed the ingress-egress-tests branch 5 times, most recently from b96a4fb to a01c72a Compare September 3, 2021 02:45
Weil0ng
Weil0ng reviewed Sep 3, 2021
View reviewed changes
Copy link
Contributor

@Weil0ng Weil0ng left a comment

Choose a reason for hiding this comment

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

Thanks Xinyuan! Left a minor comment.

@xinyuanzzz xinyuanzzz force-pushed the ingress-egress-tests branch 2 times, most recently from 7dd7ec5 to 9f61f9f Compare September 3, 2021 04:53
Weil0ng
Weil0ng reviewed Sep 3, 2021
View reviewed changes
Weil0ng
Weil0ng approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@Weil0ng Weil0ng left a comment

Choose a reason for hiding this comment

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

Thanks Xinyuan!

@Weil0ng
Copy link
Contributor

Weil0ng commented Sep 3, 2021 •
edited by maintainer-s-little-helper bot
Loading

test-me-please

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig DirectRouting Check connectivity with direct routing

Failure Output

FAIL: Pods are not ready in time: timed out waiting for pods with filter  to be ready: 4m0s timeout expired

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.16-net-next' hit: #17176 (89.57% similarity)

This was referenced Sep 3, 2021
Closed
Closed
Closed
@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Sep 3, 2021
Closed
@xinyuanzzz
ebpf unit testing -- snat_v4_process test
445ca78 
This commit adds a thorough test on snat_v4_process based on our ebpf
unit testing framework.

Please note that the variables used in func test_snat_v4_process are
defined globally to avoid a problem caused by CMock (issue:ThrowTheSwitch/CMock#364).

Signed-off-by: Xinyuan Zhang <zhangxinyuan@google.com>
@Weil0ng
Copy link
Contributor

Weil0ng commented Sep 22, 2021

test-me-please

@xinyuanzzz
Copy link
Contributor Author

Hi @Weil0ng, there were two tests failed. The problem for ConformanceEKS (ci-eks) is a duplicate of #16938 which has not yet been resolved. I think the failure is not caused by this PR.

@brb brb added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Dec 3, 2021
@brb
Copy link
Member

brb commented Dec 3, 2021

@xinyuannn I think this PR would benefit from #17227 (comment) too. Moving to draft until resolved.

@brb brb marked this pull request as draft December 3, 2021 13:36
@pchaigno pchaigno unassigned brb Mar 2, 2022
@brb
Copy link
Member

brb commented Jul 4, 2022

We recently added the BPF unit testing framework - https://docs.cilium.io/en/latest/contributing/testing/bpf/. The proposed mocking no longer is needed.

@brb brb closed this Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfastab jrfastab jrfastab approved these changes

@brb brb Awaiting requested review from brb

+1 more reviewer

@Weil0ng Weil0ng Weil0ng approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. 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.
5 participants
@xinyuanzzz @Weil0ng @brb @jrfastab @aanm