Grow the resource_version monotonically when watching
#131
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nolar If they are not already assigned, you can assign the PR to them by writing 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 |
k8s-ci-robot
added
size/M
| @@ -152,7 +152,7 @@ def get_values(*args, **kwargs): | |||
| # opaque value we cannot interpret it and order it so rely | |||
| # on k8s returning the events completely and in order | |||
| calls.append(call(_preload_content=False, watch=True, | |||
| resource_version="3")) | |||
| resource_version="5")) | |||
There was a problem hiding this comment.
Here is the uncertainty:
The comment above this line says that we should not interpret the values and should rely on Kubernetes to return the events in order.
But Kubernetes does not return them in order.
Should we start interpreting the resource_version then?
PS: Full quote:
ideally we want 5 here but as rv must be treated as an
opaque value we cannot interpret it and order it so rely
on k8s returning the events completely and in order
There was a problem hiding this comment.
Are you sure Kubernetes do not return them in order? Have you anything to confirm that it is by design that order is not assured?
There was a problem hiding this comment.
@mitar There is an example in kubernetes-client/python#819
Codecov Report
@@ Coverage Diff @@
## master #131 +/- ##
==========================================
+ Coverage 92.84% 92.97% +0.12%
==========================================
Files 13 13
Lines 1328 1352 +24
==========================================
+ Hits 1233 1257 +24
Misses 95 95
Continue to review full report at Codecov.
|
|
Moved the detailed description to kubernetes-client/python#819 for discussion of the possible ways to solve the problem (it is not so straightforward). |
| # on the first listing call (the objects are sorted randomly). | ||
| if (resource_version is not None and | ||
| (self.resource_version is None or | ||
| self.resource_version < resource_version)): |
There was a problem hiding this comment.
I think indentation is off here, no?
|
This PR looks reasonable and at worst does not do anything. But if order is not really assured, then much more than just this change should probably be done, no? I mean, that assumption is all around the codebase. |
|
Closing this PR as the implementation is not correct. The proper way to get list the resources normally, get a |
See kubernetes-client/python#819 for the explanation of the problem, dilemma, and examples.
With this proposal, the remembered
resource_versiongrows monotonically, i.e. never decreasing.Specifically, after the
list...()operation is finished, it will remember the maximum of all the resource_versions of all currently existing objects. In case of disconnection and reconnection, it will continue from the last seen state of all objects — as expected.