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

Make iptables initialization error non fatal #1497

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Nov 5, 2020

In large scale clusters, xtables lock may be hold by kubelet/
kube-proxy/ portmap for a long time, especially when there are many
service rules in nat table. antrea-agent may not be able to acquire the
lock in short time. If the agent blocks on the lock or quit itself, the
CNI server won't be running, causing all CNI requests to fail. If the
Pods' restart policy is Always and there are dead Pods, container
runtime will keep retrying calling CNIs, during which portmap is called
first, leading to more xtables lock contention.

This patch makes iptables initialization error non fatal and uses a
goroutine to retry it until success. The agent will start the CNI server
anyway and handle the CNI Del requests but won't handle CNI Add requests
until the network is ready.

Fixes #1499

@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-all-features-conformance: to trigger conformance tests with all alpha features enabled.
  • /skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
  • /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).

@tnqn
Copy link
Member Author

tnqn commented Nov 5, 2020

/test-all

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. I think that blocking in CNI Add is the right thing to do but hopefully it doesn't create additional issues.

nit: in the commit message, s/leading to more xtables lock competitor/leading to more xtables lock contention/

@@ -44,6 +45,10 @@ import (
"github.com/vmware-tanzu/antrea/pkg/ovs/ovsconfig"
)

const (
networkReadyTimeout = 30 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the default kubelet / container runtime timeout for CNI Add? could you add the info as a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

test/integration/agent/route_test.go Outdated Show resolved Hide resolved
test/integration/agent/route_test.go Outdated Show resolved Hide resolved
test/integration/agent/route_test.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
func (c *Client) initIPTablesOnce(done func()) {
defer done()
for {
time.Sleep(2 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

curious about why you put a sleep before the first try?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was a mistake, thanks for catching it, now I understand why Initialize took more than 2 seconds in integration test even if I didn't make the lock hold..

pkg/agent/util/iptables/lock.go Outdated Show resolved Hide resolved
@tnqn
Copy link
Member Author

tnqn commented Nov 6, 2020

/test-all

@tnqn
Copy link
Member Author

tnqn commented Nov 6, 2020

/test-all

@codecov-io
Copy link

codecov-io commented Nov 6, 2020

Codecov Report

Merging #1497 (bcfb209) into master (714af4d) will increase coverage by 0.46%.
The diff coverage is 70.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1497      +/-   ##
==========================================
+ Coverage   68.16%   68.62%   +0.46%     
==========================================
  Files         165      165              
  Lines       13107    13163      +56     
==========================================
+ Hits         8934     9033      +99     
+ Misses       3237     3193      -44     
- Partials      936      937       +1     
Flag Coverage Δ
integration-tests 45.59% <54.54%> (-0.23%) ⬇️
kind-e2e-tests 55.03% <59.45%> (+0.67%) ⬆️
unit-tests 42.42% <32.14%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
pkg/agent/util/iptables/lock.go 81.81% <ø> (ø)
pkg/agent/route/route_linux.go 66.20% <55.55%> (+0.83%) ⬆️
pkg/agent/cniserver/server.go 76.00% <70.58%> (-1.14%) ⬇️
pkg/agent/agent.go 51.12% <80.00%> (+1.27%) ⬆️
pkg/agent/util/iptables/iptables.go 62.50% <100.00%> (ø)
pkg/ovs/openflow/ofctrl_bridge.go 71.14% <0.00%> (-0.80%) ⬇️
.../agent/flowexporter/connections/conntrack_linux.go 30.55% <0.00%> (-0.44%) ⬇️
... and 15 more

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

LGTM

antoninbas
antoninbas previously approved these changes Nov 6, 2020
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, thanks for fixing this so quick

@tnqn
Copy link
Member Author

tnqn commented Nov 9, 2020

/test-all

In large scale clusters, xtables lock may be hold by kubelet/
kube-proxy/ portmap for a long time, especially when there are many
service rules in nat table. antrea-agent may not be able to acquire the
lock in short time. If the agent blocks on the lock or quit itself, the
CNI server won't be running, causing all CNI requests to fail.  If the
Pods' restart policy is Always and there are dead Pods, container
runtime will keep retrying calling CNIs, during which portmap is called
first, leading to more xtables lock contention.

This patch makes iptables initialization error non fatal and uses a
goroutine to retry it until success. The agent will start the CNI server
anyway and handle the CNI Del requests but won't handle CNI Add requests
until the network is ready.
@tnqn
Copy link
Member Author

tnqn commented Nov 9, 2020

@antoninbas @jianjuns I rebased on master and fixed a lint error, could you approve again?

/test-all

@tnqn
Copy link
Member Author

tnqn commented Nov 9, 2020

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

@antoninbas antoninbas merged commit dceef6b into antrea-io:master Nov 9, 2020
antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Nov 10, 2020
In large scale clusters, xtables lock may be hold by kubelet/
kube-proxy/ portmap for a long time, especially when there are many
service rules in nat table. antrea-agent may not be able to acquire the
lock in short time. If the agent blocks on the lock or quit itself, the
CNI server won't be running, causing all CNI requests to fail.  If the
Pods' restart policy is Always and there are dead Pods, container
runtime will keep retrying calling CNIs, during which portmap is called
first, leading to more xtables lock contention.

This patch makes iptables initialization error non fatal and uses a
goroutine to retry it until success. The agent will start the CNI server
anyway and handle the CNI Del requests but won't handle CNI Add requests
until the network is ready.
antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Nov 10, 2020
In large scale clusters, xtables lock may be hold by kubelet/
kube-proxy/ portmap for a long time, especially when there are many
service rules in nat table. antrea-agent may not be able to acquire the
lock in short time. If the agent blocks on the lock or quit itself, the
CNI server won't be running, causing all CNI requests to fail.  If the
Pods' restart policy is Always and there are dead Pods, container
runtime will keep retrying calling CNIs, during which portmap is called
first, leading to more xtables lock contention.

This patch makes iptables initialization error non fatal and uses a
goroutine to retry it until success. The agent will start the CNI server
anyway and handle the CNI Del requests but won't handle CNI Add requests
until the network is ready.
antoninbas pushed a commit that referenced this pull request Nov 11, 2020
In large scale clusters, xtables lock may be hold by kubelet/
kube-proxy/ portmap for a long time, especially when there are many
service rules in nat table. antrea-agent may not be able to acquire the
lock in short time. If the agent blocks on the lock or quit itself, the
CNI server won't be running, causing all CNI requests to fail.  If the
Pods' restart policy is Always and there are dead Pods, container
runtime will keep retrying calling CNIs, during which portmap is called
first, leading to more xtables lock contention.

This patch makes iptables initialization error non fatal and uses a
goroutine to retry it until success. The agent will start the CNI server
anyway and handle the CNI Del requests but won't handle CNI Add requests
until the network is ready.
@tnqn tnqn deleted the iptables-error branch December 12, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

antrea-agent kept crashing on iptables error after restarting a Node with many Pods running on it
6 participants