-
Notifications
You must be signed in to change notification settings - Fork 3
TRITON-2529 Add support in CLB for configuring global timeouts via metadata #8
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
Conversation
Allow users to customize HAProxy timeout values through the new
cloud.tritoncompute:timeouts metadata key. The format uses a JSON-like
syntax: {queue:0,connect:5000,client:60000,server:180000}
All timeout parameters are optional - unspecified values use defaults:
- queue: 0 (unlimited)
- connect: 2000ms
- client: 55000ms
- server: 120000ms
Changes:
- Add TIMEOUTS_KEY metadata constant
- Add TimeoutConfig struct and default constants
- Add parse_timeouts() function for parsing metadata
- Convert 001-defaults.cfg to Askama template for dynamic rendering
- Update configure_haproxy() to use dynamic defaults
- Add comprehensive unit tests for timeout parsing
- Add documentation in README.md
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Confirming: I see a Claude credit in this branch's first commit, so RFD 187 attribution applies, and you're taking responsibility for this, correct? I've pinged @TritonDataCenter/triton-engineering for this. Also, I will be opening a TRITON- ticket. The lack of description here needs to be fixed, but the summary on this PR is perfect for said ticket. Anything in this PR's conversation that's ticket-pertinent will be pasted there. This will include test-notes and other things that would go in a ticket. |
Added copyright notice for Edgecast Cloud LLC.
nshalman
left a comment
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.
I am comfortable with this. Thank you!
|
@blovett-ec I will need the following put here for transfer into the ticket:
|
danmcd
left a comment
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.
I've added what I think is a decent statement-of-problem-and-solution in the ticket. I will need your testing notes, esp. in light of comments below.
| input.trim().eq_ignore_ascii_case("true") | ||
| } | ||
|
|
||
| /// Parse timeout overrides from metadata |
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.
Don't we have a Rust crate that'll do this for us? New parsing code feels like a place for security problems, even in a safer language like Rust.
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.
@nshalman may be able to answer this better, esp. with our new Rust monorepo.
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.
Possibly a case of, "yes, but it'd be like bringing an atom-bomb to a knife fight," scenario. Will let @nshalman confirm/deny and mark-as-resolved as appropriate.
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.
Per a discussion with Dan, on the one hand, this syntax is consistent with the one we shoehorned onto the portmap which needed to reasonably extend but be backward compatible with the nodejs implementation.
On the other hand, this is a totally new key so we aren't bound by that constraint and we could use e.g. actual json or whatever.
Another option is to share the code for parsing this mini json-like format rather than having two versions of it... A refactor we could probably get Claude to do...
So yes, the custom "parser" in this PR is a bit of a code smell the more I stare at it...
I might take a swing at consolidating the code and pushing that improvement to this branch...
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.
I have refactored the code. Please take a look, @danmcd and @blovett-ec to make sure you're both okay with the tweak.
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.
works for me.
danmcd
left a comment
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.
Forgot to check request-changes.
danmcd
left a comment
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.
Removing my request-changes. Still have a question about json parsing, but less blocking.
- Change TimeoutConfig fields from u32 to std::time::Duration - Support timeout value formats: plain number (ms), 'ms' suffix, 's' suffix - Clamp client/server timeouts to maximum of 60 minutes - Fail on invalid timeout values instead of falling back to defaults - Add getter methods (queue_ms, etc.) for HAProxy template rendering 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
danmcd
left a comment
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.
My +1 stands even after cargo fmt.
| input.trim().eq_ignore_ascii_case("true") | ||
| } | ||
|
|
||
| /// Parse timeout overrides from metadata |
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.
Possibly a case of, "yes, but it'd be like bringing an atom-bomb to a knife fight," scenario. Will let @nshalman confirm/deny and mark-as-resolved as appropriate.
Extract a generic parse_kv_pairs function to handle the {key:value,...}
parsing pattern used by both health check and timeout configuration.
- Replace HealthCheckParams tuple with HealthCheckConfig struct
- Add set_from_kv methods to HealthCheckConfig and TimeoutConfig
- Use strum-derived enums (HealthCheckKey, TimeoutKey) for exhaustive
key matching instead of hardcoded strings
- Unknown parameters now log warnings instead of returning errors
- Add tests for parse_kv_pairs function
🤖 Generated with [Claude Code](https://claude.com/claude-code)
```console
$ RUST_LOG=warn cargo test -- --nocapture 2>&1 | grep "error message"
Too small port error message: listen port out of valid range (1-65534): 0
Too big port error message: Invalid listen port: 99999
Invalid port error message: Invalid listen port: 99999
```
Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Incidentally, I think for future improvements the time has come to split this out into multiple files. I'm okay with this one landing in one big file. |
Uh oh!
There was an error while loading. Please reload this page.