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

Dont restart/reload shinken with sudo #1488

Merged
merged 2 commits into from
Mar 31, 2015

Conversation

aviau
Copy link
Contributor

@aviau aviau commented Jan 30, 2015

sudo should not be needed by the shinken user.

In my case, shinken was not in the sudoers so the command did not work.

@aviau aviau changed the title Dont restart shinken with sudo Dont restart/reload shinken with sudo Jan 30, 2015
@naparuba
Copy link
Contributor

Need to test and validate, because a service is not done to be launch by a normal user.

@aviau
Copy link
Contributor Author

aviau commented Jan 31, 2015

@naparuba I did, and it works. I was using ws-arbiter to reload/restart and it wouldn't work because shinken was not in the sudoers. Removing sudo fixed the issue.

@titilambert
Copy link
Contributor

Hello !
Could we imagine the reload command" like: "make a soft restart inside python code"

  • do not kill/start a new process
  • the process end its main loop and restart the starting processing
  • this could be triggered by signal/http post command

@gst @naparuba what do you think about ?

@naparuba
Copy link
Contributor

naparuba commented Feb 4, 2015

The main question is not about how to catch teh information, but how to
manage it after ward. currently the reload is as long as a restart for the
main process inerested, the arbiter. so the interest about reload with the
same pid is void :)

On Wed, Feb 4, 2015 at 5:10 AM, Thibault Cohen notifications@github.com
wrote:

Hello !
Could we imagine the reload command" like: "make a soft restart inside
python code"

  • do not kill/start a new process
  • the process end its main loop and restart the starting processing
  • this could be triggered by signal/http post command

@gst https://github.com/gst @naparuba https://github.com/naparuba
what do you think about ?


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

@Seb-Solon
Copy link
Contributor

It is in my opinion. Sending a signal to restart instead of killing does not require the same privileges right? I think the idea here is to avoid the sudo issue by only sending signals.

@naparuba
Copy link
Contributor

naparuba commented Feb 4, 2015

yes it's a bonus indeed, but it will be the only gain as we don't have a
trully quick reload without pain (because when you reload you forgot your
old job unless you are threading, but then your start will be longer... :(
).

But after all with the "host addition" we can look at why not "reinject"
all hosts with such a signal? don't know. Let's first code it ^^

On Wed, Feb 4, 2015 at 1:41 PM, Sébastien Coavoux notifications@github.com
wrote:

It is in my opinion. Sending a signal to restart instead of killing does
not require the same privileges right? I think the idea here is to avoid
the sudo issue by only sending signals.


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

@gst
Copy link
Contributor

gst commented Feb 4, 2015

Could we imagine the reload command" like: "make a soft restart inside python code"

if it's only to allow changes to hosts/services/contacts/etc configurations then it should be rather easy to do it in a way that would(should)n't impact (too heavily I mean) anything.

In the Arbiter.run() method (which contains the "deepest main" loop of arbiter main thread):
check for a flag "do_reload_config" (or better name), if set -> launch a thread that's going to load/parse the config on the side in a new Config() instance, once the config is (correctly) loaded/parsed, set another flag to inform main thread that this is done ; back in Arbiter.run(): check for this second flag, if it's set : break that loop and rebind the "running" config (self.conf) with the one loaded by the sub-thread.. (something as simple as "self.conf = self.reloaded_conf" (which would be set by the sub-thread)) and reset the concerned flags. The arbiter main thread will then be back in Daemon.do_main_loop() which will do_loop_turn() again -> Arbiter.run() again and resume the execution of the main arbiter thread in Arbiter.run() so with the dispatch of the config to the different daemons.

but yes untested on my side

what could be a problem is the arbiter modules -> if they have internally linked to the arbiter conf (module.my_bind_of_conf = arbiter.conf) for some reason then they will continue to use the previous conf, which would be completely bad ofcourse and in this case then we would probably need to replace the config "in place", that is no "arbiter.conf = new_conf" but some kind of "arbiter.conf.replace_with(new_conf)" ;; not sure that this is the case / is even possible in fact or anyway.

@gst
Copy link
Contributor

gst commented Feb 4, 2015

not sure that this is the case / is even possible in fact or anyway.

I mean not sure that a shinken arbiter module can do bind itself directly to the arbiter.conf.

@Seb-Solon Seb-Solon modified the milestone: 2.4 (Noteworthy Nagapie) **we will find :)** Mar 9, 2015
@Seb-Solon
Copy link
Contributor

IMO this fix is legit. The sudo is important if you use the OS start system (service..) but here we directly call the init script that should be executable by shinken / any user.

That's how script are chmoded in init.d usually (ref : a newly setup debian server)

@Seb-Solon
Copy link
Contributor

Merging this. Sudo command will mostly fail because it has no tty if you dont configure the sudoers not to ask for it.

Seb-Solon added a commit that referenced this pull request Mar 31, 2015
Fix: Dont restart/reload shinken with sudo
@Seb-Solon Seb-Solon merged commit a3fe32f into shinken-solutions:master Mar 31, 2015
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.

5 participants