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.

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

[Windows] Support traceflow #3022

Merged
merged 1 commit into from May 10, 2022
Merged

[Windows] Support traceflow #3022

merged 1 commit into from May 10, 2022

Conversation

gran-vmv
Copy link
Contributor

@gran-vmv gran-vmv commented Nov 11, 2021

Signed-off-by: gran gran@vmware.com

@gran-vmv gran-vmv self-assigned this Nov 11, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2021

Codecov Report

Merging #3022 (2093295) into main (627239c) will decrease coverage by 18.49%.
The diff coverage is n/a.

❗ Current head 2093295 differs from pull request most recent head 19e66d8. Consider uploading reports for the commit 19e66d8 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3022       +/-   ##
===========================================
- Coverage   64.69%   46.19%   -18.50%     
===========================================
  Files         279      246       -33     
  Lines       39784    35914     -3870     
===========================================
- Hits        25738    16592     -9146     
- Misses      12051    17691     +5640     
+ Partials     1995     1631      -364     
Flag Coverage Δ
e2e-tests 46.19% <ø> (?)
kind-e2e-tests ?
unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/ipfix/ipfix_process.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/ipfix/ipfix_registry.go 0.00% <0.00%> (-100.00%) ⬇️
.../agent/flowexporter/priorityqueue/priorityqueue.go 0.00% <0.00%> (-92.41%) ⬇️
pkg/ipfix/ipfix_intermediate.go 0.00% <0.00%> (-90.91%) ⬇️
...lowaggregator/clickhouseclient/clickhouseclient.go 0.00% <0.00%> (-90.77%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 4.58% <0.00%> (-88.08%) ⬇️
pkg/agent/flowexporter/connections/conntrack.go 0.00% <0.00%> (-85.72%) ⬇️
.../agent/flowexporter/connections/conntrack_linux.go 0.00% <0.00%> (-85.49%) ⬇️
pkg/util/flowexport/flowexport.go 0.00% <0.00%> (-84.62%) ⬇️
...agent/flowexporter/connections/deny_connections.go 0.00% <0.00%> (-83.91%) ⬇️
... and 164 more

@gran-vmv gran-vmv changed the title WIP: Enable traceflow e2e test on Windows Enable traceflow e2e test on Windows Nov 12, 2021
lzhecheng
lzhecheng previously approved these changes Nov 16, 2021
Copy link
Contributor

@lzhecheng lzhecheng left a comment

Choose a reason for hiding this comment

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

LGTM

@lzhecheng
Copy link
Contributor

/test-integration

tnqn
tnqn previously approved these changes Nov 17, 2021
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@lzhecheng
Copy link
Contributor

/test-windows-e2e

@gran-vmv gran-vmv changed the title Enable traceflow e2e test on Windows WIP: Enable traceflow e2e test on Windows Nov 17, 2021
@gran-vmv gran-vmv force-pushed the tf-win-e2e branch 3 times, most recently from d4a31a2 to 311cf4d Compare November 18, 2021 09:46
@gran-vmv gran-vmv marked this pull request as draft November 19, 2021 02:54
@gran-vmv gran-vmv force-pushed the tf-win-e2e branch 8 times, most recently from cc50a2f to cf8e369 Compare November 23, 2021 02:27
@gran-vmv
Copy link
Contributor Author

@wenyingd According to latest test result, the "tunnel-first-packet-issue" still exists in our private testbed.

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, only two minor comments.

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
require.NoError(t, err)
svcIPv6Name = svcIPv6.Name
}

// Mesh ping to activate tunnel on Windows Node
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add todo here to explain this is a workaround, and will be removed after Windows OVS fixes the issue that first packet is possibly dropped on tunnel because the ARP entry doesn't exist in host cache. The OVS issue link is: openvswitch/ovs-issues#253

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Added TODO here.

@gran-vmv gran-vmv force-pushed the tf-win-e2e branch 2 times, most recently from b7828aa to 30fc40c Compare April 28, 2022 03:16
@gran-vmv
Copy link
Contributor Author

/test-windows-e2e

wenyingd
wenyingd previously approved these changes May 5, 2022
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

@gran-vmv
Copy link
Contributor Author

gran-vmv commented May 5, 2022

/test-all
/test-windows-all

@gran-vmv
Copy link
Contributor Author

gran-vmv commented May 5, 2022

/test-ipv6-all
/test-ipv6-only-all
/test-flexible-ipam-e2e

@gran-vmv
Copy link
Contributor Author

gran-vmv commented May 5, 2022

/test-all-features-conformance
/test-multicluster-e2e

}
if encapMode != config.TrafficEncapModeNoEncap {
// https://github.com/antrea-io/antrea/issues/897
skipIfProviderIs(t, "kind", "Skipping inter-Node Traceflow test for Kind because of #897")
Copy link
Member

Choose a reason for hiding this comment

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

OVS datapath has switched to kernel, all issues related to #897 should be resolved now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed.

test/e2e/traceflow_test.go Show resolved Hide resolved
@@ -328,15 +340,16 @@ func testTraceflowIntraNode(t *testing.T, data *TestData) {
}
}()

antreaPod, err := data.getAntreaPodOnNode(node1)
if err = data.waitForNetworkpolicyRealized(antreaPod, allowAllEgressName, v1beta2.K8sNetworkPolicy); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why it no longer needs to wait for policy realization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed with wenying, and reverted this change.

@gran-vmv
Copy link
Contributor Author

gran-vmv commented May 6, 2022

/test-windows-e2e

ci/jenkins/test.sh Outdated Show resolved Hide resolved
test/e2e/traceflow_test.go Outdated Show resolved Hide resolved
@gran-vmv
Copy link
Contributor Author

gran-vmv commented May 9, 2022

/test-conformance
/test-e2e
/test-ipv6-e2e
/test-multicluster-e2e

Signed-off-by: gran <gran@vmware.com>
@gran-vmv
Copy link
Contributor Author

/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all
/test-flexible-ipam-e2e
/test-multicluster-e2e

@gran-vmv
Copy link
Contributor Author

/test-all-features-conformance
/test-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label May 10, 2022
@tnqn tnqn merged commit 94311f9 into antrea-io:main May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants