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: Test both fast memory and slower disk mounted filesystem #13927

Closed
wants to merge 1 commit into from

Conversation

serathius
Copy link
Member

@serathius serathius commented Apr 11, 2022

Appearance of data corruption issues depends on disk performance characteristics. In our current testing we only check one type (expect tmpfs usually mounted in linux under /tmp).

This PR adds test jobs that ensure that we cover both in memory and hdd disk as a short term solution for the problem.

@codecov-commenter
Copy link

Codecov Report

Merging #13927 (767f241) into main (dd08e15) will increase coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 767f241 differs from pull request most recent head 9ff5760. Consider uploading reports for the commit 9ff5760 to get more accurate results

@@            Coverage Diff             @@
##             main   #13927      +/-   ##
==========================================
+ Coverage   72.51%   72.57%   +0.05%     
==========================================
  Files         469      469              
  Lines       38411    38411              
==========================================
+ Hits        27853    27875      +22     
+ Misses       8778     8754      -24     
- Partials     1780     1782       +2     
Flag Coverage Δ
all 72.57% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raft/rafttest/node.go 95.00% <0.00%> (-5.00%) ⬇️
client/v3/concurrency/session.go 88.63% <0.00%> (-4.55%) ⬇️
client/v3/leasing/txn.go 88.09% <0.00%> (-3.18%) ⬇️
client/pkg/v3/transport/listener.go 57.18% <0.00%> (-2.40%) ⬇️
client/v3/retry_interceptor.go 65.45% <0.00%> (-0.91%) ⬇️
raft/node.go 86.69% <0.00%> (-0.86%) ⬇️
server/etcdserver/v3_server.go 78.17% <0.00%> (+0.17%) ⬆️
client/v3/watch.go 93.17% <0.00%> (+0.20%) ⬆️
client/v3/leasing/kv.go 90.69% <0.00%> (+0.33%) ⬆️
pkg/proxy/server.go 61.01% <0.00%> (+0.33%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd08e15...9ff5760. Read the comment docs.

@serathius
Copy link
Member Author

cc @ptabor @ahrtr for opinion.

@serathius serathius mentioned this pull request Apr 12, 2022
28 tasks
@ahrtr
Copy link
Member

ahrtr commented Apr 13, 2022

I think the mounted ramfs only makes sense for functional/stress test?

@serathius
Copy link
Member Author

Interesting enough, #13922 detected by integration tests was only reproducable on ramfs and not hdd. If we want to depend on ETCD_VERIFY checks in integration tests we should also cover different disk performance.

@ahrtr
Copy link
Member

ahrtr commented Apr 13, 2022

nteresting enough, #13922 detected by integration tests was only reproducable on ramfs and not hdd

Not really, I reproduced the issue on my a linux VM (not powerful, 2CPU, 8GB RAM) on hdd. Somehow I couldn't reproduce this issue on my local MacBook, which is more powerful (2.4 GHz 8-Core Intel Core i9, 32 GB 2667 MHz DDR4).

@@ -20,14 +22,32 @@ jobs:
run: |
echo "${TARGET}"
case "${TARGET}" in
linux-amd64-e2e)
linux-amd64-e2e-release)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth to maintain the 'release', that seems to be just 'unopinionated' variant of the same tasts as ramfs vs. ext4.

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

The test fails:
Error: EACCES: permission denied, scandir '/home/runner/work/etcd/etcd/ext4/lost+found'

@serathius
Copy link
Member Author

This will require a high level design.

@serathius serathius closed this May 6, 2022
@serathius serathius deleted the fs branch June 15, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants