-
Notifications
You must be signed in to change notification settings - Fork 105
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
Marathon pluggable #138
Conversation
@@ -5,6 +5,11 @@ | |||
- consul | |||
|
|||
- hosts: mesos_masters |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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'} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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'
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. |
There was a problem hiding this comment.
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"
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 |
44ec3d9
to
fecb37e
Compare
@@ -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(",") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)?
286cf62
to
1df0b22
Compare
4bb9073
to
a009e26
Compare
Giving this branch a spin on my machine. My initial beef with it is -
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. |
4e4dca7
to
5546917
Compare
Added some code here to deal with #270 |
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 -
Thoughts ? |
would agree with @tayzlor. will make it easier to get in, if we split it up |
The main thing stopping this is the packer images having to be rebuild. And decisions around plugin system. |
Closing this one in favour of #357 |
No description provided.