-
Notifications
You must be signed in to change notification settings - Fork 158
feat(interceptor): Adds direct-pod routing for cold starts to reduce latency when kube-proxy rule propagation is slow. #1585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
AtharvaPakade
wants to merge
1
commit into
kedacore:main
Choose a base branch
from
AtharvaPakade:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,24 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/caarlos0/env/v11" | ||
| ) | ||
|
|
||
| // DirectPodRoutingMode controls whether and when the interceptor routes requests | ||
| // directly to a ready pod IP instead of through the ClusterIP service. | ||
| type DirectPodRoutingMode string | ||
|
|
||
| const ( | ||
| // DirectPodRoutingDisabled never bypasses the ClusterIP service (default). | ||
| DirectPodRoutingDisabled DirectPodRoutingMode = "disabled" | ||
| // DirectPodRoutingColdStartOnly bypasses the ClusterIP service only on cold | ||
| // starts, reducing latency when kube-proxy rules are slow to propagate. | ||
| DirectPodRoutingColdStartOnly DirectPodRoutingMode = "cold-start-only" | ||
| ) | ||
|
|
||
| // Serving is configuration for how the interceptor serves the proxy | ||
| // and admin server | ||
| type Serving struct { | ||
|
|
@@ -53,10 +66,21 @@ type Serving struct { | |
| EnableColdStartHeader bool `env:"KEDA_HTTP_ENABLE_COLD_START_HEADER" envDefault:"true"` | ||
| // LogRequests enables/disables logging of incoming requests | ||
| LogRequests bool `env:"KEDA_HTTP_LOG_REQUESTS" envDefault:"false"` | ||
| // DirectPodRouting controls when the interceptor routes directly to a pod IP | ||
| // instead of the ClusterIP service. Valid values: "disabled", "cold-start-only". | ||
| DirectPodRouting DirectPodRoutingMode `env:"KEDA_HTTP_DIRECT_POD_ROUTING" envDefault:"disabled"` | ||
| } | ||
|
|
||
| // MustParseServing parses standard configs and returns the | ||
| // newly created config. It panics if parsing fails. | ||
| func MustParseServing() Serving { | ||
| return env.Must(env.ParseAs[Serving]()) | ||
| s := env.Must(env.ParseAs[Serving]()) | ||
| switch s.DirectPodRouting { | ||
| case DirectPodRoutingDisabled, DirectPodRoutingColdStartOnly: | ||
| // valid | ||
| default: | ||
| panic(fmt.Sprintf("invalid KEDA_HTTP_DIRECT_POD_ROUTING value %q: must be %q or %q", | ||
| s.DirectPodRouting, DirectPodRoutingDisabled, DirectPodRoutingColdStartOnly)) | ||
| } | ||
|
Comment on lines
+78
to
+84
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be implemented directly close to the struct by implementing the TextMarshaller interface so that MustParseServing stays clean: func (m *DirectPodRoutingMode) UnmarshalText(text []byte) error {
switch DirectPodRoutingMode(text) {
case DirectPodRoutingDisabled, DirectPodRoutingColdStartOnly:
*m = DirectPodRoutingMode(text)
return nil
default:
return fmt.Errorf("invalid value %q: must be %q or %q",
text, DirectPodRoutingDisabled, DirectPodRoutingColdStartOnly)
}
} |
||
| return s | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package middleware | ||
|
|
||
| const ( | ||
| schemeHTTP = "http" | ||
| schemeHTTPS = "https" | ||
| ) |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldnt this be a simple boolean? Internally we later only use a boolean as well.
Is there a reason for why we would use direct pod routing only on cold starts according to this config (didnt check the full PR yet)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fedosin Suggested in the review that we should give option for different modes comment link section
2. Scope: cold-start-only vs. all requestsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I missed that comment, quoting parts of it as I'm missing the "why":
To be honest this doesn't really explain why we should use the ClusterIP for warm requests and thus build a solution that is split between cold and warm requests? A single implementation just using Endpoint IPs seems simpler.
We also solve the issue, that existing pooled connections in the http.Transport are not targeting new pods. If we always connect to ClusterIP the TCP connection is kept open to the existing pods, new pods are only reached during new connections which is currently a problem.
knative also does not differentiate between cold and warm requests AFAIK.
I'm not sure what is meant here with the blast radius, but two paths (=different cold and warm paths) increase complexity compared to just one.
If this direct pod routing is broken, users would disable it completely anyways, changing it from "always" to "cold-start-only" or such would still give them broken cold start requests, right?
Let me know if I misunderstood @Fedosin
I think we plan to enable it by default in the near future anyways, adding another step between us now and this goal is IMO not worth it which is why I see this as an on and off feature.