LLT-6736: Add command 'reload' and support SIGHUP#1844
Conversation
5271577 to
c052054
Compare
c052054 to
4d76ce4
Compare
4d76ce4 to
3541688
Compare
|
+1 |
stalowyjez
left a comment
There was a problem hiding this comment.
Lgtm, just a few minor comments/questions
Needed for config reload so that reload is triggered only when the file actually changes.
Adds CLI Reload command support. Allows the daemon event loop to receive an already-parsed config from the Reload handler.
Adds support for CLI reload command and SIGHUP signal.
Logging reload requires a persistent handle to the subscriber reload layers. This handle is returned from setup_logging so the daemon loop can swap the log writer and filter level on reload.
3541688 to
b16cb31
Compare
|
@stalowyjez do you have any other comments? |
| mut config: NordVpnLiteConfig, | ||
| config_file: &ConfigFile, |
There was a problem hiding this comment.
Could these two be unified? It's not very obvious what are the differences between the two parameters
There was a problem hiding this comment.
I was thinking about that but wasn't sure if I should. What is in the ConfigFile describes the config file, path to it and it's hash. NordVpnLiteConfig contains parsed content of a config file. That's why I wasn't sure if I should mix them.
There was a problem hiding this comment.
I would personally prefer something like RunningConfig { parsed, path, hash }, but I'm not sure if it would make it more complicated with the borrow checker..
|
|
||
| #[derive(Clone)] | ||
| pub struct ConfigFile { | ||
| pub path: String, |
There was a problem hiding this comment.
What do you think about PathBuf for a more ergonomic way of working with owned paths?
| rt.block_on(daemon::daemon_event_loop(config)) | ||
| rt.block_on(daemon::daemon_event_loop( | ||
| config, | ||
| opts.config_path.clone(), |
There was a problem hiding this comment.
How does this work with relative config and log paths when we detach? (since the daemon changes working directory, either to / or opts.working_directory)
Do we need to convert to canonical paths before detaching?
There was a problem hiding this comment.
For log_file_path there is already a statement in readme:
log_file_path - a path to store the daemon's logs, needs be absolute, otherwise will be relative to working-directory when daemonized
I assumed similar rule for config-file.
IMO it make sense that relative path is determined towards the working directory.
There was a problem hiding this comment.
yes but the working directory of the daemon changes after it's "detached".
So if you launch nordvpnlite start -c config.json from /home/user/, the daemon later tries to read /config.json on reload, instead of /home/user/config.json.
We probably need to call canonicalize() before passing the config_path into the daemon_event_loop
There was a problem hiding this comment.
Yes, you are correct. But is such case, when config file is relative, shouldn't the user launch the app with --working-directory /home/user/? I assumed that we should follow similar logic as for log_file_path
There was a problem hiding this comment.
Actually log path is canonicalized when reading the config, but canonicalizing again after detach would result a different path.
There was a problem hiding this comment.
Before the changes, the config and log path were resolved only once before "detaching" the daemon.
So if you nordvpnlite start -c config.json from /home/user/, the config is read and parsed from /home/user/config.json.
After nordvpnlite reload
The daemon will try to read the config from /config.json, but since it doesn't exist, it will:
- Create a new default config
- Since the default config does not contain a valid API token, the daemon will exit with error breaking the VPN connection.
As you mentioned nordvpnlite start -c config.json --working-directory /home/user/ would solve this, provided the user gave the correct --working-directory, IMHO this is not a very smooth and polished user experience.
I'm just pointing this out, If you are already aware and this was a conscious design decision, please document the expected behavior in the read me.
| } | ||
| Some(Err(e)) => { | ||
| error!("Config file changed but failed to parse: {e}"); | ||
| Err(e) |
There was a problem hiding this comment.
Would Ok(CommandResponse::Err(e)) here return the error to the client caller?
There was a problem hiding this comment.
here is what we get in log file:
2026-07-03T10:59:21.854830Z ERROR nordvpnlite::command_listener: 201: Config file changed but failed to parse: invalid type: unit variant, expected newtype variant at line 12 column 13
There was a problem hiding this comment.
Yes but what do you get in the cli calling nordvpnlite reload? And does the daemon keep running the old config?
There was a problem hiding this comment.
On CLI I am getting
Error: InvalidResponse("EOF while parsing a value at line 1 column 0")
daemon is not restarted and continues with the old configuration
| fn hash_bytes(bytes: &[u8]) -> u64 { | ||
| use std::collections::hash_map::DefaultHasher; | ||
| use std::hash::{Hash, Hasher}; | ||
| let mut hasher = DefaultHasher::new(); | ||
| bytes.hash(&mut hasher); | ||
| hasher.finish() | ||
| } |
There was a problem hiding this comment.
I wonder how would this approach handle whitespace or formatting changes in the config file?
Would it trigger reload causing a reconnect?
Maybe we could just use PartialEq to compare the pending config against the current config?
There was a problem hiding this comment.
With PartialEq the problem is that changes to AdapterType::Custom will not be catch. #1844 (comment)
There was a problem hiding this comment.
Adding a whitespace to a file should trigger a reload. At least that was my team intention during this task refinement.
There was a problem hiding this comment.
Yes, but that means it will trigger a reconnect -> packet leak, even though nothing changed in the config functionally.
If you are aware of that and the risk is accepted, then it's fine by me.
| rt.block_on(daemon::daemon_event_loop( | ||
| config, | ||
| opts.config_path.clone(), | ||
| logging_handle, |
There was a problem hiding this comment.
logging_handle will be dropped inside daemon_event_loop since it's moved here. Could this lead to unflushed logs during teardown?
| config_path: String, | ||
| mut logging_handle: logging::LoggingHandle, | ||
| ) -> Result<(), NordVpnLiteError> { | ||
| let mut config_file = ConfigFile::new(config_path); |
There was a problem hiding this comment.
Looking at this again, this is called after possible detaching of the daemon.
In case of a relative config_path like we already discussed, I suspect the std::fs::read(&path) could already fail when calculating the hash, even before reload is called.
Do you think it would be worth to at least add a log to?
let hash = std::fs::read(&path)
.map(|b| Self::hash_bytes(&b))
.unwrap_or(0);
Problem
Currently to change the nordvpnlite configuration while the deamon is running we need to stop it, change the config file and restart the daemon.
The packets may leak during this operation.
Solution
This PR doesn't address the packets leak problem yet but prepares a base for future changes.
It introduces:
reloadcommand supportBoth triggers causes the the daemon to restart with a new configuration.
Verification
Happy path verification
Daemon does not restart when config.json not changed
☑️ Definition of Done checklist