Create uploader actor

[Ruadhri17]

Proposed changes

This PR adds new uploader actor that works in similar manner to downloader_actor. Later on it will be used in both tedge-log-plugin and tedge-configuration-plugin to take responsibility for uploading content to tedge file repository. More in #2301.

TODO:

• Add test that checks if backoff strategy is working.

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

• Replace current upload mechanism with the uploader actor in new tedge plugins #2301

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]

Codecov Report

Merging #2302 (8501559) into main (0047f08) will increase coverage by 0.0%.
The diff coverage is 83.0%.

❗ Current head 8501559 differs from pull request most recent head dfd790d. Consider uploading reports for the commit dfd790d to get more accurate results

Additional details and impacted files

Files	Coverage Δ
crates/common/log_manager/src/error.rs	0.0% <ø> (ø)
crates/common/tedge_utils/src/file.rs	67.7% <ø> (ø)
...rates/extensions/tedge_config_manager/src/error.rs	0.0% <ø> (ø)
crates/extensions/tedge_config_manager/src/lib.rs	91.4% <100.0%> (ø)
crates/extensions/tedge_log_manager/src/error.rs	0.0% <ø> (ø)
crates/extensions/tedge_log_manager/src/lib.rs	89.0% <100.0%> (+0.4%)	⬆️
crates/common/upload/src/error.rs	85.7% <85.7%> (ø)
...rates/extensions/tedge_config_manager/src/tests.rs	92.1% <95.8%> (+0.2%)	⬆️
crates/extensions/tedge_log_manager/src/tests.rs	92.0% <95.6%> (-0.2%)	⬇️
crates/extensions/c8y_log_manager/src/actor.rs	75.4% <33.3%> (-0.5%)	⬇️
... and 8 more

... and 7 files with indirect coverage changes

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
319	0	3	319	100	1h5m27.413s

[albinsuresh]

The overall code looks fine with the basic retry mechanism. But, unlike the downloader which could resume a download from the point where it was interrupted, I didn't see a similar resumption mechanism here. Is there any plan to add such a multi-part upload functionality to this uploader? It doesn't necessarily have to be in this PR, but would be a good follow-up addition.

BTW, I saw that the plan is to update the log plugin and config plugin to use this in a different PR. I'd recommend that you update one of them in this PR itself so that the functionality is well tested from a consumer as well. Else it's just a library that no one uses, relying purely on unit tests.

[Ruadhri17]

The overall code looks fine with the basic retry mechanism. But, unlike the downloader which could resume a download from the point where it was interrupted, I didn't see a similar resumption mechanism here. Is there any plan to add such a multi-part upload functionality to this uploader? It doesn't necessarily have to be in this PR, but would be a good follow-up addition.

I would leave the partial upload for next PR as it is more tricky than download. According to the rfc-editor this is very inconsistent and require some changes on server side. Another thing is that we would have to edit the exponential backoff mechanism because of 400 BAD REQUEST respond which at this point will not trigger retry mechanism.

[albinsuresh]

The main changes look fine. Just some minor niggles and I'm happy to approve once those are ironed out.

[albinsuresh]

A library returning anyhow errors is highly discouraged. You should be defining concrete error types with the use of thiserror instead. Anyhow is usually used at the application level where the errors just need to be reported.

[Bravo555]

IMO there are valid use cases for using anyhow in a library crate. What matters the most is the intent, what do we expect users to do. We do not expect code which calls the uploader to behave differently between error variants of e.g. "this does not name a valid file" or "path is not valid unicode", we simply return these errors and show them to the user. We don't have to maintain any Rust API compatibility, we can add or remove error variants at any time.
So I think in this case using an error variant for "file error" and then using anyhow for telling the user what exactly went wrong is acceptable.
anyhow::Error is also like Box<dyn Error> + 'static, but also guarantees Send and Sync. I originally added it to InvalidFileName error variant because i wanted to use either io::Error or Utf8Error as a source, but using anyhow!("error details") directly... I wouldn't say it's an antipattern.

I was recently reading Zero To Production In Rust and there's a free chapter about error handling. There is a dedicated section about anyhow vs thiserror, but I'd recommend reading the entire chapter for context.

[albinsuresh]

Thanks for the link. Gave me a new perspective. I guess the following is the key point:

Library authors cannot (or do not want to) make assumptions on the intent of their users. They steer away from being opinionated (to an extent) - enums give users more control, if they need it.

and I completely agree to your following point:

We do not expect code which calls the uploader to behave differently between error variants of e.g. "this does not name a valid file" or "path is not valid unicode", we simply return these errors and show them to the user.

Here is where I'm wondering what value anyhow adds if all we wanted to do was to report the reason to the end-user. Why couldn't it be just a String value? Using anyhow instead, is just an overkill, right? As the user is less likely to introspect the source chain any further, as that was our fundamental assumption when we picked anyhow for "opaque" errors in the first place.

[rina23q]

Except anyhow in a library crate, looks good to me

[albinsuresh]

I still have my reservations against the use of anyhow to represent a source in error variants like InvalidFileName where that enum variant itself was self-explanatory and I doubt if the source would really add any more context. But, that's a parallel discussion that should not block this PR. Hence approving as the rest of the code looks fine,

[gligorisaev]

QA has thoroughly checked the feature and here are the results:

• Test for ticket exists in the test suite.
• QA has tested the bug and could not reproduce it anymore.