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

kola: Increase amount of disk corruption in verity test #515

Merged
merged 1 commit into from
Apr 4, 2024
+2 −2
Merged

kola: Increase amount of disk corruption in verity test #515

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions kola/tests/misc/verity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ func VerityCorruption(c cluster.TestCluster) {
// get usr device, probably vda3
usrdev := util.GetUsrDeviceNode(c, m)

// write zero bytes to first 10 MB
c.MustSSH(m, fmt.Sprintf(`sudo dd if=/dev/zero of=%s bs=1M count=10 status=none`, usrdev))
// write zero bytes to first 100 MB
c.MustSSH(m, fmt.Sprintf(`sudo dd if=/dev/zero of=%s bs=1M count=100 status=none`, usrdev))
Comment on lines +99 to +100
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to sound ignorant here, but I thought that basically changing one bit in read-only /usr partition should trigger a failure to boot, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but only when it gets read and even with the ls if the contents are cached, this is not the case

Copy link
Contributor

Choose a reason for hiding this comment

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

the flush can be done directly by dd oflag=dsync

Copy link
Member

@jepio jepio Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

oflag=dsync means sync after every write - super slow. oflag=direct (no caching) and conv=fsync (flush after all writes) might be better.

Copy link
Contributor

@ader1990 ader1990 Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On NVME:

$: time dd if=/dev/zero of=t bs=1M count=100 status=none oflag=dsync

real    0m0.114s
user    0m0.000s
sys     0m0.057s

$: time dd if=/dev/zero of=t bs=1M count=100 status=none

real    0m0.057s
user    0m0.000s
sys     0m0.053s

Copy link
Contributor

@ader1990 ader1990 Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On SSD:

$: time dd if=/dev/zero of=t bs=1M count=100 status=none oflag=dsync

real    0m0.514s
user    0m0.001s
sys     0m0.347s
$: time dd if=/dev/zero of=t bs=1M count=100 status=none

real    0m0.330s
user    0m0.001s
sys     0m0.265s

Copy link
Member

@jepio jepio Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

alright "super slow" may be exaggerated :) but it does get much slower for bigger writes from within a vm:

core@localhost ~ $ time sudo dd if=/dev/zero of=/dev/vdb bs=1M count=1000 status=none oflag=direct conv=fsync

real    0m0.651s
user    0m0.001s
sys     0m0.009s
core@localhost ~ $ time sudo dd if=/dev/zero of=/dev/vdb bs=1M count=1000 status=none oflag=dsync

real    0m3.348s
user    0m0.001s
sys     0m0.010s

Copy link
Member Author

Choose a reason for hiding this comment

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

Could that lead to the ssh command failing by triggering the panic? Currently it's two separate commands, one that writes the zeros but is not expected to fail but places the corruption successfully, and the second one that is expected to fail. We could merge them into one but maybe the test logic is cleaner this way.

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Merging the commands would also work when combined with && to keep the error checking for the first one. When settings flags for dd to remove the sync we would just have to do enough tests to make sure that the test doesn't become flaky, or we could also keep the extra sync just to be sure.


// make sure we flush everything so the filesystem has to go through to the device backing verity before fetching a file from /usr
// (done in one execution because after flushing command itself runs the corruption could already be detected,
Expand Down