ENT-14434: Move reactor-pluigin logic into cf-reactor daemon loop - #6335
ENT-14434: Move reactor-pluigin logic into cf-reactor daemon loop#6335victormlg wants to merge 1 commit into
Conversation
2e7ed0e to
dfd91c6
Compare
larsewi
left a comment
larsewi
left a comment
There was a problem hiding this comment.
Please add some more context. E.g., explain why chose to make cf-reactor plugin its own process. Also I expected cf-reactor in nova to already fork out and make it a daemon, but I have not seen this code removed.
larsewi
left a comment
larsewi
left a comment
There was a problem hiding this comment.
There one major issue here. If the nova fork dies, then core will continue running and no one will restart the nova reactor again. With this architecture, core must be responsible for restarting the nova fork if it dies.
553a07c to
aa6d538
Compare
7228cb8 to
be91f14
Compare
larsewi
left a comment
larsewi
left a comment
There was a problem hiding this comment.
Please write a test where you kill the nova cf-reactor and see that it comes back. Maybe also test reaching that limit that you set.
be91f14 to
a7fd033
Compare
victormlg
changed the title
|
I updated the context of this PR to match what it currently contains |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
a7fd033 to
5d32781
Compare
The code for the reactor-plugin was split up into different functions to be integrated inside the new cf-reactor daemon in core. The implementation was tweaked to use select(2) instead of poll(2), to support cf-reactor on different platforms. However, the reactor-plugin remains linux only. Ticket: ENT-14434 Signed-off-by: Victor Moene <victor.moene@northern.tech>
5d32781 to
9effa41
Compare
|
|
||
| int all_fds[N_ALL_FDS]; | ||
| // the first num_nova_fds fds are populated with nova fds | ||
| size_t num_nova_fds = ReactorNovaInitialize(all_fds, N_ALL_FDS); |
There was a problem hiding this comment.
We probably need to differentiate between ReactorNovaInitialize failed and ReactorNovaInitialize has no FDs to contribute.
| signal(SIGINT, HandleReactorSignals); | ||
| signal(SIGTERM, HandleReactorSignals); | ||
| signal(SIGBUS, HandleReactorSignals); | ||
| signal(SIGHUP, HandleReactorSignals); | ||
| signal(SIGUSR1, HandleReactorSignals); | ||
| signal(SIGUSR2, HandleReactorSignals); | ||
| /* Writing to a pipe whose spawned process already exited (e.g. cfbs | ||
| * rejecting its arguments before reading its stdin) must fail with EPIPE | ||
| * rather than terminate the whole daemon. Set after ReactorNovaInitialize(), | ||
| * so that the spawner and the processes it execs keep the default handling. */ | ||
| signal(SIGPIPE, SIG_IGN); |
There was a problem hiding this comment.
These should probably happen before ConnectAndListen because that's the one place where the can sit for an unbound time. And right now it sits there without a way to shut it down cleanly.
| static void HandleReactorSignals(int signum) | ||
| { | ||
| HandleSignalsForDaemon(signum); | ||
| if (IsPendingTermination()) | ||
| { | ||
| ReactorNovaTerminate(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| static void HandleReactorSignals(int signum) | |
| { | |
| HandleSignalsForDaemon(signum); | |
| if (IsPendingTermination()) | |
| { | |
| ReactorNovaTerminate(); | |
| } | |
| } | |
| static void HandleReactorSignals(int signum) | |
| { | |
| HandleSignalsForDaemon(signum); | |
| if (IsPendingTermination()) | |
| { | |
| TERMINATE = 1; | |
| } | |
| signal(signum, HandleReactorSignals); /* re-arm the wrapper */ | |
| } |
You will need to re-arm this wrapper, otherwise the internal handler will get call directly the next time the signal is triggered
|
|
||
| ENTERPRISE_FUNC_1ARG_DECLARE(int, ReactorEnterpriseMain, bool, no_fork); | ||
| ENTERPRISE_VOID_FUNC_0ARG_DECLARE(void, ReactorNovaTerminate); | ||
| ENTERPRISE_FUNC_2ARG_DECLARE(size_t, ReactorNovaInitialize, ARG_UNUSED int*, fds, ARG_UNUSED size_t, max_size); |
There was a problem hiding this comment.
Put ARG_UNUSED in the declarations. It belongs on stub definitions only.
| pid_t existing_pid = ReadPID("cf-reactor.pid"); | ||
| if ((existing_pid != -1) && (kill(existing_pid, 0) == 0)) | ||
| { | ||
| Log(LOG_LEVEL_ERR, "Another instance of cf-reactor is already running, terminating"); |
There was a problem hiding this comment.
Should this message be rather "was already running, terminated" since you are calling kill() In the if expression? Maybe it would be good to mention what PID it was that you killed for debugging issues with unexpected running cf-reactor processes.
4 participants
Context
So originally, we wanted to have two daemons: one for cf-reactor and one for the agent driven cfengine code. Then it has been decided to merge these two binaries into a single one. In this PR, we move the cf-reactor daemon to core, split the reactor-plugin in different functions, and integrate them inside the daemon loop of cf-reactor
Merge together: https://github.com/cfengine/enterprise/pull/996 https://github.com/cfengine/nova/pull/2696