fix: Include software plugin output in workflow log #2895

albinsuresh · 2024-05-22T14:21:20Z

Proposed changes

Include software plugin logs in their respective workflow logs
Update the log location of software-management log type in tedge-log-plugin.toml to point to the workflow logs
~~Handle this log type path change during upgrade~~: Won't do

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

#2835

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

crates/core/plugin_sm/src/plugin.rs

codecov · 2024-05-22T14:32:21Z

Codecov Report

Attention: Patch coverage is 70.49180% with 126 lines in your changes are missing coverage. Please review.

Project coverage is 77.9%. Comparing base (15b7e26) to head (a507d7a).
Report is 16 commits behind head on main.

Additional details and impacted files

Files	Coverage Δ
crates/common/tedge_utils/src/signals.rs	`66.6% <100.0%> (+66.6%)`	⬆️
...tedge_agent/src/tedge_operation_converter/actor.rs	`39.9% <ø> (-10.5%)`	⬇️
...tedge_agent/src/tedge_operation_converter/tests.rs	`94.7% <100.0%> (+0.2%)`	⬆️
crates/core/tedge_api/src/lib.rs	`100.0% <ø> (ø)`
crates/core/tedge_api/src/workflow/mod.rs	`60.5% <ø> (ø)`
crates/core/tedge_api/src/workflow/supervisor.rs	`73.8% <ø> (ø)`
crates/extensions/tedge_log_manager/src/lib.rs	`78.4% <100.0%> (ø)`
crates/extensions/tedge_log_manager/src/tests.rs	`90.8% <100.0%> (+<0.1%)`	⬆️
crates/core/plugin_sm/src/log_file.rs	`66.6% <66.6%> (-25.0%)`	⬇️
crates/extensions/c8y_mapper_ext/src/converter.rs	`83.7% <90.0%> (+<0.1%)`	⬆️
... and 7 more

... and 4 files with indirect coverage changes

github-actions · 2024-05-22T14:48:19Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
452	0	3	452	100	1h16m48.587524999s

crates/common/logged_command/Cargo.toml

albinsuresh · 2024-05-28T15:03:35Z

crates/extensions/tedge_log_manager/src/tests.rs

@@ -149,7 +149,10 @@ async fn default_plugin_config() {
 read_to_string(tempdir.path().join("tedge-log-plugin.toml")).unwrap();
 let plugin_config_toml: Table = from_str(&plugin_config_content).unwrap();

- let agent_logs_path = format!("{}/agent/software-*", tempdir.path().to_string_lossy());
+ let agent_logs_path = format!(
+ "{}/agent/workflow-software_*",


In an offline discussion with @reubenmiller, we agreed to keep this breaking change in the target file path and stop logging to the former location.

On a related note, we were wondering if we could drop the workflow- prefix from all the log files, since we we're not writing any other kind of log file into this directory anymore. But, this is to be discussed and done in a different PR.

crates/core/tedge_api/Cargo.toml

albinsuresh · 2024-05-31T13:00:56Z

crates/core/tedge_api/src/workflow/log/command_log.rs

+ self.log_command_and_output("", result).await
+ }
+
+ pub async fn log_command_and_output(


This function is the only new addition. Rest was just moved.

didier-wenzek

Approved with a suggestion for the dependency issue on nix.

As a personal point of view, I see the produced log file a bit more difficult to read.
In the following excerpt, one has to understand that there are 3 levels of execution

The software_update operation is running the executing step.
This triggers /usr/bin/sudo "/etc/tedge/sm-plugins/apt" "prepare"
In turn, this triggers apt-get" "--quiet" "--yes" "update

----------------------[ software_update @ executing | time=2024-05-31T14:56:07.268064944Z ]----------------------

State:    {"logPath":"/var/log/tedge/agent/workflow-software_update-c8y-mapper-44202.log","status":"executing","updateList":[{"modules":[{"action":"install","name":"rolldice","version":"::apt"}],"type":"
apt"}]}

Action:   builtin

----- $ /usr/bin/sudo "/etc/tedge/sm-plugins/apt" "prepare"
Exit status: 0 (OK)

stderr (EMPTY)

stdout <<EOF
Executing command: "apt-get" "--quiet" "--yes" "update"
Hit:1 http://fr.archive.ubuntu.com/ubuntu jammy InRelease
Get:2 http://fr.archive.ubuntu.com/ubuntu jammy-updates InRelease [128 kB]
Get:3 https://packages.microsoft.com/repos/ms-teams stable InRelease [5 931 B]

didier-wenzek · 2024-05-31T14:31:08Z

crates/core/plugin_sm/src/plugin.rs

- async fn prepare(&self, logger: &mut BufWriter<File>) -> Result<(), SoftwareError>;
+ async fn prepare(&self, command_log: Option<&mut CommandLog>) -> Result<(), SoftwareError>;


What's the motivation for making optional all the command loggers?

To cover the case where the logPath is not provided in the payload of the command (since it is optional) when an external component has overridden the workflows, but still using the builtin actions for the actual installation.

I now understand that it is not easy for simple overrides of the workflow to remove an existing field from the payload, as the workflow system only appends/updates the values to the existing payload. But I'm not fully sure if it is completely impossible to guarantee that the logPath would always be there, as long as the tedge-agent at least initiates the workflow execution.

crates/core/tedge_api/Cargo.toml

albinsuresh · 2024-06-03T07:26:39Z

As a personal point of view, I see the produced log file a bit more difficult to read.

At least to reduce one level of information overload, I could get rid of those Executing command: statements. But TBH, I personally find that info useful.

Instead, does grouping the output of the 3-subcommands: prepare, update-list and finalize with >>> and <<< markers, along with clear terminal demarcation of ----- for those individual commands, as shown below improve things? At least to convey that this single builtin action consists of multiple command executions?

@reubenmiller Any suggestions?

----------------------[ software_update @ executing | time=2024-06-03T07:09:32.619545328Z ]----------------------

State:    {"logPath":"/var/log/tedge/agent/workflow-software_update-c8y-mapper-465352.log","status":"executing","updateList":[{"modules":[{"action":"install","name":"c8y-remote-access-plugin"}],"type":"default"}]}

Action:   builtin

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

----- $ /usr/bin/sudo "/etc/tedge/sm-plugins/apt" "prepare"
Exit status: 0 (OK)

stderr (EMPTY)

stdout <<EOF
Executing command: "apt-get" "--quiet" "--yes" "update"
Hit:1 http://deb.debian.org/debian bookworm InRelease
Get:2 http://deb.debian.org/debian bookworm-updates InRelease [55.4 kB]
Get:3 http://deb.debian.org/debian-security bookworm-security InRelease [48.0 kB]
Get:4 http://deb.debian.org/debian-security bookworm-security/main amd64 Packages [157 kB]
Fetched 260 kB in 0s (576 kB/s)
Reading package lists...
EOF
-----

----- $ /usr/bin/sudo "/etc/tedge/sm-plugins/apt" "update-list"
Exit status: 0 (OK)

stderr (EMPTY)

stdout <<EOF
Executing command: "apt-get" "--quiet" "--yes" "-o" "DPkg::Options::=--force-confold" "install" "--allow-downgrades" "--no-install-recommends" "c8y-remote-access-plugin"
Reading package lists...
Building dependency tree...
Reading state information...
c8y-remote-access-plugin is already the newest version (1.0.2~319+gae8dd22).
0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
EOF
-----

----- $ /usr/bin/sudo "/etc/tedge/sm-plugins/apt" "finalize"
Exit status: 0 (OK)

stderr (EMPTY)

stdout <<EOF
Executing command: "apt-get" "--quiet" "--yes" "autoremove"
Reading package lists...
Building dependency tree...
Reading state information...
0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
EOF
-----

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

----------------------[ software_update @ successful | time=2024-06-03T07:09:35.461668921Z ]----------------------

…plugin.toml

albinsuresh requested review from didier-wenzek, jarhodes314, rina23q, a team and reubenmiller as code owners May 22, 2024 14:21

albinsuresh temporarily deployed to Test Pull Request May 22, 2024 14:21 — with GitHub Actions Inactive

albinsuresh commented May 22, 2024

View reviewed changes

crates/core/plugin_sm/src/plugin.rs Outdated Show resolved Hide resolved

albinsuresh temporarily deployed to Test Auto May 22, 2024 14:36 — with GitHub Actions Inactive

didier-wenzek reviewed May 23, 2024

View reviewed changes

crates/common/logged_command/Cargo.toml Outdated Show resolved Hide resolved

albinsuresh commented May 28, 2024

View reviewed changes

albinsuresh temporarily deployed to Test Pull Request May 31, 2024 10:51 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto May 31, 2024 10:57 — with GitHub Actions Inactive

albinsuresh force-pushed the fix/2835/unified-software-mgmt-logs branch from 5f76e27 to 4c49128 Compare May 31, 2024 12:32

albinsuresh temporarily deployed to Test Pull Request May 31, 2024 12:32 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto May 31, 2024 12:39 — with GitHub Actions Inactive

albinsuresh requested a review from didier-wenzek May 31, 2024 12:55

albinsuresh commented May 31, 2024

View reviewed changes

crates/core/tedge_api/Cargo.toml Outdated Show resolved Hide resolved

albinsuresh commented May 31, 2024

View reviewed changes

albinsuresh temporarily deployed to Test Pull Request May 31, 2024 13:14 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto May 31, 2024 15:38 — with GitHub Actions Inactive

didier-wenzek approved these changes May 31, 2024

View reviewed changes

albinsuresh temporarily deployed to Test Pull Request June 3, 2024 06:50 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto June 3, 2024 06:56 — with GitHub Actions Failure

albinsuresh added 3 commits June 3, 2024 10:12

fix(thin-edge#2835): Include software plugin output in workflow log

8acc105

Use workflow log paths for software-management log type in tedge-log-…

4fbf29f

…plugin.toml

Move command log structs into tedge_api

a507d7a

albinsuresh force-pushed the fix/2835/unified-software-mgmt-logs branch from fb963e1 to a507d7a Compare June 3, 2024 10:12

albinsuresh temporarily deployed to Test Pull Request June 3, 2024 10:13 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto June 3, 2024 10:23 — with GitHub Actions Inactive

albinsuresh added this pull request to the merge queue Jun 3, 2024

Merged via the queue into thin-edge:main with commit dfc31ed Jun 3, 2024
33 checks passed

albinsuresh deleted the fix/2835/unified-software-mgmt-logs branch June 4, 2024 03:29

reubenmiller added the theme:software Theme: Software management label Jun 6, 2024

reubenmiller changed the title ~~fix(#2835): Include software plugin output in workflow log~~ fix: Include software plugin output in workflow log Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Include software plugin output in workflow log #2895

fix: Include software plugin output in workflow log #2895

albinsuresh commented May 22, 2024 •

edited

codecov bot commented May 22, 2024 •

edited

github-actions bot commented May 22, 2024 •

edited

albinsuresh May 28, 2024

albinsuresh May 31, 2024

didier-wenzek left a comment

didier-wenzek May 31, 2024

albinsuresh Jun 3, 2024 •

edited

albinsuresh commented Jun 3, 2024

		async fn prepare(&self, logger: &mut BufWriter<File>) -> Result<(), SoftwareError>;
		async fn prepare(&self, command_log: Option<&mut CommandLog>) -> Result<(), SoftwareError>;

fix: Include software plugin output in workflow log #2895

fix: Include software plugin output in workflow log #2895

Conversation

albinsuresh commented May 22, 2024 • edited

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented May 22, 2024 • edited

Codecov Report

github-actions bot commented May 22, 2024 • edited

Robot Results

albinsuresh May 28, 2024

Choose a reason for hiding this comment

albinsuresh May 31, 2024

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

didier-wenzek May 31, 2024

Choose a reason for hiding this comment

albinsuresh Jun 3, 2024 • edited

Choose a reason for hiding this comment

albinsuresh commented Jun 3, 2024

albinsuresh commented May 22, 2024 •

edited

codecov bot commented May 22, 2024 •

edited

github-actions bot commented May 22, 2024 •

edited

albinsuresh Jun 3, 2024 •

edited