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
lsdlinshunda · Apr 10, 2017 · bd24f04 · bd24f04
1 parent 5f1ca4f
commit bd24f04
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 34 deletions.
diff --git 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,25 +155,34 @@ 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.
     vector<PathSetPB> path_sets_copy(path_sets);
     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.
     vector<PathSetPB> path_sets_copy(path_sets);
     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"));
   }
 }
 

diff --git 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));
     }
   }