-
Notifications
You must be signed in to change notification settings - Fork 12
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
[RFC] Configuration file #30
Comments
This is indeed a good idea. I was going to write up some guidelines regarding its implementation, but then came to the conclusion that it would actually take less time to just implement it than to write out an implementation manual. Sorry in case you were looking forward to working on this. So a rudimentary implementation is now in the master branch. You need to compile evsieve with the
(I am using these feature gates to prevent features that are not yet ready for stabilization from delaying an 1.4 release.) Question 1I think that letting the (default) configuration file contain essentially arguments for the evsieve program may indeed be the best option. Any other option would essentially require designing a second API. Like you suggest, I've implemented For example, if you put the following files on disk:
Then invoking evsieve as
Maybe in the future we may want to support some configuration formats used by third-party programs as well. We could use In a proper implementation, the configuration files would get lexed the same way the shell lexes them, i.e. parsing No proper shell lexer has been implemented yet, so for now putting clauses like So far I've been trying to minimize the amount of dependencies I use, because evsieve is a program running as root and I don't want near-npm levels of untrusted dependencies in it. On one hand I don't want to include another dependency for a feature most people won't use, on the other hand, maybe writing such a lexer by hand is just too error-prone... (I've been wondering for a long time whether I should get a bit looser with the dependencies. It is seriously slowing down some developments.) Question 2Option three seems like the only sensible option, because I don't like edge cases. It's not that difficult to implement either. It may make a hypothetical future feature of being able to update configuration files without restarting evsieve a bit more convoluted to implement, but allowing recursive configuration files makes it for example possible to separate the file that defines the input devices from the file that defines the mappings, so I suppose it is worth including support for it. An issue potentially related to the shell lexer question above is: do we want to allow glob expansion inside the configuration files as well? That would make it possible to do something like While we're at it, do we expand environment variables in the configuration files, like Some thoughts on reloading configuration filesIf we're adding support for configuration files, then I suppose it would be nice to be able to be able to update those configuration files without having to restart evsieve as well. However, the codebase of evsieve doesn't currently have the infrastructure to change the applicable mappings after initialization, so that would be another big feature after configuration scripts. There two levels of possible support for reloading configuration files:
As you might guess, the second option is more complicated to implement than the first. Not impossible, but its going to require some refactoring. As for reloading configuration files, this can be done in two ways as well:
Option two seems a bit more user-friendly. And also a bit more convoluted to implement. If we go for option (2), do we want to enable configuration file reloading by default, or only if the user passes a special flag for it? If we want to make it default, we may want to wait until it is implemented before stabilizing the Some more thoughtsSince evsieve is probably running as root, and being able to edit evsieve's configuration files trivially allows you to escalate to root by using (In addition to checking the file permissions, we of course also neet to check the permission/ownership/sticky-bits on all parent directories of the configuration files, of course.) If so, should evsieve also try to to check the file ACLs? (This is complicated, and an argument could be made that if the user is using ACLs, then they better know what they are doing so there is no need to babysit them.) Some point to avoid a future breaking change: currently, |
No worries. Thank you actually, for spending time to implement a feature I want. (And of course, thanks for this great project in the first place.)
Yeah I was surprised when As of whether using an existing lexer is a good idea, honestly I'm kind of torn too. On one hand it's probably more reliable and supports many features that the user might expect (e.g. quoted strings, inline comments, escape sequences, etc.); on the other hand the feature-richness is maybe not actually desirable. It could be argued that we only want to support a very restrictive set of grammar. I guess it depends on whether the existing lexer allows feature selection (only enabling the minimum set that we need): if so I'm in favour; if not I do not have a strong opinion.
That feels like overengineering to me, at least for the moment. You'd also have to consider whether glob matching on directories should be allowed (
The main benefit of this that I can see is built-in templating support. However I think very few people are going to find a use for it though, especially considering Systemd already has unit templating in the form of "@ units" just in case. This also ties into the wider lexer issue. There are so many design decisions to make regarding syntax and so many foot guns to dodge. Unless supported by a well-tested lexer crate, I think the benefits are greatly outweighed by the risks.
My use of evsieve is rather simple (just mappings), so I don't have an opinion either way.
These two options aren't mutually exclusive no? I like the first option because it's simple. The second one though, I think if it is to be implemented, it should be opt-in with a flag. Less likelihood of surprising the user. P.S. Speaking of flags, I really like your implementation using
LGTM. I'd try to be lazy and find a crate for this though, although trying is the first step towards failure.
I know nothing about ACLs, so I'll leave this up to you. |
I ended up implementing a basic shell lexer myself. I've come to the conclusion that allowing wildcards such as Furthermore, automatic reload of configuration files is indeed a bad idea after some more thought. As for manual reload... Currently evsieve exits upon receiving a In evsieve I have intentionally taken the traditional interpretation of SIGHUP because it is actually useful in some cases: if you were to type some badly designed evsieve command that grabs your keyboard and leaves you unable to stop it with Ctrl+C (e.g. you grab your keyboard but forgot a corresponding If evsieve were to stop interpreting SIGHUP as an exit signal, then it may become more difficult to close evsieve in some situations, to the annoyance of the user. It would be possible for the behaviour to become "evsieve interprets I could make the A third alternative would be using some other mechanism to inform evsieve to reload their configuration files? |
I agree.
This makes sense. Also it may be undesirable to tightly couple the reload behaviour to the
There are signals [Service]
Environment=PYTHONPATH=/usr/lib/python3.10/site-packages:$PYTHONPATH
Type=simple
ExecStart=/usr/bin/glustereventsd --pid-file /var/run/glustereventsd.pid
ExecReload=/bin/kill -SIGUSR2 $MAINPID
KillMode=control-group
PIDFile=/var/run/glustereventsd.pid It doesn't break compatibility which is great too. |
will this be implemented? |
Configuration files by themselves are already implemented, but not stabilized yet (i.e. I might change the format of configuration files without warning). You can use them if you compile evsieve with the
Reloading configuration files is something that has not been implemented yet. I do not have any specific timeline about when I was planning to work on that. (Okay, I did kinda forget that I wanted to stabilize configuration files by version 1.5. Thanks for reminding me.) |
I think it'd be great if instead of passing every value on CLI, the user could specify a config file.
One immediate tangible benefit is we can then have a static
evsieve.service
file (for those who use systemd) which packagers can install, and the user edits the configuration file in/etc
instead of the service file. The whole configuration process would be somewhat more "Linux-idiomatic" this way.I took a brief look at the current parser code and it seems fairly simple to reuse it. And I am willing to try implement this feature. However I have some questions regarding how to best move forward, hence this issue.
Question 1: config file format
It appears to me that the simplest and most consistent way to do this is to have the exact same format in the configuration file as in the command line. E.g.
/etc/evsieve/main.conf
I would tokenize this with a simple lexer and feed it directly to
arguments::parser::implement
. Are there any obvious foot guns I'm missing with this approach? Is there perhaps already a good lexer crate that I can use?Question 2: parsing the config file
There's the question of "how much
--config
usage should we allow?". From least complex to most, we can:--config
declaration in config files.--config
options.main.conf
load both.The question is of course what's the correct tradeoff between simplicity and feature.
Question 3: implementation details
Two options here:
Config
toarguments::parser::Argument
, and letarguments::parser::implement
handle it.--config
flag a special "macro-like" option, in the sense that all loading (recursively if necessary) inarguments::parser::parse
.Personally I actually like the second option more, since it makes a clean separation of parsing (CLI and files) and implementing. But I would like to hear others' opinions first.
The text was updated successfully, but these errors were encountered: