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

Changes to make default route programming correct in multi-npu platforms #4774

Merged
merged 3 commits into from
Jun 29, 2020

Conversation

abdosi
Copy link
Contributor

@abdosi abdosi commented Jun 14, 2020

- Why I did it
Changes to make default route programming correct in multi-npu platforms where frr is not running in host namespace.
Change is to set correct administrative distance of default route created by docker container so
that default route learn from FRR has higher priority and classify as best route.

Also made NAMESPACE* environment variable available for all dockers
so that it can be used when needed.

- How I did it
Delete and add back default route added by docker network with correct metrics/administrative distance.

- How to verify it

Before Change

  • vtysh:
show ip route 0.0.0.0/0
Routing entry for 0.0.0.0/0
  Known via "kernel", distance 0, metric 0, best
  Last update 00:00:45 ago
  * 240.127.1.1, via eth0

Routing entry for 0.0.0.0/0
  Known via "bgp", distance 200, metric 0
  Last update 06:38:11 ago
    10.1.0.8, via PortChannel4005
    10.1.0.10, via PortChannel4006
  • Kernel:
ip route show 0.0.0.0/0
default via 240.127.1.1 dev eth0

After Change

  • vtysh:
show ip route 0.0.0.0/0
Routing entry for 0.0.0.0/0
  Known via "kernel", distance 210, metric 0
  Last update 00:35:10 ago
    240.127.1.1, via eth0

Routing entry for 0.0.0.0/0
  Known via "bgp", distance 200, metric 0, best
  Last update 06:35:45 ago
  * 10.1.0.8, via PortChannel4005
  * 10.1.0.10, via PortChannel4006
  • Kernel:
ip route show 0.0.0.0/0
default proto 186 src 8.0.0.2 metric 20
        nexthop via 10.1.0.8  dev PortChannel4005 weight 1
        nexthop via 10.1.0.10  dev PortChannel4006 weight 1
default via 240.127.1.1 dev eth0 metric 3523215360

correct in multi-asic platform where frr is not running
in host namespace. Change is to set correct administrative distance.
Also make NAMESPACE* enviroment variable available for all dockers
so that it can be used when needed.

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
@abdosi
Copy link
Contributor Author

abdosi commented Jun 15, 2020

retest vsimage please

@abdosi abdosi requested a review from arlakshm June 16, 2020 00:40
Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

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

As comments.
One question and a some typos to fix.

# The top byte of the value is interpreted as the Administrative Distance and
# the low three bytes are read in as the metric.
# so here we are programming administrative distace of 210 (210 << 24) > 200 (for routes learn via IBGP)
ip route add 0.0.0.0/0 via $GATEWAY_IP dev eth0 metric 3523215360
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to modify the route, instead of removing/adding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavel-shirshov i tried two option:

  • ip route change (This command fails)

  • ip route replace (It creates one more new default route with metric)

so i feel cleaner is just to delete and add it back.

Copy link
Contributor

@pavel-shirshov pavel-shirshov Jun 17, 2020

Choose a reason for hiding this comment

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

I think you need ip route change command. What error message do you see when you apply this command?
Why is the remove/add sequence cleaner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavel-shirshov :
On using route change getting error "RTNETLINK answers: No such file or directory"
ip route change 0.0.0.0/0 via 240.127.1.1 dev eth0 metric 200
RTNETLINK answers: No such file or directory

My point for cleaner was w.r.t to "ip route replace" since it creates one more new default route with metric so we will have two routes.
If we del/add then there is only one and it's cleaner in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavel-shirshov Can you please review this. I have address your comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

@abdosi I think it is better to "change" or check that the route was removed. Otherwise we have a chance to introduce another route without removing the previous one.

Copy link
Contributor Author

@abdosi abdosi Jun 26, 2020

Choose a reason for hiding this comment

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

@pavel-shirshov added check to add default route only if docker default route exist and delete is successful

dockers/docker-fpm-frr/docker_init.sh Outdated Show resolved Hide resolved
dockers/docker-fpm-frr/docker_init.sh Outdated Show resolved Hide resolved
@abdosi
Copy link
Contributor Author

abdosi commented Jun 17, 2020

retest vsimage please

only if default route exist and delete is successful.
@abdosi
Copy link
Contributor Author

abdosi commented Jun 29, 2020

retest mellanox please

@abdosi abdosi merged commit 15440b6 into sonic-net:master Jun 29, 2020
@abdosi abdosi deleted the default_route branch June 29, 2020 18:38
abdosi added a commit that referenced this pull request Jul 5, 2020
…rms (#4774)

* Changes to make default route programming
correct in multi-asic platform where frr is not running
in host namespace. Change is to set correct administrative distance.
Also make NAMESPACE* enviroment variable available for all dockers
so that it can be used when needed.

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

* Fix review comments

* Review comment to check to add default route
only if default route exist and delete is successful.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants