Disable activator probe optimisation when exec probe is specified #12250
Conversation
The activator probe relies on Queue Proxy being able to perform the required probes, but in the case of exec probes it can't. This means that activator can start proxying to pods before they're ready if an exec probe is used. This commit disables the optimisation and relies on the kubernetes readiness signal instead if an exec probe is used. Note: because we also use the probe for autodetecting mesh mode (unless explicitly told whether mesh mode is on in the config) we still do probe even if an exec probe is passed, we just _also_ require kubernetes to report ready.
Codecov Report
@@ Coverage Diff @@
## main #12250 +/- ##
==========================================
- Coverage 87.32% 87.30% -0.03%
==========================================
Files 195 195
Lines 9584 9600 +16
==========================================
+ Hits 8369 8381 +12
- Misses 936 939 +3
- Partials 279 280 +1
Continue to review full report at Codecov.
|
|
/assign @markusthoemmes @psschwei @dprotaso |
There was a problem hiding this comment.
This LGTM, but I'll give @markusthoemmes and/or @dprotaso a chance to weigh in (though I guess Markus already did in the issue...)
| @@ -253,7 +263,7 @@ func (rw *revisionWatcher) probePodIPs(ready, notReady sets.String) (succeeded s | |||
| dest := dest // Standard Go concurrency pattern. | |||
| probeGroup.Go(func() error { | |||
| ok, notMesh, err := rw.probe(egCtx, dest) | |||
| if ok { | |||
| if ok && (ready.Has(dest) || rw.enableProbeOptimisation) { | |||
There was a problem hiding this comment.
I know that Go prefers fewer parentheses, but I still had to stop and think here for a split second (this is a Scrooge-level nit, albeit of the McDuck variety rather than Ebenezer...)
There was a problem hiding this comment.
I'm probably missing obvious, but I don't think there's a way to put any more parentheses on this line?
There was a problem hiding this comment.
Never mind me... I wasn't looking at the right commit
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| enableProbeOptimisation := true | ||
| if rp := rev.Spec.GetContainer().ReadinessProbe; rp != nil && rp.Exec != nil { |
There was a problem hiding this comment.
I haven't doublechecked: What's the current state of probes for multi-container services? Are we still only allowing probes on the main container? If so, then this is fine. If not, this'd have to change to check if any other container has any probes on them as well.
There was a problem hiding this comment.
Looks like we do indeed disallow probes for multicontainers:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz, markusthoemmes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #11693.
The activator probe optimisation relies on Queue Proxy being able to perform the
required probes, but in the case of exec probes it can't. This means
that activator can start proxying to pods before they're ready if an
exec probe is used. This commit disables the optimisation and relies on
the kubernetes readiness signal instead if an exec probe is used.
Note: because we also use the probe for autodetecting mesh mode (unless
explicitly told whether mesh mode is on in the config) we still do probe
even if an exec probe is passed, we just also require kubernetes to
report ready.
Release Note