fix: use FTS for log_upload and config_snapshot operations#2440
fix: use FTS for log_upload and config_snapshot operations#2440Bravo555 merged 2 commits intothin-edge:mainfrom
log_upload and config_snapshot operations#2440Conversation
a3f27ad to
329f6b5
Compare
Codecov Report
Additional details and impacted files
|
Robot Results
|
793a3bf to
c7849f5
Compare
e0bb75f to
ac7dcd6
Compare
log_upload and config_snapshot operations
ac7dcd6 to
be9fa00
Compare
rina23q
left a comment
There was a problem hiding this comment.
Overall looks good. The most important for me is where tedge-mapper-c8y should keep the files by FTS.
I will not argue the contrary. The c8y mapper was already complex and the current design adds complexity.
This is correct up to the point one needs to share some mutable state. With Do we have such mutable shared state here? Requests are sent over senders, which are Can code clarity be improved without a deep refactoring? I see some improvements, but I would be happy to merge this PR as is, to meet the 0.13.1 release, and address improvements later.
|
That was my intention as well, to merge the PR as it is now, since it contains crucial fixes, and look at addressing this complexity later, when we're less pressed for 1.0. But on my side I can say that this spreading of the state has slowed me down noticeably, so I feel this is a bit of a tech debt and will need to be paid back later. |
| /// Used to denote as type of what operation was the file downloaded from the FTS. | ||
| /// | ||
| /// Used to dispatch download result to the correct operation handler. | ||
| pub enum FtsDownloadOperationType { |
There was a problem hiding this comment.
You could just reuse CumulocitySupportedOperations instead of this new struct.
There was a problem hiding this comment.
By using a different enum, I wanted to underline that these are the only operations in which we need to download from the FTS. By having a new enum with only these 2 variants, I don't have to do a catch-all _ => {} in matches, so that when a new variant is possibly added, the compiler will automatically highlight all the matches where this variant is not handled.
| pub url: String, | ||
|
|
||
| // used to automatically remove the temporary file after operation is finished | ||
| pub destination_dir: tempfile::TempDir, |
There was a problem hiding this comment.
May be we could store the DownloadRequest itself here instead of the url and destination_dir separately.
There was a problem hiding this comment.
I'd agree, except that DownloadRequest contains a plain PathBuf and here we have tempdile::TempDir, which is a temporary directory that is automatically freed on destruction. I've chosed to use this instead of manually removing the parent directory download of the file as then we don't have to remember to do it in the download result handler, but but it's still not ideal.
How to handle is a bit tricky because not all operations need to create a temporary directory, so we don't want to automatically create one for every operation, and I'd rather use something which is automatically removed on destruction, so we don't forget. The most awkward thing is storing it between different different stages of the operation (handle download, handle upload, etc.), because we don't use it for the data, we use it for the behaviour.
| clear_cmd_topic: message.topic.clone(), | ||
| c8y_binary_url: binary_upload_event_url.to_string(), | ||
| operation: CumulocitySupportedOperations::C8yLogFileRequest, | ||
| event_response_id, |
There was a problem hiding this comment.
There's no point creating/sending the event before this download. So, it's better to move that to handle_fts_log_download right before the binary_upload_event_url is created.
didier-wenzek
left a comment
There was a problem hiding this comment.
Approved.
The code is more complex than one might wish - but this is not due to this PR which only adds on complexity already here. To make things simpler we have to rethink the way the mapper interact with the file transfer service. This design work will be done in the context of firmware management.
Makes tedge-mapper-c8y use the File Transfer Service (FTS) when processing `log_upload` or `config_snapshot` operations. Previously tedge-mapper-c8y implicitly assumed that it's installed on the same device as the tedge-agent and tried to access the file-transfer directory directly. Now it correctly downloads the files in the operation's `tedgeUrl` parameter via HTTP. This implementation directly uses downloader and waits until the download is done, which means it will block if the download takes a long time.
This commit changes the use of `Downloader` inside `log_upload` and `config_snapshot` handlers to use `DownloaderActor` instead. The advantage of this is that downloading from the FTS no longer blocks and other such operations can be processed in the meantime. I will argue, however, that this also had negative effects on code complexity and maintainability. Whereas previously the download was in a single place and one could trace the control flow easily, now the state with maintaining the state of the download is spread very thin across many different functions and variables. The effect of the current architecture is that for every complex operation where multiple steps are required, for every transition `step1 -> step2` (e.g. before the download and after the download) one has to create: - a piece of state where we await the response from the actor that performs the effect associated with the transition, e.g. `pending_fts_download_operations` for `DownloaderActor` - a function `step1` which starts the transition to `step2` by sending a message `request` - a function `step2` which gets the result `response` - a function which listens to `response` from the actor, and inspects the response to dispatch it to correct function which performs the next step I believe that Rust's async/await was created to solve, among others, exactly this problem, by transforming every async function into a state machine which automatically handles transistions between `.await` points which block.
3d9c6d9 to
752bb22
Compare
TODO
add a test which confirms that download no longer blockswill not be done because it's too complex for the current architecture @reubenmiller for questionsProposed changes
Makes tedge-mapper-c8y use the File Transfer Service (FTS) when processing
log_uploadorconfig_snapshotoperations.Previously tedge-mapper-c8y implicitly assumed that it's installed on the same device as the tedge-agent and tried to access the file-transfer directory directly. Now it correctly downloads the files in the operation's
tedgeUrlparameter via HTTP.Types of changes
Paste Link to the issue
#2390
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
The 1st commit introduces the FTS download operation by using the
Downloaderdirectly, fixing the bug.The 2nd commit in this PR removes the use of
Downloaderinsidelog_uploadandconfig_snapshothandlers to useDownloaderActorinstead.The advantage of this is that downloading from the FTS no longer blocks and other such operations can be processed in the meantime.
I will argue, however, that this also had negative effects on code complexity and maintainability. Whereas previously the download was in a single place and one could trace the control flow easily, now the state with maintaining the state of the download is spread very thin across many different functions and variables.
The effect of the current architecture is that for every complex operation where multiple steps are required, for every transition
step1 -> step2(e.g. before the download and after the download) one has to create:pending_fts_download_operationsforDownloaderActorstep1which starts the transition tostep2by sending a messagerequeststep2which gets the resultresponseresponsefrom the actor, and inspects the response to dispatch it to correct function which performs the next stepI believe that Rust's async/await was created to solve, among others, exactly this problem, by transforming every async function into a state machine which automatically handles transistions between
.awaitpoints which block.