Stop duplicate messages being sent by tedge-log-plugin and tedge-configuration-plugin on file reload

[Bravo555]

Proposed changes

This fix consists of 2 parts:

1. NotifyStream impl was changed to make use of a notify-debouncer-full crate to delay and debounce fs events. This should also impact other consumers of NotifyStream.
2. Watch parameters of tedge-log-plugin were changed to only listen to Modified events, no longer listening to FileCreated events.

This fix was confirmed to produce only 1 reload when using following editors:

• nano
• vim
• neovim

More details regarding both parts can be found in their respective commit messages.

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

• Updating /etc/tedge/plugins/tedge-log-plugin.toml results in multiple log_upload metadata messages being sent #2451

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

[codecov]

Codecov Report

Merging #2454 (351e8b8) into main (6fe2820) will increase coverage by 0.0%.
Report is 19 commits behind head on main.
The diff coverage is 90.8%.

Additional details and impacted files

Files	Coverage Δ
...rates/extensions/tedge_config_manager/src/actor.rs	67.8% <0.0%> (ø)
crates/extensions/tedge_log_manager/src/actor.rs	66.8% <0.0%> (-0.5%)	⬇️
crates/common/tedge_utils/src/notify.rs	93.2% <92.9%> (+2.0%)	⬆️

... and 65 files with indirect coverage changes

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
362	0	3	362	100	49m8.029999999s

[reubenmiller]

2. Watch parameters of tedge-log-plugin were changed to only listen to Modified events, no longer listening to FileCreated events.

Are we also sure that this change does not affect replacing existing files using mv and cp? We have some system test coverage for the configuration plugin, but none for the log manager. We should double check that we are not breaking any existing behaviour.

Also, shouldn't we also be doing the same handling for the configuration manager?

[Bravo555]

Are we also sure that this change does not affect replacing existing files using mv and cp?

Moving files and creating files both also emit Modified event

We have some system test coverage for the configuration plugin, but none for the log manager. We should double check that we are not breaking any existing behaviour.

This is a good candidate for a Rust integration test, which I'll add for this PR.

Also, shouldn't we also be doing the same handling for the configuration manager?

We probably should. I'll update the configuration manager as well.

[Bravo555]

This is the branch that allows us to only listen to FsEvent::Modified. The actual sequence returned by lower-level notify-debouncer-full is:

[crates/common/tedge_utils/src/notify.rs:77] &notify_event = DebouncedEvent {
    event: Event {
        kind: Create(
            File,
        ),
        paths: [
            "/etc/tedge/plugins/tedge-log-plugin.toml",
        ],
        attr:tracker: None,
        attr:flag: None,
        attr:info: None,
        attr:source: None,
    },
    time: Instant {
        tv_sec: 23846,
        tv_nsec: 112231843,
    },
}
[crates/common/tedge_utils/src/notify.rs:77] &notify_event = DebouncedEvent {
    event: Event {
        kind: Access(
            Close(
                Write,
            ),
        ),
        paths: [
            "/etc/tedge/plugins/tedge-log-plugin.toml",
        ],
        attr:tracker: None,
        attr:flag: None,
        attr:info: None,
        attr:source: None,
    },
    time: Instant {
        tv_sec: 23846,
        tv_nsec: 112263954,
    },
}
[crates/common/tedge_utils/src/notify.rs:77] &notify_event = DebouncedEvent {
    event: Event {
        kind: Remove(
            File,
        ),
        paths: [
            "/etc/tedge/plugins/tedge-log-plugin.toml~",
        ],
        attr:tracker: None,
        attr:flag: None,
        attr:info: None,
        attr:source: None,
    },
    time: Instant {
        tv_sec: 23846,
        tv_nsec: 112322524,
    },
}

when saving the file with nvim, or:

[crates/common/tedge_utils/src/notify.rs:77] &notify_event = DebouncedEvent {
    event: Event {
        kind: Create(
            File,
        ),
        paths: [
            "/etc/tedge/plugins/tedge-log-plugin.toml",
        ],
        attr:tracker: None,
        attr:flag: None,
        attr:info: None,
        attr:source: None,
    },
    time: Instant {
        tv_sec: 23953,
        tv_nsec: 87955441,
    },
}
[crates/common/tedge_utils/src/notify.rs:77] &notify_event = DebouncedEvent {
    event: Event {
        kind: Access(
            Close(
                Write,
            ),
        ),
        paths: [
            "/etc/tedge/plugins/tedge-log-plugin.toml",
        ],
        attr:tracker: None,
        attr:flag: None,
        attr:info: None,
        attr:source: None,
    },
    time: Instant {
        tv_sec: 23953,
        tv_nsec: 87977201,
    },
}

when file is copied. Picking up AccessKind::Close(AccessMode::Write) allows us to reload appropriately.

[didier-wenzek]

Approved. The code is clear and correct. Using notify-debouncer-full was a great idea!

[didier-wenzek]

Are we also sure that this change does not affect replacing existing files using mv and cp?

This is working as expected.

[albinsuresh]

LGTM.

[Bravo555]

This is a good candidate for a Rust integration test, which I'll add for this PR.

Instead of doing integration tests for both tedge-log-plugin and tedge-configuration-plugin, I've decided to only add 1 unit test: notify::tests::modify_emitted_for_move_copy_create_delete that checks if FsEvent::Modified is emitted when the file is modified, deleted, created, or renamed.

To add relevant tests for both plugins, I'd need to rewrite the plugins themselves a little bit, and we're currently more pressed for other stuff, so that should be sufficient for now.

[albinsuresh]

Some queries to better understand the test logic.