Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Marathon pluggable #138

Closed
wants to merge 7 commits into from
Closed

Marathon pluggable #138

wants to merge 7 commits into from

Conversation

enxebre
Copy link
Contributor

@enxebre enxebre commented May 6, 2015

No description provided.

@@ -5,6 +5,11 @@
- consul

- hosts: mesos_masters
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be changed so not promping all the time when we have more frameworks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that we would change this in a future PR, or now? I'd rather standardise on the approach now so we can just rinse and repeat for each "add-on".

At a high-level I guess this means setting an environment variable for marathon_enabled (via the bootstrap scripts aws/do/etc) and setting its default to true?

Maybe we come up with some convention - framework_FRAMEWORKNAME_enabled (or something like that). then we can just have env vars like -

framework_marathon_enabled
framework_jenkins_enabled

thoughts?

we also need to consider other "add-ons" which are not mesos frameworks. e.g. #134, #55

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh lets set this up now, so we can easily add more later. definitely seems like a thing that would go in the bootstrap scripts. That naming convention looks simple enough.

@tayzlor
Copy link
Member

tayzlor commented May 7, 2015

I think as part of this PR we are going to want some docs around how to create an Add-on (for future add-ons), and how to contribute them / developing for apollo in the main readme

@@ -13,7 +13,8 @@ ZONE=${APOLLO_AWS_ZONE:-eu-west-1b}
MASTER_SIZE=${MASTER_SIZE:-m1.medium}
SLAVE_SIZE=${SLAVE_SIZE:-m1.medium}
NUM_SLAVES=${NUM_SLAVES:-1}

FRAMEWORK_MARATHON_ENABLED=${FRAMEWORK_MARATHON_ENABLED:-'Y'}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be alot of duplication in the aws and digitalocean bootstrap scripts, can we make a common bootstrap, so when we add more providers or frameworks later its easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree. I'll move to its own file the plugins settings. We can do more refactor later.
Ideally/eventually the only settings inside /some-cloud/config-default.sh should be the credential ones

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For common settings / environment variables we can either use the existing boostrap/apollo-env.sh (which is included everywhere) or add some other common ones that get included so we don't need to repeat ourselves

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apollo-env.sh seems like the rational place for it

@@ -9,3 +9,4 @@ packer/build
.wercker
*.cache
ssh.config
apollo-plugins
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would just call this 'plugins'

@tayzlor
Copy link
Member

tayzlor commented May 8, 2015

to fix the test failures you'll need to rebase off master


We want Apollo core to be as much slim as possible providing a cloud agnostic mesos cluster with an autodiscovery system based on consul for multi-service tasks.

This reduce the impact of core changes allowing to the user customize Apollo behaviour via plugins for satisfying the requirements of a given project.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"reduces"

"allowing the user"

"customise"

@tayzlor
Copy link
Member

tayzlor commented May 8, 2015

Can we keep the scope of this PR to just providing the plugin (with a static variable will be ok for backwards compatibility). For handling environment variables i've opened #152

@enxebre enxebre force-pushed the marathon-pluggable branch 2 times, most recently from 44ec3d9 to fecb37e Compare May 10, 2015 18:05
@@ -45,6 +46,7 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
}
end
mesos_zk_url = "zk://"+master_infos.map{|master| master[:ip]+":2181"}.join(",")+"/mesos"
marathon_zk_url = "zk://"+master_infos.map{|master| master[:ip]+":2181"}.join(",")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need +"/marathon" on the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's handled inside the role as for the same ips it needs /marathon and /mesos

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, just found it a bit weird that we seem to do it for the mesos one but not for the other - do we want to make the behaviour consistent here (one way or other)?

@enxebre enxebre force-pushed the marathon-pluggable branch 5 times, most recently from 286cf62 to 1df0b22 Compare May 12, 2015 14:31
@enxebre enxebre force-pushed the marathon-pluggable branch 3 times, most recently from 4bb9073 to a009e26 Compare May 15, 2015 16:20
@tayzlor
Copy link
Member

tayzlor commented May 31, 2015

Giving this branch a spin on my machine. My initial beef with it is -

root@apollo-mesos-master-1:~# docker images
mesosphere/marathon      v0.8.1              69c22351652e        2 days ago          1.118 GB

The image is massive and it has to be downloaded 3 times (on each master) - so the provisioning time is increased enormously :'(

Aside from that, it functionally works well.

@tayzlor tayzlor force-pushed the marathon-pluggable branch from 4e4dca7 to 5546917 Compare May 31, 2015 12:28
@tayzlor
Copy link
Member

tayzlor commented Jun 1, 2015

Added some code here to deal with #270

@tayzlor
Copy link
Member

tayzlor commented Jun 22, 2015

This isn't going far, probably because the PR is huge. Maybe we should chunk it up to make it a bit easier to review / merge?

We could split it into 3 PRs -

  • 1 PR for the packer box updates (should be backward compatible anyway)
  • 1 PR that just provides the plugin mechanism with no plugins
  • 1 PR for the marathon plugin (would be dependent on the 2nd one, but we could review the plugin and get it in ahead paving the way for some other plugins)

Thoughts ?

@wallies
Copy link
Contributor

wallies commented Jun 22, 2015

would agree with @tayzlor. will make it easier to get in, if we split it up

@enxebre
Copy link
Contributor Author

enxebre commented Jun 22, 2015

The main thing stopping this is the packer images having to be rebuild. And decisions around plugin system.
I'll move out from this PR the packer related stuff so we can create a separate PR with it and others packer updates

@enxebre
Copy link
Contributor Author

enxebre commented Jun 24, 2015

this is being split in to #338 and #323
This PR will be use to provided the plugin mechanism

@enxebre
Copy link
Contributor Author

enxebre commented Jul 1, 2015

Closing this one in favour of #357

@enxebre enxebre closed this Jul 1, 2015
@enxebre enxebre deleted the marathon-pluggable branch July 1, 2015 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants