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

Reorganize the code for configuring Windows networking into dedicated package #5159

Merged
merged 3 commits into from
May 21, 2024
Merged

Reorganize the code for configuring Windows networking into dedicated package #5159

merged 3 commits into from
May 21, 2024

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Jun 20, 2023 •
edited
Loading

This commits reorganizes Windows-related code for configuring Windows networking
into dedicated package pkg/agent/util/winnet, enhancing clarity and making it
easier for developers to locate and understand the Windows-specific functionality.
Besides, an interface implemented by the Windows-specific networking utility
functions is added to the package, facilitating easier testing and enhancing code
maintainability.

Furthermore, additional unit tests are added for the file
pkg/agent/route/route_windows.go, based on the testing code in the packages.
This enhances the code coverage of file route_windows.go.

@hongliangl hongliangl force-pushed the 20230619-windows-route-ut branch 2 times, most recently from 892d699 to 55a75ef Compare July 25, 2023 08:06
@hongliangl hongliangl marked this pull request as ready for review July 25, 2023 08:06
@hongliangl hongliangl changed the title [WIP] Add unit tests for pkg/agent/route Add unit tests for pkg/agent/route Jul 25, 2023
@hongliangl hongliangl added the area/test Issues or PRs related to unit and integration tests. label Jul 25, 2023
@hongliangl hongliangl added this to the Antrea v1.14 release milestone Jul 25, 2023
@hongliangl hongliangl force-pushed the 20230619-windows-route-ut branch 5 times, most recently from fe1a900 to 10671e6 Compare July 31, 2023 09:05
@hongliangl hongliangl mentioned this pull request Aug 23, 2023
Merged
@hongliangl hongliangl force-pushed the 20230619-windows-route-ut branch 2 times, most recently from 6b99ad8 to d7d36d9 Compare August 27, 2023 00:29
antoninbas
antoninbas reviewed Sep 19, 2023
View reviewed changes
pkg/agent/route/route_windows.go Outdated Show resolved Hide resolved
pkg/agent/util/winnetutil/netutil.go Outdated Show resolved Hide resolved
pkg/agent/util/winnetutil/netutil.go Outdated Show resolved Hide resolved
pkg/agent/util/winnetutil/netutil.go Outdated Show resolved Hide resolved
wenyingd
wenyingd reviewed Sep 21, 2023
View reviewed changes
pkg/agent/route/route_windows.go Show resolved Hide resolved
pkg/agent/util/interfaces.go Outdated Show resolved Hide resolved
pkg/agent/util/net.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the 20230619-windows-route-ut branch 2 times, most recently from 7e0ebd8 to 6f9fdb8 Compare September 21, 2023 04:08
antoninbas
antoninbas reviewed Sep 21, 2023
View reviewed changes
pkg/agent/util/winnet/interfaces.go Outdated Show resolved Hide resolved
pkg/agent/util/winnet/interfaces.go Outdated Show resolved Hide resolved
wenyingd
wenyingd approved these changes May 11, 2024
View reviewed changes
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl
Copy link
Contributor Author

@antoninbas Could you help have a final confirm?

antoninbas
antoninbas approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl
Copy link
Contributor Author

/test-windows-all

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-all

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

2 similar comments
@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

hongliangl commented May 16, 2024 •
edited
Loading

@antoninbas I saw that some flaky tests get failed for test-windows-containerd-e2e for all PRs. Is there any risks to merge this one?

@antoninbas
Copy link
Contributor

@antoninbas I saw that some flaky tests get failed for test-windows-containerd-e2e for all PRs. Is there any risks to merge this one?

That's a bit concerning, because I just ran the job for a different PR and it passed on the first try: #6090
TestConnectivity/testPodConnectivityDifferentNodes is also a pretty "basic" test, so it's not a good sign that it is failing. Maybe we should look into that?

@hongliangl
Copy link
Contributor Author

@antoninbas I saw that some flaky tests get failed for test-windows-containerd-e2e for all PRs. Is there any risks to merge this one?

That's a bit concerning, because I just ran the job for a different PR and it passed on the first try: #6090 TestConnectivity/testPodConnectivityDifferentNodes is also a pretty "basic" test, so it's not a good sign that it is failing. Maybe we should look into that?

Yes, we should look into it. I found the test TestConnectivity/testPodConnectivityDifferentNodes also got a failure in #6312 and there is no change of go code. I'll create an issue about this.

@hongliangl
Copy link
Contributor Author

@antoninbas I saw that some flaky tests get failed for test-windows-containerd-e2e for all PRs. Is there any risks to merge this one?

That's a bit concerning, because I just ran the job for a different PR and it passed on the first try: #6090 TestConnectivity/testPodConnectivityDifferentNodes is also a pretty "basic" test, so it's not a good sign that it is failing. Maybe we should look into that?

Yes, we should look into it. I found the test TestConnectivity/testPodConnectivityDifferentNodes also got a failure in #6312 and there is no change of go code. I'll create an issue about this.

@antoninbas I saw that some flaky tests get failed for test-windows-containerd-e2e for all PRs. Is there any risks to merge this one?

That's a bit concerning, because I just ran the job for a different PR and it passed on the first try: #6090 TestConnectivity/testPodConnectivityDifferentNodes is also a pretty "basic" test, so it's not a good sign that it is failing. Maybe we should look into that?

Yes, we should look into it. I found the test TestConnectivity/testPodConnectivityDifferentNodes also got a failure in #6312 and there is no change of go code. I'll create an issue about this.

Just ignore it. I found you have created it.

@antoninbas
Copy link
Contributor

@hongliangl

Just ignore it. I found you have created it.

I didn't, this is a different issue. Please open a new issue and follow-up on this.

@antoninbas
Copy link
Contributor

/test-windows-containerd-e2e

@hongliangl hongliangl mentioned this pull request May 20, 2024
Open
@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

7 similar comments
@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-containerd-e2e

@hongliangl
Copy link
Contributor Author

hongliangl commented May 21, 2024 •
edited
Loading

@antoninbas I run the test test-windows-containerd-e2e 5 times and all passed. Could we merge this first? IMO, the tests TestConnectivity/{testPingLargeMTU, testPodConnectivityDifferentNodes} failure might not be caused by this PR because I found they got failed in other PRs.

Besides, @wenyingd helped trigger the testtest-windows-containerd-e2e several times with the main branch code and didn't reproduce the failure. Is it possible that there was something abnormal with the Windows testbed environment during the time that tests were failing frequently?

@antoninbas
Copy link
Contributor

Besides, @wenyingd helped trigger the testtest-windows-containerd-e2e several times with the main branch code and didn't reproduce the failure. Is it possible that there was something abnormal with the Windows testbed environment during the time that tests were failing frequently?

Yes, it is always possible. I will merge now that you have confirmed the job is passing for this PR.

@antoninbas antoninbas merged commit 166db3b into antrea-io:main May 21, 2024
58 of 61 checks passed
@hongliangl hongliangl deleted the 20230619-windows-route-ut branch May 21, 2024 05:27
@hongliangl
Copy link
Contributor Author

Besides, @wenyingd helped trigger the testtest-windows-containerd-e2e several times with the main branch code and didn't reproduce the failure. Is it possible that there was something abnormal with the Windows testbed environment during the time that tests were failing frequently?

Yes, it is always possible. I will merge now that you have confirmed the job is passing for this PR.

Thanks for merging this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoninbas antoninbas antoninbas approved these changes

@wenyingd wenyingd wenyingd approved these changes

@tnqn tnqn Awaiting requested review from tnqn

@qiyueyao qiyueyao Awaiting requested review from qiyueyao

Assignees
No one assigned
Labels
area/OS/windows Issues or PRs related to the Windows operating system. area/test Issues or PRs related to unit and integration tests.
Projects
None yet
Milestone
Antrea v2.1 release
Development

Successfully merging this pull request may close these issues.
5 participants
@hongliangl @antoninbas @wenyingd @qiyueyao @luolanzone