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

make the configuration of engines specific to master or minion #259

Merged
merged 3 commits into from
Nov 9, 2016
Merged

make the configuration of engines specific to master or minion #259

merged 3 commits into from
Nov 9, 2016

Conversation

kakwa
Copy link
Contributor

@kakwa kakwa commented Oct 5, 2016

the engines are now configured using the following pillars:

  • salt.master.engines
  • salt.minion.engines

instead of a global salt.engines pillar.

Note: the pillar.example provided seems to assume this behaviour.
(the pillar is salt.master.engines.slack and not salt.engines.slack)

https://github.com/saltstack-formulas/salt-formula/blob/master/pillar.example#L83

the engines are now configured using the following pillars:

* salt.master.engines
* salt.minion.engines

instead of a global salt.engines pillar.

Note: the pillar.example provided seems to assume this behaviour.
(the pillar is salt.master.engines.slack and not salt.engines.slack)
@herisanu
Copy link

herisanu commented Oct 5, 2016

@kakwa as far as i know, you can have an engine run on a single minion
https://docs.saltstack.com/en/latest/topics/engines/index.html

I my opinion it should stay split up this way as you otherwise will configure the engine on all minions, the same as the master.

@kakwa
Copy link
Contributor Author

kakwa commented Oct 5, 2016

hummm, my pull request is actually implementing that split.

Right now, the pillar used in the master.d/engine.conf and minion.d/engine.conf templates is salt.engines in both cases:

Which means that there is no split. The salt.engines pillar will configure the master and the minion with the same parameters if set.

@herisanu
Copy link

herisanu commented Oct 5, 2016

what i say, you can have the case in which a specific minion needs to have an engine configured. For example, we're having one that listens to an sqs queue and does things off of that.

Just saying, there are engines that may run on specific minions and if we're going with salt.engines, they you wouldn't be able to configure that.

@kakwa
Copy link
Contributor Author

kakwa commented Oct 5, 2016

My PR does the exact opposite, it permits to configure engines on minion and master separatly.

The current code uses the salt.engines pillar:

salt['pillar.get']('salt:engines')

This is the current code, it's from https://github.com/saltstack-formulas/salt-formula/blob/master/salt/files/master.d/engine.conf#L4

@aboe76
Copy link
Member

aboe76 commented Oct 5, 2016

@kakwa and @herisanu , I think we should have both options,

For the salt minion:
salt['pillar.get']('salt:engines')
salt['pillar.get']('salt:minion:engines')

For the salt master:
salt['pillar.get']('salt:engines')
salt['pillar.get']('salt:master:engines')

This way we keep the old option working without issues, and create a specific pillar engine for each type.

Using the old salt.engines pillar and merging it with the new
salt.[master|minion].engines pillar.
This way, it doesn't break previous behavior and permits to define
common engines on master and minion.
In the merge, the salt.[master|minion].engines pillar takes precedence
if conflict as it's the more specific pillar.
@kakwa
Copy link
Contributor Author

kakwa commented Oct 5, 2016

@aboe76

It's a good idea as it doesn't break the current formula behavior (and it's API).

I've implementing a merge between common and specific pillar.

I'm not sure it's the proper way to merge pillars however (sorry, I'm new to saltstack)

@aboe76
Copy link
Member

aboe76 commented Oct 5, 2016

@kakwa I have tested this only problem is that the pillar data:
salt:minion:engines, will end up f_defaults.conf

I think the same thing will happen with the data in salt:master:engines...

f_defaults jinja needs to be aware that it should ignore salt:minion:engines / salt:master:engines keys.

@aboe76
Copy link
Member

aboe76 commented Oct 5, 2016

@kakwa solution:
in the f_defaults.conf the following should be adjusted.

{% set reserved_keys = ['master', 'minion', 'cloud', 'salt_cloud_certs', 'engines'] -%}

Copy link
Member

@aboe76 aboe76 left a comment

Choose a reason for hiding this comment

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

@kakwa this looks good, @gravyboat can you take a look?

@aboe76
Copy link
Member

aboe76 commented Nov 9, 2016

ping @gravyboat can this be merged?

@gravyboat gravyboat merged commit 0104374 into saltstack-formulas:master Nov 9, 2016
@gravyboat
Copy link
Contributor

Done, thanks!

@aboe76
Copy link
Member

aboe76 commented Nov 9, 2016

thanks.

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

Successfully merging this pull request may close these issues.

4 participants