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

Fix route deletion for Service ClusterIP and LoadBalancerIP #4711

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Mar 16, 2023

When proxyAll is enabled, AntreaProxy needs to install routes in the host network namespace to redirect traffic to OVS for load balancing. For a Service with multiple ports, multiple ServicePorts are generated and processed. The previous code installed the route for a ClusterIP or a LoadBalancerIP multiple times when such a Service was created, and uninstalled the route multiple times when it was deleted, leading to a few problems.

This patch adds a serviceIPRouteReferences which tracks the references of Service IPs' routes. The key is the Service IP and the value is the the set of ServiceInfo strings. With the references, we install a route exactly once as long as it's used by any ServicePorts and uninstall it exactly once when it's no longer used by any ServicePorts.

This patch also fixes an issue that the route for ClusterIP was not removed on Windows Nodes after the Service was removed.

The patch also makes shared LoadBalancerIP work correctly.

Fixes #4361

@tnqn tnqn added this to the Antrea v1.11 release milestone Mar 16, 2023
@tnqn tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Mar 16, 2023
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
@@ -542,8 +594,8 @@ func (p *proxier) installServices() {
}
}
// If previous Service which has ClusterIP should be removed, remove ClusterIP routes.
if svcInfo.ClusterIP() != nil {
if err := p.routeClient.DeleteClusterIPRoute(pSvcInfo.ClusterIP()); err != nil {
if pSvcInfo.ClusterIP() != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

// ServicePort (which is the unit of the processing), a Service IP route may be required by several ServicePorts.
// With the references, we install a route exactly once as long as it's used by any ServicePorts and uninstall it
// exactly once when it's no longer used by any ServicePorts.
// It applies to ClusterIP and LoadBalancerIP.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we could merge #3889, I think we could remove the code change for ClusterIP in this PR in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Previous releases had windows route issue, so better to merge this one before #3889 to make backport easier.

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

hongliangl
hongliangl previously approved these changes Mar 17, 2023
Copy link
Contributor

@hongliangl hongliangl left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl
Copy link
Contributor

/test-all

When proxyAll is enabled, AntreaProxy needs to install routes in the
host network namespace to redirect traffic to OVS for load balancing.
For a Service with multiple ports, multiple ServicePorts are generated
and processed. The previous code installed the route for a ClusterIP or
a LoadBalancerIP multiple times when such a Service was created, and
uninstalled the route multiple times when it was deleted, leading to a
few problems.

This patch adds a serviceIPRouteReferences which tracks the references
of Service IPs' routes. The key is the Service IP and the value is the
the set of ServiceInfo strings. With the references, we install a route
exactly once as long as it's used by any ServicePorts and uninstall it
exactly once when it's no longer used by any ServicePorts.

This patch also fixes an issue that the route for ClusterIP was not
removed on Windows Nodes after the Service was removed.

Fixes antrea-io#4361

Signed-off-by: Quan Tian <[email protected]>
@tnqn
Copy link
Member Author

tnqn commented Mar 17, 2023

/test-all

@tnqn tnqn merged commit d2c4ef8 into antrea-io:main Mar 17, 2023
@tnqn tnqn deleted the fix-svc-lb-ip branch March 17, 2023 12:33
This was referenced Mar 24, 2023
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
…o#4711)

When proxyAll is enabled, AntreaProxy needs to install routes in the
host network namespace to redirect traffic to OVS for load balancing.
For a Service with multiple ports, multiple ServicePorts are generated
and processed. The previous code installed the route for a ClusterIP or
a LoadBalancerIP multiple times when such a Service was created, and
uninstalled the route multiple times when it was deleted, leading to a
few problems.

This patch adds a serviceIPRouteReferences which tracks the references
of Service IPs' routes. The key is the Service IP and the value is the
the set of ServiceInfo strings. With the references, we install a route
exactly once as long as it's used by any ServicePorts and uninstall it
exactly once when it's no longer used by any ServicePorts.

This patch also fixes an issue that the route for ClusterIP was not
removed on Windows Nodes after the Service was removed.

Fixes antrea-io#4361

Signed-off-by: Quan Tian <[email protected]>
@tnqn tnqn added the kind/bug Categorizes issue or PR as related to a bug. label Jan 23, 2024
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. action/release-note Indicates a PR that should be included in release notes. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

route entry would disappear after the LoadBalancer is registered
3 participants