Service to suggest addons based on running processes #3904

holgerfriedrich · 2023-12-08T00:53:43Z

This is a first draft of a new finder to suggest addons based on the software running on the OH server.
It checks the process list and tries to detect known software.

It has been mentioned in
#3868 (comment)

It will require changes in the addon.xml of the binding as well, but no change to the xsd, as the given match-property is reused. Here an example for influxdb.

       <discovery-methods>
               <discovery-method>
                       <service-type>process</service-type>
                       <match-properties>
                               <match-property>
                                       <name>command</name>
                                       <regex>.*influxd.*</regex>
                               </match-property>
                       </match-properties>
               </discovery-method>
       </discovery-methods>

@andrewfg Maybe you can have a short look if I forgot something before I try to create a full distro package to test it.
Any recommendation on how to test in a simpler way would be very welcome....

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

mherwege · 2023-12-08T06:30:07Z

@holgerfriedrich Nice to see a next step.

The easy way to test this is to put an xml file in the userdata/addons folder with the additional discovery content. Any xml file with valid content will be picked up from there. So there is no need to do a full build.
I am using Eclipse for development and have always been testing this functionality this way, running from Eclipse and adding an xml file with discovery criteria. If you use another IDE, you may have to replace the bundle.

A few questions and remarks I have:

Shouldn’t the process info be cached and refreshed at intervals? My concern is getting the processes could be slow and might result in a slow when called from the UI.
Would this work equally well on Windows and linux? Or would it require multiple criteria to be defined.
We went out of our way to be able to switch off discovery for UPnP and mDNS. I don’t think it is required here as this has lower impact. But we should all agree on it.

...rocess/src/main/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java

kaikreuzer · 2023-12-08T06:58:40Z

We went out of our way to be able to switch off discovery for UPnP and mDNS. I don’t think it is required here as this has lower impact. But we should all agree on it.

I would prefer to have the same mechanism in place for all the discoveries. Having processes scanned in the background might not be something everybody wants.

mherwege · 2023-12-08T07:38:54Z

Having processes scanned in the background

The PR is it is does not cache the running processes anywhere, so there is no background scanning. It will just make a call to get the processes when called from the suggestions REST endpoint. In the UI that means: when running the setup wizard or when opening the addon store. We can of course allow to disable that call as well. The main reason to be able to enable/disable was to avoid having all UPnP/mDNS dependencies loaded and running in the background only for addon discovery. As there is no extra dependency here, and as long as there is no scheduled scan and result caching, I don't see the point. But of course, see my concern about caching. If it is required, I think this becomes a different discussion.

kaikreuzer · 2023-12-08T07:51:31Z

But of course, see my concern about caching.

Yes, that's why I was assuming that the logic of the PR will be changed to do some background scanning in future.

andrewfg · 2023-12-08T09:54:00Z

no change to the xsd, as the given match-property is reused.

I am glad to hear that :) the match-property model should suit most kinds of finder.

andrewfg · 2023-12-09T00:29:58Z

Any recommendation on how to test in a simpler way would be very welcome....

None really. It crosses over from addons to core so the only way to test is a full distro.

mherwege · 2023-12-09T11:59:17Z

The easy way to test this is to put an xml file in the userdata/addons folder with the additional discovery content. Any xml file with valid content will be picked up from there. So there is no need to do a full build.

@andrewfg @holgerfriedrich I think this is the easy way to test without a full build. You can test in the IDE.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich · 2023-12-09T17:47:26Z

Thank you all for the comments.

I finally got it working for knxd.....

18:35:45.323 [TRACE] [very.addon.process.ProcessAddonFinder] - Candidate binding-knx, pattern ".*knxd"
18:35:45.324 [TRACE] [very.addon.process.ProcessAddonFinder] - Candidate binding-knx, pattern ".*knxd.exe$"
18:35:45.324 [DEBUG] [very.addon.process.ProcessAddonFinder] - Suggested add-on found: binding-knx

I will create a corresponding PR in addons repo. The finder allows to specify multiple patterns.
Windows and Linux can be handled using separate patterns if needed.

I did not manage yet to test without building a distro package. Still not sure how to do core development with Visual Studio Code.

For now, it is without caching and background scanning. getSuggestedAddons should finish quickly, so for now no need to extend this.

holgerfriedrich · 2023-12-09T22:22:22Z

Both this PR and PR in addons repo ready for review.

The UI (add-on store) shows suggestions only for bindings. The wizard picks up all suggested add-ons.

mherwege · 2023-12-11T11:48:57Z

The UI (add-on store) shows suggestions only for bindings.

PR openhab/openhab-webui#2213 created to solve this.

...covery.addon/src/main/java/org/openhab/core/config/discovery/addon/AddonFinderConstants.java

mherwege · 2023-12-11T12:07:57Z

features/karaf/openhab-core/src/main/feature/feature.xml

@@ -83,6 +83,12 @@
 		<feature dependency="true">openhab.tp-jmdns</feature>
 	</feature>

+	<feature name="openhab-core-config-discovery-addon-process" version="${project.version}">


If you add this feature to openhab-runtime-base you will not need to add this finder to the constants in AddonFinderConstants.java. The feature will then automatically get installed and not through the service. The only reason to do it through the service is to allow enabling/disabling.

I think someone did already ask to have enabling/disabling. ??

For me, that's conditional. If retrieving the processes is too high a load, we should. Otherwise I don't see the point. Right now, the implementation does not cache anything and does a call to retrieve the processes any time the suggestions are retrieved. If that is quick, no need to enable/disable a little piece of code. There is nothing running in the background in separate threads either.
@kaikreuzer Did not react on my feedback on it. If he feels strongly, then, of course it needs to be there, but it should also be added to the configuration parameters.

@mherwege I am OK with that.

done as suggested, just did the verification - still works

mherwege · 2023-12-11T12:17:46Z

...rocess/src/main/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java

+
+    private static final String COMMAND = "command";
+
+    public static final String SERVICE_TYPE = SERVICE_TYPE_PROCESS;


You can eliminate these constants from AddonFinderConstants.java and define them here instead with the same logic. Only refer to ADDON_SUGGESTION_FINDER in AddonFinderConstants.java.

...but I had to make ADDON_SUGGESTION_FINDER public...

...rocess/src/main/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

mherwege · 2023-12-11T21:13:54Z

LGTM

andrewfg · 2023-12-15T19:56:29Z

@holgerfriedrich just for the avoidance of doubt: di you adapt to the new addon.xml schema?
@kaikreuzer / @mherwege I would suggest to wait until after those new addon.xml schema PRs have been merged.

mherwege · 2023-12-15T20:07:41Z

@holgerfriedrich just for the avoidance of doubt: di you adapt to the new addon.xml schema? @kaikreuzer / @mherwege I would suggest to wait until after those new addon.xml schema PRs have been merged.

There is no impact of the new format for this PR. There are no discovery parameters. So no reason to wait if a maintainer agrees.

andrewfg · 2023-12-15T20:39:28Z

There are no discovery parameters.

@holgerfriedrich the whole point of the revised schema is because you mis-used the match-properties as if they would be discovery-parameters. Therefore you need to change your mis-used properties to parameters instead.

@mherwege for info..

mherwege · 2023-12-15T20:57:53Z

@andrewfg I think you confuse this PR with this one: #3920

The other one is the one I made the change for, and is not yet ready.

andrewfg · 2023-12-15T21:00:36Z

The other one is the one I made the change for

Oops! Sorry.

kaikreuzer

lgtm, thanks!

...rocess/src/main/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java

…in/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer · 2023-12-17T14:02:09Z

@holgerfriedrich I just tried the latest distro snapshot (3787) and see this log during startup:

15:00:02.417 [WARN ] [ternal.xml.AddonInfoAddonsXmlProvider] - File 'openhab-addons.xml' has invalid content

Any idea why this happens?

kaikreuzer · 2023-12-17T14:04:23Z

Hm, I have an idea: My version of the file is from Dec 10, i.e. it wasn't overwritten during the update. We will need to fix this, but it isn't relevant for the milestone release as the file didn't exist in the previous milestone.

holgerfriedrich · 2023-12-17T14:14:55Z

@holgerfriedrich I just tried the latest distro snapshot (3787) and see this log during startup:
15:00:02.417 [WARN ] [ternal.xml.AddonInfoAddonsXmlProvider] - File 'openhab-addons.xml' has invalid content
Any idea why this happens?

@kaikreuzer this happened before when the schema was adapted. I think we have to improve logging in the addon info provider.

Hm, I have an idea: My version of the file is from Dec 10, i.e. it wasn't overwritten during the update. We will need to fix this, but it isn't relevant for the milestone release as the file didn't exist in the previous milestone.

This is good. In case of parser failure, all of the suggestions finders won't work.

May it be a problem for users of the daily snapshots? - then we should create a new issue....

andrewfg · 2023-12-17T14:18:04Z

Any idea why this happens?

@holgerfriedrich / @kaikreuzer / @mherwege - we changed the schema between two snapshots; so perhaps the new snapshot failed (correctly) to parse an addons.xml file from a prior snapshot based on the prior schema. Therefore I think that in the final build this will probably have all reconciled again. => But please do check before the final final release!

andrewfg · 2023-12-17T14:28:21Z

I think we have to improve logging in the addon info provider.

A few things..

I did already improve the logging so that each entry has its regex validated when the individual binding entries are initialized.
Your problem above is that the whole file is read in one go before the individual entries are validated, and as you say any single bad entry will kill the whole load.
The good news is that -- if the new schema is on the server -- the add-ons CI build will check each add-on individually and fail specifically for that add-on.

So I think that once everything has synchronized between the build processes of openhab-core and openhab-addons then the problem should have been resolved..

mherwege · 2023-12-17T16:00:57Z

Hm, I have an idea: My version of the file is from Dec 10, i.e. it wasn't overwritten during the update. We will need to fix this, but it isn't relevant for the milestone release as the file didn't exist in the previous milestone.

The file is in the userdata folder. Does it get deleted and recreated with an upgrade? If not we need to make sure this is done?

kaikreuzer · 2023-12-17T16:02:40Z

As mentioned above: "it wasn't overwritten during the update." So yes, it is an issue for anyone using snapshot updates and it will be a problem in future for anyone else. So we need to fix this.

mherwege · 2023-12-17T16:10:18Z

I don’t know how the installation and upgrades work exactly. @J-N-K can you help with this? Is it something to be done in the distro? Should the file go somewhere else to be removed and recreated in an upgrade? The code in core only takes care of reading the file that is there.

andrewfg · 2023-12-17T16:19:46Z

For the avoidance of doubt..

If it is a clean (over-write) install then surely any pre-existing openhab-addos.xml will be overwritten by the contents of the openhab-4.1.0-SNAPSHOT.zip
If it is an upgrade (incremental) install via a script e.g. via openHabian then I guess the script can decide which files to overwrite and which not

What other scenarios are we dealing with than the above?

mherwege · 2023-12-17T16:27:42Z

This file should always be overwritten, as it will contain the latest info when upgrading.

The issue is here: https://github.com/openhab/openhab-distro/blob/3721abe2ed8ec20134db021e33ef3fc5d6c3164d/features/distro/src/main/feature/feature.xml#L13

override should be true to force an overwrite.

I am not behind a computer now, so tough to create a PR, but it should be done to correct this.

J-N-K · 2023-12-17T16:30:44Z

I guess there are several solutions:

Adapt the update script, so the file get's deleted on upgrade (add an additional line after l. 323 / l. 744
Change the file location to userdata/etc and add it to userdata_sysfiles.lst
Change the file location to userdata/cache

I would probably vote for the last one, then it SHOULD be recreated when you clear the cache (which especially for snapshots might be an issue otherwise).

J-N-K · 2023-12-17T16:34:06Z

Or (even better) do it like @mherwege suggests.

andrewfg · 2024-01-22T13:23:57Z

@holgerfriedrich I have a couple of (admittedly very late) questions concerning this finder resp. PR..

Does it work on all Operating Systems i.e. MacOS, Windows, Linux, resp. other Unix platforms?
Does it detect all types of process i.e. both regular executables and also services?
Does it detect scripts being run by script processor application e.g. something like python running the script.py script?

holgerfriedrich · 2024-01-22T17:13:59Z

@andrewfg I use Java to obtain a list of processes, see https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/ProcessHandle.html#allProcesses()
The output is different on Linux and Windows. getProcessCommandProcess() was introduced to harmonize the output on different OS. Windows will return empty entries for some processes. May be due to user permissions.

I tested with Linux and Windows. The trace output which dumps the process list got removed in one of the last commits.
logger.trace("Processes visible: {}", processList.toString()); - maybe you want to add it and see.

andrewfg · 2024-01-22T19:52:53Z

So to answer (some of) my own questions:

At least on Linux the finder DOES identify processes that were started as a service. See log listings below. HOWEVER in such cases the command parameter is 'null', and the arguments[0] parameter can be irrelevant (the important information may be in arguments[n] where 'n' may vary from case to case). => Therefore the current version of this finder service is NOT able to suggest addons based on running services.
At least on Linux the finder DOES identify processes for scripts that are being run by a script execution application (see the Python example in the list below). However the current version of this finder service is NOT able to suggest addons based on running script services.

command:null, arguments:[], commandLine:/lib/systemd/systemd-journald 
command:null, arguments:[-c, /etc/mosquitto/mosquitto.conf], commandLine:/usr/sbin/mosquitto -c /etc/mosquitto/mosquitto.conf 
command:null, arguments:[-n, -iNONE], commandLine:/usr/sbin/rsyslogd -n -iNONE 
command:null, arguments:[-u, /home/openhabian/grott/grott.py], commandLine:/usr/bin/python3 -u /home/openhabian/grott/grott.py 
command:null, arguments:[/dev/serial1, bcm43xx, 3000000, flow, -], commandLine:/usr/bin/hciattach /dev/serial1 bcm43xx 3000000 flow - 
command:null, arguments:[], commandLine:/usr/libexec/bluetooth/bluetoothd 
command:null, arguments:[-w, -q], commandLine:/usr/sbin/dhcpcd -w -q

Therefore I will shortly create a new PR to improve this finder so that it can discover addons that depend on running services and/or scripts..

holgerfriedrich · 2024-01-22T22:25:04Z

@andrewfg Ok, for sure this is not working with script processors. Until now, only the main executable can be matched.

Are you going to add a new match property for arguments or do you want to extend the command to the full command line? For the latter one we would have to change existing descriptions for example from <regex>(?i).*[/\\](knxd)(\.exe)?$</regex> to <regex>(?i).*[/\\](knxd)(\.exe)?( .*)$</regex> (which might lead to false positives in case of similar directory names pointing to the executable)? Or are you thinking of a different approach?

andrewfg · 2024-01-22T23:25:40Z

I am thinking of keeping the existing property command as it is. And adding a second property commandLine. That should avoid breaking anything existing.

NEVERTHELESS I note that your existing code may take arguments[0] as a fallback if command is null. However from my tests on Linux, it seems to me that arguments[-1] (which doesn’t actually exist) would be the proper fallback for command. => So can you please tell me on what platform / test case did you find that arguments[0] would be the equivalent of command?

holgerfriedrich requested a review from a team as a code owner December 8, 2023 00:53

Service to suggest addons based on running processes

279fcc6

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich force-pushed the pr-finder-process branch from 4c11c65 to 279fcc6 Compare December 8, 2023 00:56

mherwege reviewed Dec 8, 2023

View reviewed changes

...rocess/src/main/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java Outdated Show resolved Hide resolved

holgerfriedrich added 4 commits December 9, 2023 16:20

add missing files

990d244

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

properly register

056d2ac

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

allow multiple match criteria

6bb17eb

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

exception handling

20fd530

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich force-pushed the pr-finder-process branch from de5e398 to 20fd530 Compare December 9, 2023 16:23

holgerfriedrich changed the title ~~[WIP] Service to suggest addons based on running processes~~ Service to suggest addons based on running processes Dec 9, 2023

holgerfriedrich mentioned this pull request Dec 9, 2023

addoninfo for new add-on suggestion finder "process" openhab/openhab-addons#16031

Merged

mherwege reviewed Dec 11, 2023

View reviewed changes

...covery.addon/src/main/java/org/openhab/core/config/discovery/addon/AddonFinderConstants.java Outdated Show resolved Hide resolved

mherwege reviewed Dec 11, 2023

View reviewed changes

...covery.addon/src/main/java/org/openhab/core/config/discovery/addon/AddonFinderConstants.java Outdated Show resolved Hide resolved

mherwege reviewed Dec 11, 2023

View reviewed changes

...rocess/src/main/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java Outdated Show resolved Hide resolved

reviewer comments: no standalone feature

d905cf1

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich requested a review from mherwege December 11, 2023 20:17

mherwege approved these changes Dec 11, 2023

View reviewed changes

kaikreuzer approved these changes Dec 16, 2023

View reviewed changes

...rocess/src/main/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java Outdated Show resolved Hide resolved

Update bundles/org.openhab.core.config.discovery.addon.process/src/ma…

e018186

…in/java/org/openhab/core/config/discovery/addon/process/ProcessAddonFinder.java Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer merged commit 4e634c6 into openhab:main Dec 16, 2023
2 of 3 checks passed

kaikreuzer added the enhancement An enhancement or new feature of the Core label Dec 16, 2023

kaikreuzer added this to the 4.1 milestone Dec 16, 2023

holgerfriedrich deleted the pr-finder-process branch December 16, 2023 10:41

J-N-K mentioned this pull request Dec 17, 2023

Fix addon discovery XML not overwritten openhab/openhab-distro#1615

Merged

andrewfg mentioned this pull request Jan 23, 2024

AddonFinderProcess fixes #4061

Merged


		private static final String COMMAND = "command";

		public static final String SERVICE_TYPE = SERVICE_TYPE_PROCESS;

Service to suggest addons based on running processes #3904

Service to suggest addons based on running processes #3904

Conversation

holgerfriedrich commented Dec 8, 2023

mherwege commented Dec 8, 2023

kaikreuzer commented Dec 8, 2023

mherwege commented Dec 8, 2023 • edited Loading

kaikreuzer commented Dec 8, 2023

andrewfg commented Dec 8, 2023

andrewfg commented Dec 9, 2023

mherwege commented Dec 9, 2023

holgerfriedrich commented Dec 9, 2023

holgerfriedrich commented Dec 9, 2023 • edited Loading

mherwege commented Dec 11, 2023

mherwege Dec 11, 2023

Choose a reason for hiding this comment

andrewfg Dec 11, 2023

Choose a reason for hiding this comment

mherwege Dec 11, 2023

Choose a reason for hiding this comment

andrewfg Dec 11, 2023

Choose a reason for hiding this comment

holgerfriedrich Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

mherwege Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

holgerfriedrich Dec 11, 2023

Choose a reason for hiding this comment

mherwege commented Dec 11, 2023

andrewfg commented Dec 15, 2023

mherwege commented Dec 15, 2023 • edited Loading

andrewfg commented Dec 15, 2023

mherwege commented Dec 15, 2023

andrewfg commented Dec 15, 2023

kaikreuzer left a comment

Choose a reason for hiding this comment

kaikreuzer commented Dec 17, 2023

kaikreuzer commented Dec 17, 2023

holgerfriedrich commented Dec 17, 2023 • edited Loading

andrewfg commented Dec 17, 2023 • edited Loading

andrewfg commented Dec 17, 2023 • edited Loading

mherwege commented Dec 17, 2023

kaikreuzer commented Dec 17, 2023

mherwege commented Dec 17, 2023

andrewfg commented Dec 17, 2023

mherwege commented Dec 17, 2023 • edited Loading

J-N-K commented Dec 17, 2023

J-N-K commented Dec 17, 2023

andrewfg commented Jan 22, 2024 • edited Loading

holgerfriedrich commented Jan 22, 2024 • edited Loading

andrewfg commented Jan 22, 2024 • edited Loading

holgerfriedrich commented Jan 22, 2024

andrewfg commented Jan 22, 2024

mherwege commented Dec 8, 2023 •

edited

Loading

holgerfriedrich commented Dec 9, 2023 •

edited

Loading

holgerfriedrich Dec 11, 2023 •

edited

Loading

mherwege Dec 11, 2023 •

edited

Loading

mherwege commented Dec 15, 2023 •

edited

Loading

holgerfriedrich commented Dec 17, 2023 •

edited

Loading

andrewfg commented Dec 17, 2023 •

edited

Loading

andrewfg commented Dec 17, 2023 •

edited

Loading

mherwege commented Dec 17, 2023 •

edited

Loading

andrewfg commented Jan 22, 2024 •

edited

Loading

holgerfriedrich commented Jan 22, 2024 •

edited

Loading

andrewfg commented Jan 22, 2024 •

edited

Loading