Repository navigation
L2Status controller: refactor to leader node manages resources - #2938
Merged
Merged
Conversation
oribon
changed the title
oribon
changed the title
|
Thanks you started to work on fix so fast. I believe you found the root cause of the issue. Yesterday I spot an ServiceL2Status instance with 2 owning pods on one of mine clusters. |
Contributor
|
@oribon Can we somehow add an E2E test for that .. or at the bare minimum a unit test? |
oribon
force-pushed
the
another_status_fix
branch
2 times, most recently
from
January 27, 2026 10:50
beceb5b to
1aedeaa
Compare
Member
Author
|
@andreaskaris added |
Contributor
|
So I tinkered with this a little bit, with: Check with: Then, stop the worker that the l2advertisement originates from: With the fix, it looks like no cleanup is happening (expected, according to the code): |
oribon
changed the title
oribon
force-pushed
the
another_status_fix
branch
from
January 27, 2026 14:33
5b4c2d1 to
0e1a968
Compare
Contributor
|
I checked with the old code, and there are 2 scenarios. Note that in both scenarios, because it's a hard shutdown, the speaker pod on the shut down worker node is not evacuated (but the node reports as NotReady). Therefore, the API server will not clean up via ownerReference. In both scenarios, the bug is triggered, and the servicel2status gets 2 ownerreferences (old and new speaker pod), and then tries to update the immutable .status.node field without success. Difference between scenario a) and b): a) Instantaneous failover, bug reproduced, no cleanup: Labels show the new node, and 2 owner references, whereas the status was not updated: b) Failover very slow - however, we have resource cleanup, as soon as the node goes into NotReady state: Using the following debug statements to analyze the behavior: case a): It seems that the memberlist suspect failed detection works - we see this here on both remaining pods: speaker-7995k (kind-control-plane): speaker-nbmq6 (kind-worker2): kind-worker2 wins the new election: speaker-7995k (kind-control-plane): speaker-nbmq6 (kind-worker2): The speaker on kind-worker2 has advertisements: And it can list the existing servicel2status, which isn't owned by it, though: Now, it creates a new state: Ignoring the fact that we do not want this pod to manage this resource, the first problem here is that the old owner reference isn't removed, and later on we will see that this Existing and desired status aren't equal: So, we run CreateOrPatch now: With 2 ownerReferences, and the updated status. However, the status value is immutable: That immutable error comes from the CEL validation which doesn't allow changes to Node, ServiceName or ServiceNamespace in the status field. Note that this now repeats with an expontential backoff: case b): Both nodes detect that kind-worker failed: speaker-zlc97 (kind-control-plane): speaker-vcwtb (kind-worker2): Both check if they should announce, kind-worker2 announces: speaker-zlc97 (kind-control-plane): speaker-vcwtb (kind-worker2): So now, kind-worker2 runs the layer2 status controller - it has advertisements: And it can list the existing servicel2status, which isn't owned by it, though: Now, it creates a new state: Existing and desired status aren't equal: So, we run a CreateOrPatch now, with both speakers listed as owner ref, and other fields set to kind-worker2: The update to status.node fails: That same sequence is now triggered for a while, in a loop, because the controller cannot update the resource. After a while, the controller notices that something went wrong with the servie announcement (we can see that the curl didn't work) - that coincides with the node NotReady of kind-worker, so I imagine that Therefore, we now delete the servicel2status: Later, the laer2_controller announces the l2 service again: And now everything converges correctly, with no redundant servicel2status CRs. |
| // and keep/create only our own. | ||
| var existingStatus *v1beta1.ServiceL2Status | ||
| for i := range serviceL2statuses.Items { | ||
| item := &serviceL2statuses.Items[i] |
Contributor
There was a problem hiding this comment.
nit: with newer go versions making copies of loop variables, wouldn't simply for _, item := range serviceL2statuses.Items work? followed by r.Delete(ctx, &item)
| if err := r.Delete(ctx, &serviceL2statuses.Items[i+1]); err != nil && !errors.IsNotFound(err) { | ||
| deleteRedundantErrs = append(deleteRedundantErrs, err) | ||
| } | ||
| // We are the (sole) leader for this service - delete statuses from other nodes |
Contributor
There was a problem hiding this comment.
nit: the comment is mislreading: the comment below is correct and says "// Delete statuses from other nodes / redundant statuses belonging to our node". Also, it's redundant to have the 'delete statuses from other nodes ..' stuff both in line 97 and line 106
| if len(serviceL2statuses.Items) > 0 { | ||
| state = &serviceL2statuses.Items[0] | ||
| if existingStatus != nil { | ||
| state = existingStatus |
Contributor
There was a problem hiding this comment.
nit / thinking out loud: what about turning this on its head?
line 99:
var state *v1beta1.ServiceL2Status
line 102:
if item.Labels[LabelAnnounceNode] == r.NodeName && state == nil {
state = item
line 116:
if state == nil {
state = &v1beta1.ServiceL2Status{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "l2-",
Namespace: r.Namespace,
},
}
}
Contributor
|
I tested the newest version of the PR and it looks good with the newest proposal: |
Contributor
|
Please check the nits, squash, and I can give my lgtm :-) Sorry for the lengthy posts |
oribon
force-pushed
the
another_status_fix
branch
from
January 28, 2026 08:39
0e1a968 to
0055ebb
Compare
Contributor
|
/lgtm |
Previously each controller managed only its own resources, allowing: 1. nodes could potentially "steal" eachother's resources (a bug caused by the controller using the wrong index) 2. on abrupt shutdown of a node its resource stays dangling (because the pod is never deleted until next boot, ownerref gc doesn't kick in) Here, we refactor the logic so that the leader node for a service makes sure there is only one resource for the service (that it manages). This solves (1) because the leader enforces only one resource, and solves (2) because if a new leader is elected for the service (through memberlist) then it deletes the dangling resource. In the case a leader node is abruptly shutdown but it can be the only node announcing the service (e.g due to nodeSelectors on the L2Adv) we would still have a dangling resource. Signed-off-by: Ori Braunshtein <[email protected]> Assisted-by: Cursor
oribon
force-pushed
the
another_status_fix
branch
from
January 28, 2026 09:19
0055ebb to
730a4d4
Compare
Contributor
|
/lgtm still subject to CI |
oribon
changed the title
Contributor
|
Hi! Are there any plans to cut a new release that includes this fix? The latest release (v0.15.3) was published before this PR was merged, so the fix is currently only available on |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously each controller managed only its own resources, allowing:
(a bug caused by the controller using the wrong index)
(because the pod is never deleted until next boot, ownerref
gc doesn't kick in)
Here, we refactor the logic so that the leader node for a service
makes sure there is only one resource for the service (that it manages).
This solves (1) because the leader enforces only one resource,
and solves (2) because if a new leader is elected for the service
(through memberlist) then it deletes the dangling resource.
In the case a leader node is abruptly shutdown but it can be the only
node announcing the service (e.g due to nodeSelectors on the L2Adv)
we would still have a dangling resource.
Is this a BUG FIX or a FEATURE ?:
/kind bug
What this PR does / why we need it:
Attempts to fix #2930
Special notes for your reviewer:
Release note: