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

Periodically check timeout of running Traceflow requests #1179

Merged
merged 3 commits into from
Sep 18, 2020
Merged

Periodically check timeout of running Traceflow requests #1179

merged 3 commits into from
Sep 18, 2020

Conversation

jianjuns
Copy link
Contributor

Check if running Traceflow requests are timeout every 1 minute.
Change the Traceflow timeout period to 2 minutes.
Deallocate DP tag after a Traceflow request succeeds or is timeout.

@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /test-e2e: to trigger e2e tests.
  • /skip-e2e: to skip e2e tests.
  • /test-conformance: to trigger conformance tests.
  • /skip-conformance: to skip conformance tests.
  • /test-whole-conformance: to trigger all conformance tests on linux.
  • /skip-whole-conformance: to skip all conformance tests on linux.
  • /test-networkpolicy: to trigger networkpolicy tests.
  • /skip-networkpolicy: to skip networkpolicy tests.
  • /test-windows-conformance: to trigger windows conformance tests.
  • /skip-windows-conformance: to skip windows conformance tests.
  • /test-windows-networkpolicy: to trigger windows networkpolicy tests.
  • /skip-windows-networkpolicy: to skip windows networkpolicy tests.
  • /test-hw-offload: to trigger ovs hardware offload test.
  • /skip-hw-offload: to skip ovs hardware offload test.
  • /test-all: to trigger all tests (except whole conformance).
  • /skip-all: to skip all tests (except whole conformance).

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2020 •
edited
Loading

Codecov Report

Merging #1179 into master will increase coverage by 11.61%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1179       +/-   ##
===========================================
+ Coverage   31.23%   42.85%   +11.61%     
===========================================
  Files         105      144       +39     
  Lines        9863    16445     +6582     
===========================================
+ Hits         3081     7047     +3966     
- Misses       6489     8776     +2287     
- Partials      293      622      +329
Flag Coverage Δ
#integration-tests 29.29% <ø> (-1.95%) ⬇️
#unit-tests 41.35% <100.00%> (?)

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

Impacted Files Coverage Δ
pkg/controller/traceflow/controller.go 66.30% <100.00%> (ø)
pkg/agent/util/iptables/iptables.go 10.52% <0.00%> (-59.48%) ⬇️
pkg/ovs/ovsctl/ofctl.go 38.55% <0.00%> (-12.24%) ⬇️
pkg/agent/types/networkpolicy.go 16.66% <0.00%> (-8.34%) ⬇️
pkg/ovs/openflow/ofctrl_bridge.go 71.57% <0.00%> (-0.67%) ⬇️
...is/controlplane/v1beta1/zz_generated.conversion.go 0.29% <0.00%> (-0.29%) ⬇️
pkg/agent/openflow/testing/mock_openflow.go 5.71% <0.00%> (-0.13%) ⬇️
pkg/ovs/ovsctl/interface.go 0.00% <0.00%> (ø)
pkg/apis/controlplane/sets.go 0.00% <0.00%> (ø)
pkg/agent/proxy/types/types.go 0.00% <0.00%> (ø)
... and 69 more

@jianjuns
Periodically check timeout of running Traceflow requests
ac8f96e 
Check if running Traceflow requests are timeout every 1 minute.
Change the Traceflow timeout period to 2 minutes.
Deallocate DP tag after a Traceflow request succeeds or is timeout.
antoninbas
antoninbas reviewed Sep 1, 2020
View reviewed changes
test/e2e/traceflow_test.go Outdated Show resolved Hide resolved
test/e2e/traceflow_test.go Outdated Show resolved Hide resolved
test/e2e/traceflow_test.go Outdated Show resolved Hide resolved
@jianjuns jianjuns force-pushed the tf-clean branch 2 times, most recently from 0d4da6a to 82538ea Compare September 11, 2020 21:26
antoninbas
antoninbas reviewed Sep 11, 2020
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.

Some comments, otherwise LGTM; thanks for moving it to unit tests

pkg/controller/traceflow/controller_test.go Outdated Show resolved Hide resolved
pkg/controller/traceflow/controller_test.go Outdated Show resolved Hide resolved
pkg/controller/traceflow/controller_test.go Show resolved Hide resolved
pkg/controller/traceflow/controller_test.go Outdated Show resolved Hide resolved
pkg/controller/traceflow/controller_test.go Outdated Show resolved Hide resolved
@jianjuns jianjuns force-pushed the tf-clean branch 3 times, most recently from 698ed6a to 151e925 Compare September 12, 2020 04:30
tnqn
tnqn previously approved these changes Sep 14, 2020
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

pkg/controller/traceflow/controller_test.go Outdated Show resolved Hide resolved
antoninbas
antoninbas previously approved these changes Sep 14, 2020
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

@jianjuns
Copy link
Contributor Author

/test-all

@jianjuns
Copy link
Contributor Author

/test-e2e

tnqn
tnqn previously approved these changes Sep 15, 2020
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

@jianjuns jianjuns dismissed stale reviews from tnqn and antoninbas via b01487b September 16, 2020 19:33
@jianjuns jianjuns force-pushed the tf-clean branch 2 times, most recently from b01487b to 0d83d2a Compare September 16, 2020 19:38
@jianjuns
Copy link
Contributor Author

/test-all

@jianjuns
Copy link
Contributor Author

/test-e2e

@jianjuns
Copy link
Contributor Author

/test-e2e

@jianjuns
Copy link
Contributor Author

/test-e2e

@jianjuns
Copy link
Contributor Author

@tnqn @antoninbas : I need another review.
Made two changes after the last approvals:

  1. I found the resource CreationTimestamp does not include miliseconds, so changed to use Time.Unix() to check timeout.
  2. Changed Agent tag deallocation to look up the cached Traceflow session by name, but not use Traceflow.Status.DataplaneTag directly, as the filed might be unset by Controller after Controller deallocates the tag.

tnqn
tnqn approved these changes Sep 17, 2020
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

@jianjuns
Copy link
Contributor Author

/teat-all

@jianjuns
Copy link
Contributor Author

/test-all

@jianjuns
Copy link
Contributor Author

/test-networkpolicy

@jianjuns jianjuns merged commit eb2de15 into antrea-io:master Sep 18, 2020
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

@antoninbas antoninbas antoninbas left review comments

@gran-vmv gran-vmv Awaiting requested review from gran-vmv

@lzhecheng lzhecheng Awaiting requested review from lzhecheng

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jianjuns @antrea-bot @codecov-commenter @antoninbas @tnqn @vmwclabot