Skip to content

statedb tables: add fields to devices and route tables #39178

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
Merged
merged 2 commits into from
May 7, 2025
Merged

statedb tables: add fields to devices and route tables #39178

merged 2 commits into from
May 7, 2025

Conversation

naveenachyuta
Copy link
Contributor

adding operational status to devices table and priority to route table. it is pre-requisite for adding auto-discovery of bgp peers feature to bgp control plane

Please ensure your pull request adheres to the following guidelines:

  • [ X] For first time contributors, read Submitting a pull request
  • [X ] All code is covered by unit and/or runtime tests where feasible.
  • [ X] All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • [X ] All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

This PR is a pre-requisite to fix github issue #37315
for adding auto-discovery of bgp peers (proposal: https://github.com/cilium/design-cfps/blob/main/cilium/CFP-37315-bgp-auto-discovery.md), we need access to operational state of links and priority of routes.
route priority will be used to create bgp sessions with the default gateway of lowest priority default route
operational state of the link will be used to check if the link through which default gateway is reachable is up or not

Steps used to test:

  1. make kind-bgp-v4
  2. KIND_CLUSTER_NAME=bgp-cplane-dev-v4 make kind-image
  3. cilium install --chart-directory install/kubernetes/cilium -f contrib/containerlab/bgp-cplane-dev-v4/values.yaml --set image.override="localhost:5000/cilium/cilium-dev:local" --set image.pullPolicy=Never --set operator.image.override="localhost:5000/cilium/operator-generic:local" --set operator.image.pullPolicy=Never
  4. kubectl exec -it [cilium pod id] -n kube-system -- bash
  5. cilium-dbg shell
  6. db/show routes, db/show devices
datapath: update route and device statedb tables

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 26, 2025
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 26, 2025
@naveenachyuta naveenachyuta marked this pull request as ready for review April 26, 2025 17:18
@naveenachyuta naveenachyuta requested a review from a team as a code owner April 26, 2025 17:18
@joamaki
Copy link
Contributor

joamaki commented Apr 28, 2025 •
edited
Loading

Looking quite good to me, but please also update the test case https://github.com/cilium/cilium/blob/main/pkg/datapath/linux/testdata/device-controller-tables.txtar.

You can just add the column to the test input data, e.g. before:

-- routes_dummy0.table --
Destination      Source        Gateway   Table   Scope
192.168.0.1/32   192.168.0.1             255     254

and after:

-- routes_dummy0.table --
Destination      Source        Gateway   Table   Scope  Priority
192.168.0.1/32   192.168.0.1             255     254    123456

The db/cmp command parses the header row in the expected file and only compares the columns that are listed. The order of the columns or the whitespacing do not matter as long as the header and rows are aligned the same way.

Please expand both the device and route table expected outputs to include the new columns. This way we'll know both that we're populating it correctly from netlink and that we're showing it nicely in e.g. db/show devices.

Running the test again after adding the column to the header line will show how it's rendered and you can then update the expected data accordingly. As tests rely on ability to manipulate links you'll need to run them as privileged:

cd pkg/datapath/linux
go test -c
PRIVILEGED_TESTS=1 sudo -E ./linux.test -test.run TestDevicesControllerScript -test.v

You can find more info about the script tests at https://docs.cilium.io/en/latest/contributing/development/hive/#testing-with-hive-script.

@naveenachyuta
Copy link
Contributor Author

Thanks for the input. i have added testdata as per your request. Please review and let me know if there any concerns.
@joamaki

@aanm aanm requested a review from joamaki April 29, 2025 09:14
@naveenachyuta
Copy link
Contributor Author

hello @joamaki , please let me know if you are okay with the PR. This is a pre-requisite for autodiscovery of bgp peers. Once this is merged, I will be able to create a PR for it. Thank you for your time.

@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label May 2, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 2, 2025
@joamaki
Copy link
Contributor

joamaki commented May 2, 2025

/test

@joamaki joamaki enabled auto-merge May 2, 2025 06:50
@naveenachyuta
Copy link
Contributor Author

thank you @joamaki , what is the process to get this merged?

@maintainer-s-little-helper
Copy link

Commit 819666f does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 2, 2025
auto-merge was automatically disabled May 2, 2025 15:26

Head branch was pushed to by a user without write access

@naveenachyuta naveenachyuta force-pushed the add_fields_to_device_and_route_tables branch from 819666f to eb063fb Compare May 2, 2025 15:26
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 2, 2025
@naveenachyuta
Copy link
Contributor Author

i rebased the master so looks like "auto-merge" got removed.

@joamaki
Copy link
Contributor

joamaki commented May 5, 2025

Please remove the merge commit and rebase instead

@naveenachyuta naveenachyuta force-pushed the add_fields_to_device_and_route_tables branch from eb063fb to 4cb82b5 Compare May 5, 2025 14:57
@naveenachyuta
statedb tables: add fields to devices and route tables
89d360c 
adding operational status to devices table and priority to route table
it is pre-requisite for adding auto-discovery of bgp peers feature to bgp control plane

Fixes: cilium#37315
Signed-off-by: Naveen Achyuta <naveen.achyuta22@gmail.com>
@naveenachyuta naveenachyuta force-pushed the add_fields_to_device_and_route_tables branch from 4cb82b5 to 59ea262 Compare May 5, 2025 14:58
@naveenachyuta
Copy link
Contributor Author

@joamaki i removed the merge commit. what do i need to do to get this merged? Thanks

@joamaki
Copy link
Contributor

joamaki commented May 6, 2025

@joamaki i removed the merge commit. what do i need to do to get this merged? Thanks

The changes look good, we'll just need to now make sure CI passes and then we'll be able to merge.

@naveenachyuta
add testdata for devices and route table
9e75c64 
add priority to route table and operstatus to devices table in testdata for coverage
Fixes: 89d360c (add testdata as per the comment from joamaki)

Signed-off-by: Naveen Achyuta <naveen.achyuta22@gmail.com>
@naveenachyuta naveenachyuta force-pushed the add_fields_to_device_and_route_tables branch from 59ea262 to 9e75c64 Compare May 6, 2025 18:36
@naveenachyuta
Copy link
Contributor Author

@joamaki there are 12 pending checks. not sure how to trigger it.

@joamaki
Copy link
Contributor

joamaki commented May 7, 2025

/test

@joamaki joamaki enabled auto-merge May 7, 2025 06:30
@joamaki joamaki added this pull request to the merge queue May 7, 2025
Merged via the queue into cilium:main with commit 8346287 May 7, 2025
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@gentoo-root gentoo-root Awaiting requested review from gentoo-root gentoo-root is a code owner automatically assigned from cilium/sig-datapath

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@naveenachyuta @joamaki