Add c8y-firmware-plugin #1719
Add c8y-firmware-plugin #1719
Conversation
rina23q
force-pushed
the
feature/1696/firmware-plugin
branch
2 times, most recently
from
6388c57
to
e953604
Compare
The first skeleton to receive SmartREST messages from c8y Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
rina23q
force-pushed
the
feature/1696/firmware-plugin
branch
3 times, most recently
from
bf9a920
to
aea4839
Compare
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
rina23q
force-pushed
the
feature/1696/firmware-plugin
branch
5 times, most recently
from
6bdcabe
to
9c64424
Compare
There was a problem hiding this comment.
One aspect that I didn't fully understand from the impl is how the child devices would send their current firmware details to C8Y using the set firmware message? There was some contract for the c8y-config-plugin to send the initial config list to tedge/c8y. Something similar is needed here as well, right?
crates/common/tedge_config/src/tedge_config_cli/models/seconds.rs
Outdated
Show resolved
Hide resolved
crates/common/tedge_config/src/tedge_config_cli/models/seconds.rs
Outdated
Show resolved
Hide resolved
| if smartrest_request.device == self.tedge_device_id { | ||
| warn!("c8y-firmware-plugin does not support firmware operation for the main tedge device. \ | ||
| Please define a custom operation handler for the c8y_Firmware operation."); | ||
| Ok(()) |
There was a problem hiding this comment.
Seeing that Ok(()) here surprised me a bit as I thought that it'd make the operation succeed in the cloud. But since that's not the case, it's fine. May be worth adding a comment that we're ignoring this request so that some other plugin on the machine can handle it.
There was a problem hiding this comment.
May be worth adding a comment that we're ignoring this request so that some other plugin on the machine can handle it.
I think it's our basic principle, if plugin doesn't address either parent device or child device, ignore the operations coming from c8y, so that user's custom plugin can consume them.
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
- Improve log messages - Add validation of response from child device - Add `--init` option as a placeholder for the future enhancements Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
rina23q
force-pushed
the
feature/1696/firmware-plugin
branch
from
9c64424
to
4402a17
Compare
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
| pub default_http_bind_address: IpAddress, | ||
|
|
||
| /// Default firmware operation timeout in seconds | ||
| pub default_firmware_timeout: Seconds, |
There was a problem hiding this comment.
| pub default_firmware_timeout: Seconds, | |
| pub default_firmware_update_timeout: Seconds, |
| use tokio::sync::Mutex; | ||
|
|
||
| // FIXME!: Think of good text | ||
| const AFTER_HELP_TEXT: &str = r#"We will write later!"#; |
I think we can split the initial setting of the firmware when the child device starts in another PR, as we still need to think about the interface here. In the meantime the child device adapter/connector can update the firmware in the |
| @@ -58,6 +58,15 @@ pub fn create_directory_with_mode(dir: impl AsRef<Path>, mode: u32) -> Result<() | |||
| perm_entry.create_directory(dir.as_ref()) | |||
| } | |||
|
|
|||
| pub fn create_file_with_mode( | |||
There was a problem hiding this comment.
I'd rather create a single create_file function that accepts the user, group and mode as Options. This single function would be used everywhere and would help us avoid having to come up with so many variants like create_file_with_mode and create_file_with_user_group for each combination.
There was a problem hiding this comment.
This can be done independently as this PR is already large.
That said I would suggest yet another direction (for later!). I agree with Albin that we cannot have a method per combination, but I also understand the need for something more explicit than a function with 3 optional arguments. So I would add with_user, with_group and with_mode method to PermissionEntry. So we could write: PermissionEntry::new().with_mode(644).create_file(path, content).
| id: op_id.to_string(), | ||
| }) | ||
| } | ||
| fn get_file_path(&self, op_id: &str) -> PathBuf { |
There was a problem hiding this comment.
| fn get_file_path(&self, op_id: &str) -> PathBuf { | |
| fn get_operation_file_path(&self, op_id: &str) -> PathBuf { |
To be more contextual.
| @@ -598,16 +607,22 @@ pub struct FirmwareOperationEntry { | |||
| } | |||
|
|
|||
| impl FirmwareOperationEntry { | |||
| fn create_file(&self) -> Result<(), FirmwareManagementError> { | |||
| let path = PersistentStore::get_file_path(&self.operation_id); | |||
| fn create_file( | |||
There was a problem hiding this comment.
It feels more like this create_file or rather create_operation_file and overwrite_file functions should have been part of the PresistentStore as creating a file in the store should have been a function of that store.
| // TODO! We should make it configurable by tedge config later. | ||
| const PERSISTENT_DIR_PATH: &str = "/var/tedge"; | ||
|
|
||
| pub const CACHE_DIR_NAME: &str = "cache"; | ||
| pub const FILE_TRANSFER_DIR_NAME: &str = "file-transfer"; | ||
| pub const PERSISTENT_STORE_DIR_NAME: &str = "firmware"; |
There was a problem hiding this comment.
These constants better belong on the firmware_manager module itself so that they are portable when we migrate this plugin to the actor model with a different main.
| const PERSISTENT_DIR_PATH: &str = "/var/tedge"; | ||
|
|
||
| pub const CACHE_DIR_NAME: &str = "cache"; | ||
| pub const FILE_TRANSFER_DIR_NAME: &str = "file-transfer"; |
There was a problem hiding this comment.
TODO for later: This constant ideally should have been defined in some tedge-api crate so that all plugins using the file-transfer functionality could reuse it.
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
1. Move the creation of 3 directories (cache, file-transfer, firmware) to `--init` option. 2. The path of `/var/tedge` is given as argument of FirmwareManager, so that test can use /tmp directory instead. 3. Remove permission check of persistent store files. 4. Stop using anyhow::Error, instead, FirmwareManagement error is used. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
rina23q
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Robot Results
Passed Tests
|
- Improve error handling - Add more integration tests Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
rina23q
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
|
I believe now this PR needs only minor fixes, therefore, change the state from Draft to Open. |
rina23q
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
rina23q
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
rina23q
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| .await?; | ||
|
|
||
| // Assert the c8y_Firmware operation status mapping to EXECUTING(501) and FAILED(502) | ||
| mqtt_tests::assert_received_all_expected( |
There was a problem hiding this comment.
Don't you need to make sure that the timeout period has elapsed before doing this assertion? With a sleep may be.
There was a problem hiding this comment.
The firmware response timeout is 1 sec. We wait 5 seconds to check if all expected message are received inside assert. I don't think we need sleep.
rina23q
force-pushed
the
feature/1696/firmware-plugin
branch
from
fa125e9
to
f98d246
Compare
rina23q
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| @@ -58,6 +58,15 @@ pub fn create_directory_with_mode(dir: impl AsRef<Path>, mode: u32) -> Result<() | |||
| perm_entry.create_directory(dir.as_ref()) | |||
| } | |||
|
|
|||
| pub fn create_file_with_mode( | |||
There was a problem hiding this comment.
This can be done independently as this PR is already large.
That said I would suggest yet another direction (for later!). I agree with Albin that we cannot have a method per combination, but I also understand the need for something more explicit than a function with 3 optional arguments. So I would add with_user, with_group and with_mode method to PermissionEntry. So we could write: PermissionEntry::new().with_mode(644).create_file(path, content).
| Ok(()) | ||
| } | ||
|
|
||
| // TODO: Move some to functions |
There was a problem hiding this comment.
I did this now with 813e745!
rina23q
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
It sounds good. I'd like to address in next PR! |
rina23q
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
rina23q
force-pushed
the
feature/1696/firmware-plugin
branch
from
f4e570e
to
150fc0b
Compare
rina23q
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
- Revert the ResponsePayload.status to non Option type. - Change the type of http_proxy as only one http_proxy is used. - handle_child_device_firmware_update_response() publishes mqtt messages directly inside the function. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
rina23q
force-pushed
the
feature/1696/firmware-plugin
branch
from
2b54971
to
c45e82c
Compare
rina23q
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
rina23q
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Proposed changes
This PR should implement as described in the ticket #1696.
The following memo might help you to understand the code better.
c8y-firmware-plugin --initcreates 3 required directories:/var/tedge/cache,/var/tedge/firmware, and/var/tedge/file-transfer. Make sure that/var/tedge/file-transfercan be also created bytedge-agentupon the first POST request./tmpfirst, then move to/var/tedge/cache. If/var/tedge/cachedoesn't exist,this plugin creates the directory in runtimereturns error./var/tedge/file-transfer/<child-id>/firmware_updatewill keep a symlink of the file.If this directory doesn't exist, this plugin creates the directory in runtime.If/var/tedge/file-transferdirectory doesn't exist, returns error./<child-id>/firmware_updatedirectory will be created in runtime.sha256field of request and response JSON is the sha256 of file itself. Checksum if they downloaded correct file.tedge config set firmware.timeout 200tedge config set firmware.child.update.timeout 200.statusfield in the response from child device is invalid but operation ID is correct, the operation status is treatedNoneinternally. From user, it will be an error.NOT in this PR's scope
c8y_Firmware. This will be created by user.TODO before merge
c8y-firmware-plugin --help.--initcreates directories.firmware.timeouttofirmware.child.update.timeout.Types of changes
Paste Link to the issue
#1696
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
Directory/file permissions under
/var/tedgeand its security./var/tedgeis created bytedge-agent --initowned bytedge:tedge./var/tedge/file-transferis created bytedge-agentowned bytedge:tedge uponPUT` request. (timing of child device bootstrapping of config plugin)/var/tedge/file-transfer/<child-id>is created bytedge-agentowned bytedge:tedgeuponPUTrequest. (timing of child device bootstrapping of config plugin)/var/tedge/file-transfer/<child-id>/config_updateis created byc8y-configuration-pluginowned byroot:root.This plugin needs them. Now c8y-firmware-plugin is supposed to be run by
tedgeuser, therefore, they will be owned bytedge:tedge. But it is a security risk as any one can modify via tedge-agent http server./var/tedge/cache/<file>/var/tedge/file-transfer/<child-id>/firmware_update/<symlink>