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 read route tag value from Netlink Message and take appropriate action #3353

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

abdosi
Copy link
Contributor

@abdosi abdosi commented Nov 5, 2024

What I did:

Added support to compile and load code to read yaml file in
routesync.cpp. In routesyncd we read tag from constants.yml and take action
of not to send route to appdb and also fallback to default route.

Why I did:

Corresponding PR on FRR : sonic-net/sonic-buildimage#20692 to send route tag to fpmsyncd.
PR to specify constants.yml tag value: sonic-net/sonic-buildimage#20224
PR for installing libyaml package: sonic-net/sonic-buildimage#20698

@abdosi
Copy link
Contributor Author

abdosi commented Nov 9, 2024

@prsunny : can you help review this.

catch (const exception &e)
{
cout << "Exception \"" << e.what() << "\" had been thrown in daemon in loading constants.yml" << endl;
route_tag_not_to_appdb = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Non T2 devices might always print the exception log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only if fails to reads constants.yml which should not happen. this attribute are define in constants.yml (irrespective of the role)
https://github.com/sonic-net/sonic-buildimage/pull/20224/files#diff-e6f2fe13a6c276dc2f3b27a5bef79886f9c103194be4fcb28ce57375edf2c23c

@@ -693,6 +710,13 @@ void RouteSync::onRouteMsg(int nlmsg_type, struct nl_object *obj, char *vrf)
destipprefix[strlen(vrf)] = ':';
}

tag = rtnl_route_get_priority(route_obj);

if (tag == route_tag_not_to_appdb)
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand on pizza box, route_tag_not_to_appdb is zero. If for any route the priority is zero then route will not be added to app_db. So the route will always has non-zero priority?

Copy link
Contributor Author

@abdosi abdosi Nov 13, 2024

Choose a reason for hiding this comment

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

it should not be zero. It always reads from constants.yml. Only time this will be zero in case of fail condition of loading/reading the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should not be zero. It always reads from constants.yml. Updated the fail case to be non-zero as 0xffffffff

if (tag == route_tag_not_to_appdb)
return;
else if (tag == route_tag_fallback_to_default_route)
route_eligible_for_fallback_to_default_route = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Signed-off-by: Abhishek Dosi <[email protected]>
rlhui pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Nov 18, 2024
* Added package to compile and link yaml parser in C++

* Update Dockerfile.j2

To compile and link changes done in PR: sonic-net/sonic-swss#3353
---------

Signed-off-by: Abhishek Dosi <[email protected]>
@prsunny prsunny requested a review from dgsudharsan November 22, 2024 19:51
/debs/libsaivs_1.0.0_amd64.deb \
/debs/syncd-vs_1.0.0_amd64.deb \
/debs/swss_1.0.0_amd64.deb
RUN apt-get update
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please explain motivation for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgsudharsan : To resolve lyaml-cpp dependency of swss debian package. As apt install take care of resolve all dependency

@@ -1714,6 +1738,12 @@ void RouteSync::onRouteMsg(int nlmsg_type, struct nl_object *obj, char *vrf)
fvVector.push_back(wt);
}

if (route_eligible_for_fallback_to_default_route)
{
FieldValueTuple tag("fallback_to_default_route", "true");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this field processed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants