troglobit / finit

Fast init for Linux. Cookies included
https://troglobit.com/projects/finit/
MIT License
622 stars 61 forks source link

Add s6 notify support to mdevd plugin #327

Closed hongkongkiwi closed 1 year ago

hongkongkiwi commented 1 year ago

Could you please add S6 readyness support to mdevd:

https://github.com/troglobit/finit/blob/master/plugins/mdevd.c#L41

#define MDEVD_ARGS   "-C -O 4 -D %n"

https://github.com/troglobit/finit/blob/master/plugins/mdevd.c#L87

    if (debug)
        snprintf(line, sizeof(line), "[S12345789] cgroup.system notify:s6 pid:!%s @%s:%s %s -v3 %s -- %s",
             MDEVD_DAEMONPIDFILE, MDEVD_DAEMONUSER, MDEVD_DAEMONGROUP, cmd, MDEVD_ARGS, MDEVD_DESC);
    else
        snprintf(line, sizeof(line), "[S12345789] cgroup.system notify:s6 pid:!%s @%s:%s %s %s -- %s",
             MDEVD_DAEMONPIDFILE, MDEVD_DAEMONUSER, MDEVD_DAEMONGROUP, cmd, MDEVD_ARGS, MDEVD_DESC);
troglobit commented 1 year ago

Ah yes, thanks for reminding me! I knew there was something obvious I had forgot ...

troglobit commented 1 year ago

Pending #328, this issue and plugin may be completely dropped in favor of a simple .conf file.

hongkongkiwi commented 1 year ago

Whats the thinking to drop this as a plugin? I thought the idea was to get it to startup extremely early before anything else is loaded.

troglobit commented 1 year ago

The plugin and the new notify features are logically equivalent to the two .conf lines in #328. Since mdevd runs as a monitored service, as compared to plain BusyBox mdev, it will have to start as regular tasks and services do in runlevel S. In many cases I'd recommend going with just mdev, which would be quicker, but you seem to have built a lot of logic around mdevd.

troglobit commented 1 year ago

Andy did you see my reply, understand the reasoning, and are OK with my removing mdevd as a plugin (and softdep in other plugins) in Finit?

This task and the /dev conditions support are the only two remaining features for the v4.4 release, and I'd really like to finalize it this month. Because the next 6-12 months I will be extremely busy in a customer project where I'll be needing the v4.4 release.

hongkongkiwi commented 1 year ago

I'm having an issue with this, but the issue doesn't make any sense to me at present (mdevd will crash the first time it runs then it will run fine), perhaps we leave it as is? Maybe instead we fix the issue where we don't want to build either plugin #331 and it can be switched off and added to the config manually for 4.4.

Whats strange is it originally worked, now it doesn't, it's most likely my system, but probably no rush to remove this right as it's a compile time option anywho.

troglobit commented 1 year ago

Very odd, the two ways of starting mdevd+coldplug (plugin and .conf) are equivalent. I can check if I can see the same issue as you. Otherwise I'd rather remove the plugin.

It was added in this release cycle, so a lot easier to revert it now, because leaving it in means it's a supported feature and someone else will start using it as well. Meaning I can never really remove it ...

Removing it now is a lot easier than adding a lot of extra checks and do manual tests to fix #331 in a manner that actually makes those plugins not check if mdevd is disabled.

troglobit commented 1 year ago

Spent a while trying to reproduce your issue, without success I'm afraid. Stable as a rock here, and I might add, it runs at the same point in time compared to the plugin.

hongkongkiwi commented 1 year ago

Thanks for looking into it, go ahead and make the change. There might be something else strange going on.

troglobit commented 1 year ago

Alright, thank you. Hope you find the root cause of your problems!

hongkongkiwi commented 1 year ago

For the new S6 mechanism, how long does the finit wait for the s6 notify signal before restarting a service? Can this delay be modified?

troglobit commented 1 year ago

Not sure I'm following here, there is no such mechanism to restart if there is no readiness notification.

troglobit commented 1 year ago

Readiness notification added to plugin in 180859a and was subsequently dropped in f6bfbc8, as discussed previously.