Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: test if watch is sequential #18264

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

WIP: test if watch is sequential #18264

wants to merge 2 commits into from
59 −0

Conversation

ah8ad3
Copy link
Contributor

@ah8ad3 ah8ad3 commented Jul 2, 2024 •
edited
Loading

WIP:
Added member id in WatchResponse.
Change validateOrdered to check for MemberId for each event, as sequential should check for each process.

Fixes #18141

@k8s-ci-robot
Copy link

Hi @ah8ad3. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@ah8ad3 ah8ad3 force-pushed the robustness-test-watch-sequential branch from 1aa13de to 9347f8e Compare July 2, 2024 06:29
@serathius serathius marked this pull request as draft July 2, 2024 08:28
@ah8ad3 ah8ad3 force-pushed the robustness-test-watch-sequential branch from 9347f8e to caaf1f0 Compare July 2, 2024 13:10
@ah8ad3
Copy link
Contributor Author

ah8ad3 commented Jul 2, 2024

Printed the revisions it seems not checking revision 0 is not working here

    watch.go:150: Request revision: {Key: Revision:2 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:2 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:574 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:646 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:709 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:773 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:850 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:894 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:936 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:964 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:1036 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:1069 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:1092 WithPrefix:true WithProgressNotify:true WithPrevKV:false}
    watch.go:150: Request revision: {Key: Revision:2 WithPrefix:true WithProgressNotify:true WithPrevKV:false}

First one is 2 and revision is incremental.

serathius
serathius reviewed Jul 2, 2024
View reviewed changes
tests/robustness/watch.go Outdated Show resolved Hide resolved
serathius
serathius reviewed Jul 2, 2024
View reviewed changes
tests/robustness/watch.go Outdated Show resolved Hide resolved
serathius
serathius reviewed Jul 2, 2024
View reviewed changes
tests/robustness/watch.go
@@ -143,3 143,27 @@ external:
t.Errorf("Progress notify does not match, expect: %v, got: %v", expectProgressNotify, gotProgressNotify)
}
}

func validateWatchSequential(t *testing.T, reports []report.ClientReport) {
for _, r := range reports {
Copy link
Member

@serathius serathius Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about 2 concurrent client watch requests to the same server? I think we need to process in different order than reports. I think we should only consider watch requests if they don't intersect in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution came to my mind is to create a list of all WatchResponse that got sorted by Time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to collect (per member) pairs (last revision, time) from watch responses and sort them by time. Then go through all watch requests and search the (last revision, time) to find the last one before sending the request. First event revision should be always greater or equal one we found.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Jul 3, 2024
@ah8ad3
Copy link
Contributor Author

ah8ad3 commented Jul 3, 2024

Can you please take another look? @serathius

@k8s-ci-robot
Copy link

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.

@serathius
Copy link
Member

Please rebase

@ah8ad3
WIP: add member id to client watch reponses, add validateWatchSequent…
ab6921b 
…ial to check memberID in events

Signed-off-by: ah8ad3 <[email protected]>
@ah8ad3
Combine watch responses for concurrent client requests and find last …
f7c6c33 
…revision based one time of response

Signed-off-by: ah8ad3 <[email protected]>
@ah8ad3 ah8ad3 force-pushed the robustness-test-watch-sequential branch from a6ec18c to f7c6c33 Compare August 5, 2024 10:11
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ah8ad3
Once this PR has been reviewed and has the lgtm label, please assign spzala for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ah8ad3
Copy link
Contributor Author

ah8ad3 commented Aug 5, 2024 •
edited
Loading

Please rebase

Done, PTAL @serathius

At end i will squash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serathius serathius serathius left review comments

Assignees
No one assigned
Labels
area/robustness-testing area/testing do-not-merge/work-in-progress needs-ok-to-test needs-rebase size/M
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test if watch is sequential
3 participants
@ah8ad3 @k8s-ci-robot @serathius