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

Allow additional mesos master and slave configs #56

Conversation

lhoss
Copy link
Contributor

@lhoss lhoss commented Aug 24, 2016

This allows very flexible additional configurations.

Example usage, that we use in prod. both for the master and slave:

  mesos_master_additional_configs:
  - name: MAX_COMPLETED_FRAMEWORKS
    value: 50
  - name: MAX_COMPLETED_TASKS_PER_FRAMEWORK
    value: 30

  mesos_slave_additional_configs:
  - name: RESOURCES
    value: "{{mesos_slave_resources}}"
  - name: HADOOP_HOME
    value: "{{hadoop_home}}"

@ernestas-poskus any chance to get a review and potential merge ?!

@ernestas-poskus
Copy link
Member

@ernestas-poskus
Copy link
Member

@lhoss can you provide links to documentation, are you sure all additional config begins with MESOS_* ?


# Additional configs
{% for config_item in mesos_master_additional_configs %}
export MESOS_{{config_item.name}}={{config_item.value}}
Copy link
Member

Choose a reason for hiding this comment

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

👍 creative

@lhoss
Copy link
Contributor Author

lhoss commented Aug 30, 2016

I bumped the version, but now it conflicts. How should we proceed? ( Should I solve by merging latest master into my branch first ?)

@lhoss
Copy link
Contributor Author

lhoss commented Aug 30, 2016

@lhoss can you provide links to documentation, are you sure all additional config begins with MESOS_* ?

http://mesos.apache.org/documentation/latest/configuration/

By setting the environment variable MESOS_OPTION_NAME (the option name with a MESOS_ prefix added to it)

@ernestas-poskus
Copy link
Member

I bumped the version, but now it conflicts. How should we proceed? ( Should I solve by merging latest master into my branch first ?)

@lhoss rebase on master, there were changes of mesos_playbook_version

@ernestas-poskus
Copy link
Member

ernestas-poskus commented Aug 31, 2016

I have checked

http://mesos.apache.org/documentation/latest/configuration/

It says

By setting the environment variable MESOS_OPTION_NAME (the option name with a MESOS_ prefix added to it).

I am not sure whether this is implicitly set by MESOS, in this case it must be defined here explicitly before other expoerts, please define it and make 'MESOS_' as configurable variable.

@lhoss lhoss force-pushed the additional_mesos_master_slave_configs branch from 446dd5e to ada1223 Compare September 2, 2016 08:09
@lhoss lhoss force-pushed the additional_mesos_master_slave_configs branch from ada1223 to 0aa2349 Compare September 2, 2016 08:27
@lhoss
Copy link
Contributor Author

lhoss commented Sep 2, 2016

@ernestas-poskus rebased 👍

@ernestas-poskus
Copy link
Member

@lhoss

http://mesos.apache.org/documentation/latest/configuration/

Please export explicitly MESOS_OPTION_NAME="{{ mesos_option_name }}" in both slave/master templates and in defauflts/main.yml

@lhoss
Copy link
Contributor Author

lhoss commented Sep 5, 2016

@ernestas-poskus ah I forget to ask for clarification about MESOS_OPTION_NAME:
So you want me to make a variable of the string 'MESOS_' (I would call and set it mesos_option_prefix="MESOS_") and then obviously use the variable in the template ?
Despite that in Mesos this is hardcoded and will higly-probably never change.
Also there's no ENV Var called MESOS_OPTION_NAME(it's just a pattern.) To avoid confusion they should have written it like MESOS_<name of mesos option (in uppercase)>

@ernestas-poskus
Copy link
Member

ernestas-poskus commented Sep 5, 2016

@lhoss yes please make
MESOS_ as variable

and export MESOS_OPTION_NAME to that variable

so it would be explicit and obvious for others

@lhoss
Copy link
Contributor Author

lhoss commented Sep 6, 2016

and export MESOS_OPTION_NAME to that variable

ok will do now!
Are you ok with export MESOS_OPTION_PREFIX... (IMHO more precise) ?
( I hope you agree that MESOS_OPTION_NAME is not used by mesos anyway, and would just lead to more confusion ?! )

@lhoss
Copy link
Contributor Author

lhoss commented Sep 6, 2016

@ernestas-poskus For consistency then, we should also replace the 'MESOS_' string in the conf-mesos.j2 template (that I repeat wasn't enhanced in this PR). ok for you?

# Additional configs
{% for config_item in mesos_additional_configs %}
export MESOS_{{config_item.name}}={{config_item.value}}
{% endfor %}

Finally you want me to add the export to all 3 conf-mesos* files ? (Else I'ld only add to the main conf-mesos)

@ernestas-poskus
Copy link
Member

Are you ok with export MESOS_OPTION_PREFIX... (IMHO more precise) ?

👍

@ernestas-poskus
Copy link
Member

ernestas-poskus commented Sep 6, 2016

@lhoss

For consistency then, we should also replace the 'MESOS_' string in the conf-mesos.j2 template

export MESOS_QUORUM="{{ mesos_quorum }}"
export MESOS_WORK_DIR="{{ mesos_work_dir }}"
export MESOS_PORT="{{ mesos_master_port }}"

those ☝️ could be referenced in mesos_master_additional_configs and slave, additional_configs respectively.

It's up to you whether you want to do it in this PR or other

@ernestas-poskus
Copy link
Member

Good job 👍

@@ -13,6 +13,7 @@ ZK="{{ mesos_zookeeper_masters }}"
export MESOS_HOSTNAME="{{ mesos_hostname }}"

# Additional configs
export MESOS_OPTION_PREFIX={{mesos_option_prefix}}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@lhoss
Copy link
Contributor Author

lhoss commented Sep 6, 2016

It's up to you whether you want to do it in this PR or other

I'ld rather get this merged 👍

@lhoss
Copy link
Contributor Author

lhoss commented Sep 6, 2016

To be sure I exactly understand your proposed change.

taking out the hardcoded lines in the template:

export MESOS_QUORUM="{{ mesos_quorum }}"
export MESOS_WORK_DIR="{{ mesos_work_dir }}"
export MESOS_PORT="{{ mesos_master_port }}"

and add entries to the corresponding list var(s) ?
For ex. in the case of the mesos-master:

mesos_master_additional_configs:
 - name: MESOS_QUORUM
   value: "{{ mesos_quorum }}"

@ernestas-poskus
Copy link
Member

To be sure I exactly understand your proposed change.

taking out the hardcoded lines in the template:

export MESOS_QUORUM="{{ mesos_quorum }}"
export MESOS_WORK_DIR="{{ mesos_work_dir }}"
export MESOS_PORT="{{ mesos_master_port }}"
and add entries to the corresponding list var(s) ?
For ex. in the case of the mesos-master:

mesos_master_additional_configs:
 - name: MESOS_QUORUM
   value: "{{ mesos_quorum }}"

exactly

@ernestas-poskus
Copy link
Member

thank you for contribution 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants