Skip to content

BGP CP: Replaces LocalNodeStore with Local CiliumNode #28238

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
Nov 20, 2023
Merged

BGP CP: Replaces LocalNodeStore with Local CiliumNode #28238

merged 2 commits into from
Nov 20, 2023

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Sep 20, 2023 •
edited
Loading

  • Replaces the LocalNodeStore with the existing local CiliumNode throughout the BGP CP.
  • Updates unit and integration tests accordingly.
  • Adds the GetIP() method for returning the CiliumNode IP.

Fixes: #28237

@danehans danehans requested review from a team as code owners September 20, 2023 22:16
@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 Sep 20, 2023
@danehans
Copy link
Contributor Author

/test

@danehans
Copy link
Contributor Author

cc @rastislavs

harsimran-pabla
harsimran-pabla reviewed Sep 22, 2023
View reviewed changes
@danehans
Copy link
Contributor Author

/cc @rastislavs

rastislavs
rastislavs approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

LGTM

@YutaroHayakawa YutaroHayakawa requested review from a team and tommyp1ckles and removed request for a team September 26, 2023 06:38
@YutaroHayakawa
Copy link
Member

Let me mention sig-ipam since this is relevant.

@danehans
Copy link
Contributor Author

danehans commented Oct 2, 2023

Let me mention sig-ipam since this is relevant.

@YutaroHayakawa should I ping anyone directly from sig-ipam to help with the review?

@YutaroHayakawa
Copy link
Member

Tom is assigned from SIG-IPAM, but now he is not available. Let me mention @gandro 🙏

@YutaroHayakawa YutaroHayakawa requested review from gandro and removed request for tommyp1ckles October 3, 2023 00:44
gandro
gandro reviewed Oct 3, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

This is a great simplification! Looks good to me in terms of logic, but I do have a concern about logic duplication

@danehans danehans requested a review from a team as a code owner October 3, 2023 18:08
@danehans
Copy link
Contributor Author

danehans commented Oct 3, 2023

/test

@danehans
Copy link
Contributor Author

danehans commented Oct 3, 2023

/ci-runtime

gandro
gandro approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks!

@danehans
Copy link
Contributor Author

danehans commented Oct 4, 2023

/test

@danehans
Copy link
Contributor Author

/test

@danehans
Copy link
Contributor Author

/ci-runtime

@danehans
Copy link
Contributor Author

danehans commented Oct 18, 2023 •
edited
Loading

The ci-runtime test is failing due to:

--- FAIL: Test (233.85s)
    --- FAIL: Test/linuxPrivilegedIPv4OnlyTestSuite (119.21s)
        --- FAIL: Test/linuxPrivilegedIPv4OnlyTestSuite/TestArpPingHandling (114.57s)
            node_linux_test.go:2958: 
                ... obtained bool = false
                ... expected bool = true
                
        --- FAIL: Test/linuxPrivilegedIPv4OnlyTestSuite/TestArpPingHandlingForMultiDevice (0.12s)
            node_linux_test.go:3538: 
                ... obtained bool = false
                ... expected bool = true

I don't see how the changes in this PR can cause this test failure. Going to rebase to see if a PR landed that fixes this CI issue.

@gandro
Copy link
Member

gandro commented Oct 18, 2023

It does look like a known flake: #26479

@danehans danehans mentioned this pull request Oct 18, 2023
Closed
@danehans
Copy link
Contributor Author

/test

@danehans
Copy link
Contributor Author

Commit 56b1133 rebases to try resolving CI flakes.

@danehans
Copy link
Contributor Author

/test

1 similar comment
@danehans
Copy link
Contributor Author

/test

@danehans
Copy link
Contributor Author

danehans commented Nov 2, 2023

/ci-ipsec-upgrade

@danehans
Copy link
Contributor Author

danehans commented Nov 2, 2023

/ci-runtime

@danehans danehans force-pushed the issue_28237 branch 2 times, most recently from cd9c658 to 5ce34a6 Compare November 3, 2023 21:33
@rastislavs
Copy link
Contributor

/test

@rastislavs
Copy link
Contributor

@danehans I think you need to rebase this PR to make the CI pass.

I have some pending changes for the reconcilers, but don't want to block this one anymore - hopefully we can get it in soon?

@rastislavs
Copy link
Contributor

/test

@rastislavs
Copy link
Contributor

/ci-eks

@rastislavs
Copy link
Contributor

@danehans looks like ci-runtime is failing consistently with this change. Some tests in pkg/datapath/linux (privileged) seem to be using datatypes from pkg/node, so they may need an update related to your change. Could you please check that?

@danehans
pkg/node: Abstracts GetNodeIP() Method
eb10803 
Abstracts the GetNodeIP() method for use by other types.

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans
BGP CP: Replaces LocalNodeStore with Local CiliumNode
c295400 
- Replaces the LocalNodeStore with the existing local CiliumNode
  throughout the BGP CP.
- Updates unit and integration tests accordingly.
- Adds the GetIP() method for returning the CiliumNode IP.

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 18, 2023
@julianwiedmann julianwiedmann merged commit c171e58 into cilium:main Nov 20, 2023
@danehans danehans deleted the issue_28237 branch March 8, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@rastislavs rastislavs rastislavs approved these changes

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

Assignees
No one assigned
Labels
area/bgp Impacts the Border Gateway Protocol feature. area/ipam IP address management, including cloud IPAM ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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.

BGP CP: Replace LocalNodeStore with the Local CiliumNode Resource
7 participants
@danehans @YutaroHayakawa @gandro @rastislavs @nathanjsweet @harsimran-pabla @julianwiedmann