Added OpportunisticResourceEstimator module #45

treefell · 2020-09-10T14:03:35Z

Added the creation of opportunisticResourceEstimator inheriting resourceEstimator module.
Using the CommandRunner from Mesos-Command-Module to call a binary/script. The module also have access to ResourceUsage, which give the information about the resources on the agent/slave.

…dules

…bscribable

kamaradclimber

Note: I've reviewed only the beginning of the implementation. There are a few changes already.

This review needs to come with tests (even simple ones).

kamaradclimber · 2020-09-10T18:09:15Z

CMakeLists.txt

  ${CMAKE_SOURCE_DIR}/src/RunningContext.cpp
  ${CMAKE_SOURCE_DIR}/src/ConfigurationParser.cpp
  ${CMAKE_SOURCE_DIR}/src/ModulesFactory.cpp
+  ${CMAKE_SOURCE_DIR}/src/OpportunisticResourceEstimator.cpp


It should be named ResourceEstimator if nothing in its implementation is opportunistic

Good point i have renamed it CommandResourceEstimator to avoid confusion with mesos::ResourceEstimator

kamaradclimber · 2020-09-10T18:10:15Z

src/ModulesFactory.cpp

+::mesos::slave::ResourceEstimator* createResourceEstimator(
+    const ::mesos::Parameters& parameters) {
+  Configuration cfg = ConfigurationParser::parse(parameters);
+  return new OpportunisticResourceEstimator(cfg.prepareCommand, cfg.isDebugSet);


why is it named prepareCommand? It should be something related to what the binary does (something like resourceEstimatorCommand)

I forgot to change the Configuration file, i added a oversubscribableCommand variable since the function to use the command is oversubscribable

kamaradclimber · 2020-09-10T18:12:47Z

src/OpportunisticResourceEstimator.cpp

+  Try<Resources> _resources = ::mesos::Resources::parse(
+      "[{\"name\" : \"cpus\", \"type\":\"SCALAR\", \"scalar\" : {\"value\" : "
+      "\"0\"}, \"role\" : \"*\"}]");
+  Resources resources = _resources.get();


do we need to return a valid resource if to set?
Shouldn't we return something like a Failure (the "null" case of Try class)

@treefell could you answer my comment here?

Actually try to return NULL and nullptr it does not compile while i do that (i get a problem of compatibility with resources). So i went for resources with cpus at 0 as values. I will try to find another way.

Try means you can return a "success" with resources or a "failure" with an error message. Try returning a failure

src/OpportunisticResourceEstimator.cpp

kamaradclimber · 2020-09-10T18:15:43Z

src/OpportunisticResourceEstimator.cpp

+
+  Try<string> output = cmdrunner.run(m_oversubscribableCommand.get(), input);
+  if (output.isError()) {
+    return resources;


I think we should:

log the error

return the error case of the Try class (unless mesos reacts weirdly to this)

I have added a LOG(INFO) to retrieve the error
I am still investigating what i could give instead of resources

…sourceEstimator

Pull request has been modified.

St0rmingBr4in · 2020-09-11T08:20:42Z

src/CommandResourceEstimator.cpp

+using namespace process;
+using namespace mesos;


using namespace is pure evil. This is a bad practice and you don't need it since you have the other using below :)

hum i guess i was confused with the way namespace should be use. It have been removed

St0rmingBr4in · 2020-09-11T08:28:39Z

src/CommandResourceEstimator.cpp

+      const Option<Command>& oversubscribableCommand, bool isDebugMode);
+
+  virtual process::Future<Resources> oversubscribable();
+  virtual process::Future<Resources> _oversubscribable(


Even though this identifier is not causing any problems regarding the C++ standard, I would advise against starting an identifier with an underscore altogether.

Certain sets of names and function signatures are always reserved to the implementation: Each name that contains a double underscore __ or begins with an underscore followed by an uppercase letter (2.12) is reserved to the implementation for any use. Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.

If you have trouble getting the right pointer to the overloaded function please look at https://en.cppreference.com/w/cpp/language/overloaded_address

i have change most of the variable that had an _ in front of the name.

St0rmingBr4in · 2020-09-11T08:30:11Z

src/CommandResourceEstimator.cpp

+Future<Resources> CommandResourceEstimatorProcess::_oversubscribable(
+    const ResourceUsage& usage) {
+  // Mocking a resources to have valid return
+  Try<Resources> _resources = ::mesos::Resources::parse(


Same here, no underscore

St0rmingBr4in · 2020-09-11T08:41:40Z

src/CommandResourceEstimator.cpp

+    return resources;
+  }
+
+  Try<Resources> _cmdresources = ::mesos::Resources::parse(output.get());


….hpp and remove some _ un naming

Pull request has been modified.

Lqp1 · 2020-09-11T10:05:52Z

src/CommandResourceEstimator.hpp

@@ -0,0 +1,43 @@
+#ifndef __OPPORTUNISTIC_RESOURCE_ESTIMATOR_HPP__
+#define __OPPORTUNISTIC_RESOURCE_ESTIMATOR_HPP__


You renamed the file but not the guards ;)

thanks for noticing it! should be fixed

src/CommandResourceEstimator.cpp

kamaradclimber

Don't forget to add tests!

Command Resource estimator -unexisting command -existing regular command ModuleFactory for CommandResourceEstimator -empty command parameter -existing command parameter

Pull request has been modified.

Removed useless file

kamaradclimber

Thanks for the tests!

kamaradclimber · 2020-11-18T13:34:25Z

src/CommandQoSController.cpp

+
+list<::mesos::slave::QoSCorrection> CommandQoSControllerProcess::m_corrections(
+    const ResourceUsage& m_usage) {
+  LOG(INFO) << "Start Corrections timer_start_module QoSController";


this would be more appropriate for debug level I think

Yes this is more for debug, It is use to show time taken between different part of the module. like: start of module, start of the script, end of script, end of module

kamaradclimber · 2020-11-18T13:35:31Z

src/CommandQoSController.cpp

+  // wished output = [{QoSCorrection}, {QoSCorrection}, ...]
+
+  std::string stroutput = output.get();
+  std::string delimiter = ";";


I'm not sure to get why the script could not return the correct format (which seems to be map nicely with json)

Actually the problem come from the way message are supported.
Since the return must be an array of QoSCorrection there is noway to call for any message converting like JsonStringtoMessage().
I tried to convert the JsonArray to Array in c++ but did not find and easy way to do it, a part from calling external library.

Each QoSCorrection look like this:
{"type":"KILL","kill":{"executorId":{"value":"app"},"frameworkId"{"value":"frameworkID"},"containerId":{"value":"containerID"}}}
Which kind of complicated the the way to split it so i use a special character as separator

If you have any good way to do i would gladly take it!

I agree with Greg, this part looks a bit fragile. I've read your answer but have somle additional questions:

what is that "QoSscript timer_start_script QoSController" script? If this is something on our side, should it not return directly a json, like all other commands run?

if it's not in our hands, maybe there should be an intermediate wrapper to do it anyway, because I don't feel this kind of things needs to be done in this module.

thanks for the feeback, the "QoSscript timer_start_script QoScontroller" is an output line (just use to get a timestamp) to notify the start of the commandRunner running the QoSscript.rb that give the QoScorrection after receiving the ResourceUsage.
It should totally give a JSON, i will try to rework it to return a JSON.

kamaradclimber · 2020-11-18T13:36:03Z

src/CommandQoSController.cpp

+                                           const Option<Command>& _corrections,
+                                           bool isDebugMode)
+    : m_correctionsCommand(_corrections), m_isDebugMode(isDebugMode) {
+  LOG(INFO) << "new QoSController";


you can remove this line

kamaradclimber · 2020-11-18T13:37:53Z

src/CommandResourceEstimator.cpp

+    LOG(INFO) << output.error();
+    // return Failure("output from CommandRunner gave error: " +
+    // output.error());
+    return resources;


it sounds dangerous to return "0 cpus" whenever the command fail. Maybe we could:

store last result from the script (if any)

otherwise, explicitely fail

Actually i think returning previous result is more dangerous since if it keep failing it will always have the same amount thus allocating more revocable task that the Agent could take on.
Returning 0 prevent any revocable task to be allocated. In my tough if the Resource Estimator is broken revocable task should not be able to allocate.
I did try to fail, since the resource are returned to the slave it does not take fail from oversubscribable command, apart from modifying the slave i am not sure how we could make it work, if you have any suggestion i am interested

kamaradclimber · 2020-11-18T13:38:46Z

src/CommandResourceEstimator.cpp

+    const std::string& name, const Option<Command>& _oversubscribable,
+    bool isDebugMode)
+    : m_oversubscribableCommand(_oversubscribable), m_isDebugMode(isDebugMode) {
+  LOG(INFO) << "new resourceEstimator";


you can drop this line

Pull request has been modified.

Lqp1 · 2020-11-20T14:36:35Z

src/CommandQoSController.cpp

+  // wished output = [{QoSCorrection}, {QoSCorrection}, ...]
+
+  std::string stroutput = output.get();
+  std::string delimiter = ";";


I agree with Greg, this part looks a bit fragile. I've read your answer but have somle additional questions:

what is that "QoSscript timer_start_script QoSController" script? If this is something on our side, should it not return directly a json, like all other commands run?

if it's not in our hands, maybe there should be an intermediate wrapper to do it anyway, because I don't feel this kind of things needs to be done in this module.

Lqp1 · 2020-11-20T14:44:47Z

CMakeLists.txt

  ${CMAKE_SOURCE_DIR}/src/Helpers.hpp
  ${CMAKE_SOURCE_DIR}/src/Logger.hpp
  ${CMAKE_SOURCE_DIR}/src/ModulesFactory.hpp
+  ${CMAKE_SOURCE_DIR}/src/CommandResourceEstimator.hpp


Please provide a small documentation header in hpp files to quickly describe what are those methods supposed to do, what info they expect when running, ... You can get inspiration for the format if you look at Hook and Isolator headers.

thanks for the feeback, i will make it more understandable!

Lqp1 · 2020-11-20T14:47:22Z

tests/scripts/oversubscribable_empty.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+# do nothing here, so output file remains empty


OPT: You can likely change your test to avoid the need of having an empty bash script, using some standard commands for example.

Lqp1 · 2020-11-20T14:48:26Z

tests/scripts/oversubscribable.sh

+	 "role" : "*"}]
+EOM
+
+cat $INPUT_FILE > /tmp/test


You can remove this debug thing (and also comments above).

Charles Huang added 18 commits August 20, 2020 15:46

compiling version with OpportunisticResourceEstimator

a48090a

remove unwanted file

066c496

Merge branch 'master' of https://github.com/treefell/mesos-command-mo…

a2dcf44

…dules

added ResourceEstimator to ModuleFactory

789cb6b

entry point working

48f9a9e

added spawn process in OpportunisticResourceEstimator

cb7d81c

added mocked resources

217f8ee

mocked resources working

8913b4c

Oversubscribe doesn't crash

d44651f

Mesos agent running and commandRunner call the test script

67478da

added output retrieving

adee011

Convert Script.sh->return as Resources and set it as return of oversu…

954465b

…bscribable

added link to input for the script.sh

3fa5e15

removing Json part and unused code

9d9a620

move process spawn in initialize for ResourceUsage

fed3b2f

Resource usage envoyer au binaire

0167670

change a log info to be easier to read

1b01b0f

remove comment

146d4fe

kamaradclimber previously requested changes Sep 10, 2020

View reviewed changes

rename Classes to CommandResourceEstimator instead of OpportunisticRe…

89dafb4

…sourceEstimator

St0rmingBr4in previously requested changes Sep 11, 2020

View reviewed changes

rename opportunisticResourceEstimator.hpp to CommandResourceEstimator…

f492936

….hpp and remove some _ un naming

Lqp1 reviewed Sep 11, 2020

View reviewed changes

rename guard

ef88c18

kamaradclimber reviewed Sep 11, 2020

View reviewed changes

src/CommandResourceEstimator.cpp Show resolved Hide resolved

kamaradclimber previously requested changes Sep 11, 2020

View reviewed changes

Charles Huang added 2 commits October 7, 2020 10:21

segfault but should not

401886e

added simple test for CommandResourceEstimator

41b3182

Command Resource estimator -unexisting command -existing regular command ModuleFactory for CommandResourceEstimator -empty command parameter -existing command parameter

Charles Huang added 3 commits November 5, 2020 11:18

added modulefactory, and configurationParser for QoSController

6e97bca

Added simple test for QoSController and modulefactory

04068e7

Removed useless file

added Module factory prototype

df4787b

treefell requested review from Lqp1, St0rmingBr4in and kamaradclimber November 9, 2020 13:45

kamaradclimber previously requested changes Nov 18, 2020

View reviewed changes

Charles Huang added 2 commits November 18, 2020 15:48

remove LOG(INFO)

0dd5863

remove LOG(INFO) for the timer timestamps

bbb8bce

added debug level for timer timestamps

cb9cd4d

Lqp1 reviewed Nov 20, 2020

View reviewed changes

remove comment and debug from tests/script/oversubscribable.sh

03161f6

		@@ -0,0 +1,43 @@
		#ifndef __OPPORTUNISTIC_RESOURCE_ESTIMATOR_HPP__
		#define __OPPORTUNISTIC_RESOURCE_ESTIMATOR_HPP__

		@@ -0,0 +1,3 @@
		#!/bin/bash

		# do nothing here, so output file remains empty

Added OpportunisticResourceEstimator module #45

Are you sure you want to change the base?

Added OpportunisticResourceEstimator module #45

Uh oh!

Conversation

treefell commented Sep 10, 2020

Uh oh!

kamaradclimber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treefell Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treefell Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treefell Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kamaradclimber left a comment

Choose a reason for hiding this comment

Uh oh!

kamaradclimber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treefell Sep 11, 2020 •

edited

Loading

treefell Sep 11, 2020 •

edited

Loading

treefell Sep 11, 2020 •

edited

Loading