fix: aws and az fail when no response on connection check#3656
fix: aws and az fail when no response on connection check#3656Bravo555 merged 3 commits intothin-edge:mainfrom
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
3004ea2 to
5669bfe
Compare
|
NOTE: The new test that checks the connection has to be run manually because the suite is marked |
Robot Results
|
| if connection_check_success { | ||
| Ok(()) | ||
| } else { | ||
| Err(anyhow::anyhow!("Connection check failed").into()) | ||
| } |
There was a problem hiding this comment.
note: To make the tedge connect aws return a non-zero exit code when the connection check failed had to add this here, which can influence c8y where failing the connection check is apparently not a failure? I'm not sure why, and if it's intended or accidental.
There was a problem hiding this comment.
tedge connect c8y ignores connection check errors since #218.
@rina23q do you remember the motivation for this behavior?
That said this is not my main concern regarding connect/command.rs. There is so many alternative paths for c8y, aws and az that its really difficult to tell what is done for each. For instance, a few lines above this one, the agent is restarted for c8y but not for aws and az. Why? Also, you have to know that enable_software_management means restart the agent.
There was a problem hiding this comment.
Yeah I remember the decision.
At the time, the purpose of tedge connect was to establish the bridge to the cloud, and the connection check was considered an optional check because it may fail, for example, when the server is not responded, that's not thin-edge side of failure. That's why we decided to have exit code 0.
However, now we have --offline option. So, I don't mind if we return non-zero exist code when connection check fails.
When connecting to aws and azure, during "Verifying device is connected to cloud..." stage, if we don't get response from a remote broker, mark the spinner as failed instead of OK. It was being marked as OK, because the code was a bit too c8y-centric: for c8y we connect directly to the remote broker to create a device and after that switch to the bridge. When using the bridge, if remote broker doesn't respond, we know that the bridge is connected with remote broker because it's using the same configuration as the direct connection, which was successful, so we consider device status unknown instead of failing the connection. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
5669bfe to
2f09de6
Compare
Signed-off-by: reubenmiller <reuben.d.miller@gmail.com>
reubenmiller
left a comment
There was a problem hiding this comment.
Approved. Tested with AWS and it was good (both negative and positive test cases).
I pushed a minor commit to improve the readability of some of the error messages for all clouds, b347575
TODO
Proposed changes
When connecting to aws and azure, during "Verifying device is connected to cloud..." stage, if we don't get response from a remote broker, mark the spinner as failed instead of OK.
It was being marked as OK, because the code was a bit too c8y-centric: for c8y we connect directly to the remote broker to create a device and after that switch to the bridge. When using the bridge, if remote broker doesn't respond, we know that the bridge is connected with remote broker because it's using the same configuration as the direct connection, which was successful, so we consider device status unknown instead of failing the connection.
Types of changes
Paste Link to the issue
#3640
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments