Fix AddonSuggestionService configuration #5042
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nadahar
force-pushed
the
fix-addon-suggestion-service
branch
from
fec3558 to
6873510
Compare
lolodomo
reviewed
Sep 27, 2025
...very.addon/src/main/java/org/openhab/core/config/discovery/addon/AddonSuggestionService.java
Show resolved
Hide resolved
Nadahar
commented
Sep 27, 2025
...very.addon/src/main/java/org/openhab/core/config/discovery/addon/AddonSuggestionService.java
Show resolved
Hide resolved
|
I am adding logs to your PR and plan to patch my production env with that later today. |
|
I'm sorry, but I'll have to rebase this PR since #5031 has now been merged, and these two PRs shared the first commit (the unrelated nullness annotation). I'll push the rebased version soon. |
Nadahar
force-pushed
the
fix-addon-suggestion-service
branch
from
6873510 to
0f9a105
Compare
|
I tried to "patch" my production environment (5.1 milestone 1) by updating (bundle:update) the 2 bundles o.o.core and o.o.core.config.discovery.addon but unfortunately I again encountered the famous problem that original bundles (5.1 milestone 1) are reinstalled automatically and this even leads in that particular case to all my bundles being no more active (probably due to the reinstall of the main bundle o.o.core). I suppose I should build a full distribution and install it. This is not something I am used to. But at least, I was able to notice what happens at the startup with the new bundles, that is interesting but can't be considered as a real test. Just to explain my test, I stopped the OH server while all finders were disabled. Then I updated my file conf/services/addons.cfg to enable UPnP finder and then restarted OH server. First and that is new and a big improvement, a full config is now provided to the activator. I was hoping it could be the config defined in my conf/services/addons.cfg but it is probably the config that is defined in file userdata/config/org/openhab/addons.config, meaning the configuration when I stopped the openHAB server with suggestionFinderUpnp being false and not the new config. Then very shortly after, 2 seconds later, Immediately after, modified is called again, with a new value for key core-config-discovery-addon (changed from empty to ",upnp"). Unfortunately, I forgot to log calls to So it looks no so bad if the sequence is systematically this one. But to run full tests, a build of a new distribution and an installation from this build would be required. |
|
I tried again with few more logs. First when the service is activated (old config is considered): Then some services are added and unfortunately a first call to And the good configuration is loaded: Then addAddonFinderService is called followed by So it looks like |
|
@Nadahar : let me know if I was too much confused in my report. |
Maybe it is only the ProcessAddonFinder and in this case this is ok as there is no setting for this finder (always installed). It is too bad that everything stopped so early with a restore of all milestone bundles. So maybe it works properly but I cannot confirm it unfortunately. |
I can't be sure, but I suspect that this might be caused by different versions ( Alternatively, I've made a Disregard the debugging part, you don't have to do that unless you want to. You can run the installation directly from the folder created by the
I don't think it's "this or that" config - I think what happens is that the config is first retrieved from a "Karaf cache" where it has the state from when Karaf was last stopped. Then, when the config dispatcher gets up and running, it will modify that cached configuration with what it finds in
This is because
As I've said before, it doesn't really matter if I think that some of the "challenges" here goes beyond this fix. This fix only makes sure that the configuration used is the one that is valid at that point in time. The order in which components are started is beyond the realm of this, and if that should be "controlled" in any way, it should be done in some other way. As I've mentioned before, I think it might make things easier if But again, this whole startup order situation is a bit beyond the scope of this fix, which goes to the handling of the configuration.
If it's "too early" depends on what is asking IMO. It's not a problem in the sense that it will correctly report which finders are available at that time, but it won't report what finders will be available after the
No, I think it's fine and useful 👍
Try the procedure I suggested above with the
I'm not sure that this is a problem, because the motivation for disabling the finders is primarily to save system resources. In that case, they will normally have been disabled the last time the system stopped as well. But, if we want to make this really "waterproof", we'll have to make the |
|
Ok, I installed from scratch from a snapshot I built myself from your PR but with logs added. 
|
|
Now I stop OH, change my addons.cfg like that to disable IP, SDDP and USB finders and restart OH: Logs look good, we can see that the disabled finders in conf are first loaded (IpAddonFinder and SddpAddonFinder, but not USB finder) but at this time, you already have the setting that they must not be used. And later these 2 finders are uninstalled and removed: I then check what bundles are installed, this is consistent: Settings are as expected in UI: I then enable SDDP finder in Main UI, configuration is immediately updated and new finder is installed: New bundle is installed: I then disable mDNS finder in Main UI, configuration is immediately updated and finder is uninstalled: Bundle is uninstalled: I stop OH and change my conf/services/addons.cfg to diabled all finders: Logs at startup look good, the SDDP finder is loaded but then uninstalled. When isFinderEnabled is called for SddpAddonFinder, the setting is false: Bundles are as expected: No binding is suggested by Main UI: Final test, I stop OH and change my conf/services/addons.cfg to enable UPnP finder: Logs at startup look good, the UPnP finder is nstalled and when isFinderEnabled is called for UpnpAddonFinder, the setting is true: Bundles are as expected: Here are the suggested bindins in Main UI: As a conclusion, it looks to work as we expect. Bravo @Nadahar for this great improvment. |
|
In addition to positive tests, the code looks good to me even if I am not at all an expert in OSGi and can't really challenge the code changes but @Nadahar apparently succeeded to remove the hack that was previously implemented to load the configuration. Configuration is now properly provided to the activator and to the |
Are you sure you didn't mean |
|
Thank you for a thorough and well documented test @lolodomo. I'm really only waiting for @mherwege to have a chance to look at it before taking it out of draft, because I understood it as if @andrewfg isn't going to. If release of M2 is getting close, please let me know and we'll have to figure out what to do. |
Yes, fixed |
M2 is not yet planned. But we should have this fix available in a snapshot before building M2, so my suggestion is to not delay too much the review, also considering that I already reviewed it. |
Maybe @mherwege can give is a hint as to how his future schedule looks, so we know a bit about what kind of waiting time we're looking at? |
|
I didn’t test myself, but looked at the extensive test @lolodomo already did. @Nadahar I am impressed. Great work. I think this is absolutely ready for review, so we can get it merged as soon as possible. I am far from an OSGI expert myself and looked at similar patterns when coding this. As far as I can tell, the code looks good. One thing I am now thinking: I copied the configuration synchronization pattern from the FeatureInstaller code. Would it be better to remove that pattern in a similar way there as well? |
I am not a maintainer, so I have no inside info on the schedule at all. @lolodomo is probably better placed on it than I am. I got involved in this because I contributed this part of the code initially. It was as good as I succeeded in making it, but we have seen the behavior was not without its challenges. And you improved that a lot. |
As I mentioned somewhere else, the same could probably be done there yes, because of the changes I did here, the But, I didn't want to bite over too much at once, |
|
To the reviewer: As this was "a work in progress" until it suddenly wasn't, I never wrote proper commit messages. I can correct it, but that would mean a forced push, and since you squash PRs anyway, I'm not sure that it matters? |
|
There is no strict planning for milestones but generally we try to consider one milestone per month, so around 4 or 5 weeks between each milestone. |
|
@Nadahar I'd appreciate if you could squash and provide the proper commit message. |
AddonSuggestionService has had a delayed application of configuration because it was based on a timer that refreshed it every minute. This led to various issue where "wrong decision" were made based on stale information. This addresses the root cause for why the configuration wasn't delivered in a timely manner by OSGi, and removes the scheduled configuration refresh. For the configuration to be accessible to multiple bundles, its "location" must be "a region". This must be configured before any bundle tries to use the configuration. To do this very early, it has been attached to the Activator of the "main core bundle". It doesn't have to be there, as long as it will always run very early during startup. Signed-off-by: Ravi Nadahar <[email protected]>
Nadahar
force-pushed
the
fix-addon-suggestion-service
branch
from
0f9a105 to
3e3c97a
Compare
|
@holgerfriedrich Commits have been squashed and a new commit message created. I did some explanation in the commit message, so it's easy to find later - I hope that's ok. Since I had to do a force push anyway, I also rebased it on the latest |
Nadahar
changed the title
|
First of all: @Nadahar Thank you for your work on this topic! @lolodomo Thank you for the extensive test and the full documentation! @openhab/core-maintainers Overall, I think this is approach is cleaner than before, fixes the issue introduced recently, and has been tested to work. If there are some doubts regarding the way the startup is hooked into @andrewfg Do you still have any concerns regarding the change from polling config changes every minute to proper modification based triggering? M2 is still a few weeks ahead. But we should get this done in the next days to give it some more testing. |
holgerfriedrich
added
the
bug
|
@holgerfriedrich I will make a build and test it tomorrow. |
holgerfriedrich
approved these changes
Oct 8, 2025
|
@openhab/core-maintainers : I believe the label "regression" should be added to this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #5037 according to my testing.
I'm leaving it as a draft because I assume that there might be further changes. I haven't created proper commit messages either, although that doesn't seem to matter as all PRs seem to be squashed these days.
The first commit isn't really related to this, but it's needed to not get a compilation error in Eclipse when having the discovery bundles open. I've already submitted it as a part of #5031, it doesn't really matter where it's included as long as the compilation error is taken care of.
I've done the rest in two parts, first the fixes necessary to make it work, and then the cleanup of all that's no longer needed. The timer based configuration refresh has been removed, as it was only ever there because the configuration mechanism didn't do what was desired.
The crux of the fix is to modify the
org.openhab.addonsconfiguration object to have a "region" location (one that starts with?). I just called it?openhab, I have no idea what a "proper location" would be. As far as I understand, any string can be put there as long as it starts with a?. This is what enables more than one bundle to use the configuration, which then enables the configuration to be supplied via@Activateand@Modified.I'm pretty sure that with this fix, the timer can be removed from
FeatureInstalleras well, although I haven't actually tested this.I think some of you guys should test/verify that it works as intended for you as well.