mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 48477: Added ability to express dependencies between isolators.
Date Thu, 09 Jun 2016 21:12:21 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48477/#review136893
-----------------------------------------------------------


Ship it!




Very nice!

I made some slight adjustments, let me know if you see any issues!

```
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index 0e2a9c8..b151121 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -257,13 +257,17 @@ Try<MesosContainerizer*> MesosContainerizer::create(
   }

   // Create isolators for the MesosContainerizer.
-  // The order of elements in this vector is important. It
-  // defines the relative dependencies between each isolator.
-  // Isolators that appear later in the vector depend on
-  // isolators that appear earlier. Specifically, this means that
-  // the `create()` and `prepare()` calls for each isolator are
-  // serialized in the order in which they appear, while the
-  // `cleanup()` call is serialized in reverse order.
+  //
+  // The order of elements in the creation vecot is used to express
+  // ordering dependencies between isolators. Currently, the `create`
+  // and `prepare` calls for each isolator are run serially in the
+  // order in which they appear in this list, while the `cleanup` call
+  // is serialized in reverse order. The current dependencies are:
+  //
+  //   (1) The filesystem isolator must be the first isolator so
+  //       that the runtime isolators have a consistent view of
+  //       the prepared filesystem (e.g. volume mounts).
+
   const vector<pair<string, lambda::function<Try<Isolator*>(const Flags&)>>>
     creators = {
     // Filesystem isolators.
@@ -315,33 +319,34 @@ Try<MesosContainerizer*> MesosContainerizer::create(

   vector<Owned<Isolator>> isolators;

-  vector<string> types_ = strings::tokenize(flags_.isolation, ",");
-  set<string> types = set<string>(types_.begin(), types_.end());
+  const vector<string> tokens = strings::tokenize(flags_.isolation, ",");
+  const set<string> isolations(tokens.begin(), tokens.end());

-  foreach (const auto creator, creators) {
-    if (types.count(creator.first) > 0) {
+  if (isolations.size() != tokens.size()) {
+    return Error("Duplicate entries found in --isolation flag"
+                 " '" + stringify(tokens) + "'");
+  }
+
+  foreach (const auto& creator, creators) {
+    if (isolations.count(creator.first) > 0) {
       Try<Isolator*> isolator = creator.second(flags_);
       if (isolator.isError()) {
-        return Error("Could not create isolator '" +
-                     creator.first + "': " + isolator.error());
+        return Error("Failed to create isolator '" + creator.first + "': " +
+                     isolator.error());
       }
-
-      isolators.push_back(Owned<Isolator>(isolator.get()));
-      types.erase(creator.first);
+      isolators.emplace_back(isolator.get());
     } else if (ModuleManager::contains<Isolator>(creator.first)) {
       Try<Isolator*> isolator = ModuleManager::create<Isolator>(creator.first);
       if (isolator.isError()) {
-        return Error("Could not create isolator '" +
-                     creator.first + "': " + isolator.error());
+        return Error("Could not create isolator '" + creator.first + "': " +
+                     isolator.error());
       }
-
-      isolators.push_back(Owned<Isolator>(isolator.get()));
-      types.erase(creator.first);
+      isolators.emplace_back(isolator.get());
     }
   }

-  if (types.size() != 0) {
-    return Error("Unknown or unsupported isolators '" + stringify(types) + "'");
+  if (isolations.empty()) {
+    return Error("Unknown --isolation entries '" + stringify(isolations) + "'");
   }

   return new MesosContainerizer(
```

- Benjamin Mahler


On June 9, 2016, 7:31 a.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48477/
> -----------------------------------------------------------
> 
> (Updated June 9, 2016, 7:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-5581
>     https://issues.apache.org/jira/browse/MESOS-5581
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Some isolators depend on other isolators. However, we previously did
> not have a generic method of expressing these dependencies. We special
> cased the `filesystem/*` isolators to make sure that dependencies on
> them were satisfied, but no other dependencies could be expressed.
> 
> Now we use a vector to represent the pairing of isolator name to
> isolator creator function, and the relative dependencies between each
> isolator is implicit in the ordering of this vector. Previously, a
> hashmap was used to hold this pairing, but this was inadequate because
> hashmaps are inherently unordered. The new implementation using a
> vector ensures everything is processed in the order it is listed.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/containerizer.cpp c7b9744463cf8e1921dcb5e2b7dec7d4e2c0e45f

> 
> Diff: https://reviews.apache.org/r/48477/diff/
> 
> 
> Testing
> -------
> 
> make -j check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message