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

Update openflow rules when Gateway updates #4388

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

luolanzone
Copy link
Contributor

There is a bug when GatewayIP or InternalIP of active Gateway is changed, the corresponding openflow rules won't be updated until any other event triggers the flow sync process.

Fix the bug by comparing installed active Gateway's InternalIP and GatewayIP with current active Gateway. And add more unit tests to cover Gateway update event.

Signed-off-by: Lan Luo [email protected]

@luolanzone luolanzone added area/multi-cluster Issues or PRs related to multi cluster. action/backport Indicates a PR that requires backports. labels Nov 9, 2022
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #4388 (9352c8b) into main (21909f4) will increase coverage by 0.17%.
The diff coverage is 72.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4388      +/-   ##
==========================================
+ Coverage   64.85%   65.03%   +0.17%     
==========================================
  Files         397      410      +13     
  Lines       56239    56666     +427     
==========================================
+ Hits        36473    36850     +377     
- Misses      17097    17148      +51     
+ Partials     2669     2668       -1     
Flag Coverage Δ
e2e-tests 40.73% <0.00%> (?)
integration-tests 34.54% <ø> (+0.04%) ⬆️
kind-e2e-tests 47.63% <0.00%> (-0.93%) ⬇️
unit-tests 48.84% <72.50%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pkg/agent/multicluster/mc_route_controller.go 55.08% <72.50%> (+2.63%) ⬆️
...g/controller/supportbundlecollection/controller.go 44.60% <0.00%> (-31.33%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go 51.08% <0.00%> (-22.95%) ⬇️
pkg/agent/flowexporter/exporter/certificate.go 27.77% <0.00%> (-22.23%) ⬇️
pkg/ipfix/ipfix_process.go 81.25% <0.00%> (-18.75%) ⬇️
pkg/agent/cniserver/ipam/ipam_service.go 76.40% <0.00%> (-12.36%) ⬇️
pkg/agent/flowexporter/exporter/exporter.go 71.53% <0.00%> (-9.91%) ⬇️
pkg/agent/controller/networkpolicy/reject.go 66.99% <0.00%> (-5.92%) ⬇️
pkg/util/k8s/client.go 46.66% <0.00%> (-3.34%) ⬇️
pkg/util/ip/ip.go 86.99% <0.00%> (-3.26%) ⬇️
... and 57 more

pkg/agent/multicluster/mc_route_controller.go Outdated Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Outdated Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Outdated Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Outdated Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Outdated Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Outdated Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Outdated Show resolved Hide resolved
@@ -282,15 +291,13 @@ func (c *MCRouteController) syncMCFlowsForAllCIImps(activeGW *mcv1alpha1.Gateway
return err
}

activeGWNochange := c.checkGateWayIPChange(activeGW)
Copy link
Contributor

Choose a reason for hiding this comment

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

activeGWNoChange

But maybe rename it to "noActiveGWChange".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to activeGWChanged.

@@ -310,9 +329,9 @@ func (c *MCRouteController) addMCFlowsForAllCIImps(activeGW *mcv1alpha1.Gateway)
klog.V(2).InfoS("No remote ClusterInfo imported, do nothing")
return nil
}

activeGWNochange := c.checkGateWayIPChange(activeGW)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just pass "false" to addMCFlowsForSingleCIImp()? addMCFlowsForAllCIImps is called when activeGW is not set earlier, and so it cannot be noChange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right, change it to true since the name is activeGWChanged now.

@@ -323,19 +342,24 @@ func (c *MCRouteController) addMCFlowsForAllCIImps(activeGW *mcv1alpha1.Gateway)
return nil
}

func (c *MCRouteController) addMCFlowsForSingleCIImp(activeGW *mcv1alpha1.Gateway, ciImport *mcv1alpha1.ClusterInfoImport, installedCIImp *mcv1alpha1.ClusterInfoImport) error {
func (c *MCRouteController) addMCFlowsForSingleCIImp(activeGW *mcv1alpha1.Gateway, ciImport *mcv1alpha1.ClusterInfoImport,
installedCIImp *mcv1alpha1.ClusterInfoImport, activeGWNochange bool) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - just from this func perspective, maybe easier to understand if we pass an "activeGWChanged" argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@luolanzone luolanzone force-pushed the fix-gateway-update branch 2 times, most recently from fe26508 to ec3fa49 Compare November 15, 2022 08:23
jianjuns
jianjuns previously approved these changes Nov 15, 2022
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@jianjuns
Copy link
Contributor

/test-all

@jianjuns
Copy link
Contributor

@luolanzone : UBI build is failing. Could you rebase and fix that?

There is a bug when GatewayIP or InternalIP of active Gateway is
changed, the corresponding openflow rules won't be updated until
any other event triggers the flow sync process.

Fix the bug by comparing installed active Gateway's InternalIP and
GatewayIP with current active Gateway. And add more unit tests to
cover Gateway update event.

Signed-off-by: Lan Luo <[email protected]>
@luolanzone
Copy link
Contributor Author

@jianjuns I re-based and pushed the latest codes. thanks.

@jianjuns
Copy link
Contributor

Ok. Please make sure test-all and required tests passed.

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e
/test-all

1 similar comment
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e
/test-all

@luolanzone
Copy link
Contributor Author

@XinShuYang Could you help to check if jenkins has some problem to send back the test result? I can see multicluster e2e passed in the jenkins, but it shows failed here in the github check.

@XinShuYang
Copy link
Contributor

/test-multicluster-e2e

@XinShuYang
Copy link
Contributor

/test-all

@XinShuYang
Copy link
Contributor

/test-networkpolicy

heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
)

When GatewayIP or InternalIP of active Gateway changes, the
corresponding Openflow rules are not updated until any other
event triggers the flow sync process.

Fix the issue by comparing installed active Gateway's InternalIP and
GatewayIP with the current active Gateway. And add more unit tests
to cover Gateway update events.

Signed-off-by: Lan Luo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports. area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants