You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ji...@apache.org on 2016/08/09 16:27:04 UTC

[1/2] mesos git commit: Updated Linux 'MountInfoTable' entries to be sorted as expected.

Repository: mesos
Updated Branches:
  refs/heads/master 08747099b -> 187455af6


Updated Linux 'MountInfoTable' entries to be sorted as expected.

Many places in the codebase assume that the mountinfo table is sorted
according to the order: 'parent mount point < child mount point'.

However, in some cases this may not be true if (for example), a parent
mount point (say '/') is remounted to add some extra flags to it.
When this happens, the remounted file system will appear in the
mountinfo table at the point where it was remounted.

We actually encountered this problem in the wild for the case of '/'
being remounted after '/run' was mounted -- causing problems in the
'NvidiaVolume' which assumes the 'parent < child' ordering.

This commit fixes this problem by building the list of MountInfoTable
entries in sorted order when 'read()' is called. An optional flag can
be used to disable sorting produce the the original ordering.

Review: https://reviews.apache.org/r/50763/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/400e337b
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/400e337b
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/400e337b

Branch: refs/heads/master
Commit: 400e337b4054da160b7c56645f23a32cbdd1363a
Parents: 0874709
Author: Kevin Klues <kl...@gmail.com>
Authored: Tue Aug 9 09:12:53 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Tue Aug 9 09:13:08 2016 -0700

----------------------------------------------------------------------
 src/linux/fs.cpp                     | 50 ++++++++++++++++++++++++++++++-
 src/linux/fs.hpp                     | 20 +++++++++++--
 src/tests/containerizer/fs_tests.cpp | 24 +++++++++++++++
 3 files changed, 90 insertions(+), 4 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/400e337b/src/linux/fs.cpp
----------------------------------------------------------------------
diff --git a/src/linux/fs.cpp b/src/linux/fs.cpp
index f57db80..7953eff 100644
--- a/src/linux/fs.cpp
+++ b/src/linux/fs.cpp
@@ -22,10 +22,13 @@
 #include <linux/unistd.h>
 
 #include <list>
+#include <set>
+#include <utility>
 
 #include <stout/adaptor.hpp>
 #include <stout/check.hpp>
 #include <stout/error.hpp>
+#include <stout/hashmap.hpp>
 #include <stout/numify.hpp>
 #include <stout/path.hpp>
 #include <stout/strings.hpp>
@@ -40,6 +43,7 @@
 #include "linux/fs.hpp"
 
 using std::list;
+using std::set;
 using std::string;
 using std::vector;
 
@@ -82,7 +86,9 @@ Try<bool> supported(const std::string& fsname)
 }
 
 
-Try<MountInfoTable> MountInfoTable::read(const Option<pid_t>& pid)
+Try<MountInfoTable> MountInfoTable::read(
+    const Option<pid_t>& pid,
+    bool hierarchicalSort)
 {
   MountInfoTable table;
 
@@ -105,6 +111,48 @@ Try<MountInfoTable> MountInfoTable::read(const Option<pid_t>& pid)
     table.entries.push_back(parse.get());
   }
 
+  // If `hierarchicalSort == true`, then sort the entries in
+  // the newly constructed table hierarchically. That is, sort
+  // them according to the invariant that all parent entries
+  // appear before their child entries.
+  if (hierarchicalSort) {
+    int rootParentId;
+
+    // Construct a representation of the mount hierarchy using a hashmap.
+    hashmap<int, vector<MountInfoTable::Entry>> parentToChildren;
+
+    foreach (const MountInfoTable::Entry& entry, table.entries) {
+      if (entry.target == "/") {
+        rootParentId = entry.parent;
+      }
+      parentToChildren[entry.parent].push_back(std::move(entry));
+    }
+
+    // Walk the hashmap and construct a list of entries sorted
+    // hierarchically. The recursion eventually terminates because
+    // entries in MountInfoTable are guaranteed to have no cycles.
+    // We double check though, just to make sure.
+    set<int> visitedParents;
+    vector<MountInfoTable::Entry> sortedEntries;
+
+    std::function<void(int)> sortFrom = [&](int parentId) {
+      CHECK(visitedParents.count(parentId) == 0);
+      visitedParents.insert(parentId);
+
+      foreach (const MountInfoTable::Entry& entry, parentToChildren[parentId]) {
+        int newParentId = entry.id;
+        sortedEntries.push_back(std::move(entry));
+        sortFrom(newParentId);
+      }
+    };
+
+    // We know the node with a parent id of
+    // `rootParentId` is the root mount point.
+    sortFrom(rootParentId);
+
+    table.entries = std::move(sortedEntries);
+  }
+
   return table;
 }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/400e337b/src/linux/fs.hpp
----------------------------------------------------------------------
diff --git a/src/linux/fs.hpp b/src/linux/fs.hpp
index ec3b5b8..090c826 100644
--- a/src/linux/fs.hpp
+++ b/src/linux/fs.hpp
@@ -204,9 +204,23 @@ struct MountInfoTable {
     std::string source;         // mountinfo[10]: source dev, other.
   };
 
-  // If pid is None() the "self" is used, i.e., the mountinfo table
-  // for the calling process.
-  static Try<MountInfoTable> read(const Option<pid_t>& pid = None());
+  // Read the mountinfo table for a process.
+  // @param   pid     The `pid` of the process for which we should
+  //                  read the mountinfo table. If `pid` is None(),
+  //                  then "self" is used, i.e., the mountinfo table
+  //                  for the calling process.
+  // @param   hierarchicalSort
+  //                  A boolean indicating whether the entries in the
+  //                  mountinfo table should be sorted according to
+  //                  their parent / child relationship (as opposed to
+  //                  the temporal ordering of when they were
+  //                  mounted). The two orderings may differ (for
+  //                  example) if a filesystem is remounted after some
+  //                  of its children have been mounted.
+  // @return  An instance of MountInfoTable if success.
+  static Try<MountInfoTable> read(
+      const Option<pid_t>& pid = None(),
+      bool hierarchicalSort = true);
 
   // TODO(jieyu): Introduce 'find' methods to find entries that match
   // the given conditions (e.g., target, root, devno, etc.).

http://git-wip-us.apache.org/repos/asf/mesos/blob/400e337b/src/tests/containerizer/fs_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/fs_tests.cpp b/src/tests/containerizer/fs_tests.cpp
index 4cfafad..0dd212f 100644
--- a/src/tests/containerizer/fs_tests.cpp
+++ b/src/tests/containerizer/fs_tests.cpp
@@ -18,6 +18,8 @@
 
 #include <gmock/gmock.h>
 
+#include <set>
+
 #include <stout/foreach.hpp>
 #include <stout/gtest.hpp>
 #include <stout/none.hpp>
@@ -29,6 +31,7 @@
 
 #include "linux/fs.hpp"
 
+using std::set;
 using std::string;
 
 using mesos::internal::fs::MountTable;
@@ -160,6 +163,27 @@ TEST_F(FsTest, DISABLED_MountInfoTableRead)
 }
 
 
+TEST_F(FsTest, MountInfoTableReadSorted)
+{
+  // Examine the calling process's mountinfo table.
+  Try<MountInfoTable> table = MountInfoTable::read();
+  ASSERT_SOME(table);
+
+  set<int> ids;
+
+  // Verify that all parent entries appear *before* their children.
+  foreach (const MountInfoTable::Entry& entry, table->entries) {
+    if (entry.target != "/") {
+      ASSERT_TRUE(ids.count(entry.parent) == 1);
+    }
+
+    ASSERT_TRUE(ids.count(entry.id) == 0);
+
+    ids.insert(entry.id);
+  }
+}
+
+
 TEST_F(FsTest, ROOT_SharedMount)
 {
   string directory = os::getcwd();


[2/2] mesos git commit: Used hashset instead of set when sorting the mount table.

Posted by ji...@apache.org.
Used hashset instead of set when sorting the mount table.


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/187455af
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/187455af
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/187455af

Branch: refs/heads/master
Commit: 187455af6ca30d584c6c21ba1212bb00957942db
Parents: 400e337
Author: Jie Yu <yu...@gmail.com>
Authored: Tue Aug 9 09:25:21 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Tue Aug 9 09:25:21 2016 -0700

----------------------------------------------------------------------
 src/linux/fs.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/187455af/src/linux/fs.cpp
----------------------------------------------------------------------
diff --git a/src/linux/fs.cpp b/src/linux/fs.cpp
index 7953eff..0d025d3 100644
--- a/src/linux/fs.cpp
+++ b/src/linux/fs.cpp
@@ -29,6 +29,7 @@
 #include <stout/check.hpp>
 #include <stout/error.hpp>
 #include <stout/hashmap.hpp>
+#include <stout/hashset.hpp>
 #include <stout/numify.hpp>
 #include <stout/path.hpp>
 #include <stout/strings.hpp>
@@ -132,11 +133,11 @@ Try<MountInfoTable> MountInfoTable::read(
     // hierarchically. The recursion eventually terminates because
     // entries in MountInfoTable are guaranteed to have no cycles.
     // We double check though, just to make sure.
-    set<int> visitedParents;
+    hashset<int> visitedParents;
     vector<MountInfoTable::Entry> sortedEntries;
 
     std::function<void(int)> sortFrom = [&](int parentId) {
-      CHECK(visitedParents.count(parentId) == 0);
+      CHECK(!visitedParents.contains(parentId));
       visitedParents.insert(parentId);
 
       foreach (const MountInfoTable::Entry& entry, parentToChildren[parentId]) {