syncthing sync completed or not #1

shailaja · 2024-04-26T09:08:23+02:00

shailaja commented

2024-04-26 09:08:23 +02:00

No description provided.

PeterSurda commented

2024-05-03 05:28:02 +02:00

As we discussed, this needs to be refactored so that it accesses the syncthing API directly. I recommend the following code flow:

check the current event id
check the current status of the folder
while status isn't idle, long poll events, filtering for the folder changes, process them and break if folder is idle

This way there shouldn't be a gap that could cause problems, and it also shouldn't be necessary in each loop to wait for an event AND separately query the folder status. Also an events API callis automatically long poll with a timeout of 60 seconds, so that simplifies the code, the long poll is implemented on the server side.

We also need to handle a situation where the folder doesn't exist at all and therefore doesn't have a status. That should also be interpreted as non-idle, rather than throwing an exception.

Folder name can be passed as an environment variable, and the API and authentication can be passed as a volume mount from the host. There are probably other options but this one looks the simplest, and you can just copy parts of the code from https://git.bitmessage.org/Sysdeploy/ansible-modules-syncthing.

As we discussed, this needs to be refactored so that it accesses the syncthing API directly. I recommend the following code flow: 1. check the current event id 2. check the current status of the folder 3. while status isn't idle, long poll events, filtering for the folder changes, process them and break if folder is idle This way there shouldn't be a gap that could cause problems, and it also shouldn't be necessary in each loop to wait for an event AND separately query the folder status. Also an events API callis automatically long poll with a timeout of 60 seconds, so that simplifies the code, the long poll is implemented on the server side. We also need to handle a situation where the folder doesn't exist at all and therefore doesn't have a status. That should also be interpreted as non-idle, rather than throwing an exception. Folder name can be passed as an environment variable, and the API and authentication can be passed as a volume mount from the host. There are probably other options but this one looks the simplest, and you can just copy parts of the code from https://git.bitmessage.org/Sysdeploy/ansible-modules-syncthing.

shailaja force-pushed sync from 9009f205a8 to da5c266da9

2024-05-07 14:14:03 +02:00

Compare

shailaja requested review from swapnil 2024-05-07 14:14:23 +02:00

PeterSurda requested changes 2024-05-07 16:14:23 +02:00

PeterSurda left a comment

Overall it appears to be correct, but it could be nicer.

syncthing_sync.py Outdated

						
				@ -0,0 +7,4 @@

				    """ Retrieve the API key from the local Syncthing configuration file. """

				    try:

				        with open(config_file, 'r') as file:

				            config_data = json.load(file)

PeterSurda commented

2024-05-07 15:58:26 +02:00

Check here:

		collection/plugins/module_utils/syncthing_api.py
		Line 27 in 31c955c5b9
	
				    def _get_key_from_filesystem(self):

The config file is XML, not JSON.

Check here: https://git.bitmessage.org/Sysdeploy/ansible-modules-syncthing/src/commit/31c955c5b938fe59e47313e59f0b719beada1f45/collection/plugins/module_utils/syncthing_api.py#L27 The config file is XML, not JSON.

syncthing_sync.py Outdated

						
				@ -0,0 +19,4 @@

				    headers = {'X-Api-Key': api_key}

				    if method == 'GET':

				        response = requests.get(url, headers=headers)

				    elif method == 'POST':

PeterSurda commented

2024-05-07 16:04:30 +02:00

I think we only need GET.

I think we only need `GET`.

syncthing_sync.py Outdated

						
				@ -0,0 +52,4 @@

				    current_events = syncthing_request(f"{api_url}/rest/events?limit=1", api_key)

				    last_event_id = current_events[0]['id'] if current_events else 0

				    while True:

PeterSurda commented

2024-05-07 16:13:36 +02:00

It unnecessarily queries and sleeps. I'd prefer something like this:

is_idle = check_folder_idle()

while not is_idle:
    events = long_poll_events()
    for event in events:
        if event['type'] == 'StateChanged',
            event['data']['folder'] == folder_id:
            is_idle = event['data']['to'] == 'idle'
        if is_idle:
            break

I.e. it's not necessary to query the folder state in a loop, as we have the value in the event already. Long poll has a timeout so there's no need for a separate sleep.

We could also make the api_url and api_key variables global, or a class variable, or a module variable, or a class property, because it doesn't change, it's only set once, when the app starts.

It unnecessarily queries and sleeps. I'd prefer something like this: ``` is_idle = check_folder_idle() while not is_idle: events = long_poll_events() for event in events: if event['type'] == 'StateChanged', event['data']['folder'] == folder_id: is_idle = event['data']['to'] == 'idle' if is_idle: break ``` I.e. it's not necessary to query the folder state in a loop, as we have the value in the event already. Long poll has a timeout so there's no need for a separate sleep. We could also make the `api_url` and `api_key` variables global, or a class variable, or a module variable, or a class property, because it doesn't change, it's only set once, when the app starts.

PeterSurda requested changes 2024-05-07 16:17:28 +02:00

syncthing_sync.py Outdated

						
				@ -0,0 +35,4 @@

				def long_poll_events(api_url, last_event_id, api_key):

				    """ Long poll the events endpoint filtering by the last known event ID. """

				    events_url = f"{api_url}/rest/events?since={last_event_id}&timeout=60"

PeterSurda commented

2024-05-07 16:17:20 +02:00

One more thing, we can filter the events and then the code looks nicer:

events_url = f"{api_url}/rest/events?since={last_event_id}&events=StateChanged&timeout=60"

and then you don't need to check inside main for event['type'] anymore.

One more thing, we can filter the events and then the code looks nicer: `events_url = f"{api_url}/rest/events?since={last_event_id}&events=StateChanged&timeout=60"` and then you don't need to check inside `main` for `event['type']` anymore.