tedge-mapper-c8y service restart is required after a change of supported operation file contents #1959
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Robot Results
Passed Tests
Failed Tests
|
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| @@ -743,7 +743,7 @@ impl Converter for CumulocityConverter { | |||
| ) -> Result<Option<Message>, ConversionError> { | |||
| match message.ops_dir.parent() { | |||
| Some(parent_dir) => { | |||
| if parent_dir.eq(&self.cfg_dir.join("operations").join("c8y")) { | |||
| if parent_dir.eq(&self.cfg_dir.join("operations")) { | |||
There was a problem hiding this comment.
Using the operations directory instead of the operation/c8y surprises me. Are you sure we should be using this, as won't that potentially listen to other non-c8y changes as well?
For example:
/etc/tedge/operations/az
There was a problem hiding this comment.
The tedge-mapper-c8y subscribes for notifications for the path /etc/tedge/operations/c8y. So, it will receive the notifications only when there is a change in this directory. The ops_dir is derived as below.
let ops_dir = config_dir.join("operations").join("c8y")
When there is a change of operation in the parent/thin-edge device whenever the notification contains /etc/tedge/operations/c8y. The parent directory will be /etc/tedge/operations.
When there is a change of operation in the thin-edge cumulocity child device the notification contains the /etc/tedge/operations/c8y<child-id> path. In this case, the parent directory will be /etc/tedge/operations/c8y.
There was a problem hiding this comment.
So why not check the paths of the event against /etc/tedge/operations/c8y ?
- if the dir of the updated file is
/etc/tedge/operations/c8y, then the event if for the main device. - if the parent dir of the updated file is
/etc/tedge/operations/c8y, the the event if for a child device. - otherwise the event has to be ignored.
Assuming that the mapper only subscribed to /etc/tedge/operations/c8y is really fragile.
There was a problem hiding this comment.
I felt its redundant, if you think it's very fragile, then I am ok to add specific event path checks for parent n child devices.
There was a problem hiding this comment.
It's fragile because someone else might later add an apparently innocuous new subscriptions and break things in a fully unexpected manner.
Thank you for the update.
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
approved these changes
May 11, 2023
Comment on lines
+753
to
+757
| } else if message.ops_dir.eq(&self | ||
| .cfg_dir | ||
| .join("operations") | ||
| .join("c8y") | ||
| .join(get_child_id(&message.ops_dir)?)) |
There was a problem hiding this comment.
[Minor] One might have something simpler by comparing message.ops_dir.parent() with Some("/etc/tedge/operations/c8y").
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
The issue here is, if there is any change to the
custom operationfile content, then these changes are not reflected in the operation list that is present with thetedge-mapper-c8y. To get it reflected the mapper has to be restarted.On the code path, on the file change notification, the mapper is wrongly updating the child device operations list.
The proposed fix is
Dynamically update/populate the
thin-edgedevice operation list oftedge-mapper-c8ywhen there is a change in the operation file.Added integration test to verify the fix.
Types of changes
Paste Link to the issue
#1705
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments