Skip to content

rlqs: Add per route runtime overrides#22150

Closed
sergiitk wants to merge 1 commit intoenvoyproxy:mainfrom
sergiitk:rlqs-per-route-runtime
Closed

rlqs: Add per route runtime overrides#22150
sergiitk wants to merge 1 commit intoenvoyproxy:mainfrom
sergiitk:rlqs-per-route-runtime

Conversation

@sergiitk
Copy link
Copy Markdown
Contributor

Signed-off-by: Sergii Tkachenko sergiitk@google.com

Per initial review comment: #19793 (comment)
CC @yanavlasov @mattklein123

Commit Message: rlqs: Add per route runtime overrides
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #22150 was opened by sergiitk.

see: more, trace.

@sergiitk sergiitk mentioned this pull request Jul 12, 2022
@markdroth
Copy link
Copy Markdown
Contributor

Why are these being added to the override config but not the top-level config? Wouldn't it also be useful to set these in the top-level config? Although if we do add them to the top-level config, we also need to define precedence rules for them.

Also, note that gRPC does not have Envoy's concept of runtime settings, so we would be able to support these only as fixed percentages.

@sergiitk
Copy link
Copy Markdown
Contributor Author

@markdroth Because they already present at the top level. This is a follow up for adding them to the overrides, see this comment: #19793 (comment):

You might consider adding per route runtime overrides here also, default to 100%, but up to you. That could be added later also if needed.
@mattklein123

I caught up with @yanavlasov on this after the PR got merged, so adding this post-factum.

@markdroth
Copy link
Copy Markdown
Contributor

Ah, okay, I hadn't realized that these had already been added at the top level.

What is the override behavior here? If one of these fields is set in the top-level config but not set in the override config, do we inherit the value from the top-level config? Or is the idea that if an override config is present, if these fields are not set in the override config, then we treat these fields as unset, even if they are set in the top-level config? I think the latter makes more sense, but we should explicitly state how this works in the proto comments.

@sergiitk
Copy link
Copy Markdown
Contributor Author

@markdroth Don't know how it should work - as you mentioned we don't support it in gRPC. Handing this over to @tyxia.

@phlax
Copy link
Copy Markdown
Member

phlax commented Jul 19, 2022

/wait-any

@tyxia
Copy link
Copy Markdown
Member

tyxia commented Aug 10, 2022

/assign @tyxia

@zuercher
Copy link
Copy Markdown
Member

I think we're still waiting on answers/doc updates to Mark's question.

@zuercher
Copy link
Copy Markdown
Member

/wait-any

@markdroth
Copy link
Copy Markdown
Contributor

@markdroth Don't know how it should work - as you mentioned we don't support it in gRPC. Handing this over to @tyxia.

gRPC doesn't support the runtime setting, but the config.core.v3.RuntimeFractionalPercent field allows a fixed percentage to be specified (in the default_value field, which we use in gRPC in the context of route matching). So we will need to answer this question in gRPC as well.

I think the question of override semantics applies equally to gRPC and Envoy. We need to define this before this PR can be merged.

@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 28, 2022
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 5, 2022

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot closed this Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants