Keep other controller statuses when updating httproute status #11705
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
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
olix0r
reviewed
Dec 6, 2023
| @@ -6,6 +6,7 @@ use crate::{ | |||
| use ahash::{AHashMap as HashMap, AHashSet as HashSet}; | |||
| use chrono::offset::Utc; | |||
| use chrono::DateTime; | |||
| use gateway::RouteParentStatus; | |||
There was a problem hiding this comment.
if this needs to be included i'd prefer to i'd prefer to include this on line 12 like:
use linkerd_policy_controller_k8s_api::{self as k8s, gateway::{self, RouteParentStatus}, ResourceExt};
having indirect/relative imports like this gets very confusing in my experience.
olix0r
reviewed
Dec 6, 2023
Comment on lines
293
to
301
| let unowned_statuses = route | ||
| .statuses | ||
| .iter() | ||
| .filter_map(|parent_ref| self.parent_status(parent_ref, backend_condition.clone())) | ||
| .collect(); | ||
| .filter(|status| status.controller_name != POLICY_CONTROLLER_NAME) | ||
| .cloned(); | ||
| let parent_statuses = route | ||
| .parents | ||
| .iter() | ||
| .filter_map(|parent_ref| self.parent_status(parent_ref, backend_condition.clone())); |
There was a problem hiding this comment.
Can you help me understand this by adding a comment that explains why this is load bearing?
Is this essentially that we are "replacing" any statuses where the controller name is ours? I think the implicit bit here is that parent_status only returns the status of things for which we control status?
|
Unrelated to the current fix, but I noticed we are not conserving previous conditions. Looking at statuses in other resources like Pods, it seems we should? |
olix0r
approved these changes
Dec 8, 2023
alpeb
added a commit
that referenced
this pull request
Dec 14, 2023
## edge-23.12.2 This edge release includes a restructuring of the proxy's balancer along with accompanying new metrics. The new minimum supported Kubernetes version is 1.22. * Restructured the proxy's balancer ([#11750]): balancer changes may now occur independently of request processing. Fail-fast circuit breaking is enforced on the balancer's queue so that requests can't get stuck in a queue indefinitely. This new balancer is instrumented with new metrics: request (in-queue) latency histograms, failfast states, discovery updates counts, and balancer endpoint pool sizes. * Changed how the policy controller updates HTTPRoute status so that it doesn't affect statuses from other non-linkerd controllers ([#11705]; fixes [#11659]) [#11750]: #11750 [#11705]: #11705 [#11659]: #11659
alpeb
added a commit
that referenced
this pull request
Dec 14, 2023
## edge-23.12.2 This edge release includes a restructuring of the proxy's balancer along with accompanying new metrics. The new minimum supported Kubernetes version is 1.22. * Restructured the proxy's balancer ([#11750]): balancer changes may now occur independently of request processing. Fail-fast circuit breaking is enforced on the balancer's queue so that requests can't get stuck in a queue indefinitely. This new balancer is instrumented with new metrics: request (in-queue) latency histograms, failfast states, discovery updates counts, and balancer endpoint pool sizes. * Changed how the policy controller updates HTTPRoute status so that it doesn't affect statuses from other non-linkerd controllers ([#11705]; fixes [#11659]) [#11750]: #11750 [#11705]: #11705 [#11659]: #11659
adleong
added a commit
that referenced
this pull request
Dec 20, 2023
Fixes #11659 When the policy controller updates the status of an HttpRoute, it will override any existing statuses on that resource. We update the policy controller to take into account any statuses which are not controlled by Linkerd already on the resource. It will patch the final statuses to be the combination of thee non-Linkerd statuses plus the Linkerd statuses. Signed-off-by: Alex Leong <alex@buoyant.io>
Merged
adleong
added a commit
that referenced
this pull request
Dec 20, 2023
This stable release fixes two bugs in the Linkerd control plane. * Fixed an issue in the destination controller where the metadata API was not properly initialized for jobs, leading to error messages and unnecessary API calls ([#11541]) * Fixed an issue in the policy controller where it was overriding statuses on HTTPRoute resources from other controllers ([#11705]) [#11541]: #11541 [#11705]: #11705
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 #11659
When the policy controller updates the status of an HttpRoute, it will override any existing statuses on that resource.
We update the policy controller to take into account any statuses which are not controlled by Linkerd already on the resource. It will patch the final statuses to be the combination of thee non-Linkerd statuses plus the Linkerd statuses.