cni-repair controller #306
Merged
Conversation
This file contains 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
alpeb
added a commit
to linkerd/linkerd2
that referenced
this pull request
Dec 5, 2023
Followup to linkerd/linkerd2-proxy-init#306 Fixes #11073 This adds the `reinitialize-pods` container to the `linkerd-cni` DaemonSet, along with its config in `values.yaml`. Also the `linkerd-cni`'s version is bumped, to contain the new binary for this controller. ## TO-DOs - Integration test
This was referenced Dec 5, 2023
alpeb
force-pushed
the
alpeb/linkerd-reinitialize-pods
branch
from
673650b to
3ab1ec6
Compare
alpeb
added a commit
to linkerd/linkerd2
that referenced
this pull request
Dec 5, 2023
Followup to linkerd/linkerd2-proxy-init#306 Fixes #11073 This adds the `reinitialize-pods` container to the `linkerd-cni` DaemonSet, along with its config in `values.yaml`. Also the `linkerd-cni`'s version is bumped, to contain the new binary for this controller. ## TO-DOs - Integration test
olix0r
reviewed
Dec 5, 2023
olix0r
reviewed
Dec 5, 2023
mateiidavid
reviewed
Dec 6, 2023
There was a problem hiding this comment.
This looks good! Exciting to finally have it committed. Two final questions:
- Do we anticipate needing to emit events? I'm thinking for successful evictions.
- Do we need to retry evictions? Would it ever fail due to a transient request and we wouldn't process it again?
Both of those are more follow-ups if they're required at all. I think what we have now is great 👍🏻
I was expecting k8s itself to surface eviction events, but after checking it turns out that's not the case, so I'll be adding that.
If the eviction fails, I think the pod will continue on its backed-off crashloop anyways, so we'll get the event for the next failure. |
Fixes linkerd/linkerd2#11073 This fixes the issue of injected pods that cannot acquire proper network config because `linkerd-cni` and/or the cluster's network CNI haven't fully started. They are left in a permanent crash loop and once CNI is ready, they need to be restarted externally, which is what this controller does. This controller "`linkerd-reinitialize-pods`" watches over events on pods in the current node, which have been injected but are in a terminated state and whose `linkerd-network-validator` container exited with code 95, and proceeds to evict them so they can restart with a proper network config. The controller is to be deployed as an additional container in the `linkerd-cni` DaemonSet (addressed in linkerd/linkerd2#xxx). ## TO-DOs - Figure why `/metrics` is returning a 404 (should show process metrics) - Integration test
…y on reinitialize-pods
alpeb
force-pushed
the
alpeb/linkerd-reinitialize-pods
branch
from
a756322 to
c9e7e19
Compare
alpeb
force-pushed
the
alpeb/linkerd-reinitialize-pods
branch
from
c9e7e19 to
d0e2384
Compare
mateiidavid
reviewed
Dec 7, 2023
alpeb
force-pushed
the
alpeb/linkerd-reinitialize-pods
branch
from
e3388b9 to
e74450f
Compare
alpeb
force-pushed
the
alpeb/linkerd-reinitialize-pods
branch
from
e74450f to
c7d9a91
Compare
mateiidavid
reviewed
Dec 13, 2023
mateiidavid
approved these changes
Dec 14, 2023
olix0r
reviewed
Dec 19, 2023
olix0r
reviewed
Dec 26, 2023
olix0r
reviewed
Dec 26, 2023
olix0r
reviewed
Dec 26, 2023
alpeb
force-pushed
the
alpeb/linkerd-reinitialize-pods
branch
from
6344b1e to
956a8c7
Compare
|
Thanks for all the feedback @olix0r . The latest commits address all your comments, plus the tasks handling we talked about offline. And I added a unit test that also checks on the lifecycle of |
|
Oh, also looking forward for the suggestions about the timer histogram on these tasks 😉 |
olix0r
reviewed
Dec 29, 2023
olix0r
reviewed
Dec 29, 2023
olix0r
approved these changes
Jan 2, 2024
cni-repair-controller/src/lib.rs
Outdated
Comment on lines
54
to
58
| let (tx, rx) = mpsc::channel(EVENT_CHANNEL_CAPACITY); | ||
| tokio::spawn(process_events(pod_evts, tx, metrics.clone())); | ||
|
|
||
| let client = rt.client(); | ||
| tokio::spawn(process_pods(client, controller_pod_name, rx, metrics)) |
There was a problem hiding this comment.
It's probably desirable to put tracing contexts on each span for better logging/diagnostics:
Suggested change
| let (tx, rx) = mpsc::channel(EVENT_CHANNEL_CAPACITY); | |
| tokio::spawn(process_events(pod_evts, tx, metrics.clone())); | |
| let client = rt.client(); | |
| tokio::spawn(process_pods(client, controller_pod_name, rx, metrics)) | |
| let (tx, rx) = mpsc::channel(EVENT_CHANNEL_CAPACITY); | |
| tokio::spawn( | |
| process_events(pod_evts, tx, metrics.clone()) | |
| .instrument(tracing::info_span!("watch").or_current()), | |
| ); | |
| let client = rt.client(); | |
| tokio::spawn( | |
| process_pods(client, controller_pod_name, rx, metrics) | |
| .instrument(tracing::info_span!("repair").or_current()), | |
| ) |
olix0r
pushed a commit
to linkerd/linkerd2
that referenced
this pull request
Jan 5, 2024
Followup to linkerd/linkerd2-proxy-init#306 Fixes #11073 This adds the `reinitialize-pods` container to the `linkerd-cni` DaemonSet, along with its config in `values.yaml`. Also the `linkerd-cni`'s version is bumped, to contain the new binary for this controller.
adleong
pushed a commit
to linkerd/linkerd2
that referenced
this pull request
Jan 18, 2024
Followup to linkerd/linkerd2-proxy-init#306 Fixes #11073 This adds the `reinitialize-pods` container to the `linkerd-cni` DaemonSet, along with its config in `values.yaml`. Also the `linkerd-cni`'s version is bumped, to contain the new binary for this controller.
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.
Fixes linkerd/linkerd2#11073
This fixes the issue of injected pods that cannot acquire proper network config because
linkerd-cniand/or the cluster's network CNI haven't fully started. They are left in a permanent crash loop and once CNI is ready, they need to be restarted externally, which is what this controller does.This controller "
linkerd-cni-repair-controller" watches over events on pods in the current node, which have been injected but are in a terminated state and whoselinkerd-network-validatorcontainer exited with code 95, and proceeds to delete them so they can restart with a proper network config.The controller is to be deployed as an additional container in the
linkerd-cniDaemonSet (addressed in linkerd/linkerd2#11699).This exposes two custom counter metrics:
linkerd_cni_repair_controller_queue_overflow(in the spirit of the destination controller'sendpoint_updates_queue_overflow) andlinkerd_cni_repair_controller_deleted