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

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
@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

@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.

serathius
serathius reviewed Aug 9, 2024
View reviewed changes
tests/robustness/watch.go
for _, r := range reports {
for _, op := range r.Watch {
for _, resp := range op.Responses {
if len(resp.Events) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Need to double check, but I think we could consider recording progress notify here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the responses are aggregated here we can do.

serathius
serathius reviewed Aug 9, 2024
View reviewed changes
tests/robustness/watch.go
for i, c := range combinedWatchResponses[resp.MemberId] {
// Reports are sorted by time, find first greater or equal and use previous one.
if resp.Time >= c.time {
if i == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Why skip the first one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review, i'm using an logic here which is find the first equal or greater and use previous one. And there is nothing before 0.
And if i remember correctly we aggregate all responses here, and there is nothing before the first response.

serathius
serathius reviewed Aug 14, 2024
View reviewed changes
tests/robustness/watch.go
if op.Request.Revision != 0 {
continue
}
for _, resp := range op.Responses {
Copy link
Member

@serathius serathius Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if len(resp.Events) == 0 {
continue
}

if resp.Events[0].Revision < lastSeenRevision {
t.Errorf("Watch has gone back")
}
lastSeenRevision = resp.Events[len(resp.Events)-1].Revision

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 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