Add configuration operation support to c8y-mapper #2303

rina23q · 2023-09-29T01:29:09Z

Proposed changes

Todo:

Fix why te/../cmd/config.. topic is not picked up by mapper. Check subscription
Improve error management. Remove all unwraps.
Add unit tests
Fix issues found by unit tests
Delete a file from file-transfer-repo after operation is successful
Add tedge config like c8y.enable.config_update and c8y.enable.config_snapshot

Follow-up:

Add timer and timeout if operation takes too long together with log_upload cases
Update robot tests (which requires tedge-configuration-plugin package)
Refactor as suggested in comment => Add try_get() and try_get_by_external_id() to entity_store #2340
Use uploader actor once Create uploader actor #2302 is done

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#2292

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

github-actions · 2023-09-29T09:35:42Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
314	0	3	314	100	55m20.587999999s

crates/core/c8y_api/src/smartrest/download.rs

crates/extensions/c8y_mapper_ext/src/actor.rs

crates/extensions/c8y_mapper_ext/src/config_operations.rs

albinsuresh · 2023-10-03T07:05:50Z

crates/extensions/c8y_mapper_ext/src/config_operations.rs

+                        &response.config_type,
+                        external_id.as_ref().to_string(),
+                    )
+                    .await; // We need to get rid of this await, otherwise it blocks


To be replaced by a call to the config uploader actor once #2302 is done.

crates/extensions/c8y_mapper_ext/src/config_operations.rs

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

Upon receiving c8y_UploadConfigFile request, c8y-mapper does * create a new config_snapshot command * upload a config file to c8y on successful state of config_snapshot cmd Upon receiving c8y_DownloadConfigFile request, c8y-mapper does * download a target file from remote if not available in cache directory * create a new config_update command * remove a symlink on successful state of config_update cmd For both matadata, c8y-mapper sends supported config types message and supported operations to c8y Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

codecov · 2023-10-11T18:02:00Z

Codecov Report

Merging #2303 (6aa7b9e) into main (7509cbf) will increase coverage by 0.3%.
Report is 7 commits behind head on main.
The diff coverage is 81.6%.

Additional details and impacted files

Files	Coverage Δ
crates/common/tedge_config_macros/impl/src/lib.rs	`95.2% <100.0%> (ø)`
...rates/common/tedge_config_macros/impl/src/query.rs	`95.2% <100.0%> (+1.7%)`	⬆️
crates/extensions/c8y_mapper_ext/src/error.rs	`0.0% <ø> (ø)`
crates/extensions/c8y_mapper_ext/src/lib.rs	`87.5% <100.0%> (+4.1%)`	⬆️
crates/extensions/c8y_mapper_ext/src/log_upload.rs	`77.0% <100.0%> (+0.1%)`	⬆️
...common/tedge_config_macros/impl/src/input/parse.rs	`39.7% <0.0%> (-0.6%)`	⬇️
...mon/tedge_config_macros/impl/src/input/validate.rs	`94.3% <94.7%> (+0.2%)`	⬆️
...re/c8y_api/src/smartrest/smartrest_deserializer.rs	`93.4% <0.0%> (-0.2%)`	⬇️
crates/core/tedge_api/src/messages.rs	`92.2% <0.0%> (-0.2%)`	⬇️
.../tedge_config/src/tedge_config_cli/tedge_config.rs	`81.8% <75.0%> (-8.0%)`	⬇️
... and 6 more

... and 12 files with indirect coverage changes

Slash is reserved for file system, therefore, using '/' inside tedgeUrl causes creating unexpected directories in the file transfer repository. Since '/' is still valid as config type, replacing slashes only in paths Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

reubenmiller

We're going to have to include the packaging and system tests in this PR so that we avoid publishing a tedge-mapper which sends the configuration operation to a component that does not exist. So even though the additional packaging changes are not directly related to the PR, I don't see any easy way to merge this PR without fully switching over from c8y-configuration-plugin to tedge-configuration-plugin.

I've started preparing the packaging/updated system tests, however the following still needs to be done in order to assist:

Add a lib.rs file and call it from the main.rs file (similar to tedge_log_plugin/src/main.rs) - Covered by refactor to use lib file to enable calling from the multi-call binary #2339
Add a build.rs file - Covered by fix: add missing build.rs script to use the auto generated git based versioning #2338 - so this PR can be rebased on top
~~Add the tedge-configuration-plugin to the multi-call binary setup in tedge~~ N/A as a follow up PR can do this.

Update

I just saw that there are feature flags included in this PR to disable this feature until we're ready with the packaging. So I don't have any problem merging.

reubenmiller

Approved

crates/extensions/c8y_mapper_ext/src/actor.rs

crates/extensions/c8y_mapper_ext/src/config.rs

crates/extensions/c8y_mapper_ext/src/converter.rs

albinsuresh · 2023-10-12T07:11:21Z

crates/extensions/c8y_mapper_ext/src/config_operations.rs

+            .get_by_external_id(&snapshot_request.device.clone().into())
+            .ok_or_else(|| UnknownDevice {
+                device_id: snapshot_request.device.to_string(),
+            })?;


I see this pattern repeating a lot these days (for restart, log_upload etc) and this will just increase. May be it's time to add try_get_by_external_id and try_get API counterparts which will return that Err<UnknownDevice> result instead of None as returned by the existing APIs. It can be done in a follow-up PR though.

Addressed in the PR #2340

albinsuresh · 2023-10-12T07:31:05Z

crates/extensions/c8y_mapper_ext/src/config_operations.rs

+            "http://{}/tedge/file-transfer/{}/config_snapshot/{}-{}",
+            &self.config.tedge_http_host,
+            external_id,
+            snapshot_request.config_type.replace('/', ":"),


With this, there is the risk of different config types abc:xyz and abc/xyz conflicting. The snapshot of the first file might get overwritten by the second one. As long as our request processing is serial, this many not be an issue. But still, why not allow those extra dirs when the type has / in it? What's the harm?

Our config_type allows to use / (it's even a kind of default when user doesn't give specific type, then using file path as type). However, since / is considered directory separator, so somehow need to escape. Then I introduce replacing with : .

If we don't escape, when plugin uploads to http://localhost:8888/tedge/file-transfer/test-device/config_snapshot//etc/tedge/tedge.toml-1234 for the type /etc/tedge/tedge.toml, the file will be stored /var/tedge/test-device/config_snapshot/etc/tedge/tedge.toml-1234. Too many directories.

crates/extensions/c8y_mapper_ext/src/config_operations.rs

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

albinsuresh

LGTM

albinsuresh · 2023-10-12T10:13:06Z

crates/core/c8y_api/src/smartrest/smartrest_deserializer.rs

-pub enum SmartRestVariant {
-    SmartRestLogRequest,
+#[derive(Debug)]
+pub enum SmartRestOperationVariant {


Suggested change

pub enum SmartRestOperationVariant {

pub enum SmartRestOperation {

As commented in thin-edge#2303#discussion_r1356364285, we have many patterns using get() or get_by_external_id() and map them None to Err. The new APIs returns Error::UnknownEntity instead of None. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

rina23q temporarily deployed to Test Pull Request September 29, 2023 01:40 — with GitHub Actions Inactive

rina23q had a problem deploying to Test Pull Request October 2, 2023 18:51 — with GitHub Actions Failure

rina23q temporarily deployed to Test Pull Request October 2, 2023 22:09 — with GitHub Actions Inactive

albinsuresh reviewed Oct 3, 2023

View reviewed changes

rina23q added 2 commits October 11, 2023 13:27

Remove file_transfer_dir from C8yMapperConfig

7aa0dc2

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

rina23q force-pushed the feature/2098/c8y-mapper-for-config-ops branch from f93b9f1 to a8830b1 Compare October 11, 2023 17:42

rina23q had a problem deploying to Test Pull Request October 11, 2023 17:55 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request October 11, 2023 20:09 — with GitHub Actions Failure

rina23q marked this pull request as ready for review October 11, 2023 20:35

rina23q force-pushed the feature/2098/c8y-mapper-for-config-ops branch 2 times, most recently from 9c3decd to 406aece Compare October 11, 2023 20:46

rina23q added 2 commits October 11, 2023 22:47

Add tedge config to enable/disable config features on c8y-mapper

26b0c72

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

rina23q force-pushed the feature/2098/c8y-mapper-for-config-ops branch from 406aece to 26b0c72 Compare October 11, 2023 20:47

rina23q had a problem deploying to Test Pull Request October 11, 2023 21:00 — with GitHub Actions Failure

reubenmiller requested changes Oct 12, 2023

View reviewed changes

rina23q temporarily deployed to Test Pull Request October 12, 2023 02:21 — with GitHub Actions Inactive

reubenmiller approved these changes Oct 12, 2023

View reviewed changes

albinsuresh reviewed Oct 12, 2023

View reviewed changes

rina23q added 2 commits October 12, 2023 11:45

Add SmartRestOperationVariant enum

2d8085f

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

Apply SmartRestOperationVariant to c8y-mapper and fix topic filters

6aa7b9e

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

rina23q temporarily deployed to Test Pull Request October 12, 2023 10:03 — with GitHub Actions Inactive

albinsuresh approved these changes Oct 12, 2023

View reviewed changes

rina23q mentioned this pull request Oct 12, 2023

Update c8y-mapper to handle config commands #2292

Closed

3 tasks

rina23q merged commit 1dffbb6 into thin-edge:main Oct 12, 2023
18 checks passed

rina23q deleted the feature/2098/c8y-mapper-for-config-ops branch October 12, 2023 10:50

rina23q mentioned this pull request Oct 12, 2023

Add try_get() and try_get_by_external_id() to entity_store #2340

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration operation support to c8y-mapper #2303

Add configuration operation support to c8y-mapper #2303

rina23q commented Sep 29, 2023 •

edited

github-actions bot commented Sep 29, 2023 •

edited

albinsuresh Oct 3, 2023

codecov bot commented Oct 11, 2023 •

edited

reubenmiller left a comment •

edited

reubenmiller left a comment

albinsuresh Oct 12, 2023

rina23q Oct 12, 2023

albinsuresh Oct 12, 2023

rina23q Oct 12, 2023

albinsuresh left a comment

albinsuresh Oct 12, 2023

	pub enum SmartRestOperationVariant {
	pub enum SmartRestOperation {

Add configuration operation support to c8y-mapper #2303

Add configuration operation support to c8y-mapper #2303

Conversation

rina23q commented Sep 29, 2023 • edited

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

github-actions bot commented Sep 29, 2023 • edited

Robot Results

albinsuresh Oct 3, 2023

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2023 • edited

Codecov Report

reubenmiller left a comment • edited

Choose a reason for hiding this comment

reubenmiller left a comment

Choose a reason for hiding this comment

albinsuresh Oct 12, 2023

Choose a reason for hiding this comment

rina23q Oct 12, 2023

Choose a reason for hiding this comment

albinsuresh Oct 12, 2023

Choose a reason for hiding this comment

rina23q Oct 12, 2023

Choose a reason for hiding this comment

albinsuresh left a comment

Choose a reason for hiding this comment

albinsuresh Oct 12, 2023

Choose a reason for hiding this comment

rina23q commented Sep 29, 2023 •

edited

github-actions bot commented Sep 29, 2023 •

edited

codecov bot commented Oct 11, 2023 •

edited

reubenmiller left a comment •

edited