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

db_stress: db_stress fails on custom filesystems. #9352

Closed
wants to merge 1 commit into from
Closed

db_stress: db_stress fails on custom filesystems. #9352

wants to merge 1 commit into from

Conversation

aravind-wdc
Copy link
Contributor

db_stress listener service always uses default filesystem to operate,
causing it to not recognize custom filesystem (like ZenFS plugin FS).
Pass the env to db_stress listener with the correct filesystem
information, so it can open the user intended filesystem.

Signed-off-by: Aravind Ramesh Aravind.Ramesh@wdc.com

@aravind-wdc aravind-wdc mentioned this pull request Jan 3, 2022
Open
@aravind-wdc
db_stress: db_stress fails on custom filesystems.
3a160df 
db_stress listener service always uses default filesystem to operate,
causing it to not recognize custom filesystem (like ZenFS plugin FS).
Pass the env to db_stress listener with the correct filesystem
information, so it can open the user intended filesystem.

Signed-off-by: Aravind Ramesh <Aravind.Ramesh@wdc.com>
@aravind-wdc
Copy link
Contributor Author

Please review.
db_stress fails when run on custom filesystems like ZenFS, which operates as a plugin to RocksDB.
"sudo ./db_stress --ops_per_thread=1000 --reopen=5 --fs_uri=zenfs://dev:nvmeXnY"
This is because it uses a default filesystem, in UniqueIdVerifier() constructor, causing it to not access the correct filesystem. So I had passed the env to the Uniqueidverifier constructor and get the correct filesystem interface from it, in the earlier patch, but I got feedback that when fault injection is enabled, we do not want to use the FaultInjection file system in dbstresslistener.

riversand963
riversand963 suggested changes Jan 4, 2022
View reviewed changes
db_stress_tool/db_stress_common.cc
@@ -16,6 +16,7 @@
#include "util/file_checksum_helper.h"
#include "util/xxhash.h"

ROCKSDB_NAMESPACE::Env* db_stress_listener_env = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can db_stress_env be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for regular case, but when fault_injection is enabled, the raw_env is set to the FaultInjectionTestFs(), and so are the db_stress_env and env_wrapper_guard. This results in faultinjection filesystem to be used every where.
But earlier, I have got feedback that it is better not to use the the fault injection filesystem in DbStressListener.
(I had sent a previous iteration of this patch using db_stress_env).
So current approach does not let DbStressListener to use the fault injection FS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for providing the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

DbStressListener uses the FileSystem ptr to access a unique_ids file. What would be ideal is adding support to FaultInjectionFS so that we can control whether we inject errors to certain file types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks for the suggestions, I will look into this.
Earlier PR I sent was #9175 , one suggestion by pdillinger was to copy the raw_env before it was wrapped by the FaultFS(this current version does that). Could you please take a look at that also. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@riversand963 @pdillinger I looked at modifying FaultInjectionTestFS to not inject errors to specific file types, but I am a bit not convinced as the .unique_ids file does not have any type attributed to it, could you please add more context on how you want it done.
My current implementation is based on @pdillinger feedback in PR #9175.
db_stress is broken for us, would appreciate if you can take a look into this. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can filter by file type and name. I am thinking about something similar to the following in FaultInjectionTestFS/FaultInjectionTestEnv.

if (file name fully matches) {
  return;  // skip injecting errors
} else if (file type falls into one of the specified types) {
  return; // skip injecting errors
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer this approach of accessing the right abstraction layer rather than complicating FaultInjectionTestFS with specific exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having a single env with centralized knowledge of what types of file to skip for fault injection is simpler and more scalable than having multiple envs around. In this case, we need to skip "unique-ids" for listeners, so we add a new stress_listener_env. In the future, what if we need to skip fault injection for another file or type of files?

db_stress_tool/db_stress_tool.cc
@@ -32,6 +32,8 @@ namespace ROCKSDB_NAMESPACE {
namespace {
static std::shared_ptr<ROCKSDB_NAMESPACE::Env> env_guard;
static std::shared_ptr<ROCKSDB_NAMESPACE::DbStressEnvWrapper> env_wrapper_guard;
static std::shared_ptr<ROCKSDB_NAMESPACE::DbStressEnvWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can env_wrapper_guard be used?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it gets fault injection

pdillinger
pdillinger approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

db_stress_tool/db_stress_tool.cc
@@ -32,6 +32,8 @@ namespace ROCKSDB_NAMESPACE {
namespace {
static std::shared_ptr<ROCKSDB_NAMESPACE::Env> env_guard;
static std::shared_ptr<ROCKSDB_NAMESPACE::DbStressEnvWrapper> env_wrapper_guard;
static std::shared_ptr<ROCKSDB_NAMESPACE::DbStressEnvWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

No, it gets fault injection

db_stress_tool/db_stress_common.cc
@@ -16,6 +16,7 @@
#include "util/file_checksum_helper.h"
#include "util/xxhash.h"

ROCKSDB_NAMESPACE::Env* db_stress_listener_env = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer this approach of accessing the right abstraction layer rather than complicating FaultInjectionTestFS with specific exceptions.

@facebook-github-bot
Copy link
Contributor

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@riversand963
Copy link
Contributor

I prefer this approach of accessing the right abstraction layer rather than complicating FaultInjectionTestFS with specific exceptions.

Just note that a future PR will exempt INFO log files from IO error injection. To implement this, we will need the logic that I advocate here.

riversand963
riversand963 approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

LGTM to unblock user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riversand963 riversand963 riversand963 approved these changes

@pdillinger pdillinger pdillinger approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aravind-wdc @facebook-github-bot @riversand963 @pdillinger