Added scheduler plugin functionality #5421

tsydd · 2017-06-09T12:20:58Z

Adds ability to customize scheduler logic using external plugins

jdef · 2017-06-14T18:59:52Z

Thanks for the PR! A few points:

We strongly prefer to accept PR's against master and backport, as needed, to prior releases. This PR is against releases/1.3, not master. Please retrofit this code and open a PR against master instead. If it lands there, then we'll consider backporting it to prior releases.
Nice example plugin! We have a public repo for those though (https://github.com/mesosphere/marathon-example-plugins), which is a better fit than the plugin-interface project, which we generally reserve for interfaces only. We're cleaning up the existing examples too in an effort to improve things for the community. Your example would do well here.
The integration of this plugin into the main scheduling logic appears to "further refine" existing scheduler decisions (vs. replacing them alltogether); documenting this nuance is likely important.
Guice is the bane of our existence. Seriously. Can we find a way to lessen the integration with it?

tsydd · 2017-06-15T12:03:18Z

Rebased onto master. I have no idea how to get rid of guice when integrating PluginManager into ResourceMatcher

jdef

Thanks for the updates! Left a few more comments.

jdef · 2017-06-15T12:43:49Z

src/main/scala/mesosphere/marathon/core/scheduler/SchedulerPluginConfigurer.scala

+  try {
+    SchedulerPluginRunner.plugins = pluginManager.plugins[SchedulerPlugin].toList
+  } catch {
+    case e: Throwable =>


prefer NonFatal to Throwable

jdef · 2017-06-15T12:45:24Z

src/main/scala/mesosphere/marathon/core/scheduler/SchedulerPluginConfigurer.scala

+  * @since 6/15/17.
+  */
+class SchedulerPluginConfigurer @Inject() (pluginManager: PluginManager) {
+  private[this] val log = LoggerFactory.getLogger(getClass)


prefer StrictLogging

jdef · 2017-06-15T12:46:35Z

src/main/scala/mesosphere/marathon/core/scheduler/SchedulerPluginConfigurer.scala

+    SchedulerPluginRunner.plugins = pluginManager.plugins[SchedulerPlugin].toList
+  } catch {
+    case e: Throwable =>
+      log.error("Failed to load plugins", e)


is it really worth using error level logging if the plan is to rethrow the exception? maybe debug or info level would be more appropriate?

jdef · 2017-06-15T12:47:46Z

src/main/scala/mesosphere/marathon/core/scheduler/SchedulerPluginRunner.scala

+  /**
+    * Is populated by [[mesosphere.marathon.core.scheduler.SchedulerPluginConfigurer]]
+    */
+  var plugins: List[SchedulerPlugin] = List.empty


dislike global mutable state. let's avoid this

I also dislike this approach. But I don't see another way to get list of plugins here. Can you suggest how to fix it?

Would it be possible to introduce a SchedulerPluginModule where we inject the PluginsManager and hold the list of SchedulerPlugins and wire this to the needed classes?

It doesn't solve the issue. The issue is that we have to integrate different scopes: guice (pluginManager) and global (ResourceMatcher).

ResourceMatcher.matchResources(..) is called by InstanceOpFactoryImpl where we do have the plugin manager. At this point we could either pass a list of SchedulerPlugin or you could hide this list behind trait abstracting this list. Would this be an option?

brat002 · 2017-06-23T22:35:12Z

Does any chance to merge this PR in master soon?

janisz

Missing documentation in https://github.com/mesosphere/marathon/blob/master/docs/docs/plugin.md

unterstein · 2017-07-11T12:25:53Z

hey @tsydd,

still on it? Do you need assistance or a sparring partner for the var plugins: List[SchedulerPlugin] discussion?

Thanks
Johannes

tsydd · 2017-07-12T11:09:23Z

Updated plugin.md

tsydd · 2017-09-05T09:54:36Z

I've fixed pr according to your comments. Can it be merged now?

unterstein

Thank you very much for addressing our feedback! LGTM, did some minor doc adaptions, hope this is ok :)

unterstein · 2017-09-13T12:46:33Z

Hey @jdef, could you please have a look at the addressed changes? This would be awesome :)

jdef

lgtm. thanks!

jeschkies · 2017-09-15T13:45:18Z

Shouldn't this run through the CI once?

jdef · 2017-09-15T14:08:43Z

yes @jeschkies it should definitely run through CI

unterstein · 2017-09-15T14:12:54Z

Thanks @jeschkies for pointing out that there was no CI run yet!

I am wondering why we dont have CI for community contributions. Anyways, I am currently setting up a phabricator review just for running CI and if its done, I`ll post the results here and merge if everything is fine 🎉

unterstein · 2017-09-15T14:18:22Z

Phab review here:
https://phabricator.mesosphere.com/D1065

Jenkins job here:
https://jenkins.mesosphere.com/service/jenkins/job/public-marathon-phabricator/1025/

unterstein · 2017-09-15T17:19:46Z

CI passed, happy to ship this commit!

Thank you very much @tsydd!

    .  o ..                  
     o . o o.o                
          ...oo               
            __[]__          Shippin'  
         __|_o_o_o\__         it!
         \""""""""""/         
          \. ..  . /          
     ^^^^^^^^^^^^^^^^^^^^

brat002 · 2017-09-18T18:19:32Z

Thanks all. Great work!

zen-dog requested review from timcharper, jdef and ichernetsky June 14, 2017 15:39

tsydd changed the base branch from releases/1.3 to master June 15, 2017 11:56

jdef reviewed Jun 15, 2017

View reviewed changes

janisz suggested changes Jun 23, 2017

View reviewed changes

Added scheduler plugin functionality

d6bce1b

Updated docs

59421c7

unterstein approved these changes Sep 13, 2017

View reviewed changes

jdef approved these changes Sep 13, 2017

View reviewed changes

timcharper approved these changes Sep 15, 2017

View reviewed changes

unterstein merged commit 3e61a17 into d2iq-archive:master Sep 15, 2017

kensipe mentioned this pull request Sep 26, 2017

bump to marathon 1.5.1 dcos/dcos#1967

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added scheduler plugin functionality #5421

Added scheduler plugin functionality #5421

tsydd commented Jun 9, 2017 •

edited

Loading

jdef commented Jun 14, 2017

tsydd commented Jun 15, 2017

jdef left a comment

jdef Jun 15, 2017

tsydd Jun 15, 2017

jdef Jun 15, 2017

tsydd Jun 15, 2017

jdef Jun 15, 2017

tsydd Jun 15, 2017

jdef Jun 15, 2017

tsydd Jun 15, 2017

unterstein Jul 4, 2017

tsydd Jul 12, 2017

unterstein Jul 12, 2017

tsydd Sep 4, 2017

brat002 commented Jun 23, 2017

janisz left a comment

unterstein commented Jul 11, 2017

tsydd commented Jul 12, 2017

tsydd commented Sep 5, 2017

unterstein left a comment •

edited

Loading

unterstein commented Sep 13, 2017

jdef left a comment

jeschkies commented Sep 15, 2017

jdef commented Sep 15, 2017

unterstein commented Sep 15, 2017 •

edited

Loading

unterstein commented Sep 15, 2017 •

edited

Loading

unterstein commented Sep 15, 2017

brat002 commented Sep 18, 2017

Added scheduler plugin functionality #5421

Added scheduler plugin functionality #5421

Conversation

tsydd commented Jun 9, 2017 • edited Loading

jdef commented Jun 14, 2017

tsydd commented Jun 15, 2017

jdef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brat002 commented Jun 23, 2017

janisz left a comment

Choose a reason for hiding this comment

unterstein commented Jul 11, 2017

tsydd commented Jul 12, 2017

tsydd commented Sep 5, 2017

unterstein left a comment • edited Loading

Choose a reason for hiding this comment

unterstein commented Sep 13, 2017

jdef left a comment

Choose a reason for hiding this comment

jeschkies commented Sep 15, 2017

jdef commented Sep 15, 2017

unterstein commented Sep 15, 2017 • edited Loading

unterstein commented Sep 15, 2017 • edited Loading

unterstein commented Sep 15, 2017

brat002 commented Sep 18, 2017

tsydd commented Jun 9, 2017 •

edited

Loading

unterstein left a comment •

edited

Loading

unterstein commented Sep 15, 2017 •

edited

Loading

unterstein commented Sep 15, 2017 •

edited

Loading