Sync resumption for SoUD syncs and cleanup handling #8183

bjester · 2021-07-01T17:23:46Z

Summary

Requires morango>=0.6.5
Refactors sync command logic into base class
Updates existing sync command to inherit new base class and to output the SyncSession ID for resuming
Adds new resumesync command that inherits new base class and accepts a --id parameter for a SyncSession to resume
Refactors SoUD syncing to keep session alive, and handles cleanup on disconnect on client instance and server
Integrates changes from Updates to handle addition and removal of interfaces python-zeroconf-py2compat#12
Refactors Zeroconf handling to stop restarting it and use new Zeroconf.update_interfaces

References

Resolves: #8019

Reviewer guidance

Situations that touch areas affected by this PR:

Network connects/disconnects
Other Kolibri instances appearing/disappearing from the network
Setting up Kolibri (as learner-only or importing facility)
Kolibri starts/stops
Syncing to networked Kolibris
Syncing to KDP
Learner-only device recurring syncing

Scenario: Full facility

Connect your device to a network/VPN with another Kolibri instance with a full facility
Start a fresh install of Kolibri
In the Kolibri setup wizard, choose Advanced > Import all data from an existing facility
Ensure the other Kolibri instance is discovered on the network
Import the facility from the other instance
Finish setting up Kolibri
Disconnect from the network with the other Kolibri instance
From Device > Facilities, choose SYNC for the facility you imported, and select "Local network or internet"
Verify that the other Kolibri instance is not shown as available, and keep the "Select network address" modal open
Reconnect to the network with the other Kolibri instance
Verify that the other Kolibri instance appears within 60 seconds
Complete syncing with the instance

Scenario: Learner-only

Connect your device to a network/VPN with another Kolibri 0.15 instance with a full facility
Start a fresh install of Kolibri
In the Kolibri setup wizard, choose Advanced > Import one or more user accounts from an existing facility
Ensure the other Kolibri instance is discovered on the network
Choose a facility and import a learner from it
Finish setting up Kolibri
Disconnect from the network with the other Kolibri instance
Wait a few minutes
Reconnect to the network with the other Kolibri instance
Verify that your device (eventually) syncs the instance again

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

…tween syncs Fix progress handler Keep sync session alive Remove venv accidentally added during pre-commit fixes

…us for events

jredrejo

I have tested it and hadn't found any problem with these changes.
Only the warning messages appearing multiple times on a not SoUD server that has been used to sync several SoUD:

INFO     Kolibri instance 'd34fa0e29c06817d732e4f7b81f3e2aa' joined zeroconf network; device info: {'application': 'kolibri', 'kolibri_version': '0.15.0a5.dev0+git.18.g94c233af', 'instance_id': 'd34fa0e29c06817d732e4f7b81f3e2aa', 'device_name': 'jose-xps', 'operating_system': 'Linux', 'subset_of_users_device': False}
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do automated SoUD syncing.
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
WARNING  Only Subsets of Users Devices can do this
INFO     Kolibri running on: http://172.17.0.1:10000/

kolibri/core/public/utils.py

bjester · 2021-10-06T17:56:04Z

Thanks @jredrejo! That does seem to be a small regression, which I'll look into

jamalex

Took a peek at one specific piece, per a request, and added a comment there.

kolibri/core/auth/management/utils.py

rtibbles

Code read through makes sense to me!

rtibbles · 2021-10-12T20:59:28Z

kolibri/core/discovery/utils/network/broadcast.py

+            self.name,
+            server=self.server,
+            address=self.ip,
+            port=self.port or DEFAULT_PORT,


When would we not have the port information here? (If this is just copy pasted from the previous implementation, feel free to ignore).

This does originate from the existing code, but I think you're right that we really don't need it

…updates

pcenov · 2021-10-14T15:02:24Z

Hi @bjester, today I had a look at this build on Windows, Ubuntu and Android and noticed that the auto sync is not functioning at all for the learner-only devices that I've setup on both Windows and Android using Ubuntu as a server.
Here are the logs and DB files in case you are not aware of that issue:
WindowsLogsAndDB.zip
UbuntuServerLogsAndDB.zip
The other main scenario about syncing a full facility is functioning correctly. Let me know if you need any additional info.

bjester · 2021-10-14T15:04:09Z

Thanks @pcenov, I'm aware of that and have been debugging that issue, but the cause is still unclear

pcenov · 2021-10-15T15:56:00Z

Tested again today on the latest Windows 10, Ubuntu and Android builds and can confirm that the sync functions correctly now.
I tried the scenarios listed in the description plus deleting the saved data on a learner-only device and restoring it to check whether it will sync back; disconnecting and reconnecting, deleting and adding resources and various other scenarios and didn't find any other issues.
I noticed that when I attempt to import a Facility or a channel from my local network I do see the Android learner-only device's URL as localhost (6b16) which seems incorrect but I don't know whether it's displayed intentionally or is a regression issue so bringing that to your attention as well.
@bjester @radinamatic

rtibbles

Nice - the cleanup of SoUD syncing jobs on unregistering is a nice addition too!

rtibbles · 2021-10-22T15:22:59Z

Force merging as linting issue is resolved in #8513

bjester force-pushed the morango-async branch 3 times, most recently from 20b6698 to f8ffc62 Compare July 21, 2021 18:04

bjester changed the title ~~Integration of Morango 0.6.0 changes~~ Dedicated management command for resuming a sync with Morango 0.6.0 Jul 26, 2021

bjester linked an issue Jul 26, 2021 that may be closed by this pull request

Sync command should support resuming a SyncSession through ID argument #8019

Closed

bjester changed the title ~~Dedicated management command for resuming a sync with Morango 0.6.0~~ Dedicated management command for resuming a sync with Morango 0.6.x Jul 26, 2021

indirectlylit changed the base branch from develop to release-v0.15.x July 27, 2021 18:22

indirectlylit added this to the 0.15.0 milestone Jul 27, 2021

bjester force-pushed the morango-async branch from ca39c56 to ccf2795 Compare July 30, 2021 00:44

bjester force-pushed the morango-async branch from d5ce74d to 42a662c Compare August 17, 2021 16:30

bjester force-pushed the morango-async branch from 42a662c to 73bc252 Compare August 30, 2021 16:25

bjester added 2 commits September 9, 2021 10:09

Integrate Morango sync resumption, keeping SoUD sync session alive be…

eddf5a6

…tween syncs Fix progress handler Keep sync session alive Remove venv accidentally added during pre-commit fixes

Fix cert filter handling, alter sync resume logic

bebaa57

bjester force-pushed the morango-async branch from 73bc252 to bebaa57 Compare September 9, 2021 17:10

Server-side cleanup of SoUD syncs on disconnect of SoUD instance

c35ba79

bjester added the work-in-progress Not ready for review label Sep 9, 2021

bjester changed the title ~~Dedicated management command for resuming a sync with Morango 0.6.x~~ Sync resumption for SoUD syncs and cleanup handling Sep 9, 2021

bjester marked this pull request as ready for review September 9, 2021 17:46

bjester added 6 commits September 9, 2021 11:09

Test fixes: correct facility parameter

da3845a

Update morango and update call to its cleanup command

a5568de

Fixes from testing

1014e11

Simplify client disconnect cleanup, fix bugs

f79c62d

Integration test

449c44a

Merge branch 'release-v0.15.x' into morango-async

cb46cf1

bjester added TAG: dependencies Pull requests that update a dependency file TAG: performance User-facing performance TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Sep 9, 2021

bjester requested review from jredrejo and jamalex September 9, 2021 23:13

bjester added 4 commits September 28, 2021 14:51

Integrate Zeroconf broadcast directly into process plugin, use magicb…

2e676a5

…us for events

Update SoUD cleanup to do as much as possible in a Job

3fc3139

Fixed merge conflicts

7d0f85c

Add comment regarding service browser

febd8b9

bjester mentioned this pull request Sep 29, 2021

Updates to handle addition and removal of interfaces learningequality/python-zeroconf-py2compat#12

Merged

Update zeroconf and fix tests

94c233a

jredrejo reviewed Oct 6, 2021

View reviewed changes

kolibri/core/public/utils.py Outdated Show resolved Hide resolved

Fix non-SoUD attempts to stop syncs

fe12a7f

bjester added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Oct 6, 2021

bjester requested a review from rtibbles October 6, 2021 22:54

Use task decorator

b5e41ca

jamalex requested changes Oct 11, 2021

View reviewed changes

kolibri/core/auth/management/utils.py Show resolved Hide resolved

rtibbles reviewed Oct 12, 2021

View reviewed changes

bjester added 3 commits October 12, 2021 14:17

Update how user arg influences sync filter logic

1815d83

Establish Zeroconf listener after registration

89a687a

Fix renamed command, fix missing ttl on renew, fix repeated zeroconf …

ef23aa6

…updates

bjester requested a review from jamalex October 14, 2021 14:44

Fix sync queuing issue, enhance logging, stop using deprecated warn

867b120

bjester added 2 commits October 21, 2021 15:05

Update morango, add more logging, and try to cancel SoUD jobs

91831ee

Fix test assertions after removing kwarg format

87ceedf

bjester requested review from rtibbles and jredrejo October 21, 2021 23:21

rtibbles approved these changes Oct 22, 2021

View reviewed changes

rtibbles merged commit 1797f89 into learningequality:release-v0.15.x Oct 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync resumption for SoUD syncs and cleanup handling #8183

Sync resumption for SoUD syncs and cleanup handling #8183

bjester commented Jul 1, 2021 •

edited

Loading

jredrejo left a comment

bjester commented Oct 6, 2021

jamalex left a comment

rtibbles left a comment

rtibbles Oct 12, 2021

bjester Oct 14, 2021

pcenov commented Oct 14, 2021

bjester commented Oct 14, 2021

pcenov commented Oct 15, 2021

rtibbles left a comment

rtibbles commented Oct 22, 2021

Sync resumption for SoUD syncs and cleanup handling #8183

Sync resumption for SoUD syncs and cleanup handling #8183

Conversation

bjester commented Jul 1, 2021 • edited Loading

Summary

References

Reviewer guidance

Situations that touch areas affected by this PR:

Scenario: Full facility

Scenario: Learner-only

Testing checklist

PR process

Reviewer checklist

jredrejo left a comment

Choose a reason for hiding this comment

bjester commented Oct 6, 2021

jamalex left a comment

Choose a reason for hiding this comment

rtibbles left a comment

Choose a reason for hiding this comment

rtibbles Oct 12, 2021

Choose a reason for hiding this comment

bjester Oct 14, 2021

Choose a reason for hiding this comment

pcenov commented Oct 14, 2021

bjester commented Oct 14, 2021

pcenov commented Oct 15, 2021

rtibbles left a comment

Choose a reason for hiding this comment

rtibbles commented Oct 22, 2021

bjester commented Jul 1, 2021 •

edited

Loading