[WIP] cacher: short circuit conditional progress notifier #124928
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
k8s-ci-robot
added
needs-triage
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MadhavJivrajani The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
MadhavJivrajani
force-pushed
the
feature-progress-notify
branch
from
32a08f1
to
cc80c7c
Compare
There can exist an extremely unlikely scenario where the progress requester can repeatedly request progress notifications even when the feature might not be supported. The feature checker returns `true` if any node in an etcd cluster has the feature enabled. However, it may change this to `false` if it eventually detects a node which does not have this enabled. Ideally, till we detect `false` we work under the assumption that this feature is supported, however we should stop requesting progress notifications as soon as we get a `false` in order to prevent loading etcd with redundant requests. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
MadhavJivrajani
force-pushed
the
feature-progress-notify
branch
from
cc80c7c
to
349a7ea
Compare
| // redundant requests. | ||
| if !etcdfeature.DefaultFeatureSupportChecker.Supports(storage.RequestWatchProgress) { | ||
| pr.mux.Lock() | ||
| pr.stopped = true |
There was a problem hiding this comment.
I'm not sure if we should stop the requestor here, maybe we could just change the shouldRequest function.
The question is whether we want to check the feature gate in the requestor or simply reserve/delegate the checking of that flag to the higher layers.
/cc @wojtek-t @serathius
There was a problem hiding this comment.
Yup - this is not a place where we should check it.
We're aware of the issue where it may change, but it was conscious decision and we generally don't expect setups where there are multiple etcd clusters backing a single cluster being in different versions.
So we consciously went for simplicity and accept a corner case we're not handling that should never happen.
There was a problem hiding this comment.
@MadhavJivrajani does the above make sense to you? We could always revisit this PR in the future if our assumptions are wrong.
There was a problem hiding this comment.
That makes sense to me @p0lyn0mial @wojtek-t, thanks.
For my curiosity, if we were to revisit this in the future sometime, what would be the right place to check this if not the progress checker? I would have assumed that since it can change at runtime, its better to check it at the place where its periodically requesting the additional request.
/close
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
|
@MadhavJivrajani: Closed this PR. 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-sigs/prow repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
There can exist an extremely unlikely scenario where
the progress requester can repeatedly request progress
notifications even when the feature might not be supported.
The feature checker returns
trueif any node in an etcdcluster has the feature enabled. However, it may change this
to
falseif it eventually detects a node which does nothave this enabled. Ideally, till we detect
falsewe workunder the assumption that this feature is supported, however
we should stop requesting progress notifications as soon as
we get a
falsein order to prevent loading etcd withredundant requests.
Which issue(s) this PR fixes:
xref: #124867 (comment)
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig api-machinery scalability
/cc @p0lyn0mial @wojtek-t