Re: Review Request 64386: Refactored health checks to take in executor information.

[Akash Gupta]

> On Jan. 22, 2018, 1:27 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.hpp
> > Lines 40-47 (patched)
> > 
> >
> > What meaning do you put in the difference between `UCR` and `MESOS`? 
> > For me, the difference is whether the checker library talks to the task 
> > directly or via a container runtime; as a consequence, different types of 
> > command checks is used.
> 
> Joseph Wu wrote:
> The `UCR` and `MESOS` enums should be combined for now, as there is no 
> logical difference between them at the moment.  (You could actually use 
> nested container health checks on "`MESOS`" executors, if you had the 
> necessary credentials.)
> 
> Alexander Rukletsov wrote:
> We can, but I'd like to split them up exactly for this reason: to make 
> clear for the library user which type of command health check is used (nested 
> or plain).

It might be better to rename `UCR` to `MESOS_NESTED` since the name is closer 
to what's actually happening in the health check.


> On Jan. 22, 2018, 1:27 p.m., Alexander Rukletsov wrote:
> > src/launcher/default_executor.cpp
> > Lines 551 (patched)
> > 
> >
> > I'd argue, you should use `UCR` here: the library relies on the agent 
> > (which contains UCR) for command health checks.

Yeah, that makes sense to me.


> On Jan. 22, 2018, 1:27 p.m., Alexander Rukletsov wrote:
> > src/launcher/executor.cpp
> > Lines 653-658 (patched)
> > 
> >
> > How is this used in the checker library at the moment? Do you 
> > distinguish there between Mesos and UCR?

I think I'm getting confused on what `UCR` actually means. I think maybe it's 
better to change `UCR` to `MESOS_NESTED` and from that it's easy to see that 
this should have just been `MESOS` instead of the conditional.


- Akash


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


On Jan. 17, 2018, 12:10 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64386/
> ---
> 
> (Updated Jan. 17, 2018, 12:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added information about the calling executor to the health check
> library. After the refactoring, the command health check code originally
> in the docker executor is now in the health check library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/64386/diff/8/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/64387/
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64386: Refactored health checks to take in executor information.

[Alexander Rukletsov]

> On Jan. 22, 2018, 1:27 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.hpp
> > Lines 40-47 (patched)
> > 
> >
> > What meaning do you put in the difference between `UCR` and `MESOS`? 
> > For me, the difference is whether the checker library talks to the task 
> > directly or via a container runtime; as a consequence, different types of 
> > command checks is used.
> 
> Joseph Wu wrote:
> The `UCR` and `MESOS` enums should be combined for now, as there is no 
> logical difference between them at the moment.  (You could actually use 
> nested container health checks on "`MESOS`" executors, if you had the 
> necessary credentials.)

We can, but I'd like to split them up exactly for this reason: to make clear 
for the library user which type of command health check is used (nested or 
plain).


- Alexander


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


On Jan. 17, 2018, 12:10 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64386/
> ---
> 
> (Updated Jan. 17, 2018, 12:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added information about the calling executor to the health check
> library. After the refactoring, the command health check code originally
> in the docker executor is now in the health check library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/64386/diff/8/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/64387/
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64386: Refactored health checks to take in executor information.

[Joseph Wu]

> On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote:
> > src/checks/checker.hpp
> > Lines 69-70 (original), 73-74 (patched)
> > 
> >
> > These guys can now be part of `MesosRuntimeInfo` : )

And fix the spelling in the comment tweak :)
s/containierizer/containerizer/


> On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.hpp
> > Lines 40-47 (patched)
> > 
> >
> > What meaning do you put in the difference between `UCR` and `MESOS`? 
> > For me, the difference is whether the checker library talks to the task 
> > directly or via a container runtime; as a consequence, different types of 
> > command checks is used.

The `UCR` and `MESOS` enums should be combined for now, as there is no logical 
difference between them at the moment.  (You could actually use nested 
container health checks on "`MESOS`" executors, if you had the necessary 
credentials.)


> On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.hpp
> > Lines 40-69 (patched)
> > 
> >
> > These types remind me of protobufs. I do not envision a need to make 
> > them protobufs in the nearest future. Here is what comes to my mind instead:
> > ```
> > class RuntimeOptions
> > {
> > public:
> >   enum class Type
> >   {
> >   MESOS,
> >   DOCKER,
> >   UCR
> >   };
> >   
> > private:
> >   const Type type;
> > };
> > 
> > class MesosRuntimeOptions: public RuntimeOptions
> > {
> > public:
> >   MesosRuntimeOptions(...): RuntimeOptions(MESOS), ...;
> > 
> > private:
> >   const Option taskPid;
> >   const std::vector namespaces;
> > };
> > 
> > class DockerRuntimeOptions: public RuntimeOptions
> > {
> > public:
> >   DockerRuntimeOptions(...): RuntimeOptions(Docker), ...;
> >   
> > private:
> >   const std::string dockerPath;
> >   const std::string socketName;
> >   const std::string containerName;
> > }
> > ```
> > What do you think? I would also suggest to declare these types in a 
> > separate file to minimize dependencies and hence compilation time. Also, 
> > this should likely be a separate patch ; )

I broadly agree with this refactor...

Subclassing the `RuntimeOptions` means that you'll either need:
(A) The `CheckerProcess::commandCheck()` method to up-cast to 
`DockerRuntimeOptions`.  If we put other information inside these classes (like 
`taskPid` for the `MesosRuntimeOptions`), then there will be lots of up-casting 
all over the Health checker code.

(B) Or we will need to add a virtual method to `RuntimeOptions` to hold the 
logic inside `wrapDockerExec`.  And other virtual methods as necessary to 
indirectly expose the other private fields.

I'm inclined towards (B).


Note: `taskPid` and `namespaces` are used by the Docker executor's health 
checks too.  The `docker exec` is run inside the same `net` namespace as the 
executor's PID.  So there aren't many options specific to the 
`MesosRuntimeOptions` at the moment.


- Joseph


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


On Jan. 17, 2018, 4:10 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64386/
> ---
> 
> (Updated Jan. 17, 2018, 4:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added information about the calling executor to the health check
> library. After the refactoring, the command health check code originally
> in the docker executor is now in the health check library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 

Re: Review Request 64386: Refactored health checks to take in executor information.

[Alexander Rukletsov]

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



Akash — good stuff, I like the direction it is going. Let's do another 
iteration to further clean up the library interface.


src/checks/checker.hpp
Lines 69-70 (original), 73-74 (patched)


These guys can now be part of `MesosRuntimeInfo` : )



src/checks/checker.hpp
Lines 100-102 (original), 107-109 (patched)


And these guys — part of `UCRuntimeInfo`!



src/checks/checker_process.hpp
Lines 40-47 (patched)


What meaning do you put in the difference between `UCR` and `MESOS`? For 
me, the difference is whether the checker library talks to the task directly or 
via a container runtime; as a consequence, different types of command checks is 
used.



src/checks/checker_process.hpp
Lines 40-69 (patched)


These types remind me of protobufs. I do not envision a need to make them 
protobufs in the nearest future. Here is what comes to my mind instead:
```
class RuntimeOptions
{
public:
  enum class Type
  {
  MESOS,
  DOCKER,
  UCR
  };
  
private:
  const Type type;
};

class MesosRuntimeOptions: public RuntimeOptions
{
public:
  MesosRuntimeOptions(...): RuntimeOptions(MESOS), ...;

private:
  const Option taskPid;
  const std::vector namespaces;
};

class DockerRuntimeOptions: public RuntimeOptions
{
public:
  DockerRuntimeOptions(...): RuntimeOptions(Docker), ...;
  
private:
  const std::string dockerPath;
  const std::string socketName;
  const std::string containerName;
}
```
What do you think? I would also suggest to declare these types in a 
separate file to minimize dependencies and hence compilation time. Also, this 
should likely be a separate patch ; )



src/checks/checker_process.cpp
Lines 467-473 (patched)


This looks good to me, but it is not exactly what the original code is 
doing. Can you please do this change in a separate patch? It is hard to spot 
such changes (and the bisect or revert if necessary), when they are combined 
with moving code around. Thanks!



src/launcher/default_executor.cpp
Lines 551 (patched)


I'd argue, you should use `UCR` here: the library relies on the agent 
(which contains UCR) for command health checks.



src/launcher/default_executor.cpp
Lines 565-567 (original), 568-570 (patched)


And here is the beauty of your approach: these guys should go into 
`UCRContainerInfo` runtime!



src/launcher/executor.cpp
Lines 653-658 (patched)


How is this used in the checker library at the moment? Do you distinguish 
there between Mesos and UCR?


- Alexander Rukletsov


On Jan. 17, 2018, 12:10 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64386/
> ---
> 
> (Updated Jan. 17, 2018, 12:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added information about the calling executor to the health check
> library. After the refactoring, the command health check code originally
> in the docker executor is now in the health check library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/64386/diff/8/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/64387/
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64386: Refactored health checks to take in executor information.

[Akash Gupta]

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

(Updated Jan. 17, 2018, 12:10 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, Joseph Wu, and Michael Park.


Changes
---

Fixed linux test break on Docker `COMMNAD` health check


Repository: mesos


Description
---

Added information about the calling executor to the health check
library. After the refactoring, the command health check code originally
in the docker executor is now in the health check library.


Diffs (updated)
-

  src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
  src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
  src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
  src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
  src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
  src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
  src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 


Diff: https://reviews.apache.org/r/64386/diff/8/

Changes: https://reviews.apache.org/r/64386/diff/7-8/


Testing
---

See https://reviews.apache.org/r/64387/


Thanks,

Akash Gupta

Re: Review Request 64386: Refactored health checks to take in executor information.

[Akash Gupta]

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

(Updated Jan. 16, 2018, 8:37 a.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, Joseph Wu, and Michael Park.


Changes
---

Updated to depend on new docker `PATH` fix.


Repository: mesos


Description
---

Added information about the calling executor to the health check
library. After the refactoring, the command health check code originally
in the docker executor is now in the health check library.


Diffs
-

  src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
  src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
  src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
  src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
  src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
  src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54 
  src/launcher/executor.cpp 794bf7ca07e68c7d83993546c134f85cac5a68e3 


Diff: https://reviews.apache.org/r/64386/diff/6/


Testing
---

See https://reviews.apache.org/r/64387/


Thanks,

Akash Gupta

Re: Review Request 64386: Refactored health checks to take in executor information.

[Akash Gupta]

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

(Updated Jan. 12, 2018, 2:36 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, Joseph Wu, and Michael Park.


Changes
---

Refactored health checks with Alex's feedback.


Summary (updated)
-

Refactored health checks to take in executor information.


Repository: mesos


Description (updated)
---

Added information about the calling executor to the health check
library. After the refactoring, the command health check code originally
in the docker executor is now in the health check library.


Diffs (updated)
-

  src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
  src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
  src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
  src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
  src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
  src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54 
  src/launcher/executor.cpp 794bf7ca07e68c7d83993546c134f85cac5a68e3 


Diff: https://reviews.apache.org/r/64386/diff/6/

Changes: https://reviews.apache.org/r/64386/diff/5-6/


Testing
---

See https://reviews.apache.org/r/64387/


Thanks,

Akash Gupta