Adds support for global circuit budget

[davinci26]

Solves #6001

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a579ec3) 78.70% compared to head (e4a5cbb) 78.77%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6013      +/-   ##
==========================================
+ Coverage   78.70%   78.77%   +0.07%     
==========================================
  Files         138      138              
  Lines       19717    19745      +28     
==========================================
+ Hits        15518    15555      +37     
+ Misses       3895     3887       -8     
+ Partials      304      303       -1     

Files	Coverage Δ
cmd/contour/servecontext.go	83.88% <100.00%> (+0.03%)	⬆️
internal/dag/gatewayapi_processor.go	93.74% <100.00%> (+<0.01%)	⬆️
internal/dag/httpproxy_processor.go	91.94% <100.00%> (+0.01%)	⬆️
internal/dag/ingress_processor.go	97.61% <100.00%> (+0.01%)	⬆️
internal/dag/policy.go	95.65% <100.00%> (+0.12%)	⬆️
pkg/config/parameters.go	86.39% <ø> (ø)
cmd/contour/serve.go	20.77% <75.00%> (+1.27%)	⬆️

[davinci26]

removing @clayton-gonsalves since he is on PAT leave (if you see this feel free to review anyway).

Adding @shadialtarsha and @seth-epps as well

[shadialtarsha]

I think the code makes sense to me.
I have 2 questions:

1. What will happen if not all GlobalCircuitBreakerDefaults attributes are defined? I think they will have the value 0 and we will default to Envoy settings again which makes sense but I would clarify this in the doc.
2. Wondering if we can write e2e tests for this feature. I know it might be tricky.

[skriss]

Thanks for the PR @davinci26! Overall this makes sense to me, added some comments.

[skriss]

Could you explain why you want to expose this as a metric? We don't really have any other metrics like this, so it's a little unusual.

[davinci26]

Could you explain why you want to expose this as a metric?

We want to alert and build graphs on this as a defence in depth. We have discovered during a deployment that we accidentally deleted those annotations and that caused issues and that was really painful to debug.

Basically it helps with the discoverability of what people are setting across the cluster.

[sunjayBhatia]

Hm, I would expect something like this to be exposed via resource Status or something similar rather than a Contour xDS server metric

Also I believe we have an issue/PR to expand on cluster circuit breaker metrics: https://www.envoyproxy.io/docs/envoy/latest/configuration/upstream/cluster_manager/cluster_stats#circuit-breakers-statistics (here: #5950)

I don't want to hold up the rest of the change, so maybe lets pull the metrics part out into another PR?

[davinci26]

Hm, I would expect something like this to be exposed via resource Status or something similar rather than a Contour xDS server metric

Personally I take a more liberal approach to metrics in the sense that, if contour makes the decision to set a specific value it would be useful for operators to know the decision at runtime, especially if it affect things

[davinci26]

I don't want to hold up the rest of the change, so maybe lets pull the metrics part out into another PR?

I won't die on this hill :D, So I can separate the changes into two PRs. But my general take is that adding the metric is pretty "cheap" and overall useful. I.e. there have been cases where I wish I had this metric tracked.

[davinci26]

@skriss this is ready for another review round

[davinci26]

@skriss unless there is an embarrassing bug somewhere this is ready

[davinci26]

@skriss maybe we close/merge this today before the holiday break?

[skriss]

Looks like we got some merge conflicts, if you can resolve those I'll take another look

[davinci26]

@skriss merge conflicts resolved

[skriss]

This LGTM, I'm approving with comments that need to be addressed but can be done in a follow-up PR if preferred. Will leave for another maintainer to take a look as well. Thanks @davinci26!

[davinci26]

@projectcontour/maintainers can someone PTAL as well

[sunjayBhatia]

i think everything LGTM minus the metric part, I wonder if we can pull that out into a separate PR so we can discuss a little more?

[davinci26]

i think everything LGTM minus the metric part, I wonder if we can pull that out into a separate PR so we can discuss a little more?

Will do it in a separate PR

[davinci26]

there is more cleanup to be made

[davinci26]

I think I got all the references now