C8y mapper handling c8y_LogfileRequest #2108

albinsuresh · 2023-07-28T09:00:13Z

Proposed changes

Todo (in this PR's scope)

C8y mapper todo the mapping of c8y_LogfileRequest to tedge log_upload command
C8y mapper converts supported log types message to c8y format
Supported operation file creation when it receives log_upload metadata topic
Combine file upload logic for both log and config -> Move to Refactoring c8y upload file API and renaming of LogRequest struct #2203 as it's refactoring of the existing code
Clear the retain messages after operations finishes
Dedicated actor for http uploads to avoid blocking the mapper actor

Todo out of this PR

Rename the filed maximumLines to lines in specs, both tedge-log-plugin and c8y-mapper. Update c8y-mapper and tedge-log-plugin specs #2127
Write a robot test for tedge-log-plugin and new c8y-mapper

Much later (not in the drop 1 scope)

Streaming file upload from C8yHttpProxy actor

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

#2048

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

codecov · 2023-07-28T09:15:18Z

Codecov Report

Merging #2108 (eb82107) into main (e8881a3) will decrease coverage by 0.8%.
Report is 17 commits behind head on main.
The diff coverage is 76.2%.

Additional details and impacted files

Files Changed	Coverage Δ
crates/core/tedge_api/src/lib.rs	`100.0% <ø> (ø)`
crates/core/tedge_api/src/topic.rs	`94.5% <ø> (ø)`
crates/extensions/c8y_mapper_ext/src/error.rs	`0.0% <ø> (ø)`
crates/extensions/c8y_mapper_ext/src/serializer.rs	`81.1% <0.0%> (-0.4%)`	⬇️
crates/extensions/tedge_log_manager/src/lib.rs	`88.6% <ø> (ø)`
crates/extensions/c8y_mapper_ext/src/config.rs	`56.6% <40.7%> (-5.9%)`	⬇️
crates/core/tedge_api/src/mqtt_topics.rs	`77.9% <51.4%> (+2.4%)`	⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs	`78.2% <70.5%> (+0.1%)`	⬆️
crates/extensions/tedge_log_manager/src/actor.rs	`69.7% <70.5%> (-6.2%)`	⬇️
crates/extensions/tedge_log_manager/src/tests.rs	`92.2% <71.4%> (+0.3%)`	⬆️
... and 9 more

... and 14 files with indirect coverage changes

github-actions · 2023-07-28T14:34:40Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
257	0	5	257	100	46m27.899s

didier-wenzek · 2023-08-08T12:27:24Z

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

@@ -241,7 +241,7 @@ pub struct SmartRestLogRequest {
    pub date_from: OffsetDateTime,
    #[serde(deserialize_with = "to_datetime")]
    pub date_to: OffsetDateTime,
-    pub needle: Option<String>,
+    pub search_text: Option<String>,


;-) The needle term is a legacy from unix naming

crates/core/tedge_api/src/cmd_topic.rs

crates/core/tedge_api/src/messages.rs

crates/core/tedge_mapper/Cargo.toml

didier-wenzek

I'm considering to close this PR and to start from scratch.must be remo

Indeed:

the feature flag must be replaced by a runtime flag.
the definition of the log request must be aligned with Create generic tedge log plugin #2086
the mapping to entity topics must be aligned with Digital twin #2122.

crates/extensions/c8y_mapper_ext/src/log_upload.rs

crates/core/tedge_api/src/messages.rs

crates/extensions/c8y_mapper_ext/src/actor.rs

crates/core/tedge_mapper/Cargo.toml

didier-wenzek · 2023-08-23T15:51:45Z

This commit 70d6c77 removes duplicated work on this PR and #2086.

Roughly, all the structures introduced in tedge_log_manager::json.rs have been deprecated in favor of those introduced by this PR in tedge_api::messages.rs.

@Ruadhri17 can you please review this commit 70d6c77

crates/extensions/tedge_log_manager/src/json.rs

crates/extensions/c8y_http_proxy/src/actor.rs

crates/extensions/c8y_http_proxy/src/handle.rs

didier-wenzek

To be done before merge:

Load the c8y mapper capability configuration from disk or the command line.
See 69e0e72

To be done as follow up tasks:

Dedicated actor for http uploads to avoid blocking the mapper actor
Load the topic root prefix from the configuration (instead of taking te as a default).
Update tedge_log_plugin to build topics using MqttSchema.

didier-wenzek · 2023-08-31T17:05:57Z

crates/extensions/c8y_mapper_ext/Cargo.toml

@@ -17,6 +17,7 @@ camino = { workspace = true }
 clock = { workspace = true }
 json-writer = { workspace = true }
 logged_command = { workspace = true }
+nanoid = { workspace = true }


We can consider to move the generation of a unique id for a command in MqttSchema as this will have to be done for all command types.

didier-wenzek

Approved.

A new setting has been added to enable log_management based on tedge-log-plugin.

Till c8y-log-plugin is deprecated, one has to explicitly turn on the log management capability before starting the mapper.

$ sudo tedge config set c8y.enable.log_management true
$ tegde-mapper c8y &
$ tedge-log-plugin &

rina23q

Tested and no bugs, so approve it.

Note for testing with child devices

I got confused because I was unsure which component would create a new child device on cloud. What I understood after testing now:

Running tedge-log-plugin with --device device/<child-name>// creates a child device directory <child-name> under /etc/tedge/operations/c8y. Also, it creates a supported operation file c8y_LogfileRequest under the directory.
So, c8y-mapper creates a new child device and declare supported operation accordingly. It is nothing new, our conventional way for supporting child devices.

What I misunderstood is that I had to send an MQTT message described here to create a child device. Unfortunately, it is not yet supported as c8y-mapper does not subscribe to te/+/+ topics.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

- Remove the feature flag `log_upload`. One should not have to recompile to opt-in or opt-out a capability - Introduce a struct with a flag per capability. The plan is to extend this struct with all the features a user might want to opt-in. - This record of capabilities enabled by the user will be load from disk on start-up. This has not been done yet. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

tedge-log-plugin were processing all commands twice, on the init state as well as on the executing state. With this fix log-tedge-plugin publish an executing event on init and only starts executing when receiving the message sent by itself. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

tedge-log-plugin were parsing the empty message sent on the command topics to clear the associated command, leading to incorrect error messages: ERROR tedge_log_manager::actor: Incorrect log request payload: Failed to parse response with: EOF while parsing a value at line 1 column 0 Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Till c8y-log-plugin is deprecated, one has to explicitly turn on the log management capability before starting the mapper. ``` $ sudo tedge config set c8y.enable.log_management true $ tegde-mapper c8y & $ tedge-log-plugin & ``` Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

reubenmiller

Approved. Though I discovered a few problems with the tedge-log-plugin (#2212), but that is not the scope of this PR.

reubenmiller · 2023-09-03T12:42:45Z

After doing some manual testing:

Thank you.
3. There are some issues with reloading types.  However, that issue was also present in `c8y-log-plugin`. When `tedge-log-plugin.toml` is edited, plugin reload types few times (in my case 3):
Yes, we still have to find a way to avoid extra events for a single file update.

Additional comment: We had some changes in tracing level, for example in c8y-log-plugin we use DEBUG log level for debug flag whereas in tedge-log-plugin we still use TRACE.

Yes, it would be good to fix that, and use DEBUG.

This is covered in #2212

reubenmiller · 2023-09-04T02:39:58Z

Integration tests using the new tedge-log-plugin are included in #2213

tedgeUrl is now using the device name instead of "main" for the main device. This change is made on the PR thin-edge#2108. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

tedgeUrl is now using the device name instead of "main" for the main device. This change is made on the PR #2108. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com> Co-authored-by: Didier Wenzek <didier.wenzek@free.fr>

albinsuresh had a problem deploying to Test Pull Request July 28, 2023 09:10 — with GitHub Actions Failure

albinsuresh force-pushed the c8y-mapper-ops-handling branch from da5c7b7 to 68c3036 Compare July 28, 2023 13:50

albinsuresh had a problem deploying to Test Pull Request July 28, 2023 14:01 — with GitHub Actions Failure

rina23q temporarily deployed to Test Pull Request July 28, 2023 14:13 — with GitHub Actions Inactive

rina23q changed the title ~~C8y mapper handling c8y_LogfileRequest #2017~~ C8y mapper handling c8y_LogfileRequest Jul 28, 2023

rina23q mentioned this pull request Jul 28, 2023

Make log plugin usable for different clouds #2048

Closed

6 tasks

rina23q force-pushed the c8y-mapper-ops-handling branch from cd2e7d5 to 68c3036 Compare July 28, 2023 16:26

rina23q had a problem deploying to Test Pull Request July 28, 2023 16:37 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request August 2, 2023 20:46 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request August 3, 2023 21:19 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request August 3, 2023 21:42 — with GitHub Actions Failure

rina23q mentioned this pull request Aug 8, 2023

Create generic tedge log plugin #2086

Merged

13 tasks

rina23q force-pushed the c8y-mapper-ops-handling branch from ed0f8a4 to 6f13c4e Compare August 8, 2023 13:15

rina23q temporarily deployed to Test Pull Request August 8, 2023 13:25 — with GitHub Actions Inactive

didier-wenzek reviewed Aug 8, 2023

View reviewed changes

didier-wenzek temporarily deployed to Test Pull Request August 8, 2023 16:21 — with GitHub Actions Inactive

didier-wenzek force-pushed the c8y-mapper-ops-handling branch from 8add896 to 921e61e Compare August 14, 2023 12:53

didier-wenzek temporarily deployed to Test Pull Request August 14, 2023 13:06 — with GitHub Actions Inactive

didier-wenzek requested changes Aug 14, 2023

View reviewed changes

didier-wenzek force-pushed the c8y-mapper-ops-handling branch from 921e61e to 422ec18 Compare August 17, 2023 08:16

didier-wenzek temporarily deployed to Test Pull Request August 17, 2023 08:29 — with GitHub Actions Inactive

didier-wenzek force-pushed the c8y-mapper-ops-handling branch from 422ec18 to a9c25f4 Compare August 23, 2023 10:14

didier-wenzek temporarily deployed to Test Pull Request August 23, 2023 10:28 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Pull Request August 23, 2023 16:01 — with GitHub Actions Inactive

didier-wenzek reviewed Aug 23, 2023

View reviewed changes

crates/extensions/tedge_log_manager/src/json.rs Outdated Show resolved Hide resolved

didier-wenzek reviewed Aug 23, 2023

View reviewed changes

crates/extensions/c8y_http_proxy/src/actor.rs Outdated Show resolved Hide resolved

didier-wenzek reviewed Aug 23, 2023

View reviewed changes

crates/extensions/c8y_http_proxy/src/handle.rs Outdated Show resolved Hide resolved

didier-wenzek reviewed Aug 31, 2023

View reviewed changes

didier-wenzek had a problem deploying to Test Pull Request September 1, 2023 08:19 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request September 1, 2023 09:02 — with GitHub Actions Inactive

didier-wenzek approved these changes Sep 1, 2023

View reviewed changes

Bravo555 approved these changes Sep 1, 2023

View reviewed changes

rina23q approved these changes Sep 1, 2023

View reviewed changes

rina23q and others added 12 commits September 1, 2023 15:35

Add tests and feature flag for log_upload

a8d07fa

Clear the retain message once the operation is done

36d239a

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Let c8y mapper use the entity store for log commands

b6af895

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Add enum OperationType

b1584e1

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Add MqttSchema::topics returning specific topics

1967e25

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

impl Into<TopicFilter> for Vec<TopicFilter>

72dbd1a

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Let the c8y mapper use tedge_api::MqttSchema

9f269a8

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Add doc comments to HttpSchema

fd7dba3

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the c8y-mapper-ops-handling branch from 52f6572 to eb82107 Compare September 1, 2023 13:40

didier-wenzek temporarily deployed to Test Pull Request September 1, 2023 13:53 — with GitHub Actions Inactive

reubenmiller approved these changes Sep 3, 2023

View reviewed changes

didier-wenzek merged commit 47baff1 into thin-edge:main Sep 3, 2023

rina23q mentioned this pull request Sep 4, 2023

Update tedge plugin specs after c8y-mapper change #2216

Merged

11 tasks

albinsuresh deleted the c8y-mapper-ops-handling branch September 12, 2023 08:34

didier-wenzek mentioned this pull request Sep 15, 2023

Restart operation on child-devices #2245

Merged

17 tasks

Ruadhri17 mentioned this pull request Sep 29, 2023

Parse log plugin topics using new mqtt schema #2305

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C8y mapper handling c8y_LogfileRequest #2108

C8y mapper handling c8y_LogfileRequest #2108

albinsuresh commented Jul 28, 2023 •

edited by rina23q

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

github-actions bot commented Jul 28, 2023 •

edited

Loading

didier-wenzek Aug 8, 2023

didier-wenzek left a comment •

edited

Loading

didier-wenzek commented Aug 23, 2023

didier-wenzek left a comment •

edited

Loading

didier-wenzek Aug 31, 2023

didier-wenzek left a comment

rina23q left a comment

reubenmiller left a comment •

edited

Loading

reubenmiller commented Sep 3, 2023

reubenmiller commented Sep 4, 2023

C8y mapper handling c8y_LogfileRequest #2108

C8y mapper handling c8y_LogfileRequest #2108

Conversation

albinsuresh commented Jul 28, 2023 • edited by rina23q Loading

Proposed changes

Todo (in this PR's scope)

Todo out of this PR

Much later (not in the drop 1 scope)

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

github-actions bot commented Jul 28, 2023 • edited Loading

Robot Results

didier-wenzek Aug 8, 2023

Choose a reason for hiding this comment

didier-wenzek left a comment • edited Loading

Choose a reason for hiding this comment

didier-wenzek commented Aug 23, 2023

didier-wenzek left a comment • edited Loading

Choose a reason for hiding this comment

didier-wenzek Aug 31, 2023

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

rina23q left a comment

Choose a reason for hiding this comment

Note for testing with child devices

reubenmiller left a comment • edited Loading

Choose a reason for hiding this comment

reubenmiller commented Sep 3, 2023

reubenmiller commented Sep 4, 2023

albinsuresh commented Jul 28, 2023 •

edited by rina23q

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

github-actions bot commented Jul 28, 2023 •

edited

Loading

didier-wenzek left a comment •

edited

Loading

didier-wenzek left a comment •

edited

Loading

reubenmiller left a comment •

edited

Loading