Enhance error handling of policy reconciler #1667

tnqn · 2020-12-16T17:11:51Z

The reconciler might fail to install flows due to transient OVS error. It should add the IPBlocks to the PolicyRule when it retries, in which case "update" method is called. This patch fixes it and adds an unit test for it.

Fixes #1635

tnqn · 2020-12-16T17:15:45Z

/test-all

codecov-io · 2020-12-16T17:16:03Z

Codecov Report

Merging #1667 (6fa3bd0) into master (9d3d10b) will increase coverage by 1.10%.
The diff coverage is 59.43%.

@@            Coverage Diff             @@
##           master    #1667      +/-   ##
==========================================
+ Coverage   63.31%   64.42%   +1.10%     
==========================================
  Files         170      181      +11     
  Lines       14250    15494    +1244     
==========================================
+ Hits         9023     9982     +959     
- Misses       4292     4484     +192     
- Partials      935     1028      +93

Flag	Coverage Δ
e2e-tests	`47.52% <38.95%> (?)`
kind-e2e-tests	`53.10% <51.75%> (-2.29%)`	⬇️
unit-tests	`40.82% <27.20%> (-0.46%)`	⬇️

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

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
.../agent/apiserver/handlers/networkpolicy/handler.go	`58.33% <ø> (ø)`
...gent/controller/noderoute/node_route_controller.go	`60.98% <0.00%> (+14.51%)`	⬆️
pkg/agent/stats/collector.go	`97.72% <ø> (ø)`
pkg/antctl/antctl.go	`100.00% <ø> (ø)`
pkg/antctl/transform/addressgroup/transform.go	`0.00% <0.00%> (ø)`
pkg/antctl/transform/controllerinfo/transform.go	`0.00% <ø> (ø)`
pkg/antctl/transform/networkpolicy/transform.go	`0.00% <0.00%> (ø)`
pkg/antctl/transform/version/transform.go	`44.82% <ø> (ø)`
pkg/controller/networkpolicy/tier.go	`90.00% <ø> (ø)`
... and 105 more

antoninbas

LGTM, but I need a clarification regarding the necessity of forgetRuleImmediately. I understand why it could be an "optimization" to release the rule ID immediately when there is an error, but I don't understand why it is necessary for code correctness.

pkg/agent/controller/networkpolicy/reconciler.go

pkg/agent/controller/networkpolicy/reconciler_test.go

suwang48404 · 2020-12-16T22:11:18Z

LGTM.

I suspect @antoninbas and @Dyanngg may be better reviewer than I here.

tnqn · 2020-12-17T05:23:38Z

@antoninbas @suwang48404 I just realized the BatchInstallPolicyRuleFlows method is not atomical because it doesn't roll back some conjunction cache in agent upon failure and confirmed with @Dyanngg. So using it would introduce another bug if there is transient error in OVS. I don't think of how to make the method atomical with simple change. I have to use another approach to track each openflow rule's realization status... Let me know if you have good idea for this problem.

tnqn · 2020-12-17T10:41:48Z

/test-all

tnqn · 2020-12-17T13:15:21Z

/test-all

antoninbas · 2020-12-17T20:42:06Z

@antoninbas @suwang48404 I just realized the BatchInstallPolicyRuleFlows method is not atomical because it doesn't roll back some conjunction cache in agent upon failure and confirmed with @Dyanngg. So using it would introduce another bug if there is transient error in OVS. I don't think of how to make the method atomical with simple change. I have to use another approach to track each openflow rule's realization status... Let me know if you have good idea for this problem.

I would think that addressing this TODO https://github.com/vmware-tanzu/antrea/blob/8b7430cc193bd657ca00277a417406ca9558df9b/pkg/agent/openflow/network_policy.go#L848 would be the right thing to do, but I am not super familiar with that code TBH.

It seems that you went with a different approach. Does it cover all possible error cases when adding flows, are just the one Su ran into?

Dyanngg · 2020-12-17T23:14:54Z

It seems that you went with a different approach. Does it cover all possible error cases when adding flows, are just the one Su ran into?

I would say what Su discovered is the only possible error case I can think of as of now that the update function cannot handle. The update function has does all the computation as add except for the egress ipBlock. This was also the approach I had in my if I were to fix this issue.

tnqn · 2020-12-18T05:15:44Z

@antoninbas I agree that making the BatchInstallPolicyRuleFlows method atomical would be the best. However I am not super familiar with that code either. It would take some time for me to figure out how to achieve it, and guess it's not a simple change. @Dyanngg might comment.

This patch fixes the case Su reported, it should be the only case could happen if InstallPolicyRuleFlows is atomical, unfortunally even that method is not at the moment. Besides, the update method calls multiple openflow methods, which could cause potential issues.
We may need to revise the openflow interfaces first, then we can switch to the atomical interfaces in reconciler, that will simplify the code in reconciler a lot I guess.

antoninbas · 2020-12-18T05:17:16Z

@tnqn @Dyanngg thanks for the clarifications

antoninbas

minor comments, otherwise LGTM

pkg/agent/controller/networkpolicy/reconciler_test.go

tnqn

@antoninbas addressed your comments, thanks for the review.

pkg/agent/controller/networkpolicy/reconciler_test.go

tnqn · 2020-12-18T11:22:40Z

/test-all

pkg/agent/controller/networkpolicy/reconciler_test.go

The reconciler might fail to install flows due to transient OVS error. It should add the IPBlocks to the PolicyRule when it retries, in which case "update" method is called. This patch fixes it and adds an unit test for it.

tnqn · 2020-12-21T02:22:00Z

/test-all

The reconciler might fail to install flows due to transient OVS error. It should add the IPBlocks to the PolicyRule when it retries, in which case "update" method is called. This patch fixes it and adds an unit test for it.

vmwclabot added the cla-not-required label Dec 16, 2020

tnqn added this to the Antrea v0.12.0 release milestone Dec 16, 2020

tnqn requested review from Dyanngg, suwang48404 and antoninbas December 16, 2020 17:12

antoninbas reviewed Dec 16, 2020

View reviewed changes

pkg/agent/controller/networkpolicy/reconciler.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/reconciler_test.go Outdated Show resolved Hide resolved

tnqn force-pushed the policy-update branch 2 times, most recently from 45695a6 to fa9bb85 Compare December 17, 2020 10:41

tnqn force-pushed the policy-update branch 2 times, most recently from 34743cf to a12ab7b Compare December 17, 2020 12:44

tnqn mentioned this pull request Dec 18, 2020

Openflow networkpolicy interfaces should be atomic to avoid inconsistency and potential incorrect behavior #1673

Closed

antoninbas reviewed Dec 18, 2020

View reviewed changes

pkg/agent/controller/networkpolicy/reconciler_test.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/reconciler_test.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/reconciler_test.go Show resolved Hide resolved

tnqn commented Dec 18, 2020

View reviewed changes

pkg/agent/controller/networkpolicy/reconciler_test.go Show resolved Hide resolved

pkg/agent/controller/networkpolicy/reconciler_test.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/reconciler_test.go Outdated Show resolved Hide resolved

tnqn force-pushed the policy-update branch from a12ab7b to 55d8d39 Compare December 18, 2020 11:21

antoninbas previously approved these changes Dec 18, 2020

View reviewed changes

pkg/agent/controller/networkpolicy/reconciler_test.go Outdated Show resolved Hide resolved

Enhance error handling of policy reconciler

6fa3bd0

The reconciler might fail to install flows due to transient OVS error. It should add the IPBlocks to the PolicyRule when it retries, in which case "update" method is called. This patch fixes it and adds an unit test for it.

tnqn dismissed antoninbas’s stale review via 6fa3bd0 December 21, 2020 02:21

tnqn force-pushed the policy-update branch from 55d8d39 to 6fa3bd0 Compare December 21, 2020 02:21

antoninbas approved these changes Dec 21, 2020

View reviewed changes

antoninbas merged commit f86c262 into antrea-io:master Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance error handling of policy reconciler #1667

Enhance error handling of policy reconciler #1667

tnqn commented Dec 16, 2020 •

edited

Loading

tnqn commented Dec 16, 2020

codecov-io commented Dec 16, 2020 •

edited

Loading

antoninbas left a comment

suwang48404 commented Dec 16, 2020

tnqn commented Dec 17, 2020

tnqn commented Dec 17, 2020

tnqn commented Dec 17, 2020

antoninbas commented Dec 17, 2020

Dyanngg commented Dec 17, 2020

tnqn commented Dec 18, 2020

antoninbas commented Dec 18, 2020

antoninbas left a comment

tnqn left a comment •

edited

Loading

tnqn commented Dec 18, 2020

tnqn commented Dec 21, 2020

Enhance error handling of policy reconciler #1667

Enhance error handling of policy reconciler #1667

Conversation

tnqn commented Dec 16, 2020 • edited Loading

tnqn commented Dec 16, 2020

codecov-io commented Dec 16, 2020 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

suwang48404 commented Dec 16, 2020

tnqn commented Dec 17, 2020

tnqn commented Dec 17, 2020

tnqn commented Dec 17, 2020

antoninbas commented Dec 17, 2020

Dyanngg commented Dec 17, 2020

tnqn commented Dec 18, 2020

antoninbas commented Dec 18, 2020

antoninbas left a comment

Choose a reason for hiding this comment

tnqn left a comment • edited Loading

Choose a reason for hiding this comment

tnqn commented Dec 18, 2020

tnqn commented Dec 21, 2020

tnqn commented Dec 16, 2020 •

edited

Loading

codecov-io commented Dec 16, 2020 •

edited

Loading

tnqn left a comment •

edited

Loading