Skip to content
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

Added support for Envoy slow start mode. #4772

Merged
merged 6 commits into from
Oct 6, 2022

Conversation

tsaarni
Copy link
Member

@tsaarni tsaarni commented Oct 4, 2022

This PR adds support for slow start mode, described here in Envoy documentation. Envoy to gradually increase the amount of traffic targeted to a newly added upstream endpoint. This can be useful for example with JVM based applications, that might otherwise get overwhelmed during JIT warm-up period.

The feature is configured via struct HTTPProxy.spec.routes.services.slowStart:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: slow-start
spec:
  virtualhost:
    fqdn: www.example.com
  routes:
    - services:
        - name: java-app
          port: 80
          slowStartPolicy:
            window: 3s
            aggression: 1.0
            minWeightPercent: 10

Slow start is off by default. To enable it, only window is mandatory. Other parameters are optional.

Fixes #2296
Fixes #4687

Signed-off-by: Tero Saarni [email protected]

@tsaarni tsaarni requested a review from a team as a code owner October 4, 2022 18:26
@tsaarni tsaarni requested review from stevesloka and sunjayBhatia and removed request for a team October 4, 2022 18:26
@tsaarni tsaarni force-pushed the tsaarni/issue4687 branch from 4645127 to abb1ab5 Compare October 4, 2022 18:27
@tsaarni tsaarni added the release-note/major A major change that needs more than a paragraph of explanation in the release notes. label Oct 4, 2022
@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #4772 (24ae5e2) into main (e4a0156) will increase coverage by 0.04%.
The diff coverage is 88.70%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4772      +/-   ##
==========================================
+ Coverage   75.98%   76.02%   +0.04%     
==========================================
  Files         140      140              
  Lines       16787    16849      +62     
==========================================
+ Hits        12755    12809      +54     
- Misses       3780     3788       +8     
  Partials      252      252              
Impacted Files Coverage Δ
internal/envoy/v3/cluster.go 93.65% <73.07%> (-2.37%) ⬇️
internal/dag/dag.go 96.62% <100.00%> (+0.04%) ⬆️
internal/dag/httpproxy_processor.go 93.07% <100.00%> (+0.20%) ⬆️
internal/envoy/cluster.go 100.00% <100.00%> (ø)
internal/sorter/sorter.go 98.46% <0.00%> (-0.52%) ⬇️

@tsaarni
Copy link
Member Author

tsaarni commented Oct 4, 2022

Manual test procedure for this PR is described here https://gist.github.com/tsaarni/a22e3d69bf6c31ff0597e9f78454868a.

@skriss skriss self-requested a review October 4, 2022 20:37
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsaarni just a couple nits but overall this looks great and pretty straight-forward, thanks! I think we can include this in the upcoming 1.23 release.

apis/projectcontour/v1/httpproxy.go Outdated Show resolved Hide resolved
apis/projectcontour/v1/httpproxy.go Outdated Show resolved Hide resolved
internal/dag/httpproxy_processor.go Outdated Show resolved Hide resolved
apis/projectcontour/v1/httpproxy.go Outdated Show resolved Hide resolved
Signed-off-by: Tero Saarni <[email protected]>
@tsaarni
Copy link
Member Author

tsaarni commented Oct 5, 2022

Thank you for good comments @skriss!

@tsaarni just a couple nits but overall this looks great and pretty straight-forward, thanks! I think we can include this in the upcoming 1.23 release.

That would be great! :-)

BTW I see there is problem with actions currently but I will re-run/bump this PR to trigger the checks later, unless it sorts out itself automatically.

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the updates @tsaarni, LGTM. I'll leave for @sunjayBhatia to take a look if he wants.

SlowStartWindow: protobuf.Duration(slowStartConfig.Window),
Aggression: &envoy_core_v3.RuntimeDouble{
DefaultValue: slowStartConfig.Aggression,
RuntimeKey: "contour.slowstart.aggression",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting this is also configurable via RTDS, not sure if we will need to take advantage of that though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I did not see how to take advantage of it either, but it seems that (almost) all Runtime* message types have validation rules in .proto which makes the RuntimeKeys required. For example in this case RuntimeDouble.runtime_key has validation rule:

string runtime_key = 2 [(validate.rules).string = {min_len: 1}];

that causes config to be rejected if runtime key is not given:

[2022-10-06 06:03:23.608][1][warning][config] [source/common/config/grpc_subscription_impl.cc:126] 
gRPC config for type.googleapis.com/envoy.config.cluster.v3.Cluster rejected:
Proto constraint validation failed
(ClusterValidationError.RoundRobinLbConfig: embedded message failed validation | 
caused by RoundRobinLbConfigValidationError.SlowStartConfig: embedded message failed validation |
caused by SlowStartConfigValidationError.Aggression: embedded message failed validation |
caused by RuntimeDoubleValidationError.RuntimeKey: value length must be at least 1 characters)

So I have assumed these are mandatory, even if we would never use them.

Curiously, there are at least some cases where it is not required, like when we configure ratelimit here. In this case the message definition is missing the validation rule. Maybe that is just by mistake, since that is the only runtime_key that is missing validation in base.proto.

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just made a little comment on doc wording 👍🏽

@tsaarni
Copy link
Member Author

tsaarni commented Oct 6, 2022

LGTM, just made a little comment on doc wording 👍🏽

Very good clarification, thank you for the review!

@tsaarni tsaarni merged commit 423b8e6 into projectcontour:main Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/major A major change that needs more than a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gradual traffic increase for cold starting endpoints Progressive traffic increase for new Pods
3 participants