Design Discussion: D-Bus Signal: Notify Property Changed

[Pavel Březina]

https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSignalPropertyChanged

= 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.

== D-Bus Interface ==

=== org.freedesktop.DBus.Properties ===

==== Signals ====

* !PropertiesChanged(s interface_name, {sv} changed_properties, as 
invalidated_properties)
  * 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.

=== 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''

==== 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
  * on correct IFP termination
   * the TDB file is deleted
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
   2. Traverse the TDB and remember dn and properties for all modified 
objects
   3. Flush TDB
   4. Release the lock
   5. Create and emit D-Bus signal per each object that is exported on 
IFP bus and supports ''!PropertiesChanged'' signal

==== LDB Plugin Side ====

1. If TDB file does not exist just quit
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)

=== How To Test ===

1. Hook onto ''!PropertiesChanged'' signal, e. g. with ''dbus-monitor'̈́'
2. Trigger change of user/group
3. Signal should be recieved

=== Questions ===

1. Do we want to use ''changed_properties'' or ''invalidated_properties''

=== Authors ===
  * Pavel Březina

[Jakub Hrozek]

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 ====
> 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
> on correct IFP termination
> the TDB file is deleted
> A periodic task ''IFP: notify properties changed'' is created, it is
>
> responsible for emitting the ''!PropertiesChanged'' signal
> Periodic task flow:
> Lock TDB for read-only access

Umm, this sounds dangerous. Would the writers be blocked while the IFP
locks TDB for reading?
> Traverse the TDB and remember dn and properties for all modified
>
> objects
>   3. Flush TDB
>   4. Release the lock
>   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?
> ==== LDB Plugin Side ====
> If TDB file does not exist just quit

You mean gracefully terminate the plugin operation, right?
> 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?
> === How To Test ===
> Hook onto ''!PropertiesChanged'' signal, e. g. with ''dbus-monitor'̈́'
> 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 :-))
> Signal should be recieved
>
> === Questions ===
> 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?

[Jakub Hrozek]

On Tue, Jun 23, 2015 at 10:25:43AM +0200, Jakub Hrozek wrote:
> > === 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

~~~~~~~~~~~~~~~~~~~
                                    One more remark. The implementations
                                    shouldn't be bound to TDB, but to a
                                    keyval store, in case we wanted to
                                    convert to another database later.

[Simo Sorce]

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 ====
> > 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 ?
> > A periodic task ''IFP: notify properties changed'' is created, it is
> >
> > responsible for emitting the ''!PropertiesChanged'' signal
> > Periodic task flow:
> > 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).
> > 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.
> > 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 ====
> > 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.
> > 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 ===
> > Hook onto ''!PropertiesChanged'' signal, e. g. with ''dbus-monitor'̈́'
> > 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 :-))
> > Signal should be recieved
> >
> > === Questions ===
> > 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.