Skip to content

Adjust CacheFileTests.testCacheFileCreatedAsSparseFile for encryption at rest#81527

Merged
elasticsearchmachine merged 4 commits intoelastic:masterfrom
tlrx:adjust-cache-file-test-ear
Dec 9, 2021
Merged

Adjust CacheFileTests.testCacheFileCreatedAsSparseFile for encryption at rest#81527
elasticsearchmachine merged 4 commits intoelastic:masterfrom
tlrx:adjust-cache-file-test-ear

Conversation

@tlrx
Copy link
Copy Markdown
Member

@tlrx tlrx commented Dec 8, 2021

This test fails sometimes on CI when it is executed on encrypted filesystems: the test tries to detect the block size by writing a single byte at the end of a larger file, which should result in a single block allocated, but on encrypted filesystems it can result in more than a single block being reserved.

This commit adjusts the test so that it assumes that a default block size of 4KB is always used on Linux, and if the default block size is different then it assumes that the test is executed on an encrypted filesystem and that the allocated size stays within an upper bound.

Relates #81362 (comment)

Closes #81362

@tlrx tlrx added >test Issues or PRs that are addressing/adding tests :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.1.0 labels Dec 8, 2021
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team. label Dec 8, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

assertThat(
"Non default block size only used in test executed with encryption at rest",
file.toAbsolutePath().toString(),
startsWithIgnoringCase("/mnt/secret")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not great but I can't figure a better way to check that encryption at rest is used.

See

sudo mount /dev/mapper/secret /mnt/secret

@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented Dec 8, 2021

@elasticmachine run elasticsearch-ci/part-1 (because of #81533)

@tlrx tlrx requested a review from DaveCTurner December 8, 2021 15:39
+ cacheFile.getLength()
+ ')',
sizeOnDisk.getAsLong(),
allOf(greaterThanOrEqualTo(expectedSize), lessThanOrEqualTo(expectedSize + fourKb))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we're being too prescriptive here. We don't really care how big the file is on disk (we've already checked that it's no longer too small). It's kind of nice that you can often compute the size on disk by rounding up the actual size to the next multiple of blockSize but maybe we don't even need this assertion?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thinking about it this morning, I do agree. Being able to round up to the next 4KB is just a consequence of generating files of 1MB max. I removed the whole assertion and just kept the /mnt/secret - with an additionnal comment.

@tlrx tlrx requested a review from DaveCTurner December 9, 2021 08:51
Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@tlrx tlrx added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 9, 2021
@elasticsearchmachine elasticsearchmachine merged commit 647e5fe into elastic:master Dec 9, 2021
@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented Dec 9, 2021

Thanks David

@tlrx tlrx deleted the adjust-cache-file-test-ear branch December 9, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team. >test Issues or PRs that are addressing/adding tests v8.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] CacheFileTests.testCacheFileCreatedAsSparseFile fails on master

4 participants