staticd: correct table-id handling for static routes. #7844

vishaldhingra · 2021-01-10T08:18:59Z

I have added the regression for table-id handling, while submitting yang changes.
This PR is about to fix this problem.

Issue
[https://github.com//issues/7806]

modified the yang model for path-list. table-id should be a key, as one route can have multiple table-ids. Signed-off-by: vishaldhingra <vdhingra@vmware.com>

updated callback methods based on autogenerated code. Signed-off-by: vishaldhingra <vdhingra@vmware.com>

LabN-CI · 2021-01-10T08:56:06Z

Outdated results 💚

Basic BGPD CI results: SUCCESS, 0 tests failed

_	_
Result	SUCCESS git merge/7844 `433d59f`
Date	01/10/2021
Start	03:20:45
Finish	03:56:01
Run-Time	35:16
Total	1815
Pass	1815
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2021-01-10-03:20:45.txt
Log	autoscript-2021-01-10-03:21:44.log.bz2
Memory	492 481 428

For details, please contact louberger

NetDEF-CI · 2021-01-10T11:23:01Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16412/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Debian 10 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 10 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16412/artifact/DEB10BUILD/ErrorLog/log_lintian.txt)

W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210110-02-g433d59fe2-0 (missing) -> 7.6-dev-20210110-02-g433d59fe2-0~deb10u1
W: frr-rpki-rtrlib: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210110-02-g433d59fe2-0 (missing) -> 7.6-dev-20210110-02-g433d59fe2-0~deb10u1
W: frr-snmp: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210110-02-g433d59fe2-0 (missing) -> 7.6-dev-20210110-02-g433d59fe2-0~deb10u1
W: frr-pythontools: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210110-02-g433d59fe2-0 (missing) -> 7.6-dev-20210110-02-g433d59fe2-0~deb10u1
W: frr-doc: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210110-02-g433d59fe2-0 (missing) -> 7.6-dev-20210110-02-g433d59fe2-0~deb10u1

mjstapp · 2021-01-11T18:45:40Z

staticd/static_nb_config.c

+		    && !vrf_is_backend_netns()) {
+			snprintf(
+				args->errmsg, args->errmsg_len,
+				"%% table param only available when running on netns-based vrfs");


hmm - I think specifying the table for a route has been supported. is this new - is there a reason to impose this restriction?

yea this shouldn't be restricted. vrf_netns or vrf-lite, people should be allowed to install routes to whatever tables they want with staticd.

In the context of below PR, we had a discussion and we agreed on above check.
[(https://github.com//pull/7329)]

Oh is this an instance where we have both VRF and table ID? You may be right to disallow then.

Specifically, a user should be able to do ip route 1.1.1.1/32 2.2.2.2 table 1001 regardless of vrf-netns or vrf-lite.

as long as they can still do the above, I am fine with it.

maybe it would be clearer if the ordering of the tests in that block were different: test for the presence of a vrf first then test for a table_id and not-netns, to identify the invalid case?

@mjstapp : We need to test this only, when there is a valid tableId(Here valid means > 0).
This API gets called even when tableId is zero. So It's better we should check tableId first.
In the other case we are keep on executing vrf check, even when tableId is zero.

so ... it sounds as if you think there's a performance problem - but of course, since this is config and yang and nb and we know that is very slow, it's hard to imagine that making the intent of this block clearer is really going to have a performance impact?

if you don't want to change the order, then we need a comment - since two of us had trouble decoding the intent of the tests? "table id and vrf together allowed only for netns vrf backend"

Sure Mark, I will add the comment.
Yang and NB are slow, agreed, But we are doing our best to achieve scale.

Done, Thanks @mjstapp and @sworleys for your review.

problem: table-id gets overwritten for a given route. RCA: table-id was getting overwritten from the NB layer, So route was getting installed with the latest table-id. Fix: make the table-id as the key in the NB layer. This will program the route in zebra correctly. - Removed the table-id modify callbacks. - Moved the validate and apply table-id changes to path-list creation issue FRRouting#7347 Signed-off-by: vishaldhingra <vdhingra@vmware.com>

NetDEF-CI · 2021-01-12T18:49:55Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16483/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Debian 10 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 10 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16483/artifact/DEB10BUILD/ErrorLog/log_lintian.txt)

W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr-snmp: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gef4b6b223-0 (missing) -> 7.6-dev-20210112-00-gef4b6b223-0~deb10u1
W: frr-pythontools: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gef4b6b223-0 (missing) -> 7.6-dev-20210112-00-gef4b6b223-0~deb10u1
W: frr-doc: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gef4b6b223-0 (missing) -> 7.6-dev-20210112-00-gef4b6b223-0~deb10u1
W: frr: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gef4b6b223-0 (missing) -> 7.6-dev-20210112-00-gef4b6b223-0~deb10u1
W: frr-rpki-rtrlib: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gef4b6b223-0 (missing) -> 7.6-dev-20210112-00-gef4b6b223-0~deb10u1

LabN-CI · 2021-01-12T23:30:09Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/7844 `ef4b6b2`
Date	01/12/2021
Start	17:54:49
Finish	18:30:03
Run-Time	35:14
Total	1815
Pass	1815
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2021-01-12-17:54:49.txt
Log	autoscript-2021-01-12-17:55:45.log.bz2
Memory	482 486 421

For details, please contact louberger

mjstapp

Looks good to me now

sworleys · 2021-01-13T20:04:55Z

does this need to be backported?

vishaldhingra · 2021-01-14T05:53:28Z

does this need to be backported?

yes, do I need to raise separate PR ?

mjstapp · 2021-01-14T13:07:23Z

does this need to be backported?

yes, do I need to raise separate PR ?

Yes, we'll need another PR for the 7.5 branch. but the review process is usually quick...

vishaldhingra added 2 commits January 9, 2021 23:22

staticd: make table-id as the key for path-list

ad6149e

modified the yang model for path-list. table-id should be a key, as one route can have multiple table-ids. Signed-off-by: vishaldhingra <vdhingra@vmware.com>

staticd: autogenerated code modifications due to yang changes

d787163

updated callback methods based on autogenerated code. Signed-off-by: vishaldhingra <vdhingra@vmware.com>

polychaeta added the staticd label Jan 10, 2021

mjstapp reviewed Jan 11, 2021

View reviewed changes

vishaldhingra marked this pull request as ready for review January 11, 2021 19:49

vishaldhingra force-pushed the static branch from 433d59f to ef4b6b2 Compare January 12, 2021 15:51

mjstapp approved these changes Jan 13, 2021

View reviewed changes

sworleys approved these changes Jan 13, 2021

View reviewed changes

mjstapp merged commit 0189d02 into FRRouting:master Jan 15, 2021

vishaldhingra mentioned this pull request Jan 15, 2021

[7.5] staticd: correct table-id handling for static routes #7877

Merged

idryzhov mentioned this pull request Jan 20, 2021

"ip route" commands don't work properly with "table" parameter #7332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

staticd: correct table-id handling for static routes. #7844

staticd: correct table-id handling for static routes. #7844

vishaldhingra commented Jan 10, 2021

LabN-CI commented Jan 10, 2021 •

edited

Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Jan 10, 2021 •

edited

Loading

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

mjstapp Jan 11, 2021

sworleys Jan 11, 2021

vishaldhingra Jan 11, 2021

sworleys Jan 11, 2021

mjstapp Jan 11, 2021

vishaldhingra Jan 12, 2021 •

edited

Loading

mjstapp Jan 12, 2021

vishaldhingra Jan 12, 2021

vishaldhingra Jan 12, 2021

NetDEF-CI commented Jan 12, 2021

LabN-CI commented Jan 12, 2021

mjstapp left a comment

sworleys commented Jan 13, 2021

vishaldhingra commented Jan 14, 2021

mjstapp commented Jan 14, 2021

staticd: correct table-id handling for static routes. #7844

staticd: correct table-id handling for static routes. #7844

Conversation

vishaldhingra commented Jan 10, 2021

LabN-CI commented Jan 10, 2021 • edited Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Jan 10, 2021 • edited Loading

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishaldhingra Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NetDEF-CI commented Jan 12, 2021

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

LabN-CI commented Jan 12, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

mjstapp left a comment

Choose a reason for hiding this comment

sworleys commented Jan 13, 2021

vishaldhingra commented Jan 14, 2021

mjstapp commented Jan 14, 2021

LabN-CI commented Jan 10, 2021 •

edited

Loading

NetDEF-CI commented Jan 10, 2021 •

edited

Loading

vishaldhingra Jan 12, 2021 •

edited

Loading