Auto upload operation log on completion #2761

[albinsuresh]

Proposed changes

Auto-upload the workflow log file on operation execution. It is applicable for all the operations going through workflow system. The log path is added to the command state by the agent when it starts processing the operation (init -> scheduled). When the operation completes, the log path added by the agent is read by the converter and that file is uploaded to the cloud.

Types of changes

• [ ] Bugfix (non-breaking change which fixes an issue)
• [x] 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

Checklist

• [x] I have read the CONTRIBUTING doc
• [x] 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

[reubenmiller]

Since the other operations don't have any such logs to be uploaded, the feature currently works only for software update operation.

Though can't we still add this as a generic feature, and just for a non-zero file size before uploading it, skipping the upload if the file size is zero? That the feature will work for all operation once they actually log something.

[albinsuresh]

Though can't we still add this as a generic feature, and just for a non-zero file size before uploading it, skipping the upload if the file size is zero? That the feature will work for all operation once they actually log something.

That was my desire too, but unfortunately I couldn't have that code at one place at a generic command level, due to the way the command payload parsing is done separately for each command type, for lack of a common payload structure between commands. So, I ended up creating a reusable operation-agnostic function that can be called from any operation handler once they start logging something, but currently only used by software update handler.

[codecov[bot]]

Codecov Report

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

Project coverage is 78.0%. Comparing base (1893d1f) to head (644cd6f). Report is 17 commits behind head on main.

Additional details and impacted files

Files	Coverage Δ
...tes/core/tedge_agent/src/software_manager/tests.rs	94.4% <100.0%> (+<0.1%)	:arrow_up:
crates/core/tedge_api/src/workflow/state.rs	69.5% <100.0%> (+3.6%)	:arrow_up:
crates/extensions/c8y_mapper_ext/src/error.rs	66.6% <ø> (ø)
...es/extensions/c8y_mapper_ext/src/operations/mod.rs	92.5% <ø> (ø)
...tedge_agent/src/tedge_operation_converter/actor.rs	53.9% <50.0%> (+1.2%)	:arrow_up:
crates/extensions/c8y_mapper_ext/src/tests.rs	92.6% <98.6%> (+<0.1%)	:arrow_up:
.../tedge_config/src/tedge_config_cli/tedge_config.rs	76.5% <66.6%> (-0.1%)	:arrow_down:
crates/extensions/c8y_mapper_ext/src/config.rs	47.7% <50.0%> (+0.3%)	:arrow_up:
crates/extensions/c8y_mapper_ext/src/actor.rs	82.3% <83.3%> (+0.5%)	:arrow_up:
...ons/c8y_mapper_ext/src/operations/config_update.rs	92.8% <85.0%> (+0.2%)	:arrow_up:
... and 8 more

... and 18 files with indirect coverage changes

[github-actions[bot]]

Robot Results

:white_check_mark: Passed	:x: Failed	:next_track_button: Skipped	Total	Pass %	:stopwatch: Duration
436	0	3	436	100	1h8m50.433275s

[albinsuresh]

After an offline discussion with @reubenmiller, we have decided to do this PR slightly differently, by uploading the workflow log files for all the operations instead of operation specific log files like the one done in this PR. But currently there are many inconsistencies in how the logging happens for different operations as raised in #2835 and #2836. Those will have to be fixed as well for this feature to be useful/complete. But, the switch to uploading workflow logs can be done independently.

[albinsuresh]

@reubenmiller @didier-wenzek Have updated the PR with a common log upload routine for all ops, that doesn't require explicit invocation per operation. Streamlining the command payloads types of all the ops to implement the CommandPayload trait, which enabled the conversion to the GenericCommandPayload struct was the key to this approach.

So, the approach can be reviewed now. It's still marked draft because the parsing logic of ops like config, firmware and log are still not fully using the generic Command representation of them yet, which I'll add next, as it wasn't really needed for this feature to work. But, @didier-wenzek I have a feeling that you might be repeating the same for #2836 as part of integrating log and config actors into the workflow subsystem.

[reubenmiller]

I just tried out the feature and have a few minor points regarding the uploading of the log file:

1. We should add a more descriptive text to the uploaded log, as for a failed "software_update" operation, the uploaded log file just says "software_update" without any context that it is log file
2. Can we add the filename to the meta information (this feature was added recently in https://github.com/thin-edge/thin-edge.io/issues/2763). If we don't add a useful filename, then when a file is downloaded by the user, they lose most of the human readable context about the original file (e.g. which device did it come from, what log is it from etc.)

Example of point 1

Example of point 2

[reubenmiller]

Just some really minor points (after having another look at other aspects of the UX).

1. The allowed values for c8y.operations.auto_log_upload should be mentioned when using the --doc flag on the configuration list. Currently it only shows one example:

   $ tedge config list --doc
           c8y.operations.auto_log_upload  Auto-upload the operation log once it finishes. 
                                       Example: always
   

   Though what is nice is that when the user gives a wrong value, that they are also presented with the supported values, however some users won't even try entering a values without first knowning the supported values.

   # tedge config set c8y.operations.auto_log_upload onfailure
   Error: failed to set the configuration key: 'c8y.operations.auto_log_upload' with value: onfailure.
   
   Caused by:
   0: Failed to parse input
   1: Failed to parse flag: onfailure. Supported values are: 'never', 'always' or 'onFailure'
   

2. Use on-failure instead of onFailure (more preference to avoid upper/lower case variants)

[reubenmiller]

@albinsuresh Since the log consolidation work probably won't be finished before the next release (1.1.0), can you please change the default config value to "never". Reason is that we want to provide a consistent feature, and uploading only partially useful log files will detract too much from the usefulness of this feature.