Mark efficient watch resumption KEP as implementable #1922
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
kind/kep
| TODO: Fill in before making `Implementable`. | ||
| - [Alpha] unit tests for logic enhancing resource version tracking in reflector | ||
| - [Alpha] unit tests for newly added watch cache logic | ||
| - [Beta] Integration/e2e test for sending bookmark on kube-apiserver shutdown |
There was a problem hiding this comment.
I think just integration is fine.
| - [Alpha] unit tests for logic enhancing resource version tracking in reflector | ||
| - [Alpha] unit tests for newly added watch cache logic | ||
| - [Beta] Integration/e2e test for sending bookmark on kube-apiserver shutdown | ||
| - [Beta] Integration/e2e test for proving that resource version that |
There was a problem hiding this comment.
Integration sounds fine.
I don't think you need to e2e test this feature, I think integration tests will be easier to make exhaustive.
If you want an e2e test, I'd actually consider something that analyzes e.g. the audit log to count relists per watcher.
There was a problem hiding this comment.
Also I kind of expect these integration tests for alpha?
There was a problem hiding this comment.
Yeah - I agree that integration seems good enough.
Also switched to be required in alpha.
|
|
||
| #### Alpha -> Beta Graduation | ||
|
|
||
| TODO: Fill in before making `Implementable`. | ||
| - Tests marked as Beta implemented and stable for at least 2 weeks |
There was a problem hiding this comment.
I'd want this for alpha, personally.
|
|
||
| #### Beta -> GA Graduation | ||
|
|
||
| TODO: Fill in before making `Implementable`. | ||
| - Enabled in Beta for at least two releases without complaints |
There was a problem hiding this comment.
I think we need one release in beta in case there's a major problem. This isn't really adding api surface area so I don't know that a long beta period is necessary or useful.
I'd make the tests exhaustive instead.
There was a problem hiding this comment.
So you suggest to decrease it to 1 release?
Given it takes time until people start using a given release in production (or until cloud providers enable a version), I think having beta for two versions seems safer to me.
There was a problem hiding this comment.
Given the feature, I'll be surprised if it passes all the tests and people still find practical problems with it. I don't mind two releases, I'm just thinking maybe give ourselves the option to just go to GA after one if everything is going great?
There was a problem hiding this comment.
Beta is enabled by default so it probably doesn't change that much. I think 2 releases are a bit safer, but I don't mind single release too (we can probably adjust that before going Beta too).
There was a problem hiding this comment.
I don't have a strong opinion on the number of betas.
|
I think some test should verify that in a split-etcd backend mode (e.g., running events on a different etcd cluster), no RVs leak between resource types. |
wojtek-t
force-pushed
the
efficient_watch_reboot_implementable
branch
from
df3608d
to
56a0275
Compare
Added explicitly to one of the tests in Alpha. @lavalamp - comments addressed - PTAL |
|
This LGTM, I'll leave it open a bit for @deads2k to glance at it. |
|
|
||
| #### Alpha -> Beta Graduation | ||
|
|
||
| TODO: Fill in before making `Implementable`. | ||
| - Ad-hoc manual rolling-upgrade of kube-apiservers in 5k-node cluster |
There was a problem hiding this comment.
I'd like metrics before going to beta.
wojtek-t
force-pushed
the
efficient_watch_reboot_implementable
branch
from
56a0275
to
83ae0bd
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, wojtek-t 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
the
approved
Ref #1904