Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Collect and suggest devices for missing cluster links #5758

Draft
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@acolomb
Copy link
Contributor

commented May 30, 2019

Purpose

This is an early prototype of the "suggested devices" feature discussed on the forum. The PR includes:

  • The backend changes for collecting info about device candidates to suggest, from the ClusterConfig messages. Currently stored inside the config, because it is easy to experiment and debug. This needs to be moved to the database though, since on a large cluster this dumps a lot of data to the XML and it touches the config for every received ClusterConfig message with additional candidates. The general structure of collected data could be reused, though.

  • PR #5756 as a prerequisite for splitting the GUI's folder sharing device list in groups.

  • Another section (hidden if empty) in the GUI's Edit Folder dialog - Sharing tab. It lists both known devices that have only an indirect link to this folder (category 2 from the forum discussion), as well as completely unknown devices (category 4) that would have been added if the Introducer feature was used. The latter are included as clickable device IDs, with an appended alias hint only if all introducers agree on the name.

Known Issues

  • Actually adding unknown devices does not work correctly. The folder and device editing dialogs share a editingExisting flag, which obviously needs to be separate when launching one modal dialog from the other. Pre-filling some form fields is also non-functional, see FIXMEs in the code.

  • The new section is currently only used when editing a folder, not when adding a pending folder shared from another device. In theory, there could already be candidate devices, so showing them in that incarnation of the dialog would make sense.

  • This should not be released to the userbase at large because of the inefficient storage of candidate devices in the configuration. Large clusters will experience horrible performance!

  • Noticed while testing: The ClusterConfig message currently includes any remote device (B) which the sender (A) "shares" the folder with. This should possibly be limited to "mutually shared" links, where B also shares with A. Once the device suggestions are merged, this extra data could make it easier to trick people into sharing data with the wrong device maybe?

  • Collected candidate devices are not being cleaned up yet. Need to decide on a strategy for keeping these records clean.

  • Some smaller issues and questions marked FIXME in the code. RFC.

Testing

All devices are listed under the expected category in a cluster with five devices. Selecting the suggested devices works, except for unknown devices. Verified the config entries both in the XML as well as over the JSON API.

Screenshots

Screenshot_2019-05-31 devtest Syncthing

Documentation

TODO: Once the data storage is agreed upon, any additions need to go to the API docs. Also need a new section explaining the concept of "suggested devices" in the Usage guide. Note that this is not a major addition to the inner workings of Syncthing, just exposing data that was already available.

acolomb added some commits May 11, 2019

lib/model: Check and document additional cases in ClusterConfig.
Currently only the remote side's view of our own device and of
themselves is filtered from the ClusterConfig message.  Extend those
checks for devices that would be good candidates for additional
sharing links within the cluster, since they share the same folder ID,
but not directly with our local device.  Those might be completely
unknown devices so far.

Relevant discussion:
https://forum.syncthing.net/t/manual-intervention-for-introducer-feature/12212
lib/model: Simplify access to Folder and Device configuration.
Use the accessor methods with a device argument instead of looking up
in a temporary map of the same data.
lib/config: Add data structures to remember candidate devices.
Modeled after the pending device mechanism, the new
ObservedCandidateDevice structure stores information about remote
devices which were advertised by other remote nodes.

From the ClusterConfig message, it can collect the advertised certName
and the address list.  A list of introducers may be included, reusing
the ObservedDevice structure type.  In contrast to the pending device
entries, the Name field should hold the candidate device's name as the
introducer calls it, not the candidate's own advertised name.

Functions for maintaining duplicate-free introducer entries and
address lists are defined along with the structure type.
lib/config: Add candidate device list to folder configuration.
Include a list of candidate devices that share the folder, but not
directly with us.  Implemented as a slice internally, and serialized
to XML as repeated candidateDevice tags; to JSON as an array.

A candidate device can be recorded for this folder through the new
AddOrUpdateCandidateDevice() method, optionally with additional info
such as the introducer and its name for the candidate device.  Passed
addresses are collected into a list for possibly trying to contact the
device later.
lib/config: Include candidate device recording in config wrapper.
Add convenience method AddOrUpdateFolderCandidateDevice() to record
the given device after finding the right FolderConfiguration object.
Handles config locking and saving atomically.
lib/config: Omit empty address in ObservedDevice serialization.
Add "omitempty" to the address XML attribute.  Allows for reusing the
datatype when no address needs to be stored.
lib/model: Store candidate devices to config.
When iterating over a ClusterConfig message, implement the cases for
known or unknown devices that the sender mentions where a missing
cluster link may be established.  Only concerns devices that are
already linked to us indirectly through a folder.

Add some FIXME comments for branches that skip to the next folder
before we had a chance to record the candidate devices.
gui: Reduce code duplication among {de}selectAllDevices().
Instead of repeating the complete looping code, use an optional
boolean argument in selectAllDevices().  deSelectAllDevices() just
flips it to false and reuses the other function.
gui: Split device list in folder sharing options panel.
Split the device list into two sections, for currently shared and
unshared devices.  Makes it clearer what changes are being made, just
by looking at the checkmarks.  Initially the first section has all
checked and the second section has all unchecked.  An empty section is
hidden.

Implement device list filters for the separated categories,
temporarily stored within the GUI model's currentFolder object while
the folder editing modal dialog is active.  Adjust bulk selection
functions.
gui: List suggested devices in folder sharing options.
Include a third category (if non-empty) in the folder sharing device
list.  List the devices collected in the candidateDevices config in
two possible formats:

For already configured devices, show a check mark and the friendly
name; effectively just move entries from the "unrelated" section.

For previously unknown candidate devices, list the complete device ID
as a clickable link, which should open the "Add Device" modal dialog,
but that is not implemented yet [TODO].  If all devices listed under
introducedBy agree on a common name, show that as an alias hint after
the ID.

The lists of suggested devices are temporarily stored within the
currentFolder object, until the folder edit dialog is saved and
closed.

Implement additional {de}selectAllSuggestedDevices() functions that
only operate on the checkmarks, not the unknown devices.
gui: [WIP] Show the Add Device modal dialog to add candidates.
Implement addDeviceAndShare() to show a partially pre-filled dialog
when clicking on a suggested yet unknown candidate device for a
folder.

WARNING: This does not work correctly yet, mainly because the
editingExisting flag is shared between folder and device editing
dialogs.  This change is little tested and has more issues.

func (w *wrapper) AddOrUpdateFolderCandidateDevice(folder string,
device, introducer protocol.DeviceID, name, certName string, addresses []string) {
defer w.Save()

This comment has been minimized.

Copy link
@golangcibot

golangcibot May 30, 2019

Error return value of w.Save is not checked (from errcheck)

This comment has been minimized.

Copy link
@acolomb

acolomb May 30, 2019

Author Contributor

Copied verbatim from AddOrUpdatePendingDevice() and others. Sorry I'm new to Go and have no idea how that should be checked in conjunction with defer.

This will not live in the config anyway, long-term.

Show resolved Hide resolved lib/config/observed.go Outdated
Add empty method implementation for mocked_config_test.
Fix missing method reported in CI test.

go fmt to wrap closing curly brace for whatever reason when it does
not wrap for the two methods above...

@acolomb acolomb force-pushed the acolomb:suggest-candidate-devices branch from 761e883 to 59174cf May 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.