syncthing sync completed or not #1

shailaja · 2024-04-26T07:08:23Z

shailaja commented

2024-04-26 07:08:23 +00:00

No description provided.

shailaja added 1 commit 2024-04-26 07:08:24 +00:00

0fb38fd386

syncthing sync completed or not

PeterSurda commented

2024-05-03 03:28:02 +00: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 added 1 commit 2024-05-07 12:11:27 +00:00

9009f205a8

Syncthing Monitoring via API

shailaja force-pushed sync from 9009f205a8 to da5c266da9

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

Compare

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

PeterSurda requested changes 2024-05-07 14:14:23 +00: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 13:58:26 +00:00

Check here: 31c955c5b9/collection/plugins/module_utils/syncthing_api.py (L27)

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 14:04:30 +00: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 14:13:36 +00: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 14:17:28 +00: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 14:17:20 +00: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.

shailaja added 1 commit 2024-05-10 11:49:36 +00:00

0c5683c38b

Syncthing Monitoring via API

shailaja force-pushed sync from 0c5683c38b to 037431821b

2024-05-10 11:51:02 +00:00

Compare

shailaja requested review from PeterSurda 2024-05-10 11:51:32 +00:00

This pull request can be merged automatically.

You are not authorized to merge this pull request.

You can also view command line instructions.