KUDU-1966: Data directories can be removed erroneously Also revises the error messages in PathInstanceMetadataFile::CheckIntegrity to be in terms of data directories, since this is what end-users will be familiar with. These errors should be rare, but they can happen if a user is monkeying around with data dirs configs.
Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd Reviewed-on: http://gerrit.cloudera.org:8080/6589 Reviewed-by: Adar Dembo <[email protected]> Tested-by: Kudu Jenkins Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/bd24f04f Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/bd24f04f Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/bd24f04f Branch: refs/heads/master Commit: bd24f04fb43db9f1fcbf9a60ecc31824c3c79bfd Parents: 5f1ca4f Author: Dan Burkert <[email protected]> Authored: Fri Apr 7 08:56:24 2017 -0700 Committer: Dan Burkert <[email protected]> Committed: Mon Apr 10 17:24:43 2017 +0000 ---------------------------------------------------------------------- src/kudu/fs/block_manager_util-test.cc | 24 +++++++--- src/kudu/fs/block_manager_util.cc | 70 ++++++++++++++++++----------- 2 files changed, 60 insertions(+), 34 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/bd24f04f/src/kudu/fs/block_manager_util-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/fs/block_manager_util-test.cc b/src/kudu/fs/block_manager_util-test.cc index ee19638..f44cbca 100644 --- a/src/kudu/fs/block_manager_util-test.cc +++ b/src/kudu/fs/block_manager_util-test.cc @@ -108,7 +108,7 @@ static void RunCheckIntegrityTest(Env* env, int i = 0; for (const PathSetPB& ps : path_sets) { gscoped_ptr<PathInstanceMetadataFile> instance( - new PathInstanceMetadataFile(env, "asdf", Substitute("$0", i))); + new PathInstanceMetadataFile(env, "asdf", Substitute("/tmp/$0/instance", i))); gscoped_ptr<PathInstanceMetadataPB> metadata(new PathInstanceMetadataPB()); metadata->set_block_manager_type("asdf"); metadata->set_filesystem_block_size_bytes(1); @@ -144,7 +144,8 @@ TEST_F(KuduTest, CheckIntegrity) { path_sets_copy[1].set_uuid(path_sets_copy[0].uuid()); EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest( env_, path_sets_copy, - "IO error: File 1 claimed uuid fee already claimed by file 0")); + "IO error: Data directories /tmp/0 and /tmp/1 have duplicate instance metadata UUIDs: " + "fee")); } { // Test where the path sets have duplicate UUIDs. @@ -154,7 +155,8 @@ TEST_F(KuduTest, CheckIntegrity) { } EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest( env_, path_sets_copy, - "IO error: File 0 has duplicate uuids: fee,fi,fo,fum,fee")); + "IO error: Data directory /tmp/0 instance metadata path set contains duplicate UUIDs: " + "fee,fi,fo,fum,fee")); } { // Test where a path set claims a UUID that isn't in all_uuids. @@ -162,8 +164,8 @@ TEST_F(KuduTest, CheckIntegrity) { path_sets_copy[1].set_uuid("something_else"); EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest( env_, path_sets_copy, - "IO error: File 1 claimed uuid something_else which is not in " - "all_uuids (fee,fi,fo,fum)")); + "IO error: Data directory /tmp/1 instance metadata contains unexpected UUID: " + "something_else")); } { // Test where a path set claims a different all_uuids. @@ -171,8 +173,16 @@ TEST_F(KuduTest, CheckIntegrity) { path_sets_copy[1].add_all_uuids("another_uuid"); EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest( env_, path_sets_copy, - "IO error: File 1 claimed all_uuids fee,fi,fo,fum,another_uuid but " - "file 0 claimed all_uuids fee,fi,fo,fum")); + "IO error: Data directories /tmp/0 and /tmp/1 have different instance metadata UUID sets: " + "fee,fi,fo,fum vs fee,fi,fo,fum,another_uuid")); + } + { + // Test removing a path from the set. + vector<PathSetPB> path_sets_copy(path_sets); + path_sets_copy.resize(1); + EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest( + env_, path_sets_copy, + "IO error: 1 data directories provided, but expected 4")); } } http://git-wip-us.apache.org/repos/asf/kudu/blob/bd24f04f/src/kudu/fs/block_manager_util.cc ---------------------------------------------------------------------- diff --git a/src/kudu/fs/block_manager_util.cc b/src/kudu/fs/block_manager_util.cc index 79c13b5..8be6cde 100644 --- a/src/kudu/fs/block_manager_util.cc +++ b/src/kudu/fs/block_manager_util.cc @@ -124,47 +124,63 @@ Status PathInstanceMetadataFile::Unlock() { Status PathInstanceMetadataFile::CheckIntegrity( const vector<PathInstanceMetadataFile*>& instances) { + CHECK(!instances.empty()); + + // Note: although this verification works at the level of UUIDs and instance + // files, the (user-facing) error messages are reported in terms of data + // directories, because UUIDs and instance files are internal details. + + // Map of instance UUID to path instance structure. Tracks duplicate UUIDs. unordered_map<string, PathInstanceMetadataFile*> uuids; - pair<string, PathInstanceMetadataFile*> first_all_uuids; + + // Set of UUIDs specified in the path set of the first instance. All instances + // will be compared against this one to make sure all path sets match. + set<string> all_uuids(instances[0]->metadata()->path_set().all_uuids().begin(), + instances[0]->metadata()->path_set().all_uuids().end()); + + if (all_uuids.size() != instances.size()) { + return Status::IOError( + Substitute("$0 data directories provided, but expected $1", + instances.size(), all_uuids.size())); + } for (PathInstanceMetadataFile* instance : instances) { const PathSetPB& path_set = instance->metadata()->path_set(); - // Check that this instance's UUID wasn't already claimed. - PathInstanceMetadataFile** other = - InsertOrReturnExisting(&uuids, path_set.uuid(), instance); + // Check that the instance's UUID has not been claimed by another instance. + PathInstanceMetadataFile** other = InsertOrReturnExisting(&uuids, path_set.uuid(), instance); if (other) { - return Status::IOError(Substitute( - "File $0 claimed uuid $1 already claimed by file $2", - instance->filename_, path_set.uuid(), (*other)->filename_)); + return Status::IOError( + Substitute("Data directories $0 and $1 have duplicate instance metadata UUIDs", + (*other)->path(), instance->path()), + path_set.uuid()); } - // Check that there are no duplicate UUIDs in all_uuids. + // Check that the instance's UUID is a member of all_uuids. + if (!ContainsKey(all_uuids, path_set.uuid())) { + return Status::IOError( + Substitute("Data directory $0 instance metadata contains unexpected UUID", + instance->path()), + path_set.uuid()); + } + + // Check that the instance's UUID set does not contain duplicates. set<string> deduplicated_uuids(path_set.all_uuids().begin(), path_set.all_uuids().end()); string all_uuids_str = JoinStrings(path_set.all_uuids(), ","); if (deduplicated_uuids.size() != path_set.all_uuids_size()) { - return Status::IOError(Substitute( - "File $0 has duplicate uuids: $1", - instance->filename_, all_uuids_str)); - } - - // Check that this instance's UUID is a member of all_uuids. - if (!ContainsKey(deduplicated_uuids, path_set.uuid())) { - return Status::IOError(Substitute( - "File $0 claimed uuid $1 which is not in all_uuids ($2)", - instance->filename_, path_set.uuid(), all_uuids_str)); + return Status::IOError( + Substitute("Data directory $0 instance metadata path set contains duplicate UUIDs", + instance->path()), + JoinStrings(path_set.all_uuids(), ",")); } - // Check that every all_uuids is the same. - if (first_all_uuids.first.empty()) { - first_all_uuids.first = all_uuids_str; - first_all_uuids.second = instance; - } else if (first_all_uuids.first != all_uuids_str) { - return Status::IOError(Substitute( - "File $0 claimed all_uuids $1 but file $2 claimed all_uuids $3", - instance->filename_, all_uuids_str, - first_all_uuids.second->filename_, first_all_uuids.first)); + // Check that the instance's UUID set matches the expected set. + if (deduplicated_uuids != all_uuids) { + return Status::IOError( + Substitute("Data directories $0 and $1 have different instance metadata UUID sets", + instances[0]->path(), instance->path()), + Substitute("$0 vs $1", JoinStrings(all_uuids, ","), all_uuids_str)); } }
