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.

Sign up for GitHub

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

Jump to bottom

Openstack: fix orphaned route deletion #62729

Merged
merged 2 commits into from
Apr 30, 2018
Merged

Openstack: fix orphaned route deletion #62729

merged 2 commits into from
Apr 30, 2018

Conversation

databus23
Copy link
Contributor

@databus23 databus23 commented Apr 17, 2018 •
edited
Loading

This is a follow-up to #56258 which only got half of the work done.
The OpenStack cloud providers DeleteRoute method fails to delete routes when it can’t find the corresponding instance in OpenStack.

OpenStack cloudprovider: Fix deletion of orphaned routes

@databus23
fix route deletion
de15b85 
This is a follow-up to kubernetes#56258 which only half of the work done.
The DeleteRoute method failed to delete routes when it can’t find the corresponding node in OpenStack.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 17, 2018
@databus23 databus23 changed the title fix route deletion Openstack: fix orphaned route deletion Apr 17, 2018
@databus23 databus23 mentioned this pull request Apr 17, 2018
Merged
@databus23
Copy link
Contributor Author

/assign @dims

@edisonxiang
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 18, 2018
FengyunPan2
FengyunPan2 approved these changes Apr 18, 2018
View reviewed changes
Copy link
Contributor

@FengyunPan2 FengyunPan2 left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2018
@databus23
Copy link
Contributor Author

I just noticed there is a potential quirk that could cause valid routes for nodes to be deleted.

If there are two routes with the same DestinationCIDR one legit, one orphaned, the current logic will delete both. The legit one will be recreated by the cloud provider but its still undesirable.

I will update the PR and try to prevent this from happening

@dims
Copy link
Member

dims commented Apr 18, 2018

/hold

(per last comment from @databus23)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2018
@databus23
Copy link
Contributor Author

@dims @FengyunPan2 I updated the PR to compare next-hops when deleting orphaned routes. This should ensure we don't delete valid routes to the same DestinationCIDR. Its a bit wonky because cloudprovider.Route has no field for the the next-hop address so I had to reuse TargetNode.

Wdyt?

@databus23
ensure we delete orphaned routes with matching next-hops only
65d4147 
There might be a valid route with the same DestinationCIDR pointing to a running node.
@dims
Copy link
Member

dims commented Apr 24, 2018

@FengyunPan2 please check

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 24, 2018
@dims
Copy link
Member

dims commented Apr 30, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: databus23, dims, FengyunPan2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 59879, 62729). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 7ffd171 into kubernetes:master Apr 30, 2018
@databus23
Copy link
Contributor Author

Thanks @dims! Can we get this in 1.10.x as I would consider it a bugfix?

@dims
Copy link
Member

dims commented Apr 30, 2018

@databus23 sure, please file one https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md#initiate-a-cherry-pick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FengyunPan2 FengyunPan2 FengyunPan2 approved these changes

@NickrenREN NickrenREN Awaiting requested review from NickrenREN

Assignees

@dims dims

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@databus23 @edisonxiang @dims @k8s-ci-robot @k8s-github-robot @FengyunPan2