bgp,bugfix: parse ips when converting from slim_core to k8s service #18358

ldelossa · 2022-01-03T21:20:48Z

this fixes #16967.

previous to this commit, when converting from a slim_core Service to our
k8s Service abstraction the parsed out loadBalancersIP were not
converted to the appropriate map of net.IP objects.

they were not converted because of a guard which only does this for when
"NodePort" configuration is set to true.

with the introduction of the BGP load balancer announcement feature, we
also need to parse IPs coming from the ServiceStatus field in a
slim_core Service.

If this change is not made, both the old and new services look exactly
the same to the k8sWatcher infrastructure and when the Service gets its
load balancer IP the entire event is thrown away.

Signed-off-by: Louis DeLosSantos louis.delos@isovalent.com

sayboras

While the change looks simple, there is a lot of wisdom behind, I am going through the discussion in github issue to understand more. Right now, I don't even understand why nodeport check is there in the first place 🤔

Will be great if other team member can help to review this instead.

ldelossa · 2022-01-05T15:06:43Z

@sayboras That is where I'm currently at as well. Doing a deep dive.

It appears the linked issue can be "worked-around" by ensuring "NodePort" is enabled off the bat. I didn't suggest this yet because I need to confirm.

Also as I mention in this thread: https://cilium.slack.com/archives/C2B917YHE/p1641328782392200?thread_ts=1641245029.380700&cid=C2B917YHE

I believe a BPF backed NodePort should be completely orthogonal to the BGP Announce LB feature, as that feature just cares about getting external traffic to a load balancer, not whether that load balancer is backed by a BPF node port impl, or the vanilla K8s impl. So part of me feels like this PR will actually change into decoupling the NodePort requirement from the BGP Announce LB feature all together.

CC @aanm may have some insight.

ldelossa · 2022-01-06T17:55:50Z

daemon/cmd/daemon_main.go

@@ -1517,14 +1517,6 @@ func initEnv(cmd *cobra.Command) {
 		}
 	}

-	// This is necessary because the code inside pkg/k8s.NewService() for


This goes away, the BGP LB Announcement features are not coupled to the NodePort configuration.

ldelossa · 2022-01-06T18:00:05Z

Discussed this with @brb @aanm (separately)

The reason we do not parse IPs when "EnableNodePort" is false is to save on CPU cycles. When the umbrella feature flag "EnableNodePort" is disabled, Cilium does not care about Service events triggered by LoadBalancer Status updates.

However, when the BGP Announce LB feature is enabled, Cilium must care about these updates in order for the k8s Watcher infra to generate a new Service event.

The decision was made to keep the original "EnableNodePort" flag to continue to budget CPU if we don't need to spend it, but if BGP announcement feature is on, parse out the loadBalancerIPs so feature works without being coupled to "EnableNodePort" feature flag.

ldelossa · 2022-01-06T18:11:07Z

pkg/bgp/speaker/speaker.go

@@ -128,7 +128,7 @@ func (s *MetalLBSpeaker) OnUpdateService(svc *slim_corev1.Service) error {
 	}

 	l.Debug("adding event to queue")
-	s.queue.Add(epEvent{
+	s.queue.Add(svcEvent{


good citizen change, this is mislabeled but causes no functional difference in the code, other then logging the wrong event type.

ldelossa · 2022-01-06T18:30:52Z

/test

christarazi

Thanks, makes sense to me.

pkg/k8s/service.go

kkourt

LGTM (just a non-blocking comment).

daemon/cmd/daemon_main.go

ldelossa · 2022-01-11T15:47:08Z

/test

ldelossa · 2022-01-11T16:13:40Z

Latest failures are due to vagrant infra:

15:59:17  Failed in branch Copy code and boot vms

Rerunning tests.

ldelossa · 2022-01-11T16:13:43Z

/test

this fixes cilium#16967. previous to this commit, when converting from a slim_core Service to our k8s Service abstraction the parsed out loadBalancersIP were not converted to the appropriate map of net.IP objects. they were not converted because of a guard which only does this for when "NodePort" configuration is set to true. with the introduction of the BGP load balancer announcement feature, we also need to parse IPs coming from the ServiceStatus field in a slim_core Service. If this change is not made, both the old and new services look exactly the same to the k8sWatcher infrastructure and when the Service gets its load balancer IP the entire event is thrown away. Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>

pchaigno · 2022-01-12T10:41:18Z

/test

Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks graceful termination of service endpoints Checks client terminates gracefully on service endpoint deletion

Failure Output

FAIL: Timed out after 60.001s.

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-4.19 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: terminating containers are not deleted after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

ldelossa · 2022-01-12T15:32:25Z

/test

aditighag · 2022-01-19T00:40:23Z

@ldelossa FYI - This bug fix warranted a release note as it's user-facing.

ldelossa requested review from a team and sayboras January 3, 2022 21:20

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 Jan 3, 2022

maintainer-s-little-helper bot assigned sayboras Jan 3, 2022

sayboras added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 5, 2022

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 Jan 5, 2022

sayboras reviewed Jan 5, 2022

View reviewed changes

sayboras requested a review from a team January 5, 2022 11:41

sayboras removed their assignment Jan 5, 2022

ldelossa marked this pull request as draft January 5, 2022 15:07

ldelossa requested a review from aanm January 5, 2022 16:27

maintainer-s-little-helper bot assigned aanm Jan 5, 2022

ldelossa force-pushed the ldelossa/announce-lb-fix branch from 49421f9 to 20957d6 Compare January 6, 2022 17:54

ldelossa commented Jan 6, 2022

View reviewed changes

ldelossa added the needs-backport/1.11 label Jan 6, 2022

ldelossa marked this pull request as ready for review January 6, 2022 17:56

ldelossa requested review from a team and kkourt January 6, 2022 17:56

maintainer-s-little-helper bot assigned kkourt Jan 6, 2022

ldelossa requested a review from brb January 6, 2022 18:00

maintainer-s-little-helper bot assigned brb Jan 6, 2022

ldelossa commented Jan 6, 2022

View reviewed changes

joestringer added this to Needs backport from master in 1.11.1 Jan 6, 2022

christarazi approved these changes Jan 6, 2022

View reviewed changes

aanm requested changes Jan 7, 2022

View reviewed changes

pkg/k8s/service.go Outdated Show resolved Hide resolved

ldelossa force-pushed the ldelossa/announce-lb-fix branch from 20957d6 to f48b09e Compare January 7, 2022 02:26

maintainer-s-little-helper bot unassigned aanm Jan 7, 2022

brb reviewed Jan 10, 2022

View reviewed changes

pkg/k8s/service.go Show resolved Hide resolved

kkourt approved these changes Jan 10, 2022

View reviewed changes

daemon/cmd/daemon_main.go Show resolved Hide resolved

maintainer-s-little-helper bot unassigned kkourt Jan 10, 2022

brb approved these changes Jan 11, 2022

View reviewed changes

maintainer-s-little-helper bot unassigned brb Jan 11, 2022

ldelossa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 11, 2022

ldelossa removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 11, 2022

ldelossa force-pushed the ldelossa/announce-lb-fix branch from f48b09e to ac5f66e Compare January 11, 2022 21:49

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 Jan 12, 2022

aditighag merged commit 2275759 into cilium:master Jan 12, 2022

ldelossa deleted the ldelossa/announce-lb-fix branch January 13, 2022 16:41

aditighag mentioned this pull request Jan 14, 2022

v1.11 backports 2022-01-14 #18488

Merged

aditighag added backport-pending/1.11 and removed needs-backport/1.11 labels Jan 14, 2022

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.1 Jan 14, 2022

joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jan 18, 2022

maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.1 Jan 18, 2022

joestringer mentioned this pull request Jan 18, 2022

Prepare for release v1.11.1 #18535

Merged

christarazi mentioned this pull request Jan 26, 2022

[BGP] PodCidr routes withdrawn when svc loadbalancing feature enabled #18250

Closed

2 tasks

borkmann mentioned this pull request Apr 21, 2022

BGP not working in 1.10.10 #19509

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bgp,bugfix: parse ips when converting from slim_core to k8s service #18358

bgp,bugfix: parse ips when converting from slim_core to k8s service #18358

ldelossa commented Jan 3, 2022

sayboras left a comment •

edited

ldelossa commented Jan 5, 2022 •

edited

ldelossa Jan 6, 2022

ldelossa commented Jan 6, 2022

ldelossa Jan 6, 2022

ldelossa commented Jan 6, 2022

christarazi left a comment

kkourt left a comment

ldelossa commented Jan 11, 2022

ldelossa commented Jan 11, 2022

ldelossa commented Jan 11, 2022

pchaigno commented Jan 12, 2022 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

ldelossa commented Jan 12, 2022

aditighag commented Jan 19, 2022

bgp,bugfix: parse ips when converting from slim_core to k8s service #18358

bgp,bugfix: parse ips when converting from slim_core to k8s service #18358

Conversation

ldelossa commented Jan 3, 2022

sayboras left a comment • edited

Choose a reason for hiding this comment

ldelossa commented Jan 5, 2022 • edited

ldelossa Jan 6, 2022

Choose a reason for hiding this comment

ldelossa commented Jan 6, 2022

ldelossa Jan 6, 2022

Choose a reason for hiding this comment

ldelossa commented Jan 6, 2022

christarazi left a comment

Choose a reason for hiding this comment

kkourt left a comment

Choose a reason for hiding this comment

ldelossa commented Jan 11, 2022

ldelossa commented Jan 11, 2022

ldelossa commented Jan 11, 2022

pchaigno commented Jan 12, 2022 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

ldelossa commented Jan 12, 2022

aditighag commented Jan 19, 2022

sayboras left a comment •

edited

ldelossa commented Jan 5, 2022 •

edited

pchaigno commented Jan 12, 2022 •

edited by maintainer-s-little-helper bot