-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Ensure conections persist until done on queue-proxy drain #16080
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
base: main
Are you sure you want to change the base?
Changes from all commits
9fa4ff6
91d5b0a
8829034
0750a4d
962f690
b1f26b8
f6dd4b9
7a48a83
eab43de
f26e7d4
eb165e5
e71e1cb
1437e6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,3 +9,6 @@ | |
# Temporary output of build tools | ||
bazel-* | ||
*.out | ||
|
||
# Repomix outputs | ||
repomix*.xml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,13 @@ func (p *podTracker) Capacity() int { | |
if p.b == nil { | ||
return 1 | ||
} | ||
return p.b.Capacity() | ||
capacity := p.b.Capacity() | ||
// Safe conversion: breaker capacity is always reasonable for int | ||
// Check for overflow before conversion | ||
if capacity > 0x7FFFFFFF { | ||
return 0x7FFFFFFF // Return max int32 value | ||
} | ||
return int(capacity) | ||
} | ||
|
||
func (p *podTracker) UpdateConcurrency(c int) { | ||
|
@@ -118,7 +124,7 @@ func (p *podTracker) Reserve(ctx context.Context) (func(), bool) { | |
} | ||
|
||
type breaker interface { | ||
Capacity() int | ||
Capacity() uint64 | ||
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. Anything motivating this change? This seems unrelated to web socket drain? I've also tried something like this before but you end up with a lot of casting that I didn't feel like it was worth it because int==int64 on the arch's we support 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. Oh I just changed it because of a TODO comment for changing the type, figured I would just get it done. I can revert if you feel as though it is not worth the trouble |
||
Maybe(ctx context.Context, thunk func()) error | ||
UpdateConcurrency(int) | ||
Reserve(ctx context.Context) (func(), bool) | ||
|
@@ -721,8 +727,13 @@ func newInfiniteBreaker(logger *zap.SugaredLogger) *infiniteBreaker { | |
} | ||
|
||
// Capacity returns the current capacity of the breaker | ||
func (ib *infiniteBreaker) Capacity() int { | ||
return int(ib.concurrency.Load()) | ||
func (ib *infiniteBreaker) Capacity() uint64 { | ||
concurrency := ib.concurrency.Load() | ||
// Safe conversion: concurrency is int32 and we check for non-negative | ||
if concurrency >= 0 { | ||
return uint64(concurrency) | ||
} | ||
return 0 | ||
Comment on lines
+731
to
+736
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. infinite breaker is only really 0 or 1 so I don't think we need this extra conversion check 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. The linter was complaining which is why I did this. |
||
} | ||
|
||
func zeroOrOne(x int) int32 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
Copyright 2024 The Knative Authors | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package drain | ||
|
||
const ( | ||
// SignalDirectory is the directory where drain signal files are created | ||
SignalDirectory = "/var/run/knative" | ||
|
||
// DrainStartedFile indicates that pod termination has begun and queue-proxy is handling shutdown | ||
DrainStartedFile = SignalDirectory + "/drain-started" | ||
|
||
// DrainCompleteFile indicates that queue-proxy has finished draining requests | ||
DrainCompleteFile = SignalDirectory + "/drain-complete" | ||
|
||
// DrainCheckInterval is how often to check for drain completion | ||
DrainCheckInterval = "1" // seconds | ||
|
||
// ExponentialBackoffDelays are the delays in seconds for checking drain-started file | ||
// Total max wait time: 1+2+4+8 = 15 seconds | ||
ExponentialBackoffDelays = "1 2 4 8" | ||
) | ||
|
||
// BuildDrainWaitScript generates the shell script for waiting on drain signals. | ||
// If existingCommand is provided, it will be executed before the drain wait. | ||
func BuildDrainWaitScript(existingCommand string) string { | ||
drainLogic := `for i in ` + ExponentialBackoffDelays + `; do ` + | ||
` if [ -f ` + DrainStartedFile + ` ]; then ` + | ||
` until [ -f ` + DrainCompleteFile + ` ]; do sleep ` + DrainCheckInterval + `; done; ` + | ||
` exit 0; ` + | ||
` fi; ` + | ||
` sleep $i; ` + | ||
`done; ` + | ||
`exit 0` | ||
|
||
if existingCommand != "" { | ||
return existingCommand + "; " + drainLogic | ||
} | ||
return drainLogic | ||
} | ||
|
||
// QueueProxyPreStopScript is the script executed by queue-proxy's PreStop hook | ||
const QueueProxyPreStopScript = "touch " + DrainStartedFile |
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.
We don't support 32bit architectures - so int == int64