Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bypass duplicate message filtering on mapper startup with config flag #2632

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

Ruadhri17
Copy link
Contributor

@Ruadhri17 Ruadhri17 commented Jan 30, 2024

Proposed changes

TODO:

  • fix tests
  • add test
  • replace sync_on_startup with better name

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

#2605

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

Copy link
Contributor

github-actions bot commented Jan 30, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
390 0 3 390 100 1h1m28.051s

@@ -68,15 +68,22 @@ pub struct MessageLogWriter {
}

impl MessageLogWriter {
pub fn new<P>(log_dir: P) -> Result<MessageLogWriter, std::io::Error>
pub fn new<P>(log_dir: P, sync_on_startup: bool) -> Result<MessageLogWriter, std::io::Error>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn new<P>(log_dir: P, sync_on_startup: bool) -> Result<MessageLogWriter, std::io::Error>
pub fn new<P>(log_dir: P, truncate: bool) -> Result<MessageLogWriter, std::io::Error>

From the MessageLogWriter, there is no notion of sync. The argument just decides whether to start fresh or append to the existing.

@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch from d9ff78a to 8890ca0 Compare January 31, 2024 10:49
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request January 31, 2024 10:56 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch 2 times, most recently from 36bda79 to e8a5144 Compare January 31, 2024 17:02
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (8181668) 75.9% compared to head (a1c8bda) 75.9%.
Report is 15 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/core/c8y_api/src/json_c8y.rs 90.5% <100.0%> (+<0.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs 81.1% <100.0%> (+<0.1%) ⬆️
...s/extensions/c8y_mapper_ext/src/service_monitor.rs 95.3% <100.0%> (+<0.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs 91.8% <100.0%> (+<0.1%) ⬆️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 81.1% <75.0%> (-0.1%) ⬇️
...tes/core/tedge_agent/src/software_manager/actor.rs 55.6% <0.0%> (ø)
...tes/core/tedge_agent/src/state_repository/state.rs 92.7% <0.0%> (ø)
crates/core/tedge_api/src/message_log.rs 84.8% <90.9%> (+0.8%) ⬆️
crates/extensions/c8y_mapper_ext/src/config.rs 45.8% <50.0%> (+<0.1%) ⬆️
crates/core/tedge_api/src/entity_store.rs 92.6% <72.2%> (-0.4%) ⬇️

... and 2 files with indirect coverage changes

@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request January 31, 2024 17:09 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch from e8a5144 to 352c3cb Compare February 1, 2024 00:51
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 1, 2024 00:58 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch from 352c3cb to 9229f94 Compare February 1, 2024 08:49
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 1, 2024 08:56 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 marked this pull request as ready for review February 1, 2024 09:19
@Ruadhri17 Ruadhri17 requested review from jarhodes314, rina23q and a team as code owners February 1, 2024 09:19
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working as expected. However, the code can be a bit simpler.

crates/core/tedge_api/src/message_log.rs Outdated Show resolved Hide resolved
crates/core/tedge_api/src/message_log.rs Outdated Show resolved Hide resolved
crates/core/tedge_api/src/entity_store.rs Outdated Show resolved Hide resolved
Comment on lines 165 to +166
log_dir: P,
clean_start: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a log_dir and clean_start flag telling to truncate or not the log.

I would consider to pass only a MessageLogWriter, built from the log using MessageLogWriter::new(log_dir) or MessageLogWriter::new_truncated(log_dir):

Suggested change
log_dir: P,
clean_start: bool,
message_log: MessageLogWriter,

For that to work the MessageLogWriter has to be improved to build a MessageLogReader on the same log dir. So the entity store can use its self.message_log for reads and writes.

It might be better to address this only a follow-up PR after the 1.0 release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion is to have a MessageLog abstraction, which is passed to the entity store, out of which the MessageLogWriter and MessageLogReader instances can be retrieved.

@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch from 9229f94 to c83d5a6 Compare February 1, 2024 18:59
@@ -193,13 +198,55 @@ Entities persisted and restored
Should Have MQTT Messages c8y/s/us/plc2 message_contains=102 date_from=${timestamp} minimum=0 maximum=0
END

Entities send to cloud on restart
Execute Command tedge mqtt pub --retain 'te/factory/shop/plc1/' '{"@type":"child-device","@id":"plc1"}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you align the @id values used in this test along the lines of #2657 ?


*** Keywords ***

Re-enable Auto-registration
Execute Command sudo tedge config unset c8y.entity_store.auto_register
Restart Service tedge-mapper-c8y

Re-enable Clean start
Copy link
Contributor

@albinsuresh albinsuresh Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Re-enable Clean start
Enable clean start

Just to make things clearer, it'd be better to explicitly do tedge config set c8y.entity_store.clean_start true, as the unset might be confusing to a reader who's not familiar with the default value.

Comment on lines 218 to 228
Execute Command cat /etc/tedge/.tedge-mapper-c8y/entity_store.jsonl
${original_last_modified_time}= Execute Command date -r /etc/tedge/.tedge-mapper-c8y/entity_store.jsonl

FOR ${counter} IN RANGE 0 5
${timestamp}= Get Unix Timestamp
Restart Service tedge-mapper-c8y
Service Health Status Should Be Up tedge-mapper-c8y

# Assert that the file contents changed on restart
${last_modified_time}= Execute Command date -r /etc/tedge/.tedge-mapper-c8y/entity_store.jsonl
Should Not Be Equal ${last_modified_time} ${original_last_modified_time}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the timestamp assertion is already done in the previous test, we can either skip this whole block or at least avoid the looping and limit the check to only once.

Should Have MQTT Messages c8y/s/us/plc1 message_contains=102 date_from=${timestamp} minimum=1 maximum=1
Should Have MQTT Messages c8y/s/us/plc2 message_contains=102 date_from=${timestamp} minimum=1 maximum=1

Sleep 2s reason=Make sure that previous iteration was sent to cloud
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this sleep required? That too at the end of this test. The reason isn't clear from the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that sometimes tests just fail because of that test case:
Should Have MQTT Messages c8y/s/us message_contains=101,${prefix}plc date_from=${timestamp} minimum=2 maximum=2. It returns more messages than expected (It can be noticed for example in this run). The only solution I found to this was adding this sleep, but maybe someone else has better solution to that.

@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch from c83d5a6 to 89a5408 Compare February 2, 2024 10:34
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 2, 2024 10:43 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch from 89a5408 to 37e0e74 Compare February 2, 2024 10:49
@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch 3 times, most recently from 57c4198 to e4b0f60 Compare February 2, 2024 11:06
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
@Ruadhri17 Ruadhri17 force-pushed the message-filtering-bypass branch from e4b0f60 to a1c8bda Compare February 2, 2024 11:10
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 2, 2024 11:18 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 added this pull request to the merge queue Feb 2, 2024
Merged via the queue into thin-edge:main with commit eb93afc Feb 2, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants