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 error loop caused by failure to delete a non-existing route #3827

Merged
merged 1 commit into from May 26, 2022

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented May 25, 2022

When a LoadBalancer with ingress IP is created, corresponding route
for the ingress IP is also installed. When deleting the LoadBalancer,
corresponding route should be also uninstalled. Before uninstalling
the route, if the route is removed, then deleting the route will
get a failure and return an error. The error will trigger LoadBalancer
deletion again, and obviously, deleting the route will get a failure
and return an error another time, resulting in an error loop. This PR
fixes error loop by printing log instead of returning error when getting
the error "no such process" (such error means that the route expected to
be deleted doesn't exist).

Signed-off-by: Hongliang Liu lhongliang@vmware.com

@hongliangl hongliangl requested a review from jianjuns May 25, 2022 15:34
@hongliangl hongliangl closed this May 25, 2022
@hongliangl hongliangl reopened this May 25, 2022
@hongliangl hongliangl marked this pull request as ready for review May 25, 2022 15:34
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2022

Codecov Report

Merging #3827 (17980e1) into main (d6518c0) will increase coverage by 7.94%.
The diff coverage is 0.00%.

❗ Current head 17980e1 differs from pull request most recent head d99e856. Consider uploading reports for the commit d99e856 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3827      +/-   ##
==========================================
+ Coverage   38.20%   46.14%   +7.94%     
==========================================
  Files         114      247     +133     
  Lines       15688    35914   +20226     
==========================================
+ Hits         5993    16572   +10579     
- Misses       9215    17704    +8489     
- Partials      480     1638    +1158     
Flag Coverage Δ
e2e-tests 46.14% <0.00%> (?)
integration-tests ?

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

Impacted Files Coverage Δ
pkg/agent/multicast/mcast_controller.go 0.00% <0.00%> (ø)
pkg/agent/multicast/mcast_discovery.go 0.00% <0.00%> (ø)
pkg/agent/route/route_linux.go 27.92% <0.00%> (ø)
.../pkg/agent/flowexporter/connections/connections.go
antrea/pkg/agent/proxy/endpoints.go
antrea/pkg/ovs/openflow/ofctrl_packetin.go
antrea/pkg/util/k8s/client.go
antrea/pkg/ovs/ovsconfig/ovs_client.go
antrea/pkg/ovs/openflow/ofctrl_builder.go
antrea/pkg/version/version.go
... and 354 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.

In the commit message:

The error will trigger the event of delete the LoadBalancer again, obviously,

delete -> deleting or just "trigger LoadBalancer deletion again"

obviously -> and obviously

@@ -1298,9 +1298,10 @@ func (c *Client) deleteLoadBalancerIngressIPRoute(svcIPStr string) error {

route := generateRoute(svcIP, mask, gw, linkIndex, netlink.SCOPE_UNIVERSE)
if err := netlink.RouteDel(route); err != nil {
return fmt.Errorf("failed to delete routing entry for LoadBalancer ingress IP %s: %w", svcIP.String(), err)
klog.Warningf("Failed to delete routing entry for LoadBalancer ingress IP %s: %w", svcIP.String(), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to check and ignore only the not-found/exist error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can log not-found/exist error.

@hongliangl hongliangl changed the title Fix error loop caused by failure to delete a route Fix error loop caused by failure to delete a non-existing route May 26, 2022
When a LoadBalancer with ingress IP is created, corresponding route
for the ingress IP is also installed. When deleting the LoadBalancer,
corresponding route should be also uninstalled. Before uninstalling
the route, if the route is removed, then deleting the route will
get a failure and return an error. The error will trigger LoadBalancer
deletion again, and obviously, deleting the route will get a failure
and return an error another time, resulting in an error loop. This PR
fixes error loop by printing log instead of returning error when getting
the error "no such process" (such error means that the route expected to
be deleted doesn't exist).

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
@jianjuns
Copy link
Contributor

/test-all

@jianjuns
Copy link
Contributor

/test-ipv6-e2e

@jianjuns
Copy link
Contributor

/test-e2e

@jianjuns jianjuns merged commit 5efac86 into antrea-io:main May 26, 2022
@hongliangl hongliangl deleted the fix-proxy-error-loop branch May 27, 2022 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants