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

Fix copy of annotations into CiliumNode #25307

Merged
merged 1 commit into from May 10, 2023

Conversation

meyskens
Copy link
Member

@meyskens meyskens commented May 8, 2023

This fixes the adding of relevant annotations to the created CiliumNode object. This was caused by the local node not containing this info.

Work for this regarding BGP an notations got added in #24914 but never properly synced as the localNode did store the info. this fix takes those from the received k8s node object to generate them.

Fixes: #25301

Fix syncing of relevant node annotations into CiliumNode

@meyskens meyskens requested a review from a team as a code owner May 8, 2023 07:43
@meyskens meyskens requested a review from pippolo84 May 8, 2023 07:43
@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 May 8, 2023
@meyskens meyskens added the release-note/bug This PR fixes an issue in a previous release of Cilium. label May 8, 2023
@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 8, 2023
@margau
Copy link
Contributor

margau commented May 8, 2023

I've tested this on my setup, and, as opposed to before, it appears to work and is actually copying the annotations, leading to a working BGP control plane.

@meyskens meyskens self-assigned this May 8, 2023
@michi-covalent michi-covalent added needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch affects/v1.13 This issue affects v1.13 branch labels May 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 May 8, 2023
@michi-covalent michi-covalent added the release-blocker/1.13 This issue will prevent the release of the next version of Cilium. label May 8, 2023
@meyskens meyskens force-pushed the meyskens/fix-bgpanno-sync branch from 27c1d50 to 5d1abef Compare May 8, 2023 17:03
This fixes the adding of relevant annotations to the created CiliumNode
object. This was caused by the local node not containing this info.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
@meyskens meyskens force-pushed the meyskens/fix-bgpanno-sync branch from 5d1abef to 6c64d7f Compare May 9, 2023 09:28
@meyskens
Copy link
Member Author

meyskens commented May 9, 2023

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "http://192.168.56.12:30531" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2159/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🚀

@ldelossa
Copy link
Contributor

ldelossa commented May 9, 2023

This looks good to me, I've also tested in our BGP labs 👍

@meyskens
Copy link
Member Author

meyskens commented May 9, 2023

CI failure seems to be related to #15455

@michi-covalent
Copy link
Contributor

/test-1.26-net-next

@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 May 9, 2023
@ldelossa ldelossa merged commit a1c949c into cilium:main May 10, 2023
57 checks passed
@thorn3r thorn3r mentioned this pull request May 10, 2023
2 tasks
@thorn3r thorn3r added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels May 10, 2023
@thorn3r thorn3r added this to Backport pending to v1.13 in 1.13.4 May 17, 2023
@thorn3r thorn3r removed this from Needs backport from main in 1.13.3 May 17, 2023
@qmonnet qmonnet moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.4 Jun 9, 2023
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jul 10, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.13 in 1.13.3 Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.13 This issue affects v1.13 branch backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.13.3
Backport done to v1.13
1.13.4
Backport done to v1.13
Development

Successfully merging this pull request may close these issues.

bug: node -> ciliumnode annotation copy not working
7 participants