v1.11 Backports 2023-05-03 #25250

joamaki · 2023-05-03T15:30:21Z

Once this PR is merged, you can update the PR labels via:

for pr in 25043 25046 24770 25033 25109 25192 25174 25219; do contrib/backporting/set-labels.py $pr done 1.11; done

or with

make add-labels BRANCH=v1.11 ISSUES=25043,25046,24770,25033,25109,25192,25174,25219

[ upstream commit 32473b9 ] Log number of backends that are identified as leaked, and hence, skipped and deleted during restore. This is useful for debugging when debug logs are not enabled. Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit 662c96b ] Log orphan backends that are not associated with any services, and cleaned up post agent restart. This can be useful for debugging when debug logs are not enabled. Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit c32b001 ] During backends restore, the agent previously released backend ID in case of conflict. More importantly, the ID that was released was already restored for another backend. This will release conflicted IDs from the ID allocator, while still leaving backend entries in the backends map. The ID allocator can then assign the released ID to another backend once the restore is complete. This commit removes releasing conflicted backend IDs. The proper clean-up would happen in certain cases when backends are deleted as duplicate or orphan where they are not associated with any of the services. Fixes: 51b62f5 ("service: Add methods to acquire backend ID for svc v2") Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit ec8d3ce ] This commit revises the fix made in commit 5311f81 that handles leaked backends. Consider the following leaked backend scenarios where (1) and (2a) were previously handled: (1) Backend entries leaked, no duplicates: previously, such backends would be deleted as orphan backends after sync with kubernetes api server. (2) Backend entries leaked with duplicates: (a) backend with overlapping L3nL4Addr hash is associated with service(s) Sequence of events: Backends were leaked prior to agent restart, but there was at least one service that the backend by hash is associated with. s.backendByHash will have a non-zero reference count for the overlapping L3nL4Addr hash. (b) none of the backends are associated with services Sequence of events: All the services these backends were associated with were deleted prior to agent restart. s.backendByHash will not have an entry for the backends hash. This commit updates the logic to handle all the scenarios in one place. Fixes: b79a4a5 (pkg/service: Gracefully terminate service backends) Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

giorio94

My commit looks good. Thanks!

harsimran-pabla · 2023-05-03T17:34:50Z

BGP change #25043 is already backported in this #25139

kaworu

My commits LGTM, thanks @joamaki!

sayboras

Looks good for my commit, thanks 💯

aditighag

Looks good for my commits. Thanks!

tommyp1ckles

thanks1

[ upstream commit e1a48bb ] The commit adds a unit test case to validate various leaked backends scenarios handled in restore services. Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit b0ada08 ] The commit documents the known service backends leak issue along with the cilium versions that have the fixes. Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit 9e83a6f ] Cilium is currently affected by a known bug (cilium#24692) when NodePorts are handled by the KPR implementation, which occurs when the same NodePort is used both in the local and the remote cluster. This causes all traffic targeting that NodePort to be redirected to a local backend, regardless of whether the destination node belongs to the local or the remote cluster. This affects also the clustermesh-apiserver NodePort service, which is configured by default with a fixed port. Hence, let's add a warning message to the corresponding values file setting. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit 11e1bcc ] This is to add a small docs for version matrix between Cilium and Cilium envoy versions, which is useful with the upcoming work to move envoy proxy out of Cilium agent container. Co-authored-by: ZSC <zacharysarah@users.noreply.github.com> Signed-off-by: Tam Mach <sayboras@yahoo.com> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit db3e015 ] Before this patch, we would hit a controller-gen[1] bug when the temporary file would be of the form tmp.0oXXXXXX. This patch uses a custom mktemp template that will not trigger the bug. [1]: kubernetes-sigs/controller-tools#734 Signed-off-by: Alexandre Perrin <alex@isovalent.com> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit 9a38aec ] We've been distributing ARM architecture images for Cilium for almost two years, but neglected to mention this up front in the system requirements or the main docs page. Add this to the docs. Signed-off-by: Joe Stringer <joe@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit e695e48 ] Running the test in a cpu constrained environment, such as: ``` docker run -v $(pwd):$(pwd) -w $(pwd) --cpus=0.1 -it golang:bullseye ./inctimer.test -test.v ``` I can fairly consistency reproduce a flake where the inctimer.After does not fire in time. If I allow it to wait for an additional couple of ms, this seems to be sufficient to prevent failure. It appears that goroutine scheduling latency can be significantly delayed in cpu restricted environments. This seems unavoidable, so to fix the flake I'll allow the test to wait another 2ms to see if the inctimer eventually fires. This will also log an error for delayed test fires, so if there is any other issues we can more easily debug them in the future. Fixed: cilium#25202 Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com> Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki · 2023-05-05T07:14:08Z

Looks good for my commits. Thanks!

Hey, looks like I forgot to fix up pkg/service/service_test.go. After fixing it the same way as in the v1.12 backports I get a test failure:

FAIL: service_test.go:833: ManagerTestSuite.TestRestoreServiceWithTerminatingBackends

service_test.go:864:
    // Backends including terminating ones have been restored
    c.Assert(len(m.svc.backendByHash), Equals, 3)
... obtained int = 2
... expected int = 3

OOPS: 0 passed, 1 FAILED

Enabling debug logging I get:

level=debug msg="Upserting service" backends="[{0  {10.0.0.1 {TCP 8080} 0} false} {0  {10.0.0.2 {TCP 8080} 0} false} {0  {10.0.0.4 {TCP 8080} 0} false}]" loadBalancerSourceRanges="[]" serviceIP="{1.1.1.1 {TCP 80} 0}" serviceName= serviceNamespace= sessionAffinity=true sessionAffinityTimeout=100 subsys=service svcHealthCheckNodePort=0 svcTrafficPolicy=Cluster svcType=NodePort
level=debug msg="Resolving service" l3n4Addr="{IP:1.1.1.1 L4Addr:{Protocol:TCP Port:80} Scope:0}" subsys=service
level=debug msg="Acquired service ID" backends="[{0  {10.0.0.1 {TCP 8080} 0} false} {0  {10.0.0.2 {TCP 8080} 0} false} {0  {10.0.0.4 {TCP 8080} 0} false}]" loadBalancerSourceRanges="[]" serviceID=1 serviceIP="{1.1.1.1 {TCP 80} 0}" serviceName= serviceNamespace= sessionAffinity=true sessionAffinityTimeout=100 subsys=service svcHealthCheckNodePort=0 svcTrafficPolicy=Cluster svcType=NodePort
level=debug msg="Deleting backends from session affinity match" backends="[]" serviceID=1 subsys=service
level=debug msg="Adding new backend" backendID=1 backends="[{0  {10.0.0.1 {TCP 8080} 0} false} {0  {10.0.0.2 {TCP 8080} 0} false} {0  {10.0.0.4 {TCP 8080} 0} false}]" l3n4Addr="{10.0.0.1 {TCP 8080} 0}" loadBalancerSourceRanges="[]" serviceID=1 serviceIP="{1.1.1.1 {TCP 80} 0}" serviceName= serviceNamespace= sessionAffinity=true sessionAffinityTimeout=100 subsys=service svcHealthCheckNodePort=0 svcTrafficPolicy=Cluster svcType=NodePort
level=debug msg="Adding new backend" backendID=2 backends="[{0  {10.0.0.1 {TCP 8080} 0} false} {0  {10.0.0.2 {TCP 8080} 0} false} {0  {10.0.0.4 {TCP 8080} 0} false}]" l3n4Addr="{10.0.0.2 {TCP 8080} 0}" loadBalancerSourceRanges="[]" serviceID=1 serviceIP="{1.1.1.1 {TCP 80} 0}" serviceName= serviceNamespace= sessionAffinity=true sessionAffinityTimeout=100 subsys=service svcHealthCheckNodePort=0 svcTrafficPolicy=Cluster svcType=NodePort
level=debug msg="Adding new backend" backendID=3 backends="[{0  {10.0.0.1 {TCP 8080} 0} false} {0  {10.0.0.2 {TCP 8080} 0} false} {0  {10.0.0.4 {TCP 8080} 0} false}]" l3n4Addr="{10.0.0.4 {TCP 8080} 0}" loadBalancerSourceRanges="[]" serviceID=1 serviceIP="{1.1.1.1 {TCP 80} 0}" serviceName= serviceNamespace= sessionAffinity=true sessionAffinityTimeout=100 subsys=service svcHealthCheckNodePort=0 svcTrafficPolicy=Cluster svcType=NodePort
level=debug msg="Adding backends to affinity match map" backends="[1 2 3]" serviceID=1 subsys=service
level=debug msg="Upserting service" backends="[{0  {10.0.0.1 {TCP 8080} 0} false} {0  {10.0.0.2 {TCP 8080} 0} false} {0  {10.0.0.4 {TCP 8080} 0} true}]" loadBalancerSourceRanges="[]" serviceIP="{1.1.1.1 {TCP 80} 0}" serviceName= serviceNamespace= sessionAffinity=true sessionAffinityTimeout=100 subsys=service svcHealthCheckNodePort=0 svcTrafficPolicy=Cluster svcType=NodePort
level=debug msg="Acquired service ID" backends="[{0  {10.0.0.1 {TCP 8080} 0} false} {0  {10.0.0.2 {TCP 8080} 0} false} {0  {10.0.0.4 {TCP 8080} 0} true}]" loadBalancerSourceRanges="[]" serviceID=1 serviceIP="{1.1.1.1 {TCP 80} 0}" serviceName= serviceNamespace= sessionAffinity=true sessionAffinityTimeout=100 subsys=service svcHealthCheckNodePort=0 svcTrafficPolicy=Cluster svcType=NodePort
level=debug msg="Deleting backends from session affinity match" backends="[]" serviceID=1 subsys=service
level=debug msg="Adding backends to affinity match map" backends="[1 2 3]" serviceID=1 subsys=service
level=warning msg="Unable to add entry to affinity match map" backendID=1 error="Backend 1 already exists in 1 affinity map" serviceID=1 subsys=service
level=warning msg="Unable to add entry to affinity match map" backendID=2 error="Backend 2 already exists in 1 affinity map" serviceID=1 subsys=service
level=warning msg="Unable to add entry to affinity match map" backendID=3 error="Backend 3 already exists in 1 affinity map" serviceID=1 subsys=service
level=debug msg="Restoring service" serviceID=1 serviceIP="1.1.1.1:80" subsys=service
level=debug msg="Restoring service" l3n4Addr="{IP:1.1.1.1 L4Addr:{Protocol:NONE Port:80} Scope:0}" subsys=service
level=info msg="Restored services from maps" failedServices=0 restoredServices=1 subsys=service
level=debug msg="Restoring backend" backendID=1 l3n4Addr="10.0.0.1:8080" subsys=service
level=debug msg="Restoring backend" backendID=2 l3n4Addr="10.0.0.2:8080" subsys=service
level=debug msg="Restoring backend" backendID=3 l3n4Addr="10.0.0.4:8080" subsys=service
level=debug msg="Leaked backend entry not restored" backendID=3 l3n4Addr="{10.0.0.4 {NONE 8080} 0}" subsys=service
level=info msg="Restored backends from maps" failedBackends=0 restoredBackends=2 skippedBackends=1 subsys=service

@aditighag Any idea what's going on? Are we missing something important in v1.11?

aditighag

Sorry, missed the differences around service name +namespace changes.

aditighag · 2023-05-08T14:01:49Z

pkg/service/service_test.go

+		SessionAffinity:           true,
+		SessionAffinityTimeoutSec: 100,
+		HealthCheckNodePort:       0,
+		Name:                      "",
+		Namespace:                 "",
+		LoadBalancerSourceRanges:  []*cidr.CIDR{},
 	}
+


Can you revert these changes? Looks like there were accidentally added in the backport commit?

aditighag · 2023-05-08T14:02:52Z

pkg/service/service_test.go

+		Name:      "svc1",
+		Namespace: "ns1",


This should be the only difference between 1.11 and newer branches.

YutaroHayakawa · 2023-05-10T06:57:02Z

Since this branch comes from Jussi's fork, I cannot edit commits to apply Aditi's change. I'll move this to another PR the branch owned by me.

aditighag added 4 commits May 3, 2023 14:33

joamaki requested a review from a team as a code owner May 3, 2023 15:30

joamaki added kind/backports This PR provides functionality previously merged into master. backport/1.11 This PR represents a backport for Cilium 1.11.x of a PR that was merged to main. labels May 3, 2023

joamaki requested review from aditighag, giorio94, sayboras, kaworu, joestringer, tommyp1ckles, harsimran-pabla and nbusseneau May 3, 2023 15:30

giorio94 approved these changes May 3, 2023

View reviewed changes

kaworu approved these changes May 3, 2023

View reviewed changes

sayboras approved these changes May 4, 2023

View reviewed changes

aditighag approved these changes May 4, 2023

View reviewed changes

tommyp1ckles approved these changes May 5, 2023

View reviewed changes

aditighag and others added 7 commits May 5, 2023 10:11

pkg/service: Unit test for handling of leaked backends

7d38d23

[ upstream commit e1a48bb ] The commit adds a unit test case to validate various leaked backends scenarios handled in restore services. Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

docs: Document backends leak issue

d868cec

[ upstream commit b0ada08 ] The commit documents the known service backends leak issue along with the cilium versions that have the fixes. Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki force-pushed the pr/v1.11-backport-2023-05-03 branch from 67e69a2 to 6e40b96 Compare May 5, 2023 07:11

joamaki requested a review from aditighag May 5, 2023 07:14

aditighag requested changes May 8, 2023

View reviewed changes

YutaroHayakawa self-assigned this May 10, 2023

YutaroHayakawa closed this May 10, 2023

YutaroHayakawa mentioned this pull request May 10, 2023

v1.11 Backports 2023-05-03 Cont #25349

Merged

5 tasks

YutaroHayakawa removed their assignment Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1.11 Backports 2023-05-03 #25250

v1.11 Backports 2023-05-03 #25250

joamaki commented May 3, 2023 •

edited by YutaroHayakawa

giorio94 left a comment

harsimran-pabla commented May 3, 2023

kaworu left a comment

sayboras left a comment

aditighag left a comment

tommyp1ckles left a comment

joamaki commented May 5, 2023

aditighag left a comment •

edited

aditighag May 8, 2023

aditighag May 8, 2023

YutaroHayakawa commented May 10, 2023

v1.11 Backports 2023-05-03 #25250

v1.11 Backports 2023-05-03 #25250

Conversation

joamaki commented May 3, 2023 • edited by YutaroHayakawa

giorio94 left a comment

Choose a reason for hiding this comment

harsimran-pabla commented May 3, 2023

kaworu left a comment

Choose a reason for hiding this comment

sayboras left a comment

Choose a reason for hiding this comment

aditighag left a comment

Choose a reason for hiding this comment

tommyp1ckles left a comment

Choose a reason for hiding this comment

joamaki commented May 5, 2023

aditighag left a comment • edited

Choose a reason for hiding this comment

aditighag May 8, 2023

Choose a reason for hiding this comment

aditighag May 8, 2023

Choose a reason for hiding this comment

YutaroHayakawa commented May 10, 2023

joamaki commented May 3, 2023 •

edited by YutaroHayakawa

aditighag left a comment •

edited