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

dbus and rename plugin do not work together #404

Closed
machinekoder opened this issue Nov 25, 2015 · 20 comments
Closed

dbus and rename plugin do not work together #404

machinekoder opened this issue Nov 25, 2015 · 20 comments
Assignees
Labels

Comments

@machinekoder
Copy link
Contributor

There is a problem when the dbus and rename plugins are used at the same time. The dbus plugin seems not be aware of the renamed keys and notifies everyone with the raw key name. Furthermore it looks like the change detection does not work either.

$ kdb mount `pwd`/mkwrapper.ini system/machinekit/cnc ini type dbus rename tolower=0
$ kdb set system/machinekit/cnc/display/display test2
$ dbus-monitor --system type='signal',interface='org.libelektra',path='/org/libelektra/configuration'
ignal sender=org.freedesktop.DBus -> dest=:1.544 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.544"
signal sender=:1.545 -> dest=(null destination) serial=2 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0"
signal sender=:1.545 -> dest=(null destination) serial=3 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/BACKLASH"
signal sender=:1.545 -> dest=(null destination) serial=4 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/FERROR"
signal sender=:1.545 -> dest=(null destination) serial=5 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/HOME"
signal sender=:1.545 -> dest=(null destination) serial=6 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/HOME_FINAL_VEL"
signal sender=:1.545 -> dest=(null destination) serial=7 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/HOME_IGNORE_LIMITS"
signal sender=:1.545 -> dest=(null destination) serial=8 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/HOME_IS_SHARED"
signal sender=:1.545 -> dest=(null destination) serial=9 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/HOME_LATCH_VEL"
signal sender=:1.545 -> dest=(null destination) serial=10 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/HOME_OFFSET"
signal sender=:1.545 -> dest=(null destination) serial=11 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "system/machinekit/cnc/AXIS_0/HOME_SEARCH_VEL"
....

mkwrapper.ini is here:
https://github.com/strahlex/mkwrapper-sim/blob/param/mkwrapper.ini

@markus2330
Copy link
Contributor

When I mount it that way, I get:

system/elektra/mountpoints/system\/machinekit\/cnc/setplugins/#0#resolver
system/elektra/mountpoints/system\/machinekit\/cnc/setplugins/#1#type#type#
system/elektra/mountpoints/system\/machinekit\/cnc/setplugins/#2#rename
system/elektra/mountpoints/system\/machinekit\/cnc/setplugins/#5#ini
system/elektra/mountpoints/system\/machinekit\/cnc/setplugins/#6#sync#sync#
system/elektra/mountpoints/system\/machinekit\/cnc/setplugins/#7#resolver
system/elektra/mountpoints/system\/machinekit\/cnc/setplugins/#8#dbus

which gives dbus the keyset rename+INI got.

Possible solutions:

  1. Add dbus also to the presetstorage (as very first, you should even give ordering constraint against rename so that it cannot be mount wrongly) and do change detection then (sending out notifications still in postcommit).
  2. Global plugins (see in Global plugins #324) would give you all keys with sync flag correctly. This would also solve an additional problem (you might face later): you can implement a transaction across plugins (only in the global plugin you know that no other plugin will come after you).
  3. Implement an unrename feature that in rename that places rename also in postcommit (in first position) and reverts what it has done before.

What do you think?

@machinekoder
Copy link
Contributor Author

Solution 1 and 2 sound best. What is blocking the global plug-ins from being merged?

@markus2330
Copy link
Contributor

The PR has some unresolved reviewer comments and needs a rebase.
@tom-wa Any plans on the timeline?

After the PR we need a kdb global-mount tool so that the global mountpoints actually can be used.

@markus2330 markus2330 added the rc label Jan 12, 2016
@markus2330 markus2330 added this to the 0.8.15 milestone Jan 12, 2016
@markus2330 markus2330 removed the rc label Jan 12, 2016
@markus2330
Copy link
Contributor

Should be fixed now with global-mounts:

kdb mount `pwd`/machinekit.ini system/machinekit/cnc ini array= type rename tolower=0 # no notification needed here
kdb global-mount logchange # or dbus or both

Btw. INI plugin is not fully ready for the release yet, but should be done soon.

@markus2330
Copy link
Contributor

Please confirm when you think it is ready.

markus2330 pushed a commit that referenced this issue Feb 10, 2016
@markus2330
Copy link
Contributor

Btw. there are still notifications on any change (ADD+REM) for keys that are lowercase (wrong) in the configuration file. The renaming currently does not work bidirectional, so it does not ensure that new keys are written correctly. (See also discussion with @tom-wa #485)

@markus2330
Copy link
Contributor

(Btw. I only tested with dump, as said, INI is on its way. With current INI there are still problems.)

markus2330 pushed a commit that referenced this issue Feb 13, 2016
otherwise logging + notifications plugins would not have sync information anymore
for #404 when global plugins are used
@markus2330 markus2330 removed this from the 0.8.15 milestone Feb 13, 2016
@markus2330 markus2330 mentioned this issue Feb 16, 2016
10 tasks
@markus2330 markus2330 added this to the 0.8.16 milestone Feb 17, 2016
@markus2330 markus2330 removed this from the 0.8.16 milestone Apr 27, 2016
@markus2330
Copy link
Contributor

Still open, INI seems to change every key:

$ kdb set system/machinekit/cnc/pruconf/news fff
Set string to fff
changed key: system/machinekit/cnc/ABP
changed key: system/machinekit/cnc/ABP/SCALE
changed key: system/machinekit/cnc/ABP/STEPGEN_MAX_ACC
changed key: system/machinekit/cnc/ABP/STEPGEN_MAX_VEL

I think the most robust way is to make the change detection as early as possible so that it is not dependent on every plugin not making changing to keys.

@markus2330
Copy link
Contributor

@waht is this now resolved as dbus does not notify every value individually anymore?

@stale
Copy link

stale bot commented May 6, 2020

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@stale stale bot added the stale label May 6, 2020
@stale
Copy link

stale bot commented May 20, 2020

I closed this issue now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@markus2330
Copy link
Contributor

@atmaxinger this is (hopefully) the last important issue that requires a decision in the context of change tracking.

Can you please write a decision with a summary of this issue. The problem also needs to be better clarified:

  • What about plugins changing values back and forth (normalization, e.g. true -> 1, 1-> true)?
  • What about plugins adding and removing metadata (like spec)

#955 is also part of this issue. Would it be possible to use iconv to rename keys?

@markus2330 markus2330 reopened this Oct 28, 2022
@markus2330 markus2330 removed the stale label Oct 28, 2022
@atmaxinger
Copy link
Contributor

atmaxinger commented Oct 28, 2022

For me it looks like the main issue does not exist anymore.

My understanding is that these are two issues

  1. The dbus plugin does not use the names as generated by the rename plugin.
  2. The dbus plugin outputs all keys as changed. AFAIR this is a known issue that I also stumbled upon when I created the send-notification hook.

Number 2 is still an issue in certain cases, particularly here.

But Number 1 doesnt exist anymore, as dbus is now a notification-send hook plugin and gets executed after the poststorage phase.

This is the test setup that I have used:

kdb set system:/elektra/hook/notification/send/plugins/#0 dbus
kdb mount mkwrapper.ini user:/mkwrapper ini type rename get/case=tolower,set/case=toupper

At first, we register the dbus plugin to be used as a notification-send hook.
Then we mount mkwrapper.ini as user:/mkwrapper. The syntax for the rename plugin also changed since this issue was opened. In this example, we tell the rename plugin to transform the keys into lower case for use within Elektra, and to upper case for storage within the file.

kdb set user:/mkwrapper/cnc/display/display test1
kdb set user:/mkwrapper/cnc/display/display test2
> dbus-monitor --session type='signal',interface='org.libelektra'
signal time=1666947466.170558 sender=:1.343 -> destination=(null destination) serial=2 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyAdded
   string "user:/mkwrapper/CNC/DISPLAY/DISPLAY"
signal time=1666947537.394764 sender=:1.344 -> destination=(null destination) serial=2 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyChanged
   string "user:/mkwrapper/CNC/DISPLAY/DISPLAY"

mkwrapper.ini:

;Ni1
; Generated by the ni plugin using Elektra (see libelektra.org).

CNC/DISPLAY/DISPLAY = test2

@atmaxinger
Copy link
Contributor

atmaxinger commented Oct 28, 2022

To expand why the change tracking of the dbus plugin does not work correctly in this case: The dbus plugin does not do a value-based change tracking, but instead uses keyNeedsSync to find out whether a key has been flagged as changed.

The way the rename plugin works, is that it deletes the old key and creates a new key with the renamed name. This causes keyNeedsSync to be true for every renamed key.

This is why the dbus plugin wrongly comes to the conclusion that the key changed. (And another reason WHY we need unified change tracking in Elektra, as proposed in #4554)

@atmaxinger
Copy link
Contributor

@markus2330 there is actually a completely different new decision also in here: How should notification plugins deal with renamed keys?. If dbus uses the renamed keys, this breaks dbusrecv plugin as it will not find the keys with their renamed names in the KDB if get and set transformation are different!

@kodebach
Copy link
Member

As @markus2330 said, it's not just about renamed keys. The decision is in general: Which version of the KDB should be used for notifications/change tracking?

Currently we use the version of the KDB that is present to applications. I think that is an easy option and probably the most consistent approach. Especially, since there could be plugins that add/remove keys based on other keys.

Another option is using the version that is returned by storage plugins. I think that would just lead to inconsistencies and confusion. It would also complicate things if you want to receive notifications inside applications. If people really need this, they should probably watch the storage file (or whatever storage is used) for changes and then call the storage plugin directly, when they detect a change.

With the simple phases approach we now have, I don't think there is any other option. We can't distinguish between plugins that change key names (or add/remove keys), plugins that only change values and plugins that don't change anything.

@markus2330
Copy link
Contributor

For me it looks like the main issue does not exist anymore.

My understanding is that these are two issues

  1. The dbus plugin does not use the names as generated by the rename plugin.
  2. The dbus plugin outputs all keys as changed. AFAIR this is a known issue that I also stumbled upon when I created the send-notification hook.

Sorry, I didn't specify. This is a problem of such real-world issues, they often don't focus on some specific problem in a Elektra but to a problem a user had. I meant the problem "2.". But the problem is not specific to dbus but to how we detect changes.

And another reason WHY we need unified change tracking in Elektra, as proposed in #4554

Absolutely, I am so happy that you work on it! ❤️

How should notification plugins deal with renamed keys?

Excellent observation, this work will definitely earn the level of "master of science". Both problems are called in the scientific literature "feature-interaction problems".

Also as @kodebach said, there are other feature-interaction problems on general transformations (KeySets changing in the chain of plugins). Probably some restrictions of what plugins are allowed to do are necessary. I am not sure if renaming outside of storage plugins ever was a good idea but more about that when we have decisions.

@kodebach
Copy link
Member

Probably some restrictions of what plugins are allowed to do are necessary.

I've been advocating for this for years now (e.g. #3497 (comment)) and was always shot down... Now that new-backend is done, this will be hard to implement without touching the fundamentals again.

One option I see is to define: "All key names are fixed after the storage phase. No keys can be added, renamed or deleted" (*) This would immediately prohibit a whole bunch of existing plugins. To solve that the backend plugin would need to be adapted to call plugins added/removing/renaming keys during the storage phase. That in turn means plugins need to declare in their contract what they actually do.

(*) needs slightly different wording to allow spec to do it's job. And also needs to be adapted for kdbSet.

Copy link

github-actions bot commented Mar 2, 2024

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@github-actions github-actions bot added the stale label Mar 2, 2024
Copy link

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants