kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ale...@apache.org
Subject [1/2] kudu git commit: Fix flaky file_cache-test
Date Thu, 07 Jun 2018 04:13:40 GMT
Repository: kudu
Updated Branches:
  refs/heads/master e82dc1e2a -> ccdcf6cea


Fix flaky file_cache-test

This test became flaky after 4db748f3021711a67017fe51f30097350f343101
since that commit added a chance that a background thread opened a file
descriptor during the run of this test, which makes assertions on file
descriptor count.

This patch fixes the issue by making the test only count fds which point
to files within the test dir, rather than all open fds.

Change-Id: I5bd51c267fbc7dc954d593caedf8631f261909c9
Reviewed-on: http://gerrit.cloudera.org:8080/9900
Reviewed-by: Adar Dembo <adar@cloudera.com>
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/8719014f
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/8719014f
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/8719014f

Branch: refs/heads/master
Commit: 8719014f2523671b33fde92c110f219107d14a99
Parents: e82dc1e
Author: Todd Lipcon <todd@apache.org>
Authored: Mon Apr 2 21:17:26 2018 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Wed Jun 6 19:17:31 2018 +0000

----------------------------------------------------------------------
 src/kudu/fs/block_manager-stress-test.cc |  3 +-
 src/kudu/util/file_cache-stress-test.cc  |  1 +
 src/kudu/util/file_cache-test-util.h     | 20 +++++++---
 src/kudu/util/file_cache-test.cc         | 25 ++++++++-----
 src/kudu/util/test_util.cc               | 53 +++++++++++++++++++++++----
 src/kudu/util/test_util.h                |  7 +++-
 6 files changed, 84 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/8719014f/src/kudu/fs/block_manager-stress-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager-stress-test.cc b/src/kudu/fs/block_manager-stress-test.cc
index f5bedf3..c841ce5 100644
--- a/src/kudu/fs/block_manager-stress-test.cc
+++ b/src/kudu/fs/block_manager-stress-test.cc
@@ -540,7 +540,8 @@ TYPED_TEST(BlockManagerStressTest, StressTest) {
                << " is not a power of 2";
   }
 
-  PeriodicOpenFdChecker checker(this->env_, this->GetMaxFdCount());
+  PeriodicOpenFdChecker checker(this->env_, this->GetTestPath("*"),
+                                this->GetMaxFdCount());
 
   LOG(INFO) << "Running on fresh block manager";
   checker.Start();

http://git-wip-us.apache.org/repos/asf/kudu/blob/8719014f/src/kudu/util/file_cache-stress-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/file_cache-stress-test.cc b/src/kudu/util/file_cache-stress-test.cc
index 0ddaaef..9c51a52 100644
--- a/src/kudu/util/file_cache-stress-test.cc
+++ b/src/kudu/util/file_cache-stress-test.cc
@@ -354,6 +354,7 @@ TYPED_TEST(FileCacheStressTest, TestStress) {
   // Start the threads.
   PeriodicOpenFdChecker checker(
       this->env_,
+      this->GetTestPath("*"),           // only count within our test dir
       kTestMaxOpenFiles +               // cache capacity
       FLAGS_test_num_producer_threads + // files being written
       FLAGS_test_num_consumer_threads); // files being opened

http://git-wip-us.apache.org/repos/asf/kudu/blob/8719014f/src/kudu/util/file_cache-test-util.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/file_cache-test-util.h b/src/kudu/util/file_cache-test-util.h
index 8b0bcce..09acb68 100644
--- a/src/kudu/util/file_cache-test-util.h
+++ b/src/kudu/util/file_cache-test-util.h
@@ -20,6 +20,7 @@
 #include <thread>
 
 #include <glog/logging.h>
+#include <string>
 
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/util/countdown_latch.h"
@@ -34,9 +35,14 @@ namespace kudu {
 // process, crashing if it exceeds some upper bound.
 class PeriodicOpenFdChecker {
  public:
-  PeriodicOpenFdChecker(Env* env, int upper_bound)
+  // path_pattern: a glob-style pattern of which paths should be included while
+  //               counting file descriptors
+  // upper_bound:  the maximum number of file descriptors that should be open
+  //               at any point in time
+  PeriodicOpenFdChecker(Env* env, std::string path_pattern, int upper_bound)
     : env_(env),
-      initial_fd_count_(CountOpenFds(env)),
+      path_pattern_(std::move(path_pattern)),
+      initial_fd_count_(CountOpenFds(env, path_pattern_)),
       max_fd_count_(upper_bound + initial_fd_count_),
       running_(1),
       started_(false) {}
@@ -60,11 +66,12 @@ class PeriodicOpenFdChecker {
 
  private:
   void CheckThread() {
-    LOG(INFO) << strings::Substitute("Periodic open fd checker starting "
-        "(initial: $0 max: $1)",
-        initial_fd_count_, max_fd_count_);
+    LOG(INFO) << strings::Substitute(
+        "Periodic open fd checker starting for path pattern $0"
+        "(initial: $1 max: $2)",
+        path_pattern_, initial_fd_count_, max_fd_count_);
     do {
-      int open_fd_count = CountOpenFds(env_);
+      int open_fd_count = CountOpenFds(env_, path_pattern_);
       KLOG_EVERY_N_SECS(INFO, 1) << strings::Substitute("Open fd count: $0/$1",
                                                         open_fd_count,
                                                         max_fd_count_);
@@ -73,6 +80,7 @@ class PeriodicOpenFdChecker {
   }
 
   Env* env_;
+  const std::string path_pattern_;
   const int initial_fd_count_;
   const int max_fd_count_;
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/8719014f/src/kudu/util/file_cache-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/file_cache-test.cc b/src/kudu/util/file_cache-test.cc
index 217a2be..94c09eb 100644
--- a/src/kudu/util/file_cache-test.cc
+++ b/src/kudu/util/file_cache-test.cc
@@ -68,7 +68,14 @@ class FileCacheTest : public KuduTest {
     // Make sure it gets initialized early so that our fd count
     // doesn't get affected by it.
     ignore_result(GetStackTraceHex());
-    initial_open_fds_ = CountOpenFds(env_);
+    initial_open_fds_ = CountOpenFds();
+  }
+
+  int CountOpenFds() const {
+    // Only count files in the test working directory so that we don't
+    // accidentally count other fds that might be opened or closed in
+    // the background by other threads.
+    return kudu::CountOpenFds(env_, GetTestPath("*"));
   }
 
   void SetUp() override {
@@ -94,7 +101,7 @@ class FileCacheTest : public KuduTest {
 
   void AssertFdsAndDescriptors(int num_expected_fds,
                                int num_expected_descriptors) {
-    ASSERT_EQ(initial_open_fds_ + num_expected_fds, CountOpenFds(env_));
+    ASSERT_EQ(initial_open_fds_ + num_expected_fds, CountOpenFds());
 
     // The expiry thread may take some time to run.
     ASSERT_EVENTUALLY([&]() {
@@ -179,7 +186,7 @@ TYPED_TEST(FileCacheTest, TestBasicOperations) {
 
   // With the cache gone, so are the cached fds.
   this->cache_.reset();
-  ASSERT_EQ(this->initial_open_fds_, CountOpenFds(this->env_));
+  ASSERT_EQ(this->initial_open_fds_, this->CountOpenFds());
 }
 
 TYPED_TEST(FileCacheTest, TestDeletion) {
@@ -207,7 +214,7 @@ TYPED_TEST(FileCacheTest, TestDeletion) {
   {
     shared_ptr<TypeParam> f1;
     ASSERT_OK(this->cache_->OpenExistingFile(kFile2, &f1));
-    ASSERT_EQ(this->initial_open_fds_ + 1, CountOpenFds(this->env_));
+    ASSERT_EQ(this->initial_open_fds_ + 1, this->CountOpenFds());
     ASSERT_OK(this->cache_->DeleteFile(kFile2));
     {
       shared_ptr<TypeParam> f2;
@@ -215,10 +222,10 @@ TYPED_TEST(FileCacheTest, TestDeletion) {
     }
     ASSERT_TRUE(this->cache_->DeleteFile(kFile2).IsNotFound());
     ASSERT_TRUE(this->env_->FileExists(kFile2));
-    ASSERT_EQ(this->initial_open_fds_ + 1, CountOpenFds(this->env_));
+    ASSERT_EQ(this->initial_open_fds_ + 1, this->CountOpenFds());
   }
   ASSERT_FALSE(this->env_->FileExists(kFile2));
-  ASSERT_EQ(this->initial_open_fds_, CountOpenFds(this->env_));
+  ASSERT_EQ(this->initial_open_fds_, this->CountOpenFds());
 
   // Create a test file, open it, and let it go out of scope before
   // deleting it. The deletion should evict the fd and close it, despite
@@ -231,10 +238,10 @@ TYPED_TEST(FileCacheTest, TestDeletion) {
     ASSERT_OK(this->cache_->OpenExistingFile(kFile3, &f3));
   }
   ASSERT_TRUE(this->env_->FileExists(kFile3));
-  ASSERT_EQ(this->initial_open_fds_ + 1, CountOpenFds(this->env_));
+  ASSERT_EQ(this->initial_open_fds_ + 1, this->CountOpenFds());
   ASSERT_OK(this->cache_->DeleteFile(kFile3));
   ASSERT_FALSE(this->env_->FileExists(kFile3));
-  ASSERT_EQ(this->initial_open_fds_, CountOpenFds(this->env_));
+  ASSERT_EQ(this->initial_open_fds_, this->CountOpenFds());
 }
 
 TYPED_TEST(FileCacheTest, TestInvalidation) {
@@ -304,7 +311,7 @@ TYPED_TEST(FileCacheTest, TestHeavyReads) {
     Slice s(buf.get(), size);
     ASSERT_OK(f->Read(0, s));
     ASSERT_EQ(data, s);
-    ASSERT_LE(CountOpenFds(this->env_),
+    ASSERT_LE(this->CountOpenFds(),
               this->initial_open_fds_ + kCacheCapacity);
   }
 }

http://git-wip-us.apache.org/repos/asf/kudu/blob/8719014f/src/kudu/util/test_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index 74fe742..65e8fdc 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -17,18 +17,25 @@
 
 #include "kudu/util/test_util.h"
 
+#include <errno.h>
+#include <limits.h>
 #include <unistd.h>
 
 #include <cstdlib>
 #include <cstring>
-#include <ostream>
 #include <limits>
-#include <memory>
 #include <map>
+#include <memory>
+#include <ostream>
 #include <string>
 #include <utility>
 #include <vector>
 
+#ifdef __APPLE__
+#include <fcntl.h>
+#include <sys/param.h> // for MAXPATHLEN
+#endif
+
 #include <gflags/gflags.h>
 #include <glog/logging.h>
 #include <gtest/gtest-spi.h>
@@ -41,6 +48,7 @@
 #include "kudu/gutil/strings/util.h"
 #include "kudu/gutil/walltime.h"
 #include "kudu/util/env.h"
+#include "kudu/util/faststring.h"
 #include "kudu/util/path_util.h"
 #include "kudu/util/scoped_cleanup.h"
 #include "kudu/util/slice.h"
@@ -134,7 +142,7 @@ void KuduTest::SetUp() {
   OverrideKrb5Environment();
 }
 
-string KuduTest::GetTestPath(const string& relative_path) {
+string KuduTest::GetTestPath(const string& relative_path) const {
   return JoinPathSegments(test_dir_, relative_path);
 }
 
@@ -315,14 +323,14 @@ void AssertEventually(const std::function<void(void)>& f,
   }
 }
 
-int CountOpenFds(Env* env) {
+int CountOpenFds(Env* env, const string& path_pattern) {
   static const char* kProcSelfFd =
 #if defined(__APPLE__)
     "/dev/fd";
 #else
     "/proc/self/fd";
 #endif // defined(__APPLE__)
-
+  faststring path_buf;
   vector<string> children;
   CHECK_OK(env->GetChildren(kProcSelfFd, &children));
   int num_fds = 0;
@@ -331,11 +339,42 @@ int CountOpenFds(Env* env) {
     if (c == "." || c == "..") {
       continue;
     }
+    int32_t fd;
+    CHECK(safe_strto32(c, &fd)) << "Unexpected file in fd list: " << c;
+#ifdef __APPLE__
+    path_buf.resize(MAXPATHLEN);
+    char* buf_data = reinterpret_cast<char*>(path_buf.data());
+    if (fcntl(fd, F_GETPATH, path_buf.data()) != 0) {
+      if (errno == EBADF) {
+        // The file was closed while we were looping. This is likely the
+        // actual file descriptor used for opening /proc/fd itself.
+        continue;
+      }
+      PLOG(FATAL) << "Unknown error in fcntl(F_GETPATH): " << proc_file;
+    }
+    path_buf.resize(strlen(buf_data));
+#else
+    path_buf.resize(PATH_MAX);
+    char* buf_data = reinterpret_cast<char*>(path_buf.data());
+    auto proc_file = JoinPathSegments(kProcSelfFd, c);
+    int path_len = readlink(proc_file.c_str(), buf_data, path_buf.size());
+    if (path_len < 0) {
+      if (errno == ENOENT) {
+        // The file was closed while we were looping. This is likely the
+        // actual file descriptor used for opening /proc/fd itself.
+        continue;
+      }
+      PLOG(FATAL) << "Unknown error in readlink: " << proc_file;
+    }
+    path_buf.resize(path_len);
+#endif
+    if (!MatchPattern(path_buf.ToString(), path_pattern)) {
+      continue;
+    }
     num_fds++;
   }
 
-  // Exclude the fd opened to iterate over kProcSelfFd.
-  return num_fds - 1;
+  return num_fds;
 }
 
 namespace {

http://git-wip-us.apache.org/repos/asf/kudu/blob/8719014f/src/kudu/util/test_util.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util.h b/src/kudu/util/test_util.h
index 72873b0..8090fbc 100644
--- a/src/kudu/util/test_util.h
+++ b/src/kudu/util/test_util.h
@@ -65,7 +65,7 @@ class KuduTest : public ::testing::Test {
   // Returns absolute path based on a unit test-specific work directory, given
   // a relative path. Useful for writing test files that should be deleted after
   // the test ends.
-  std::string GetTestPath(const std::string& relative_path);
+  std::string GetTestPath(const std::string& relative_path) const;
 
   Env* env_;
 
@@ -131,7 +131,10 @@ void AssertEventually(const std::function<void(void)>& f,
                       AssertBackoff backoff = AssertBackoff::EXPONENTIAL);
 
 // Count the number of open file descriptors in use by this process.
-int CountOpenFds(Env* env);
+// 'path_pattern' is a glob-style pattern. Only paths that match this
+// pattern are included. Note that '*' in this pattern is recursive
+// unlike the usual behavior of path globs.
+int CountOpenFds(Env* env, const std::string& path_pattern);
 
 // Waits for the subprocess to bind to any listening TCP port, and returns the port.
 Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;


Mime
View raw message