Trust k8s readiness signal #12086
Trust k8s readiness signal #12086
Conversation
If k8s tells us a pod is ready before we manage to get a result via probing, it makes sense to trust that rather than waiting for a probe result. This only works when mesh autodetection is off, because otherwise we need the probe to sniff whether mesh is on.
Codecov Report
@@ Coverage Diff @@
## main #12086 +/- ##
==========================================
+ Coverage 87.44% 87.47% +0.03%
==========================================
Files 196 196
Lines 9531 9540 +9
==========================================
+ Hits 8334 8345 +11
+ Misses 923 921 -2
Partials 274 274
Continue to review full report at Codecov.
|
| func (rw *revisionWatcher) probePodIPs(ready, notReady sets.String) (succeeded sets.String, noop bool, notMesh bool, err error) { | ||
| dests := ready.Union(notReady) | ||
|
|
||
| // Short circuit case where all the current pods are already known to be healthy. | ||
| if rw.healthyPods.Equal(dests) { | ||
| return rw.healthyPods, true /*no-op*/, false /* notMesh */, nil | ||
| } | ||
|
|
||
| toProbe := sets.NewString() | ||
| healthy := sets.NewString() |
There was a problem hiding this comment.
nit: Should we prevent this allocation if the meshmode stuff aligns?
There was a problem hiding this comment.
ok, it does mean writing an else, though 😅
| // not also using the probe to sniff whether mesh is enabled, we can just | ||
| // trust k8s and mark it healthy without probing. | ||
| // TODO: replace with ready.Clone() once we have a recent-enough apimachinery (1.23+). | ||
| healthy = ready.Union(nil) |
There was a problem hiding this comment.
Is the copying necessary? If so, why?
There was a problem hiding this comment.
I think so, because we muck with healthy a few lines down, and it's probably pretty unexpected that someone passing a set in to probePodIPs would find that set modified as a side effect (e.g. otherwise the "Failed probing pods" log message would end up logging a misleading set of ready pods).
|
/assign @psschwei too |
|
@julz: GitHub didn't allow me to assign the following users: too. Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
There was a problem hiding this comment.
LGTM, but since @markusthoemmes had a question about the copying, will let him stamp it
|
/hold |
|
(thought I had to explicitly type |
|
ping @markusthoemmes |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz, markusthoemmes, psschwei 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 |
If k8s tells us a pod is ready before we manage to get a result via probing, it makes sense to trust that rather than probing ourselves. This only works when mesh autodetection is off, because otherwise we need the probe to sniff whether mesh is on.
Release Note
/assign @markusthoemmes