Skip to content

Config updates can be missed #1135

Description

@llucax

What happened?

Some time ago we switched to using polling because inotify doesn't work very well under some circumstances (like inside containers). The problem is polling is also not the panacea, as it seems like updates can be also missed if they are very close in time (my guess is that watchfiles has only 1 second resolution.

What did you expect instead?

All updates should be picked up.

Affected version(s)

No response

Affected part(s)

Configuration management (part:config)

Extra information

At this point I think we should really consider a better system for notifying about config updates, trying to watch files keep bringing issues and it is very problematic for testing, but since this might be a major undertaking, I guess for now we can just do a regular complete read of the config file, every 10 seconds or so, compare to the last read, and trigger an update if it changed. So the watcher would just be an optimization to pick up the changes more quickly.


This tests fails most of the time (I think it succeeds very seldom, when the await in the task crosses the second boundary:

import asyncio
import pathlib
from datetime import timedelta

import pytest
from frequenz.channels.file_watcher import EventType, FileWatcher


@pytest.fixture
def config_file(tmp_path: pathlib.Path) -> pathlib.Path:
    """Create a temporary config file for testing."""
    # config_file = tmp_path / "config.toml"
    config_file = pathlib.Path("/tmp/watch/config.toml")
    config_file.write_text(
        """
        [test]
        name = "test1"
        value = 42

        [logging.root_logger]
        level = "DEBUG"
        """
    )
    return config_file


async def change_config_file(config_file: pathlib.Path) -> None:
    await asyncio.sleep(0.2)
    config_file.write_text(
        """
        [test]
        name = "test2"
        value = 43

        [logging.root_logger]
        level = "INFO"
        """
    )


async def test_full_config_flow(config_file: pathlib.Path) -> None:
    """Test the complete flow of configuration management."""
    print(f"Watching {config_file}")
    file_watcher = FileWatcher(
        paths=[config_file.parent],
        event_types=set(EventType),
        force_polling=True,
        polling_interval=timedelta(seconds=0.1),
    )

    task = asyncio.create_task(change_config_file(config_file))

    async with asyncio.timeout(5):
        event = await file_watcher.receive()
        print(event)

        await task

Activity

  1. added
    priority:❓We need to figure out how soon this should be addressed
    type:bugSomething isn't working
    on Dec 18, 2024
  2. added this to the Untriaged milestone on Dec 18, 2024
  3. llucax commented on Dec 19, 2024

    @llucax
    ContributorAuthor

    I just verified this is a bug in watchfiles polling. This also fails (except for cases where the second-boundary is crossed between writes):

    import asyncio
    import pathlib
    
    import pytest
    from watchfiles import awatch
    
    
    @pytest.fixture
    def config_file(tmp_path: pathlib.Path) -> pathlib.Path:
        """Create a temporary config file for testing."""
        # config_file = tmp_path / "config.toml"
        config_file = pathlib.Path("/tmp/watch/config.toml")
        config_file.write_text("Hello")
        print("after first write", config_file.stat().st_ctime)
        return config_file
    
    
    async def change_config_file(config_file: pathlib.Path) -> None:
        await asyncio.sleep(0.2)
        config_file.write_text("World")
        print("after second write", config_file.stat().st_ctime)
    
    
    async def test_watchfiles(config_file: pathlib.Path) -> None:
        """Test the complete flow of configuration management."""
        print(f"Watching {config_file}")
    
        task = asyncio.create_task(change_config_file(config_file))
    
        try:
            async with asyncio.timeout(1):
                # Watch the parent directory for changes
                async for changes in awatch(
                    config_file.parent,
                    watch_filter=None,
                    force_polling=True,
                    poll_delay_ms=100,
                ):
                    for change_type, changed_path in changes:
                        if pathlib.Path(changed_path) == config_file:
                            print(f"Change detected: {change_type} - {changed_path}")
                            await task
                            return  # Exit after first relevant change is detected
        finally:
            task.cancel()
            print("after finished", config_file.stat().st_ctime)
  4. llucax commented on Dec 19, 2024

    @llucax
    ContributorAuthor
  5. added
    status:blockedOther issues must be resolved before this can be worked on
    on Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    part:configAffects the configuration managementpriority:❓We need to figure out how soon this should be addressedstatus:blockedOther issues must be resolved before this can be worked ontype:bugSomething isn't working

    Type

    Fields

    Priority

    None yet

    Effort

    None yet

    Affected Version

    None yet

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions