feat: inventory.json support for child devices#3755
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Robot Results
|
17ae364 to
e59cf43
Compare
Bravo555
left a comment
There was a problem hiding this comment.
One error message adjustment request and docs update, apart from that, looks good and will approve after these changes.
tests/RobotFramework/tests/cumulocity/inventory/inventory_update_child_device.robot
Show resolved
Hide resolved
| format!("Failed to open inventory file: {}", err).into(), | ||
| )); | ||
| } | ||
| }; | ||
| let inventory_json: Value = serde_json::from_reader(file).map_err(|err| { | ||
| RuntimeError::ActorError( | ||
| format!("Failed to parse inventory file contents as JSON: {}", err).into(), | ||
| ) | ||
| })?; | ||
| let Value::Object(twin_map) = inventory_json else { | ||
| return Err(RuntimeError::ActorError( | ||
| "Invalid inventory.json format: expected a JSON object".into(), |
There was a problem hiding this comment.
todo: attach the filename to the error
| let device_id = self.config.device_topic_id.clone(); | ||
| for (key, value) in inventory_map { | ||
| self.publish_twin_data(&device_id, key.clone(), value.clone()) | ||
| .await; |
There was a problem hiding this comment.
suggestion: don't have to clone here
| .await; | |
| self.publish_twin_data(&device_id, key, value).await; |
| async fn run(mut self) -> Result<(), RuntimeError> { | ||
| let mut inventory_map = self.load_inventory_json()?; | ||
| // Wait until the very fist message is received (at least the agent health status is guaranteed) | ||
| if let Some(mut msg) = self.messages.recv().await { | ||
| loop { | ||
| if let Ok((_, Channel::EntityTwinData { fragment_key })) = self | ||
| .config | ||
| .mqtt_schema | ||
| .entity_channel_of(msg.topic.as_ref()) | ||
| { | ||
| // If a twin data message for the same key is available, | ||
| // ignore the value in inventory JSON | ||
| inventory_map.remove(&fragment_key); | ||
| } | ||
|
|
||
| msg = match timeout(Duration::from_secs(1), self.messages.recv()).await { | ||
| Ok(Some(next)) => next, | ||
| _ => break, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| let device_id = self.config.device_topic_id.clone(); | ||
| for (key, value) in inventory_map { | ||
| self.publish_twin_data(&device_id, key.clone(), value.clone()) | ||
| .await; | ||
| } | ||
|
|
||
| // This is to prevent the MQTT actor from crashing while trying to send a twin message to this actor later | ||
| // after this actor has finished and closed its message box, but the MQTT actor still holds the sender half of it. | ||
| while self.messages.recv().await.is_some() { | ||
| // Do nothing | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
thought: So as I understand, basically what we do here is:
- Read the fragments from the inventory file
- Wait until the end of the "init phase", during which we remove fragments that are already published (our heuristic for "end of the init phase" being messages not being published for 1s)
- Publish all the remaining fragments
- Effectively exit, but we're forced to loop because other actors may send us messages despite us not wanting to process them anymore.
So the actor stays alive, despite not doing anything, which sucks a bit, but as the comment says, it's hard to exit without the MQTT actor possibly receiving failures after we exit.
Funnily enough, this usage pattern here could be easily handled by an in-memory broker: we could subscribe for a bit, listen to some messages, and exit without other actors even being aware of us, because sources and sinks would be decoupled, but the actor runtime works more on a peer-to-peer basis, so that's why it's hard.
Even still, perhaps upstream producers being coupled to their downstream consumers such that the producer (actor A) can't output after the consumer (actor B) exits isn't a nice property. Perhaps the runtime should simply remove the consumer message box from producer's sender list and let the producer proceed, only if A doesn't depend on B in any way. But there's also a pattern where A sends message to B and then waits for B's response, in which case it does depend on it and should know ASAP if B exits (it should then probably also exit with an error).
So we have both request/response and publish/subscribe patterns, in this case publish/susbcribe is a bit awkward.
There was a problem hiding this comment.
Even still, perhaps upstream producers being coupled to their downstream consumers such that the producer (actor A) can't output after the consumer (actor B) exits isn't a nice property.
Completely agree. I am really tempted to change the MQTT actor logic to not fail on send when a channel is closed, but just log and proceed (potentially removing that sender from its peer_senders list as well). I just wasn't fully sure if some other part of the code is expecting on the existing behaviour to handle things like shutdown requests. That's why I went with this not-so-pretty workaround. But, I'm open to do that change in a separate PR so that we can discuss the impact.
There was a problem hiding this comment.
I'm happy to accept this workaround. As for changing only the MQTT actor (because indeed actors themselves have a choice to exit or keep going if send fails!), also approve making the modification in a separate PR, I'm curious what the test results would be.
There was a problem hiding this comment.
todo: https://thin-edge.github.io/thin-edge.io/references/mappers/c8y-mapper/#base-inventory-model will also have to be updated.
tests/RobotFramework/tests/cumulocity/inventory/inventory_update_child_device.robot
Outdated
Show resolved
Hide resolved
tests/RobotFramework/tests/cumulocity/inventory/inventory_update_bootstrap.robot
Outdated
Show resolved
Hide resolved
tests/RobotFramework/tests/cumulocity/inventory/inventory_update_bootstrap.robot
Outdated
Show resolved
Hide resolved
reubenmiller
left a comment
There was a problem hiding this comment.
Approved, just need to make the very minor adjustments to the system tests tagging
9081003 to
9892efc
Compare
9892efc to
6b57b14
Compare
Proposed changes
Enable users to specify initial inventory data for child devices running tedge-agent using an inventory.json file.
Types of changes
Paste Link to the issue
#3430
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments