[SSSD] Design Discussion: D-Bus Signal: Notify Property Changed

Simo Sorce simo at redhat.com
Mon Jun 29 21:52:42 UTC 2015


On Tue, 2015-06-23 at 10:25 +0200, Jakub Hrozek wrote:
> On Tue, Jun 09, 2015 at 02:17:31PM +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSignalPropertyChanged
> 
> Sorry for the delay in review, the 1.13 Alpha release took precedence..
> 
> > 
> > = D-Bus Signal: Notify Property Changed =
> > 
> > Related ticket(s):
> >  * https://fedorahosted.org/sssd/ticket/2233
> > 
> > Related design page(s):
> >  * https://fedorahosted.org/sssd/wiki/DesignDocs/DBusResponder
> > 
> > === Problem statement ===
> > 
> > This design document describes how to implement
> > org.freedesktop.DBus.Properties.!PropertiesChanged signal for SSSD objects
> > exported in the IFP responder.
> 
> General comment:
> The problem statement should also describe why we want to implement a
> feature. In this case it's because we want to enable applications to
> subscribe to asynchronous notifications from domain users.
> 
> > 
> > == D-Bus Interface ==
> > 
> > === org.freedesktop.DBus.Properties ===
> > 
> > ==== Signals ====
> > 
> > * !PropertiesChanged(s interface_name, {sv} changed_properties, as
> > invalidated_properties)
> 
> It would be nice to format the interface in non-proportional font,
> especially the {sv} annotation is hard to read.
> 
> >  * interface_name: name of the interface on which the properties are defined
> >  * changed_properties: changed properties with new values
> >  * invalidated_properties: changed properties but the new values are not
> > send with them
> >  * this signal is emitted for every property annotated with
> > org.freedesktop.DBus.Property.!EmitsChangedSignal, this annotation may be
> > also used for the whole interface meaning that every property within this
> > interface emits the signal
> > 
> > === Overview of the solution ===
> > 
> > Changes in properties are detected in new LDB plugin inside a ''mod'' hook.
> > The plugin writes list of changed properties in a TDB-based changelog which
> > is periodically consumed by IFP responder. IFP then emits !PropertiesChanged
> > signal per each modified object.
> 
> Hmm, are you sure that mod is sufficient? Especially since you say that
> the annotation can be used for the whole interface, then it sounds like
> we might need to catch also adds and dels?
> 
> > 
> > === Implementation details ===
> > 
> > ==== TDB Format ====
> > 
> > * '''TDB Name''': ''ifp_changelog.tdb''
> > * '''Key''': dn of modified object
> > * '''Value''': chained list of modified properties in the form
> > ''total_num\0prop1\0prop2\0...\0''
> 
> What about added and deleted properties, do we also treat them as
> changed?
> 
> > 
> > ==== IFP Side ====
> > 
> > 1. TDB database is created on IFP start and deleted on IFP termination.
> >  * on IFP start:
> >   * if TDB file does not exist it is created
> >   * if TDB file exist (unexpected termination of IFP) it is flushed, we do
> > not care about the data inside

Why this ?
The whole point of using a TDB is that clients can write to it even if
the IFP is not around. If you remove it you lose any event that has
piled up since (potentially before) the crash.

> >  * on correct IFP termination
> >   * the TDB file is deleted

Same as above, why do you want to delete the TDB and lose any
notification that may come in while the IFP is down ?

> > 2. A periodic task ''IFP: notify properties changed'' is created, it is
> > responsible for emitting the ''!PropertiesChanged'' signal
> >  * Periodic task flow:
> >   1. Lock TDB for read-only access
> 
> Umm, this sounds dangerous. Would the writers be blocked while the IFP
> locks TDB for reading?

Not really dangerous, the IFP should read fats, but the TDB should not
be locked in general. TDB has the marvelous property of being multiple
read/write free with entry level locking. The idea is that clients
should just lock the records they are writing, while the IFP
periodically traverses the TDB (when to do that TBD, pardon the pun).

> >   2. Traverse the TDB and remember dn and properties for all modified
> > objects
> >   3. Flush TDB
> >   4. Release the lock

Records should be deleted one by one as they are processed.
This way if the IFP crashes while processing one event all the others
are not immediately lost, and will be reprocessed at restart. There is a
chance that the record may cause a new crash, so a record should be
deleted from the TDB before it is processed, but this should happen one
record at a time.

> >   5. Create and emit D-Bus signal per each object that is exported on IFP
> > bus and supports ''!PropertiesChanged'' signal
> 
> What would be the default period for this task?

Ideally it is not a period but instead clients send a signal (like
SIGUSR1) to the IFP process and that cause it to go and process the TDB.
When started the IFP also goes and process any event that has been
queued potentially discarding the ones that are too old. (Events need to
be timestamped).

> > 
> > ==== LDB Plugin Side ====
> > 
> > 1. If TDB file does not exist just quit
> 
> You mean gracefully terminate the plugin operation, right?

The TDB should either be created on first access, or you must make sure
it is created when the monitor starts IMO. You do not want a race where
the IFP starts after one of the sssd_be processes and these will never
notify.

> > 2. If modified object supports the signal store it in the TDB
> > 
> > === Configuration changes ===
> > 
> > In IFP section:
> > * '''ifp_notification_interval''': period of ''IFP: notify properties
> > changed'', disabled if 0, default 300 (5 minutes)
> 
> This sounds a bit too long, the changelog should be mostly empty and
> checking it is a simple r/o operation, any reason why we shouldn't check
> much more frequently?

As indicated before, there may be a cycle but in general I think we
should rather use signals to kick the process or notifying the clients.
The process ID to signal should be obtained somehow, probably by having
the IFP write it down in the TDB file as soon as it is started (or it
may be maintained by the monitor in the TDB or other file or even
communicated via internal dbus commands, so that the other processes do
not have to constantly read the pid file/record every time and rely on
the monitor to tell them when the IFP has a new PID).

> > 
> > === How To Test ===
> > 
> > 1. Hook onto ''!PropertiesChanged'' signal, e. g. with ''dbus-monitor'̈́'
> > 2. Trigger change of user/group
> 
> It should be made clear that the change can be done using /any/ update,
> ie even getent group updating a user counts. (This is mostly us bragging
> about how awesome the feature is :-))
> 
> > 3. Signal should be recieved
> > 
> > === Questions ===
> > 
> > 1. Do we want to use ''changed_properties'' or ''invalidated_properties''
> 
> Hmm, both are part of the D-Bus interface itself, right? Do we have a
> choice?


HTH,
Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list