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

Fix Traceflow IPv6 DualStack e2e error #2114

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Fix Traceflow IPv6 DualStack e2e error #2114

merged 1 commit into from
Apr 27, 2021

Conversation

gran-vmv
Copy link
Contributor

@gran-vmv gran-vmv commented Apr 21, 2021 •
edited
Loading

This PR closes #2116

@gran-vmv gran-vmv added the status/WIP Work in progress label Apr 21, 2021
@gran-vmv
Copy link
Contributor Author

/test-ipv6-e2e
/test-ipv6-only-e2e

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021 •
edited
Loading

Codecov Report

Merging #2114 (46f6e72) into main (3335e73) will increase coverage by 4.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2114      +/-   ##
==========================================
+ Coverage   61.28%   65.41%   +4.13%     
==========================================
  Files         269      269              
  Lines       20422    20424       +2     
==========================================
+ Hits        12515    13361     +846     
+ Misses       6620     5721     -899     
- Partials     1287     1342      +55
Flag Coverage Δ
e2e-tests 56.44% <100.00%> (?)
kind-e2e-tests 52.14% <100.00%> (+0.05%) ⬆️
unit-tests 41.44% <0.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...agent/controller/traceflow/traceflow_controller.go 73.14% <100.00%> (+0.19%) ⬆️
...gent/controller/networkpolicy/status_controller.go 72.60% <0.00%> (-5.48%) ⬇️
...ntroller/networkpolicy/networkpolicy_controller.go 69.35% <0.00%> (-1.62%) ⬇️
pkg/agent/openflow/network_policy.go 76.41% <0.00%> (+0.59%) ⬆️
pkg/agent/cniserver/pod_configuration.go 54.68% <0.00%> (+0.78%) ⬆️
pkg/ovs/openflow/ofctrl_bridge.go 51.10% <0.00%> (+1.10%) ⬆️
pkg/controller/egress/store/egressgroup.go 1.38% <0.00%> (+1.38%) ⬆️
pkg/apiserver/apiserver.go 91.05% <0.00%> (+1.62%) ⬆️
pkg/ovs/openflow/ofctrl_flow.go 47.36% <0.00%> (+1.75%) ⬆️
pkg/agent/flowexporter/exporter/exporter.go 77.55% <0.00%> (+2.31%) ⬆️
... and 42 more

@gran-vmv
Copy link
Contributor Author

/test-ipv6-e2e
/test-ipv6-only-e2e

@gran-vmv
Copy link
Contributor Author

/test-ipv6-e2e

@gran-vmv gran-vmv changed the title Fix Traceflow IPv6 DS e2e error [WIP] Fix Traceflow IPv6 DS e2e error Apr 21, 2021
@gran-vmv gran-vmv force-pushed the tf-ds-e2e branch 2 times, most recently from 2850800 to 10257d7 Compare April 22, 2021 03:07
@gran-vmv
Copy link
Contributor Author

/test-e2e
/test-ipv6-e2e
/test-ipv6-only-e2e

@gran-vmv
Copy link
Contributor Author

/test-e2e
/test-ipv6-e2e
/test-ipv6-only-e2e

@gran-vmv
Copy link
Contributor Author

/test-ipv6-e2e

1 similar comment
@gran-vmv
Copy link
Contributor Author

/test-ipv6-e2e

@gran-vmv
Copy link
Contributor Author

/test-conformance
/test-networkpolicy

@gran-vmv gran-vmv changed the title [WIP] Fix Traceflow IPv6 DS e2e error Fix Traceflow IPv6 DS e2e error Apr 22, 2021
@gran-vmv gran-vmv removed the status/WIP Work in progress label Apr 22, 2021
@gran-vmv gran-vmv added this to the Antrea v1.1 release milestone Apr 22, 2021
jianjuns
jianjuns previously approved these changes Apr 22, 2021
View reviewed changes
pkg/agent/controller/traceflow/traceflow_controller.go Outdated
@@ -60,7 +60,8 @@ const (
defaultWorkers = 4
// Seconds delay before injecting packet into OVS. The time of different nodes may not be completely
// synchronized, which requires a delay before inject packet.
injectPacketDelay = 5
injectPacketDelay = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we can use shorter delay? 5 seconds are long.
And for local destination, maybe 100ms is good enough?

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. I'll change to 2000ms/100ms and re-verify.

@gran-vmv
Copy link
Contributor Author

/test-all
/test-ipv6-all
/test-ipv6-only-all

@gran-vmv
Copy link
Contributor Author

/test-ipv6-all
/test-ipv6-only-all

jianjuns
jianjuns previously approved these changes Apr 25, 2021
View reviewed changes
tnqn
tnqn reviewed Apr 26, 2021
View reviewed changes
pkg/agent/controller/traceflow/traceflow_controller.go
@@ -60,7 +60,8 @@ const (
defaultWorkers = 4
// Seconds delay before injecting packet into OVS. The time of different nodes may not be completely
// synchronized, which requires a delay before inject packet.
injectPacketDelay = 5
injectPacketDelay = 2000
Copy link
Member

Choose a reason for hiding this comment

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

Please update the comment which says the unit is seconds.

Copy link
Member

Choose a reason for hiding this comment

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

And perhaps "Delay in milliseconds before ..."

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. Will fix here.

@tnqn
Copy link
Member

tnqn commented Apr 26, 2021

Not sure if others know "DS" stands for dual-stack from first eye, I only get it after looking at the original issue. "DS" is a well known shorthand for "DaemonSet", could you write the full name in the PR to avoid confusion?

@gran-vmv gran-vmv changed the title Fix Traceflow IPv6 DS e2e error Fix Traceflow IPv6 DualStack e2e error Apr 26, 2021
@gran-vmv
Copy link
Contributor Author

Not sure if others know "DS" stands for dual-stack from first eye, I only get it after looking at the original issue. "DS" is a well known shorthand for "DaemonSet", could you write the full name in the PR to avoid confusion?

Sure. Changed title of this PR.

@gran-vmv
Copy link
Contributor Author

/test-all
/test-ipv6-all
/test-ipv6-only-all

tnqn
tnqn approved these changes Apr 26, 2021
View reviewed changes
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

@gran-vmv
Copy link
Contributor Author

/test-networkpolicy
/test-ipv6-networkpolicy

@gran-vmv gran-vmv merged commit eadf092 into antrea-io:main Apr 27, 2021
@antoninbas antoninbas mentioned this pull request Apr 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@jianjuns jianjuns jianjuns left review comments

@lzhecheng lzhecheng Awaiting requested review from lzhecheng

@antoninbas antoninbas Awaiting requested review from antoninbas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Antrea v1.1 release
Development

Successfully merging this pull request may close these issues.

Traceflow e2e test failed in DualStack Cluster
5 participants
@gran-vmv @codecov-commenter @tnqn @jianjuns @vmwclabot