[backport-v1.13] ipcache: don't short-circuit InjectLabels if source differs #25077

squeed · 2023-04-24T08:46:34Z

Backporting

ipcache don't short-circuit InjectLabels if source differs #24875

InjectLabels is one of the functions responsible for synchronizing the ipcache metadata store and ip store. As such, it shouldn't shortcut when the numeric identity is the same, but the source is different; this means that an update to the ipcache isn't complete.

This can happen, for example, when there are two identities for the same IP, which can happen on daemon restart whenever a CIDR is referenced.

Signed-off-by: Casey Callendrello cdc@isovalent.com

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

for pr in 24875; do contrib/backporting/set-labels.py $pr done 1.13; done

[ upstream commit f52b4c9 ] This makes the mock allocator work similarly to the "real" one: if an ID is requested and it is not in use, then accept it. Signed-off-by: Casey Callendrello <cdc@isovalent.com>

squeed · 2023-04-24T08:48:40Z

This code has been tested by a few end-users via a hotfix branch, and they reported that it fixed the bug in question.

michi-covalent · 2023-04-24T15:00:18Z

/test-backport-1.13

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 15 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.24-kernel-5.4/1837/

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

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 14 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/1851/

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

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 14 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/1087/

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

Job 'Cilium-PR-K8s-1.21-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 10 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.21-kernel-4.19/13/

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

Job 'Cilium-PR-K8s-1.23-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 13 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.23-kernel-4.19/737/

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

Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 14 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.22-kernel-4.19/1632/

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

Job 'Cilium-PR-K8s-1.18-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 18 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.18-kernel-4.19/13/

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

Job 'Cilium-PR-K8s-1.20-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 14 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.20-kernel-4.19/1906/

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

Job 'Cilium-PR-K8s-1.19-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 14 k8s-app=cilium logs matching list of errors that must be investigated:

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.19-kernel-4.19/12/

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

Job 'Cilium-PR-K8s-1.24-kernel-4.19' has 1 failure but they might be new flake since it also hit 1 known flake: #20723 (87.00% similarity)

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

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Found 7 k8s-app=cilium logs matching list of errors that must be investigated:

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

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.

joestringer · 2023-04-24T16:26:58Z

Please use the standard scripts for backporting. The PR description is missing the ```upstream-prs ... ``` section which is used for tracking backports for PRs in main and for generating the release notes.

joestringer · 2023-04-24T16:39:25Z

Backport commits look identical to upstream which is good, but CI is failing in etcd cases due to this concerning log message:

⚠️  Found "2023-04-24T15:54:51.485343980Z level=error msg=\"Failed to replace ipcache entry with new identity after label removal. Traffic may be disrupted.\" error=\"unable to overwrite source \\\"kvstore\\\" with source \\\"custom-resource\\\"\" identity=\"{remote-node custom-resource false}\" ipAddr=10.0.1.49/32 subsys=ipcache" in logs 1 times

It looks like there was some other internal change between v1.13 and main that prevents main CI from bubbling this error up.

squeed · 2023-04-24T18:43:32Z

Please use the standard scripts for backporting. The PR description is missing the ```upstream-prs ... ``` section which is used for tracking backports for PRs in main and for generating the release notes.

I mostly did, except for forgetting to do start-backport. Shall I close this PR?

squeed · 2023-04-24T18:47:12Z

This is outputting warning messages in the etcd check, I'm investigating.

We had this issue in main, and I added a conditional to prevent this error message. Clearly that is not working here.

[ upstream commit 8d3a498 ] InjectLabels is one of the functions responsible for synchronizing the ipcache metadata store and ip store. As such, it shouldn't shortcut when the numeric identity is the same, but the source is different; this means that an update to the ipcache isn't complete. This can happen, for example, when there are two identities for the same IP, which can happen on daemon restart whenever a CIDR is referenced. Fixes: cilium#24502 Signed-off-by: Casey Callendrello <cdc@isovalent.com>

squeed · 2023-04-24T18:57:52Z

Aha, found the issue - the semantics of previouslyAllocatedIdentities changed between v1.13 and main. I can't use it to reliably silence the error message. I have a (slightly) ugly fix:

--- a/pkg/ipcache/metadata.go
+++ b/pkg/ipcache/metadata.go
@@ -176,6 +176,9 @@ func (ipc *IPCache) InjectLabels(ctx context.Context, modifiedPrefixes []netip.P
                entriesToReplace   = make(map[netip.Prefix]Identity)
                entriesToDelete    = make(map[netip.Prefix]Identity)
                forceIPCacheUpdate = make(map[netip.Prefix]bool) // prefix => force
+
+               // Just used to silence a warning where it is safe
+               oldIDs = make(map[netip.Prefix]Identity)
        )
 
        ipc.metadata.RLock()
@@ -189,6 +192,7 @@ func (ipc *IPCache) InjectLabels(ctx context.Context, modifiedPrefixes []netip.P
                                continue
                        } // else continue below to remove the old entry
                } else {
+                       oldIDs[prefix] = id
                        var newID *identity.Identity
 
                        lbls := prefixInfo.ToLabels()
@@ -283,7 +287,7 @@ func (ipc *IPCache) InjectLabels(ctx context.Context, modifiedPrefixes []netip.P
                        // upsert was rejected due to source precedence, but the
                        // identity is unchanged, then we can safely ignore the
                        // error message.
-                       oldID, ok := previouslyAllocatedIdentities[p]
+                       oldID, ok := oldIDs[p]
                        if !(ok && oldID.ID == id.ID && errors.Is(err2, &ErrOverwrite{
                                ExistingSrc: oldID.Source,
                                NewSrc:      id.Source,

joestringer · 2023-04-24T19:25:52Z

Shall I close this PR?

It should be enough to just update the PR description as per the guide here.

joestringer

Latest diff LGTM.

joestringer · 2023-04-24T19:29:50Z

/test-backport-1.13

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

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside

Failure Output

FAIL: Can not connect to service "http://192.168.56.11:31049" from outside cluster (2/10)

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

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.

squeed · 2023-04-24T19:52:54Z

PR description fixed, sorry for the oversight.

michi-covalent · 2023-04-24T23:44:31Z

ci-multicluster-1.13: unrelated to this change: [v1.13] datapath: Remove 2005 route table for IPv4 #25086 (comment)
test-1.17-4.19: vm provisioning error similar to CI: Cilium-PR-K8s-1.26-kernel-net-next Build K8s vm provisioning fail #24964.

test-1.26-net-next: i haven't see this failure before. not sure what to make of it 🤔 i'll defer to @squeed and @joestringer to decide whether the failure could be related to this change.

Stacktrace

/home/jenkins/workspace/Cilium-PR-K8s-1.26-kernel-net-next/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:453
Failed waiting for egress policy map entries
Expected
    <*errors.errorString | 0xc000621600>: {
        s: "could not ensure egress policy entries count is equal to 6: 4m0s timeout expired",
    }
to be nil
/home/jenkins/workspace/Cilium-PR-K8s-1.26-kernel-net-next/src/github.com/cilium/cilium/test/k8s/egress.go:329

michi-covalent · 2023-04-24T23:47:57Z

/test-1.17-4.19

squeed · 2023-04-25T07:16:14Z

This code doesn't directly touch egress gateways, though I make no promises that they're unrelated. I'm investigating.

squeed · 2023-04-25T08:51:07Z

marking as do-not-merge and triggering test again, I want to run the test again and get some more signal

squeed · 2023-04-25T08:51:53Z

/test-1.26-net-next

Result: vagrant failure to boot.

squeed · 2023-04-25T11:14:57Z

/test-1.26-net-next

Result: Failed: K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside

squeed · 2023-04-25T11:31:16Z

/test-only --focus="K8sDatapathEgressGatewayTest" --kernel_version=net-next --k8s_version=1.26

Result: Passed. That's a good sign.

squeed · 2023-04-25T14:25:42Z

Dug in to the failed sysdump with @jibi, and we found that, for some unknown reason, a pod was missing from the set of pods to which egress policy applied:

Source IP   Destination CIDR   Egress IP        Gateway IP
10.0.0.25   192.168.56.13/32   192.168.56.100   192.168.56.12
10.0.0.25   0.0.0.0/0          192.0.2.13       192.168.56.12
10.0.0.93   0.0.0.0/0          192.168.56.100   192.168.56.12
10.0.1.2    0.0.0.0/0          192.168.56.100   192.168.56.12

should have 6 lines, including two for 10.0.1.140. It doesn't. There are no error messages, and the datapath is directly from k8s -> egressgateway/manager.go; no detours through the ipcache or identity allocator.

At this point, I'm fairly convinced my PR didn't cause this:

It passed a retest
My PR doesn't touch the flow from CRD watcher -> egress gateway manager.
What my PR does touch, namely, the ipcache, is correct on the nodes.

squeed · 2023-04-25T15:32:17Z

Doing a quick scan of the Jenkins logs, it seems like this test has been failing somewhat frequently. I don't see a flake issue, but I we can safely say this is a flake.

squeed · 2023-04-25T15:36:01Z

At this point, I think this PR is OK to merge.

The failing tests:

test-1.19-4.19: passed, somehow PR wasn't updated
ci-multicluster: this job is broken, some kind configs are missing, CI: conformance-clustermesh-v1.13 broken #25118
Focused-Test-Run: not needed, vagrant box failed to boot
1.26-net-next: failed K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside, which is also failing for another PR. CI: 1.13 flake: K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside #25119

testutils/allocator: Respect OldNID

1713301

[ upstream commit f52b4c9 ] This makes the mock allocator work similarly to the "real" one: if an ID is requested and it is not in use, then accept it. Signed-off-by: Casey Callendrello <cdc@isovalent.com>

squeed requested a review from a team as a code owner April 24, 2023 08:46

squeed added kind/backports This PR provides functionality previously merged into master. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. labels Apr 24, 2023

maintainer-s-little-helper bot mentioned this pull request Apr 24, 2023

CI: K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master #20723

Closed

squeed force-pushed the backport-ipcache-source-fix-113 branch from 03d3e30 to 95aaca8 Compare April 24, 2023 18:59

joestringer approved these changes Apr 24, 2023

View reviewed changes

michi-covalent approved these changes Apr 24, 2023

View reviewed changes

squeed added the dont-merge/bad-bot To prevent MLH from marking ready-to-merge. label Apr 25, 2023

squeed removed the dont-merge/bad-bot To prevent MLH from marking ready-to-merge. label Apr 25, 2023

squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 25, 2023

michi-covalent merged commit d367fb3 into cilium:v1.13 Apr 25, 2023
58 of 62 checks passed

thorn3r mentioned this pull request May 17, 2023

Prepare for release v1.13.3 #25514

Merged

[backport-v1.13] ipcache: don't short-circuit InjectLabels if source differs #25077

[backport-v1.13] ipcache: don't short-circuit InjectLabels if source differs #25077

Conversation

squeed commented Apr 24, 2023 • edited

squeed commented Apr 24, 2023

michi-covalent commented Apr 24, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

joestringer commented Apr 24, 2023 • edited

joestringer commented Apr 24, 2023 • edited

squeed commented Apr 24, 2023

squeed commented Apr 24, 2023

squeed commented Apr 24, 2023

joestringer commented Apr 24, 2023

joestringer left a comment

Choose a reason for hiding this comment

joestringer commented Apr 24, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

squeed commented Apr 24, 2023 • edited

michi-covalent commented Apr 24, 2023 • edited

michi-covalent commented Apr 24, 2023

squeed commented Apr 25, 2023

squeed commented Apr 25, 2023

squeed commented Apr 25, 2023 • edited

squeed commented Apr 25, 2023 • edited

squeed commented Apr 25, 2023 • edited

squeed commented Apr 25, 2023

squeed commented Apr 25, 2023

squeed commented Apr 25, 2023 • edited

squeed commented Apr 24, 2023 •

edited

michi-covalent commented Apr 24, 2023 •

edited by maintainer-s-little-helper bot

joestringer commented Apr 24, 2023 •

edited

joestringer commented Apr 24, 2023 •

edited

joestringer commented Apr 24, 2023 •

edited by maintainer-s-little-helper bot

squeed commented Apr 24, 2023 •

edited

michi-covalent commented Apr 24, 2023 •

edited

squeed commented Apr 25, 2023 •

edited

squeed commented Apr 25, 2023 •

edited

squeed commented Apr 25, 2023 •

edited

squeed commented Apr 25, 2023 •

edited