Periodic IPTables sync #1751

siddhant94 · 2021-01-15T06:57:21Z

Fixes #628 .
Add a long running goroutine which periodically syncs antrea required iptables rules on linux.
If the sync call/operation fails (for example - xtables lock contention), the next attempt would occur at the next sync interval.

vmwclabot · 2021-01-15T06:57:27Z

@siddhant94, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

codecov-io · 2021-01-15T07:30:47Z

Codecov Report

❗ No coverage uploaded for pull request base (main@4d6bdaf). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1751   +/-   ##
=======================================
  Coverage        ?   63.19%           
=======================================
  Files           ?      192           
  Lines           ?    16436           
  Branches        ?        0           
=======================================
  Hits            ?    10386           
  Misses          ?     4986           
  Partials        ?     1064

Flag	Coverage Δ
e2e-tests	`43.99% <0.00%> (?)`
kind-e2e-tests	`50.17% <0.00%> (?)`
unit-tests	`42.73% <0.00%> (?)`

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

antoninbas

Thanks for opening this PR, I left some comments. Please make sure you sign the CLA as well.

test/integration/agent/route_test.go

pkg/agent/route/route_linux.go

antoninbas · 2021-01-15T20:43:17Z

test/integration/agent/route_test.go

+	deleteRuleCmd := deleteOption + strings.Join([]string{"-m", "comment", "--comment", "\"Antrea: jump to Antrea prerouting rules\"", "-j", "ANTREA-PREROUTING"}, " ")
+
+	// #nosec G204: ignore in test code
+	actualData, err := exec.Command("bash", "-c", deleteRuleCmd).Output()


I notice other tests doing the same thing (calling iptables through bash), but I wonder why the iptables executable cannot be invoked directly instead?

siddhant94 · 2021-01-17T23:13:16Z

Hi @antoninbas , just to collate feedback points,

Use wait.Until
Start long running from agent.go
Pass stopCh, called only once
Make sync interval top level constant
Separate integration test

I have addressed these, let me know if implementation is still inconsistent.
Also needed feedback on couple of things -

sync interval is set at 60sec, and,
Currently, routeClient initializes iptable with a retry logic(because of xtables lock). So, should the sync operation also have similar behaviour? In this PR, the sync operation on encountering error returns, which implies that next attempt would only be at next sync interval.

siddhant94 · 2021-01-18T19:29:22Z

Looking into ci failures

vmwclabot · 2021-01-18T20:14:50Z

@siddhant94, VMware has approved your signed contributor license agreement.

siddhant94 · 2021-01-18T20:48:56Z

Looking into ci failures

Fixed, updated the PR.

tnqn

@siddhant94 thanks for the PR, I left some comments.

pkg/agent/route/route_linux.go

siddhant94 · 2021-01-20T21:30:07Z

@siddhant94 thanks for the PR, I left some comments.

Updated the PR @tnqn

pkg/agent/route/route_linux.go

test/integration/agent/route_test.go

tnqn

Thanks @siddhant94. One comment about the interface argument, otherwise LGTM.

tnqn · 2021-01-21T13:53:52Z

cmd/antrea-agent/agent.go

@@ -253,6 +255,8 @@ func run(o *Options) error {

 	log.StartLogFileNumberMonitor(stopCh)

+	go routeClient.Run(stopCh, ipTablesSyncInterval)


I had a comment about the consistant in #1751 (comment). Could we move ipTablesSyncInterval to route_linux.go? For two reasons:

routeClient is an interface for both windows and linux platform, the argument doesn't make sense to windows.

the argument is not configurable and specific to routeClient internal logic so no need to be the main package. Almost all such constants/variables are declared in their own package. informerDefaultResync is the only one declared here because it's an argument of the global component informerFactory which is used by several components.

For testing, you may declare it as a variable instead.

Agreed @tnqn , this should also stay internal. I missed this in previous iteration. I was thinking of exposing a function SetSyncInterval which would override the variable, it would be meant to be called from route package's Initialization() method and from the integration test.
Let me know if this is inconsistent. Also, if there's someplace in antrea code where we have a similar issue, I can look at that and change this part.

I forgot that the integration test is in a different package. I'm ok with just declaring the variable as public, but we should comment why it's made so. We had a similar case before that an integration test needs to use package internal consistant/variable and it just made them public to solve it. However, that code has been deleted when implmenetation evolves.

@antoninbas what do you think?

typically my favorite approach is to have a way to pass it to the constructor:

const ipTablesSyncInterval = 60 * time.Second type ClientConfig struct { IPTablesSyncInterval time.Duration } func NewClient(serviceCIDR *net.IPNet, networkConfig *config.NetworkConfig, noSNAT bool, configFns ...func(*ReporterConfig)) *Client { config := ClientConfig{ IPTablesSyncInterval: defaultIPTablesSyncInterval, } for _, fn := range configFns { fn(&config) } return &Client{ ClientConfig: config, // other members } }

This way the call to NewClient does not need to change in cmd/antrea-agent/agent.go, but for the integration tests (or unit tests for that matter), we can easily provide a different value for the sync interval.

Of course that may be a bit overkill here with a single configuration parameter. So if you prefer to make the variable public and add a comment, that's fine by me.

@antoninbas I actually missed this last comment, I just noticed it after push.

tnqn

@siddhant94 The commit message seems having a very long title or missing empty line between title and body. Could you update it following https://github.com/golang/go/wiki/CommitMessage?

pkg/agent/route/route_linux.go

pkg/agent/route/route_windows.go

pkg/agent/route/interfaces.go

siddhant94 · 2021-01-27T01:01:54Z

@siddhant94 The commit message seems having a very long title or missing empty line between title and body. Could you update it following https://github.com/golang/go/wiki/CommitMessage?

Updated it as per the wiki.

tnqn

@siddhant94 The body of commit message should be wrapped to ~76 characters, otherwise it wouldn't look good from git log. And I think you don't need to add "pkg/agent/route:" as prefix as the other words can already scope the change well.

pkg/agent/route/interfaces.go

siddhant94 · 2021-01-27T21:01:10Z

@siddhant94 The body of commit message should be wrapped to ~76 characters, otherwise it wouldn't look good from git log. And I think you don't need to add "pkg/agent/route:" as prefix as the other words can already scope the change well.

@tnqn I updated the Run fn comment and removed the said prefix from commit, but, for commit body, I could only reduce it to ~100 chars. Should I just omit the body? Just keep the "Fixes" line and title/subject line.

tnqn · 2021-01-28T03:06:12Z

@siddhant94 By wrapping the body of commit message, I meant to limit each line ~76 chars.
For example, this is how the current git log shows:

commit 5cbe43181f6143f13b683f4a589dde30dfb492c9
Author: Siddhant Sinha <[email protected]>
Date:   Thu Jan 14 15:12:55 2021 +0530

    Periodically sync antrea required iptables rules

    Add a long-running goroutine, which waits for iptables to be initialised, before periodically syncing it.

    Fixes #628

By wrapping it, it appears as:

commit 5cbe43181f6143f13b683f4a589dde30dfb492c9
Author: Siddhant Sinha <[email protected]>
Date:   Thu Jan 14 15:12:55 2021 +0530

    Periodically sync antrea required iptables rules

    Add a long-running goroutine, which waits for iptables to be
    initialised, before periodically syncing it.

    Fixes #628

Add a long-running goroutine which periodically syncs iptables. To be able to configure the sync interval for integration tests, IPTablesSyncInterval is exported. Fixes antrea-io#628

siddhant94 · 2021-01-28T04:31:11Z

@tnqn I missed the "wrapped" word, thus misunderstood it completely. I have updated the commit msg, with each line within ~76 chars.

tnqn

LGTM, thanks @siddhant94

siddhant94 · 2021-01-29T06:27:24Z

LGTM, thanks @siddhant94

Thanks @tnqn @antoninbas for the patient iterations. Learnt new stuff and got to know a lot of antrea context also.

tnqn · 2021-01-29T07:06:44Z

/test-all

tnqn · 2021-01-29T12:55:06Z

/test-windows-conformance
/test-windows-networkpolicy

tnqn · 2021-01-29T13:18:55Z

/test-windows-conformance
/test-windows-networkpolicy

tnqn · 2021-01-29T13:21:14Z

@lzhecheng do you know why the commands cannot trigger windows e2e tests?

lzhecheng · 2021-01-29T23:40:57Z

@lzhecheng do you know why the commands cannot trigger windows e2e tests?

There was something wrong with Jenkins smee service. Now it should work.
/test-windows-conformance
/test-windows-networkpolicy

tnqn · 2021-02-01T16:29:46Z

/test-windows-conformance
/test-windows-networkpolicy

lzhecheng · 2021-02-02T03:50:40Z

windows-conformance is pending now but it has actually succeeded, checked on Jenkins side.

antoninbas reviewed Jan 15, 2021

View reviewed changes

siddhant94 force-pushed the syncloop-ip-tables branch from 986b7df to 7a5c7d2 Compare January 17, 2021 22:01

siddhant94 force-pushed the syncloop-ip-tables branch 2 times, most recently from d7ec98b to 179930e Compare January 18, 2021 19:08

siddhant94 force-pushed the syncloop-ip-tables branch from 179930e to 2cceaec Compare January 18, 2021 20:28

tnqn reviewed Jan 19, 2021

View reviewed changes

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

siddhant94 force-pushed the syncloop-ip-tables branch from 2cceaec to 5468132 Compare January 20, 2021 21:27

antoninbas reviewed Jan 20, 2021

View reviewed changes

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

test/integration/agent/route_test.go Outdated Show resolved Hide resolved

siddhant94 force-pushed the syncloop-ip-tables branch from 5468132 to abaed16 Compare January 21, 2021 05:14

tnqn reviewed Jan 21, 2021

View reviewed changes

siddhant94 force-pushed the syncloop-ip-tables branch from abaed16 to 1ca681f Compare January 22, 2021 22:38

Base automatically changed from master to main January 26, 2021 00:00

tnqn reviewed Jan 26, 2021

View reviewed changes

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_windows.go Outdated Show resolved Hide resolved

pkg/agent/route/interfaces.go Outdated Show resolved Hide resolved

siddhant94 force-pushed the syncloop-ip-tables branch 2 times, most recently from 49ef3ae to e312148 Compare January 27, 2021 00:59

tnqn reviewed Jan 27, 2021

View reviewed changes

pkg/agent/route/interfaces.go Outdated Show resolved Hide resolved

siddhant94 force-pushed the syncloop-ip-tables branch from e312148 to 5cbe431 Compare January 27, 2021 20:53

Periodically sync antrea required iptables rules

6c0cc35

Add a long-running goroutine which periodically syncs iptables. To be able to configure the sync interval for integration tests, IPTablesSyncInterval is exported. Fixes antrea-io#628

siddhant94 force-pushed the syncloop-ip-tables branch from 5cbe431 to 6c0cc35 Compare January 28, 2021 04:28

tnqn approved these changes Jan 29, 2021

View reviewed changes

tnqn merged commit def473c into antrea-io:main Feb 3, 2021

antoninbas mentioned this pull request Apr 17, 2021

Verify the status of required routes gateway periodically #2091

Merged

antoninbas mentioned this pull request May 26, 2021

Periodically verify that all required NodePortLocal iptables rules are present in the Node #2210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Periodic IPTables sync #1751

Periodic IPTables sync #1751

siddhant94 commented Jan 15, 2021 •

edited

Loading

vmwclabot commented Jan 15, 2021

codecov-io commented Jan 15, 2021 •

edited

Loading

antoninbas left a comment

antoninbas Jan 15, 2021

siddhant94 commented Jan 17, 2021 •

edited

Loading

siddhant94 commented Jan 18, 2021

vmwclabot commented Jan 18, 2021

siddhant94 commented Jan 18, 2021

tnqn left a comment

siddhant94 commented Jan 20, 2021

tnqn left a comment

tnqn Jan 21, 2021

siddhant94 Jan 22, 2021

tnqn Jan 22, 2021

antoninbas Jan 22, 2021

siddhant94 Jan 22, 2021

tnqn left a comment

siddhant94 commented Jan 27, 2021

tnqn left a comment

siddhant94 commented Jan 27, 2021

tnqn commented Jan 28, 2021

siddhant94 commented Jan 28, 2021

tnqn left a comment

siddhant94 commented Jan 29, 2021

tnqn commented Jan 29, 2021

tnqn commented Jan 29, 2021

tnqn commented Jan 29, 2021

tnqn commented Jan 29, 2021

lzhecheng commented Jan 29, 2021

tnqn commented Feb 1, 2021

lzhecheng commented Feb 2, 2021

		@@ -253,6 +255,8 @@ func run(o *Options) error {

		log.StartLogFileNumberMonitor(stopCh)

		go routeClient.Run(stopCh, ipTablesSyncInterval)

Periodic IPTables sync #1751

Periodic IPTables sync #1751

Conversation

siddhant94 commented Jan 15, 2021 • edited Loading

vmwclabot commented Jan 15, 2021

codecov-io commented Jan 15, 2021 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas Jan 15, 2021

Choose a reason for hiding this comment

siddhant94 commented Jan 17, 2021 • edited Loading

siddhant94 commented Jan 18, 2021

vmwclabot commented Jan 18, 2021

siddhant94 commented Jan 18, 2021

tnqn left a comment

Choose a reason for hiding this comment

siddhant94 commented Jan 20, 2021

tnqn left a comment

Choose a reason for hiding this comment

tnqn Jan 21, 2021

Choose a reason for hiding this comment

siddhant94 Jan 22, 2021

Choose a reason for hiding this comment

tnqn Jan 22, 2021

Choose a reason for hiding this comment

antoninbas Jan 22, 2021

Choose a reason for hiding this comment

siddhant94 Jan 22, 2021

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

siddhant94 commented Jan 27, 2021

tnqn left a comment

Choose a reason for hiding this comment

siddhant94 commented Jan 27, 2021

tnqn commented Jan 28, 2021

siddhant94 commented Jan 28, 2021

tnqn left a comment

Choose a reason for hiding this comment

siddhant94 commented Jan 29, 2021

tnqn commented Jan 29, 2021

tnqn commented Jan 29, 2021

tnqn commented Jan 29, 2021

tnqn commented Jan 29, 2021

lzhecheng commented Jan 29, 2021

tnqn commented Feb 1, 2021

lzhecheng commented Feb 2, 2021

siddhant94 commented Jan 15, 2021 •

edited

Loading

codecov-io commented Jan 15, 2021 •

edited

Loading

siddhant94 commented Jan 17, 2021 •

edited

Loading