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.

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

tests/e2e: add e2e test to reproduce issue 18089 #18201

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

MadhavJivrajani
Copy link
Contributor

e2e for #18089

The goal is to reproduce a DELETE event being dropped in a watch after a compaction occurs on the revision where the deletion took place. In order to reproduce this, we perform the following sequence (steps for reproduction thanks to @ahrtr):

  • PUT k v2 (assume returned revision = r2)
  • PUT k v3 (assume returned revision = r3)
  • PUT k v4 (assume returned revision = r4)
  • DELETE k (assume returned revision = r5)
  • PUT k v6 (assume returned revision = r6)
  • COMPACT r5
  • WATCH rev=r5

We should get the DELETE event (r5) followed by the PUT event (r6). However, currently we only get the PUT event with returned revision of r6 (key=k, val=v6).

/assign @ahrtr @serathius @siyuanfoundation

@MadhavJivrajani
Copy link
Contributor Author

@MadhavJivrajani
Copy link
Contributor Author

/retest

@MadhavJivrajani
Copy link
Contributor Author

/test all

@siyuanfoundation
Copy link
Contributor

Very nice! Thank you @MadhavJivrajani

@serathius
Copy link
Member

cc @ahrtr

@serathius
Copy link
Member

Please fix pull-etcd-verify

// - PUT k v3 (assume returned revision = r3)
// - PUT k v4 (assume returned revision = r4)
// - DELETE k (assume returned revision = r5)
// - PUT k v6 (assume returned revision = r6)
Copy link
Member

Choose a reason for hiding this comment

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

Side note: Even if there is not new revision for K, the DELETE event is missing as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking if we could use a modelReplay from robustness to provide a clearer explanation of expectations for the results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serathius could you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking if we could use a modelReplay from robustness to provide a clearer explanation of expectations for the results.

NO.

Robustness testing is characterized by randomness and is often conducted under high traffic conditions. But this is just a very corner case, which should be covered by e2e or integration test.

Copy link
Member

@serathius serathius Jun 27, 2024

Choose a reason for hiding this comment

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

Robustness testing is characterized by randomness and is often conducted under high traffic conditions.

Heh, I said modelReplay, which is deterministic stub of etcd.

Copy link
Member

Choose a reason for hiding this comment

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

I said modelReplay, which is deterministic stub of etcd.

Not sure we are talking about the same thing.

My understanding is the existing check on watch API guarantee should be already good enough, otherwise this issue could't be reproduced by robustness test.

Any specific additional check for such corner case isn't reasonable to be added into robustness test.

tests/e2e/delete_event_drop_issue18089_test.go Outdated Show resolved Hide resolved
@MadhavJivrajani MadhavJivrajani force-pushed the e2e-issue-18089 branch 2 times, most recently from f05cde7 to 22aee25 Compare June 27, 2024 08:31
@ahrtr
Copy link
Member

ahrtr commented Jun 27, 2024

Can we add the test case into watch_test.go? I see that there is an existing watch_delay_test.go, I think we can rename it to watch_test.go if there is no strong objection.

The goal is to reproduce a DELETE event being dropped in a watch after a compaction
occurs on the revision where the deletion took place. In order to reproduce this, we
perform the following sequence (steps for reproduction thanks to @ahrtr):
  - PUT k v2 (assume returned revision = r2)
  - PUT k v3 (assume returned revision = r3)
  - PUT k v4 (assume returned revision = r4)
  - DELETE k (assume returned revision = r5)
  - PUT k v6 (assume returned revision = r6)
  - COMPACT r5
  - WATCH rev=r5

We should get the DELETE event (r5) followed by the PUT event (r6). However, currently we only
get the PUT event with returned revision of r6 (key=k, val=v6).

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
@MadhavJivrajani
Copy link
Contributor Author

@ahrtr can you PTAL again?

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

Thanks @MadhavJivrajani

@ahrtr
Copy link
Member

ahrtr commented Jun 27, 2024

/retest

1 similar comment
@MadhavJivrajani
Copy link
Contributor Author

/retest

@ahrtr ahrtr merged commit 3cd044f into etcd-io:main Jun 28, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants