[FEATURE REQUEST] master/minion should accept a SIGHUP and reload config

[whiteinge]

[thatch45]

this might be trickier than just that, at the least salt should reload the modules on the minion

[ryan-lane]

+1 on this issue. I have plans on modifying the salt master's configuration very, very often, and would prefer to reload the config rather than needing to restart the daemon (which would cause salt outages).

[thatch45]

I will slate this for the next release then and investigate how viable it will be

[thatch45]

I have added some code to help mitigate some issues when restarting the salt master. I am still looking into how to manage this one

[steamraven]

I am running into this issue with upstart. When upstart signals a reload it sends SIGHUP to the parent salt-master. It then promptly dies without error or debug, orphaning all of the children salt-masters. Upstart then thinks the process is dead, leaving lots of orphaned children.

subsequent "start saltmaster" immediately dies off as the port is still used by the orphaned children

At the very least, the salt-master should catch the signal and kill off its children such as

salt/master.py:
  Master.start() (line 343)
        signal.signal(signal.SIGHUP, sigterm_clean)

[basepi]

Ooh, thanks for the report -- even if we don't properly reload, we need to handle SIGHUP more gracefully than that.

[godlike64]

I don't want to be just another +1, but I too am planning to modify the configuration files. Mainly I already have an /etc/salt/master.d/nodegroups.conf file where I define my nodegroups (I am still not convinced by the available external node classifiers available). As such, any modification of said file must trigger a salt-master restart which, if I'm in the middle of a highstate call, triggers not-nice things to happen.

I tried to read salt's code to see if it was relatively easy to add this, but alas I did not understand much :)

[jacksontj]

Actually I've been digging around the daemons quite a bit recently and although I don't think this will be easy, it should be possible. I'll add this to my list of things to hack on ;)

[thatch45]

Thr reason this does not work is that catching signals in python inturupts
the zmq threads and we get crashes of lost packets. We hope to get this
working with salt raet once we have implimented more of the major
components

On Sun, Jan 4, 2015, 22:12 Thomas Jackson [email protected] wrote:

Actually I've been digging around the daemons quite a bit recently and
although I don't think this will be easy, it should be possible. I'll
add this to my list of things to hack on ;)

Reply to this email directly or view it on GitHub
#570 (comment).

[jacksontj]

We already catch quite a few signals in the daemons and IIRC we don't have stuff exploding all over... I think the larger problem is going to be the varying classes that keep copies of the opts dict all over creation ;)

[thatch45]

Good point. But just keep that in mind. And generally we are only catching
sigint and sigterm which do not care if there are zmq issues.
Just making sure you know the issues we have already seen here. But this is
one reason why we daemonize child procs instead of using sigchld. Which
will also cause isses with propogating the sighup

On Sun, Jan 4, 2015, 22:28 Thomas Jackson [email protected] wrote:

We already catch quite a few signals in the daemons and IIRC we don't have
stuff exploding all over... I think the larger problem is the varying
classes that keep copies of the opts dict all over creation ;)

Reply to this email directly or view it on GitHub
#570 (comment).

[jacksontj]

Yea, I'm thinking we'll want the parent to catch the sighup and coordinate the reload amongst the children. I have some ideas, but I'll need to make some time to mess with it :)

[bbinet]

+1: I'm looking for a way to reload the nodegroups configuration only, without restarting the whole salt-master

[basepi]

@bbinet Pretty sure that currently works. At least, last time I tried. Nodegroups don't require master restart.

[bbinet]

It does not work for me:

$ salt -c /config -N a test.ping
No minions matched the target. No command was sent, no jid was assigned.

The strange thing is that in the master logs, I can see that the a nodegroup had actually matched the correct list of minions:

2015-07-03 12:16:48,382 [salt.master      ][INFO    ][178] Clear payload received with command publish
2015-07-03 12:16:48,384 [salt.utils.minions][DEBUG   ][178] Evaluating final compound matching expr: ( ( set(['hl-lxc-1-dev', 'hl-mc-9999-dev', 'hl-mc-3-dev', 'hl-mc-8888-dev', 'hl-mc-4-dev', 'hl-mc-5-dev']) ) & ( set(['cm-mc-1-dev']) ) )

But I don't know why we then get the No minions matched the target message.

Note that it works correctly when I target a nodegroup that was already existing when the salt-master was started.
Should I create a new issue specifically for that?

[amuhametov]

+1

[mustafaocak]

+1

[sevencastles]

+1

[tjyang]

+1

[Paulo-Nunes]

I know this quote is nearly 5 years old, but this is not correct:

@bbinet Pretty sure that currently works. At least, last time I tried. Nodegroups don't require master restart.

Docs:

When adding or modifying nodegroups to a master configuration file, the master must be restarted for those changes to be fully recognized.
A limited amount of functionality, such as targeting with -N from the command-line may be available without a restart.

https://docs.saltstack.com/en/latest/topics/targeting/nodegroups.html

If you are testing on the CLI: salt -N group1 test.ping, then you are good, but according to the docs not all functionality is available without a restart.

I only bring this up because for a long time now we have abandoned nodegroups due to the fact we needed a restart and when I read this comment and tested it I realized maybe we don't need a restart. I started reading the nodegroup docs again before realizing that my initial thought was correct and that a restart is in fact required.

The docs don't mention the functionality not available, but I assume top file targeting would not be available as that is where I need this.

This is all a moot point anyway because currently there is a bug in 2019.2 that prevents compound matching of NodeGroups.
#52678

So +1 for a way to reload master/minion configs. I want a way to programatically get a list of minions and assign them roles. This would mean nodegroups would be constantly changing and needing to be restarted multiple times per day.

[anthosz]

+1

[ZsBT]

Personally, I am working on my custom reactors. The master have about 200 minions, so it is quite a pain int the @ss to restart the whole master just to apply those changes.
What if we create a patch as a workaround until this will be implemented?

[krusolu]

+1

[xpicio]

+1

[Torsten-B]

+1

[aek]

Personally, I am working on my custom reactors. The master have about 200 minions, so it is quite a pain int the @ss to restart the whole master just to apply those changes. What if we create a patch as a workaround until this will be implemented?

I think the key could be update the engines opts propagating the values across the process manager. I will try to get something like this to work. It's better to relay in pillarenvs and pillars or grains for this kind of config changes that need to be reloaded or set in a more dynamic way.
There are several parts of salt that needs to be more dynamic but everything could be done in one way or another. The reactor system it's just another engine that you could extend it and enhance to fulfill the needs

[aek]

I found that there is a dynamic way to load the reactors from an external file without the need to have it statically defined in the master config.
The idea is to define the reactor key in the master config as a file path str that will be used at runtime to load the reactor config map.
You could see the code that does that here:

salt/salt/utils/reactor.py

Lines 87 to 96 in 38341cb

	if isinstance(self.opts["reactor"], str):
	try:
	with salt.utils.files.fopen(self.opts["reactor"]) as fp_:
	react_map = salt.utils.yaml.safe_load(fp_)
	except OSError:
	log.error('Failed to read reactor map: "%s"', self.opts["reactor"])
	except Exception: # pylint: disable=broad-except
	log.error(
	'Failed to parse YAML in reactor map: "%s"', self.opts["reactor"]
	)

[nschoonm]

+1

[zbarr]

+1

What if only certain config values could be modified during reload and it was carefully documented which ones could? I think it would be very difficult to do everything but some things, such as "timeout", could be pretty straightforward to hot reload.

[ZsBT]

+3 :)