Introduce v2 backend map with u32 backend ID #17235
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
Weil0ng
force-pushed
the
bemap
branch
4 times, most recently
from
109e9c4
to
7590bb8
Compare
Weil0ng
added
release-note/misc
Weil0ng
force-pushed
the
bemap
branch
2 times, most recently
from
54c848c
to
5394afe
Compare
pchaigno
added
upgrade-impact
There was a problem hiding this comment.
I don't understand why we still have references to the old map in this PR. Why are they needed? We can handle the downgrade by adding code to v1.10 for that (copy from v2 to v1).
The 2/2 part will switch operation to v2 map completely
What is not already switched to v2 in this PR?
introduce a --keep-legacy-service-backends flag to control whether we get rid of v1 map or keep it (for downgrade).
We usually just delete the maps after 3 releases, once nobody is supposed to be running a version with the old map.
Datapath is still using v1 map in this PR.
I see, that works too, |
|
Do we really need to give control over that to the user? We could switch to v2 in this PR, with add code for the v1->v2 migration. Then add code for the v2->v1 migration as a patch on top of v1.10. We leave the v1 map around if it exists (but it won't be created for new clusters) and we explicitly remove it in 3 minor releases. We took a similar approach for the policy tail call (after a rename from The current approach implemented in this PR means we'll have wasted memory on the v1 map for new clusters. The flag also seems a bit confusing to me; why would the user need to care about such low-level implementation details? |
I'm sold :) A bit confused when you say we add v2->v1 on top of 1.10, so this PR (v1->v2) would go in 1.11, correct? |
Yes, the present PR (map v1->map v2) would go in v1.11 and the reverse operation (map v2->map v1) would be implemented as a direct change on the v1.10 branch, as done for v1.7 in #13052 for example. |
|
test-1.16-netnext |
|
Managed to reproduce the Maglev Tests_NodePort test failure locally, here's a few observations:
Target service: Curl tries:
Inner Maglev map dump: New V2 backend map dump: |
|
Seems like backend selection is broken: |
|
It seems like the backend id is not interpreted correctly: From another run, here's the inner map: From a debug print from within |
pchaigno
removed
the
dont-merge/blocked
The V2 map key is u32 typed instead of u16. Upon agent (re)start: - we restore backends from v1 map and copy all entries from v1 map to v2 map if v1 map exists (this is the upgrade scenario). - we then remove v1 map and operate on v2 map. Signed-off-by: Weilong Cui <cuiwl@google.com>
|
Figured out what's going on, the offset multiplier needs to be doubled with u16 -> u32 change. |
|
test-me-please Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR: Click to show.Test NameFailure OutputIf it is a flake, comment Job 'Cilium-PR-K8s-GKE' hit: #17270 (94.72% similarity) |
|
GKE tests complain pod not ready in time, seems like a flakiness similar to #17307 |
|
test-gke |
|
test-gke |
|
Past two GKE tests report disjoint set of failures, all seem like flakes unrelated to this PR. |
Weil0ng
added
the
ready-to-merge
jibi
removed
the
ready-to-merge
|
(removed the ready-to-merge label as this still needs a review from agent and CLI teams) |
|
Still need someone from cli to take a look, wonder if @twpayne can help out here :) |
|
Got approval from all code owners, CI is passing too (gke-stable reported disjoint flakes in past two runs), marking reade-to-merge. |
Weil0ng
added
the
ready-to-merge
See commit msg.
Fixes: #16121
Downgrade is tracked in #17262