-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
runtime: flexible layering configuration. #7032
Conversation
To support TDS and to provide operator flexibility in how it relates to existing runtime layering, this PR introduces support for explicit control over layering. This eliminates the distinguished role of subdirectory and override_subdirectory, for example, in favor of arbitrary disk overlay layers. This PR deprecates the existing runtime configuration in favor of the new layered variant. As a bonus, the PR fixes some of the runtime stats and related docs. Risk level: Low Testing: various unit tests (including server_test) additions. Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
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.
Thanks this is super cool. A few high level comments to get started from a first read through.
/wait
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.
Nice flexibility improvement. mostly high level questions from me
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
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.
Quick comment, will do a full review once CI is fixed. Generally looks awesome.
/wait
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
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.
LGTM with 2 small comments. Very cool.
/wait
server_.runtime().mergeValues(overrides); | ||
} catch (const EnvoyException& e) { | ||
response.add(e.what()); | ||
return Http::Code::ServiceUnavailable; |
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 would probably return a 400 here, though I don't feel very strongly about it.
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 thinking here is that 503 better reflects the fact that there is a service, but it's not available due to missing config. But I'm also shruggie on it :)
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.
@jmarantz probably has a stronger opinion here so I might briefly ping him and see if he cares. I'm fine either way.
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.
Happy to continue discussing, but I'll take the feedback in a separate PR if that's OK and merge this one, as this is blocking some other PRs that are time sensitive. I'd like to land the TDS API proto PR that follows this so we can unblock other dependent projects that are waiting for this.
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.
👍
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.
LGTM modulo one final nit
throw EnvoyException( | ||
"Too many admin layers specified in LayeredRuntime, at most one may be specified"); | ||
} else { | ||
config_has_admin_layer_ = true; |
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.
Is there anything in the docs about requiring exactly one admin layer?
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.
Signed-off-by: Harvey Tuch <htuch@google.com>
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.
LGTM, great work. Will defer to other reviewers if they have any further comments.
Will merge given two LGTMs, please leave further feedback and I'll do a followup PR. |
To support TDS and to provide operator flexibility in how it relates to
existing runtime layering, this PR introduces support for explicit
control over layering. This eliminates the distinguished role of
subdirectory and override_subdirectory, for example, in favor of
arbitrary disk overlay layers.
This PR deprecates the existing runtime configuration in favor of the
new layered variant.
As a bonus, the PR fixes some of the runtime stats and related docs.
Risk level: Low
Testing: various unit tests (including server_test) additions.
Part of #6708.
Signed-off-by: Harvey Tuch htuch@google.com