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

impl of withdrawroute #25

Merged
merged 3 commits into from
Jun 28, 2022
Merged

impl of withdrawroute #25

merged 3 commits into from
Jun 28, 2022

Conversation

byteocean
Copy link
Contributor

Proposed Changes

This PR provides am implementation of withdrawing a single route from metalbond.

return fmt.Errorf("cannot remove route from the local announcement route table: %v", err)
}

if remaining == 0 {
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 this is wrong.
When we remove a nexthop this info should also be distributed to the peers, even if there are some remaining nexthops for this route.

Copy link
Contributor Author

@byteocean byteocean Jun 27, 2022

Choose a reason for hiding this comment

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

It is just written (actually the same way) as removeReceivedRoute(), which keeps the consistent behaviour as the metalbond server would do when it learns that a route (with a next hop) needs to be deleted.

The next hop things could be taken care of in a more thorough way in the near future, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

No. removeReceivedRoute() removes the nexthop. And then if the route has 0 nexthops, it removes the complete route from the table.
Here we want to de-announce a prior nexthop announcement. So the remaining == 0 check is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed in our Call, please add a // TODO: check logic remark above the if statement.

}

if remaining == 0 {
if err := m.distributeRouteToPeers(REMOVE, vni, dest, hop, nil); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

AnnounceRoute() calls distributeRouteToPeers() after a m.peerMtx.RLock() lock on the peer Mutex.
This should also be done here, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's true

@byteocean
Copy link
Contributor Author

This PR still has an open issue, that is referred and discussed in https://github.com/onmetal/metalbond/issues/23

@MalteJ MalteJ merged commit fe58b67 into master Jun 28, 2022
@MalteJ MalteJ deleted the withdraw_dev branch June 28, 2022 07:55
@afritzler afritzler added the enhancement New feature or request label Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants