icinga2 icon indicating copy to clipboard operation
icinga2 copied to clipboard

WIP: /var/run/icinga2/icinga2.s: reload on POST /v1/reload

Open Al2Klimov opened this issue 6 years ago • 12 comments

... and wait for the reload to complete before responding to let curl(?) block the reload command.

fixes #7301 resolves #8911 closes #8973

Al2Klimov avatar Jul 23 '19 08:07 Al2Klimov

@dnsmichi I have something (this one) to show you and the rest of team Icinga core (one nice day).

Al2Klimov avatar Jul 23 '19 13:07 Al2Klimov

@N-o-X What about this sync reload trigger as alternative to the current async one?

Al2Klimov avatar Dec 15 '20 10:12 Al2Klimov

@cla-bot check

Al2Klimov avatar Aug 04 '21 12:08 Al2Klimov

@julianbrost about this solution: https://github.com/Icinga/icinga2/pull/8973#issuecomment-903663069

Al2Klimov avatar Aug 23 '21 11:08 Al2Klimov

Feedback

  • [x] Julian
  • [ ] Noah

Al2Klimov avatar Aug 23 '21 11:08 Al2Klimov

I had a quick look at this to see if this could serve as basis for implementing one of the ideas I proposed in #9363, but for that, the listening socket is in the wrong process.

General impression after looking at this:

  • Current state does not fix yet what it says to fix in the PR description: systemd unit file is not updated yet, the systemd unit file isn't updated so far, but well, the description also says "WIP". Also, compared to safe-reload, no validation output seems to be returned.
  • In general, fork() not immediately followed by exec() is always tricky, just look at the amount of uninitialize/reinitialize workarounds, so I'm not sure about making the umbrella process even more complex that it already is. Are there any plans on extending the HTTP API with further commands? Otherwise, some simple file-based notification mechanism might be a better fit.

julianbrost avatar Apr 29 '22 12:04 julianbrost

  • Also, compared to safe-reload, no validation output seems to be returned.

Shall I fix this?

Al2Klimov avatar May 02 '22 09:05 Al2Klimov

Not right now, please focus on the reviews and comments on your Icinga DB PRs first.

julianbrost avatar May 02 '22 10:05 julianbrost

My question was Shall I fix this? Not Shall I fix this right now?

Al2Klimov avatar May 02 '22 10:05 Al2Klimov

Before fixing anything, we should first agree on the approach. Like if we can't come up with another useful purpose of the socket, all that extra HTTP handling might be overkill and something simpler could be done without that extra threading in the umbrella process.

julianbrost avatar May 02 '22 11:05 julianbrost

Overkill or not, it doesn’t reinvent the wheel and curl speaks it.

Al2Klimov avatar May 03 '22 07:05 Al2Klimov

My complaint isn't HTTP in itself but rather that in order to implement it, you have to add threads, which you have to tear down again in order to be able to fork, and then restore again.

julianbrost avatar May 03 '22 08:05 julianbrost

Would be not the first ones, would they?

Al2Klimov avatar Oct 28 '22 14:10 Al2Klimov

Given that fork() from a multi-threaded applications is problematic (that's why the current state of the PR has to tear down the threads before forking) and as the umbrella process doesn't have to (and should not) do any heavy lifting: would it be possible to just run any pending coroutines in the main loop on that thread?

Would be not the first ones, would they?

I'd rather try to get rid of any existing extra threads in the umbrella process rather than adding more.

julianbrost avatar Feb 06 '23 13:02 julianbrost

Well I could merge this new ASIO event loop with Application#RunEventLoop(). But this would make the still necessary teardown-fork(2)-setup parts even uglier. Shall I do this?

Al2Klimov avatar Feb 06 '23 13:02 Al2Klimov

Well the idea behind that idea was avoid having to tear down anything. So that doesn't sound like it would move into a direction I'd like to see.

julianbrost avatar Feb 06 '23 13:02 julianbrost

What about an early forked subprocess instead? Could communicate w/ umbrella via shared memory.

Al2Klimov avatar May 17 '23 15:05 Al2Klimov

Now as I saw #9136: What about an mmap(2)ed file in /run with some state and semaphores? This way a new CLI command icinga2 reload could communicate with the daemon w/o requiring it to have an extra thread or process.

Al2Klimov avatar Nov 21 '23 11:11 Al2Klimov