You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by bo...@apache.org on 2020/04/02 13:41:45 UTC
[impala] 02/03: IMPALA-7138: detect devicemapper volumes correctly
This is an automated email from the ASF dual-hosted git repository.
boroknagyz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 7abbde3c5d18217a84979454fffad1887e10816a
Author: Tim Armstrong <ta...@cloudera.com>
AuthorDate: Wed Apr 1 11:40:24 2020 -0700
IMPALA-7138: detect devicemapper volumes correctly
This is a more general fix for device detection that
disables the digit-stripping heuristic if a device
with the same name as a partition already exists.
E.g. for device-mapper devices, /proc/partitions might
include a partition called "dm-0" that maps directly
to the block device "dm-0" (i.e. /sys/block/dm-0).
Before this fix, we looked for a device called "dm-",
which did not exist.
Testing:
Created a dummy device-mapper device with the following command:
sudo dmsetup create 1gb-zero --table '0 1953125 zero'
Then confirmed that the device name in the logs was correct
dm-0 (rotational=true)
Other devices - sda, sda1, sda2, sdb, sdb1, etc on my system
were still detected the same as before.
Change-Id: I9d231bcf9105db8d4ab03586eab74e0644337a6f
Reviewed-on: http://gerrit.cloudera.org:8080/15631
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/util/disk-info.cc | 45 ++++++++++++++++++++++++-------------
be/src/util/filesystem-util-test.cc | 26 +++++++++++++++++++++
be/src/util/filesystem-util.cc | 17 ++++++++++++++
be/src/util/filesystem-util.h | 3 +++
4 files changed, 75 insertions(+), 16 deletions(-)
diff --git a/be/src/util/disk-info.cc b/be/src/util/disk-info.cc
index 6b55127..34159d7 100644
--- a/be/src/util/disk-info.cc
+++ b/be/src/util/disk-info.cc
@@ -31,6 +31,7 @@
#include <boost/algorithm/string/trim.hpp>
#include "gutil/strings/substitute.h"
+#include "util/filesystem-util.h"
#include "common/names.h"
@@ -80,19 +81,28 @@ void DiskInfo::GetDeviceNames() {
vector<string> fields;
split(fields, line, is_any_of(" "), token_compress_on);
if (fields.size() != 4) continue;
- string name = fields[3];
- if (name == "name") continue;
-
- // NVME devices have a special format. Try to detect that, falling back to the normal
- // method if this is not an NVME device.
- std::string nvme_basename;
- if (TryNVMETrim(name, &nvme_basename)) {
- // This is an NVME device, use the returned basename
- name = nvme_basename;
- } else {
- // Does not follow the NVME pattern, so use the logic for a normal disk device
- // Remove the partition# from the name. e.g. sda2 --> sda
- trim_right_if(name, is_any_of("0123456789"));
+ const string& partition_name = fields[3];
+ if (partition_name == "name") continue;
+
+ // Check if this is the top-level block device. If not, try to guess the
+ // name of the top-level block device.
+ bool found_device = false;
+ string dev_name = partition_name;
+ Status status =
+ FileSystemUtil::PathExists(Substitute("/sys/block/$0", dev_name), &found_device);
+ if (!status.ok()) LOG(WARNING) << status.GetDetail();
+ if (!found_device) {
+ // NVME devices have a special format. Try to detect that, falling back to the normal
+ // method if this is not an NVME device.
+ std::string nvme_basename;
+ if (TryNVMETrim(dev_name, &nvme_basename)) {
+ // This is an NVME device, use the returned basename
+ dev_name = nvme_basename;
+ } else {
+ // Does not follow the NVME pattern, so use the logic for a normal disk device
+ // Remove the partition# from the name. e.g. sda2 --> sda
+ trim_right_if(dev_name, is_any_of("0123456789"));
+ }
}
// Create a mapping of all device ids (one per partition) to the disk id.
@@ -102,12 +112,12 @@ void DiskInfo::GetDeviceNames() {
DCHECK(device_id_to_disk_id_.find(dev) == device_id_to_disk_id_.end());
int disk_id = -1;
- map<string, int>::iterator it = disk_name_to_disk_id_.find(name);
+ map<string, int>::iterator it = disk_name_to_disk_id_.find(dev_name);
if (it == disk_name_to_disk_id_.end()) {
// First time seeing this disk
disk_id = disks_.size();
- disks_.push_back(Disk(name, disk_id));
- disk_name_to_disk_id_[name] = disk_id;
+ disks_.push_back(Disk(dev_name, disk_id));
+ disk_name_to_disk_id_[dev_name] = disk_id;
} else {
disk_id = it->second;
}
@@ -135,6 +145,9 @@ void DiskInfo::GetDeviceNames() {
string line;
getline(rotational, line);
if (line == "0") disks_[i].is_rotational = false;
+ } else {
+ LOG(INFO) << "Could not read " << ss.str() << " for " << disks_[i].name
+ << " , assuming rotational.";
}
if (rotational.is_open()) rotational.close();
}
diff --git a/be/src/util/filesystem-util-test.cc b/be/src/util/filesystem-util-test.cc
index c8e744e..a8c4fad 100644
--- a/be/src/util/filesystem-util-test.cc
+++ b/be/src/util/filesystem-util-test.cc
@@ -174,3 +174,29 @@ TEST(FilesystemUtil, DirEntryTypes) {
ASSERT_EQ(entries.size(), 1);
EXPECT_TRUE(entries[0] == "impala-file");
}
+
+TEST(FilesystemUtil, PathExists) {
+ path dir = filesystem::unique_path();
+
+ // Paths to existent and non-existent dirs.
+ path subdir1 = dir / "impala1";
+ path subdir2 = dir / "impala2";
+ filesystem::create_directories(subdir1);
+ bool exists;
+ EXPECT_OK(FileSystemUtil::PathExists(subdir1.string(), &exists));
+ EXPECT_TRUE(exists);
+ EXPECT_OK(FileSystemUtil::PathExists(subdir2.string(), &exists));
+ EXPECT_FALSE(exists);
+
+ // Paths to existent and non-existent file.
+ path file1 = dir / "a_file1";
+ path file2 = dir / "a_file2";
+ ASSERT_OK(FileSystemUtil::CreateFile(file1.string()));
+ EXPECT_OK(FileSystemUtil::PathExists(file1.string(), &exists));
+ EXPECT_TRUE(exists);
+ EXPECT_OK(FileSystemUtil::PathExists(file2.string(), &exists));
+ EXPECT_FALSE(exists);
+
+ // Cleanup
+ filesystem::remove_all(dir);
+}
diff --git a/be/src/util/filesystem-util.cc b/be/src/util/filesystem-util.cc
index ea0a10a..5ec1755 100644
--- a/be/src/util/filesystem-util.cc
+++ b/be/src/util/filesystem-util.cc
@@ -173,6 +173,23 @@ Status FileSystemUtil::VerifyIsDirectory(const string& directory_path) {
return Status::OK();
}
+Status FileSystemUtil::PathExists(const std::string& path, bool* exists) {
+ error_code errcode;
+ *exists = filesystem::exists(path, errcode);
+ if (errcode != errc::success) {
+ // Need to check for no_such_file_or_directory error case - Boost's exists() sometimes
+ // returns an error when it should simply return false.
+ if (errcode == errc::no_such_file_or_directory) {
+ *exists = false;
+ return Status::OK();
+ }
+ return Status(ErrorMsg(TErrorCode::RUNTIME_ERROR, Substitute(
+ "Encountered exception while checking existence of path $0: $1",
+ path, errcode.message())));
+ }
+ return Status::OK();
+}
+
Status FileSystemUtil::GetSpaceAvailable(const string& directory_path,
uint64_t* available_bytes) {
error_code errcode;
diff --git a/be/src/util/filesystem-util.h b/be/src/util/filesystem-util.h
index 5471baa..5383d4d 100644
--- a/be/src/util/filesystem-util.h
+++ b/be/src/util/filesystem-util.h
@@ -45,6 +45,9 @@ class FileSystemUtil {
/// Returns Status::OK if it is, or a runtime error with a message otherwise.
static Status VerifyIsDirectory(const std::string& directory_path) WARN_UNUSED_RESULT;
+ /// Check if a path exists. Returns an error if this could not be determined.
+ static Status PathExists(const std::string& path, bool* exists);
+
/// Returns the space available on the file system containing 'directory_path'
/// in 'available_bytes'
static Status GetSpaceAvailable(