Skip to content

c8y-configuration-plugin panics when required folder/s are missing #1963

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 16, 2023
Merged

c8y-configuration-plugin panics when required folder/s are missing #1963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions crates/common/tedge_config/src/tedge_config_cli/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ pub enum TEdgeConfigError {

#[error(transparent)]
Multi(#[from] Multi),

#[error(transparent)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this error variant is generic enough, it's okay.

A better option to avoid "corrupting" TEdgeConfigError would have been to add a similar variant to ConfigManagementError and LogRetrievalError enums respectively and have the ConfigManagerConfig::from_tedge_config() and LogManagerConfig::from_tedge_config() functions return these specific errors instead of TEdgeConfigError.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this error case is really not ideal, as it depends on tedge_utils::paths::PathsError.

Albin's suggestion would avoid that.

DirNotFound(#[from] tedge_utils::paths::PathsError),
}

impl TEdgeConfigError {
Expand Down
7 changes: 5 additions & 2 deletions crates/extensions/c8y_config_manager/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use c8y_api::smartrest::topic::C8yTopic;
use tedge_utils::paths::validate_parent_dir_exists;

use std::path::Path;
use std::path::PathBuf;
Expand Down Expand Up @@ -86,7 +87,7 @@ impl ConfigManagerConfig {
let tedge_http_address = tedge_config.query(HttpBindAddressSetting)?;
let tedge_http_port: u16 = tedge_config.query(HttpPortSetting)?.into();

Ok(ConfigManagerConfig::new(
let config = ConfigManagerConfig::new(
config_dir,
tmp_dir,
data_dir,
Expand All @@ -95,6 +96,8 @@ impl ConfigManagerConfig {
mqtt_port,
tedge_http_address,
tedge_http_port,
))
);
validate_parent_dir_exists(config.plugin_config_path.as_path())?;
Ok(config)
}
}
2 changes: 2 additions & 0 deletions crates/extensions/c8y_log_manager/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use tedge_config::TEdgeConfig;
use tedge_config::TEdgeConfigError;
use tedge_config::TmpPathSetting;
use tedge_mqtt_ext::MqttMessage;
use tedge_utils::paths::validate_parent_dir_exists;

pub const DEFAULT_PLUGIN_CONFIG_FILE_NAME: &str = "c8y-log-plugin.toml";
pub const DEFAULT_OPERATION_DIR_NAME: &str = "c8y/";
Expand Down Expand Up @@ -54,6 +55,7 @@ impl LogManagerConfig {
let plugin_config_path = config_dir
.join(DEFAULT_OPERATION_DIR_NAME)
.join(DEFAULT_PLUGIN_CONFIG_FILE_NAME);
validate_parent_dir_exists(&plugin_config_path)?;

let plugin_config = LogPluginConfig::new(&plugin_config_path);

Expand Down
1 change: 1 addition & 0 deletions crates/extensions/tedge_file_system_ext/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ repository = { workspace = true }

[dependencies]
async-trait = "0.1"
log = "0.4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most of the other crates, we use tracing instead of log. This inconsistency is not just limited to this crate, but let's try and avoid it at least in newer code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I heard from @didier-wenzek is that trace is heavy, so I am just using the log crate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree that log is probably more that sufficient for our simple logging needs. But, the Rust ecosystem seems to be leaning more towards wider usage of tracing instead of log as you can see in rust-lang/compiler-team#331 . Anyway, we just need to stay consistent, whichever one we choose. If the mandate is to stick to log, will keep that in mind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a point we need to discuss. I have a preference for log simply because in practice our use of tracing is reduced to logging without any trace.

tedge_actors = { path = "../../core/tedge_actors" }
tedge_utils = { path = "../../common/tedge_utils", features = ["fs-notify"] }
tokio = { version = "1.23", features = ["macros"] }
Expand Down
10 changes: 8 additions & 2 deletions crates/extensions/tedge_file_system_ext/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use async_trait::async_trait;
use log::error;
use std::path::PathBuf;
use tedge_actors::futures::channel::mpsc;
use tedge_actors::futures::StreamExt;
Expand Down Expand Up @@ -121,9 +122,14 @@ impl Actor for FsWatchActor {
}

async fn run(&mut self) -> Result<(), RuntimeError> {
let mut fs_notify = NotifyStream::try_default().unwrap();
let mut fs_notify = NotifyStream::try_default().map_err(Box::new)?;
for (watch_path, _) in self.messages.get_watch_dirs().iter() {
fs_notify.add_watcher(watch_path).unwrap();
if let Err(err) = fs_notify.add_watcher(watch_path) {
error!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this approach is less invasive I'm starting to doubt if this is the right thing to do.

The issue with it is that, we let the system continue in a broken state by silently logging the issue, which users may not notice immediately. If this actor fails to watch a directly that another actor has requested for, it is a fatal failure, in my opinion. So crashing the daemon is probably the better option I feel, so that the user immediately notices that something is wrong.

Ideally, we should have a mechanism in the actor framework where this actor can communicate this failure to the requesting actor via an error and let that actor itself decide whether to crash or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are doing the validation up-front during build time and hence this error path is less likely to happen (unless someone deletes the dirs during runtime), this is okay for now. But, we'd still need to explore a better build time error exchange mechanism between actors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

"Failed to add file watcher to the {} due to: {err}",
watch_path.display()
);
}
}

loop {
Expand Down