-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
php.services.default: init #430490
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
php.services.default: init #430490
Conversation
5d20bf2 to
a7189ae
Compare
|
|
That beign said, can you provide a link with some context? Because I'm not sure I know the current state behind these ideas and thus I don't know how much effort it'd be to maintain/test this. |
i guess there was a misunderstanding - as stated i know how to build the manual... i don't know where to look for rendered modular services options, though
there is no maintenance required on your part - modular services are highly experimental and opt in... on top of that they could always be removed if they become a hassle due to being highly experimental |
|
Hey, I'd like to apologize for this important missing feature. I've just opened a |
e7d53a6 to
1e4310f
Compare
|
if there is no serious complaint with the structure of this modular service i will go ahead and merge this in a few days ... keeping a few things in mind:
notes for @roberth and anyone else who cares about modular services:
|
|
Could you add a test? Otherwise this becomes hard to maintain.
Some things, like creating a directory, are better to do as a startup script iirc, because then it also works at |
Given that this gets imported into a "traditional" NixOS evaluation, I'm not sure I follow here?
Sowwy, I was too quick to try helping before I realized this is a different kind of module 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I think trying modular services out is a good thing and I applaud the effort.
That being said, it must be clear that "highly experimental" means for me as a maintainer that the only guarantee I can give is that this expression stays syntactically valid. I'm not saying this to be mean or because I intend to break this, but because I'd like to make the implications of that statement explicit.
But what I'm actually trying to get at here: on what basis are we adding support for another init system?
NixOS/rfcs#163 was never ratified. I'm OK with the experimentation here, but I do have major reservations about supporting another init system in NixOS. Yes, this is experimental and all, but let's be honest, we have a history at making experimental stuff load-bearing (couch flakes) and I'm a little afraid that if this gets stabilized, we also add another init system (which in turn makes it harder for me to write modules that use all the features systemd has to offer because somebody wants support for another init system).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of the "highly experimental" nature of this is that we spread out and see what works and what doesn't, learn the lessons we need to learn, and refine
a large perceived and important promise of modular services was that they could be service manager agnostic - so we need to push ahead here
on what basis are we adding support for another init system?
i'd like to emphasize again... on purely an experimental basis. merge this PR and then revert it immediately after. break support for any part of this service you see fit. then fix it - or don't. these expressions can be moved out of tree at any point and that is fine... but there is value in having them in tree so we can learn from them, what works, what doesn't, etc... this is especially true for supporting alternative service managers - the exact problems you mention about "use all the features" is so useful for us to learn here. so yes, utilize all the features of systemd that you can in this module and let's see what we learn about modular services in the meantime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Systemd still takes priority. If the presence of, say, finit definitions makes systemd support worse somehow, it means that finit needs to catch up. If it doesn't have a test it can't break and you can drop that. If that's a problem, the original contributor should have added tests and listed themselves as co-maintainer for at least that part. Hint hint @aanderse ;)
This way everyone wins. You don't need to feel weighed down, and you may even get someone to help review your own PRs as well. The more maintainers the merrier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well said @roberth! i have added the test as suggested 💪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Systemd still takes priority
It's not a matter of priority. It's a matter of "what are we signing up for here?" The finit backend that this is designed for doesn't exist, at least not in-tree. I don't think nixpkgs should be a playground for APIs that we have no implementations of. And we're certainly not going to have an implementation of a finit backend without an alternate-init-system RFC being ratified for NixOS. As-is, this is dead code. And for whatever finit backend could exist out-of-tree, it is now beholden to whatever finit expressions nixpkgs upstream is shipping, rather than being able to define and evolve its own API independently.
We're also suggesting that nixpkgs is open for people to contribute services built for out-of-tree backends, which just exacerbates those problems. For instance, I really don't want someone coming along and contributing a package to nixpkgs along with a modular service that includes a nix-darwin implementation. I think the nix-darwin maintainers want the nix-darwin API to be solely within and downstream of nix-darwin, not upstream of it. Once people start contributing nix-darwin modules to nixpkgs, the lives of nix-darwin maintainers get a lot harder, as breaking changes now have to be coordinated with nixpkgs and users have to be sync'd up on that coordination.
So we're signing up for:
- Telling external backends what their API should look like, despite nixpkgs itself having no interest or investment in those implementations.
- Maintaining expressions that are not usable in-tree.
- Telling contributors that it's ok to create these problems for other systems like nix-darwin, system-manager, and home-manager that makes their maintainers' lives harder.
This should really be out-of-tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that modular services as defined require backend specific definitions in each service indicates to me that there's an issue with the abstraction, as it evidently can't represent even a basic service without an escape hatch. I'm not saying there should never be an escape hatch, but I am saying that if the only way to have "modular services" is to have every service provide configuration for every backend, then it's not very modular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modularity aspect is mostly about the ability to import any piece of service logic into any service name, and the ability to group such services together into a single service with "sub-services".
@ElvishJerricco @K900 what you describe is portability, which is separate from the modularity aspect. Portability is underdeveloped, that is very true. This system should grow more inherently portable options, of which currently the only examples are process.argv and the sub-services option.
Then why did we merge an unfinished abstraction?
- The modularity part is finished and provides value independently of any attempt at portability.
- The module system can deal with leaky abstractions. For instance, I can enable postgres and meddle with its systemd units in the same configuration. That's not to say it doesn't come at a cost, but some escape hatches are ok.
- It gets the ball rolling on the portability aspect.
- By having real services before we add more abstractions (inherently portable options), we are more likely to find the right abstractions without over-engineering them or getting stuck in analysis paralysis.
I believe portability is worth pursuing, but some things need to happen:
- Portability must not burden NixOS maintainers who have no interest in it. We've covered this somewhat in Contributor documentation for Modular Services #431396 (only merged just now), but testing is still a problem.
We've had an undocumented rule saying Nixpkgs must not import expressions from elsewhere, for security, performance and monorepo simplicity reasons.nixpkgs/ci/pinned.jsonviolates the most basic phrasing of the rule. Instead, production use of Nixpkgs must not import anything. We can still meet that requirement as long as any imported configuration managers are only used for tests; specificallypassthru-style tests which aren't built by Hydra. - Improve our documentation / communication. I think Contributor documentation for Modular Services #431396 may help, but probably needs more work.
- Add more of these "inherently portable" options. For this, having all expressions in (mostly) one repo is a real benefit. Only if we have those tests though!
I can help with documentation and reviews.
Whether portability in nixpkgs modular services becomes a success depends entirely on participation from those who want it.
I see two possible outcomes:
A. The "things that need to happen" are done and service modules in Nixpkgs are increasingly portable.
B. Those things aren't done, and portability of service modules is scrapped from Nixpkgs. Out-of-tree service modules can still choose to add their own portability, and they're only slightly worse off by the Nixpkgs decision.
(A) would be great, and (B) is also acceptable.
I would like us to provide an opportunity for (A), but if the current development approach is not deemed acceptable, we could go for (B) first and consider re-adding in-nixpkgs portability if and when all of the things to be done are covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aanderse Could you add an finit-based test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few quick words since before I'll hide back into vacation: the entire discussion makes me think that this is the kind of feature an RFC would actually make sense on (instead of it being a place where randos complain about moderation such as 177/176 with the STC doing essentially a non-job because they are no proper governance body). Like, this is the kind of discussion that would've made sense on the first introduction. And yes, I know this is an experimental feature but again, we have a history of starting to use those in a very unhealthy way, so I remain skeptical that this is what may happen here as well (I'll consider it long-term damage done by flakes from now on).
That being said, I'm sufficiently OK to not invest time to push back (as I said before, as a maintainer of the PHP-subsystem, the only guarantee I'm willing to give currently is that this file remains syntactically valid and I'm actually kinda interested to see where this is going).
However, if finit-based things get anywhere near a tested-set for Hydra, I'll change my stance very quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No effect whatsoever on channel blockers!
That's a really good delineation that I agree we should take into account while this is experimental.
The portability work is meant as a module based implementation of RFC 163, but that connection has become a bit loose due to the original author's absence.
EDIT: re 176/177, noted. We should deal with that. Enjoy your time off!
58fdb10 to
d3d4281
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't suppose it implements the notify-reload protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda, in a slightly unconventional way, if the configuration is correct
i will keep this PR as a "pristine" example of "converting" an existing service into a modular one... the next PR, adding notify-reload support, can serve as a good example of making a change to a regular service and propagating the change to a modular service
let's not muddy the waters of this PR... but thanks for mentioning that! somehow this has slipped past the team so far 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my own reference this future PR can look something like...
| Type = "notify"; | |
| Type = if cfg.settings.systemd_interval or 0 > 0 then "notify-reload" else "notify"; |
and add to finit...
finit.service = {
nohup = cfg.settings.systemd_interval or 0 == 0;
};
roberth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from modular services perspective.
I have no knowledge of php-fpm.
d3d4281 to
04b4c3c
Compare
Worth also mentioning smfh. It's not compatible with |
| process.argv = [ | ||
| "${cfg.package}/bin/php-fpm" | ||
| "-y" | ||
| configFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reload functionality is probably not useful, because nixos-rebuild switch will restart the service because of this line.
Looks like we'll need modular services to provide a fixed environment.etc-like path for this kind of config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Things done
i promised @roberth i would contribute to modular services... so here i go!
my biggest question with this PR is how can i view the documentation for this? whenever i write nixos modules i can easily build the nixos manual and view the options... i don't know how to do that here 🤔
for reference this is how i tested this module:
from there i ran
caddyand created a simpleindex.phpwithphpinfo()function to validatepassthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.