Make add-on installation resilient to a single failing add-on (upgrade resilience, #5694) #5700
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,10 @@ public class FeatureInstaller implements ConfigurationListener { | |
| // configuration as this must be waited for before trying to add feature repos | ||
| private @Nullable Map<String, Object> configMapCache; | ||
|
|
||
| private static final int MAX_INSTALL_ATTEMPTS = 5; | ||
| private Set<String> failedInstallAddons = Set.of(); | ||
| private int failedInstallAttempts; | ||
|
|
||
| @Activate | ||
| public FeatureInstaller(final @Reference ConfigurationAdmin configurationAdmin, | ||
| final @Reference FeaturesService featuresService, final @Reference KarService karService, | ||
|
|
@@ -432,41 +436,89 @@ private Set<String> getAllFeatureNamesWithPrefix(String prefix) { | |
| } | ||
| } | ||
|
|
||
| /* package-private (instead of private) for testing */ | ||
| void installFeatures(Set<String> addons) { | ||
| Set<String> failed = installFeatureSet(addons); | ||
|
|
||
| // If more than one add-on was requested and some failed, a single add-on that cannot be resolved or | ||
| // downloaded may have aborted the whole (transactional) installation and thereby blocked the other, | ||
| // working add-ons. Retry the failed add-ons individually so that one broken add-on cannot prevent the | ||
| // others from being installed. | ||
| if (!failed.isEmpty() && addons.size() > 1) { | ||
| logger.warn("Installing add-ons '{}' as a group failed. Retrying them individually so that a single " | ||
| + "failing add-on does not block the others.", String.join(", ", failed)); | ||
| Set<String> stillFailed = new HashSet<>(); | ||
| for (String addon : failed) { | ||
|
Member
There was a problem hiding this comment. Each call to Karaf’s The current fallback can therefore turn one failed group operation into one additional complete provisioning operation for every configured add-on. During a repository-wide outage, resolver failure or state-change-lock problem, a configuration containing 40 add-ons could perform one group attempt followed by 40 individual attempts, all failing for the same global reason. That amplifies load and contention during exactly the failure scenario this change is intended to make safer. Could the isolation use recursive batch splitting, or otherwise avoid one call per add-on when the failure appears to be framework-wide? Please add a test where every installation request fails for the same global reason and verify that the number of provisioning calls remains bounded. |
||
| stillFailed.addAll(installFeatureSet(Set.of(addon))); | ||
| } | ||
| failed = stillFailed; | ||
| } | ||
|
|
||
| if (failed.isEmpty()) { | ||
| failedInstallAddons = Set.of(); | ||
| failedInstallAttempts = 0; | ||
| return; | ||
| } | ||
|
|
||
| // Bound the number of automatic retries for a persistently failing set of add-ons. Without this, a | ||
| // permanently unavailable add-on (e.g. removed from the repository) is retried every minute forever, | ||
| // repeatedly refreshing bundles and never giving up, which can also block other operations. | ||
| if (failed.equals(failedInstallAddons)) { | ||
|
Member
There was a problem hiding this comment. The final error says that installation is abandoned until the add-on configuration changes or the server is restarted, but a configuration change does not reset this retry state. For example:
The same problem occurs when an add-on is removed and later re-added. Could the retry state be associated with the processed configuration revision, or explicitly reset when a genuine configuration update is handled? Periodic retries of the unchanged configuration should retain their counter, while a user configuration change should start a new retry budget. Please add a test that reaches the limit, processes a changed configuration and verifies that installation is attempted with a fresh retry budget. |
||
| failedInstallAttempts++; | ||
| } else { | ||
| failedInstallAddons = failed; | ||
| failedInstallAttempts = 1; | ||
| } | ||
|
|
||
| if (failedInstallAttempts < MAX_INSTALL_ATTEMPTS) { | ||
| logger.error("Failed installing add-ons '{}' (attempt {} of {}). Will retry.", String.join(", ", failed), | ||
| failedInstallAttempts, MAX_INSTALL_ATTEMPTS); | ||
| configMapCache = null; // make sure we retry the installation | ||
| } else { | ||
| logger.error("Failed installing add-ons '{}' after {} attempts. Giving up until the add-on configuration " | ||
| + "changes or the server is restarted. Please check repository/network access for these " | ||
| + "add-ons.", String.join(", ", failed), failedInstallAttempts); | ||
| // Intentionally do not reset configMapCache here so that the periodic sync job stops retrying. | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Installs the given set of add-on features and returns the sub-set that is not installed afterwards. | ||
| * | ||
| * @param addons the add-on feature names to install | ||
| * @return the add-on feature names that could not be installed | ||
| */ | ||
| private Set<String> installFeatureSet(Set<String> addons) { | ||
| try { | ||
| if (logger.isDebugEnabled()) { | ||
| logger.debug("Installing '{}'", String.join(", ", addons)); | ||
| } | ||
| featuresService.installFeatures(addons, EnumSet.of(FeaturesService.Option.NoAutoRefreshBundles, | ||
| FeaturesService.Option.Upgrade, FeaturesService.Option.NoFailOnFeatureNotFound)); | ||
| } catch (Exception e) { | ||
| logger.debug("Installing '{}' failed: {}", String.join(", ", addons), e.getMessage(), debugException(e)); | ||
|
Member
There was a problem hiding this comment. This now hides the actual Karaf exception at normal log levels. The later error only contains the add-on names and retry count, so users cannot determine whether the cause was a missing Maven artifact, repository or TLS failure, resolver error, bundle-start failure or state-change-lock problem. The original exception was essential to diagnosing #5694. There is also an important semantic distinction here: Karaf can throw after it has already persisted the new feature state—for example, when starting one of the affected bundles fails. In that case, the subsequent Could the installation result retain both:
The installed-state snapshot can prevent unnecessary retries, but it should not silently erase the provisioning failure. At minimum, the first and final failures should retain the original cause at an actionable log level. |
||
| } | ||
| try { | ||
| Feature[] features = featuresService.listInstalledFeatures(); | ||
| Set<String> installed = new HashSet<>(); | ||
| Set<String> failed = new HashSet<>(); | ||
|
|
||
| for (String addon : addons) { | ||
| if (anyMatchingFeature(features, withName(addon))) { | ||
| installed.add(addon); | ||
| } else { | ||
| failed.add(addon); | ||
| } | ||
| } | ||
|
|
||
| if (!installed.isEmpty() && logger.isDebugEnabled()) { | ||
| logger.debug("Installed '{}'", String.join(", ", installed)); | ||
| } | ||
| installed.forEach(this::postInstalledEvent); | ||
|
Member
There was a problem hiding this comment. This posts an installed event for every requested feature found in Karaf’s installed-state snapshot, even when the immediately preceding Karaf saves its feature state before some later deployment phases, including bundle startup. A feature can therefore appear installed while its provisioning operation still ended with a Could events be based on a more explicit result that distinguishes:
Without that distinction, this can emit misleading or duplicate installed events after failed provisioning attempts. A test should cover |
||
| return failed; | ||
| } catch (Exception e) { | ||
| logger.error("Failed retrieving features: {}", e.getMessage(), debugException(e)); | ||
| return addons; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| /* | ||
| * Copyright (c) 2010-2026 Contributors to the openHAB project | ||
| * | ||
| * See the NOTICE file(s) distributed with this work for additional | ||
| * information. | ||
| * | ||
| * This program and the accompanying materials are made available under the | ||
| * terms of the Eclipse Public License 2.0 which is available at | ||
| * http://www.eclipse.org/legal/epl-2.0 | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| */ | ||
| package org.openhab.core.karaf.internal; | ||
|
|
||
| import static org.hamcrest.CoreMatchers.is; | ||
| import static org.hamcrest.CoreMatchers.not; | ||
| import static org.hamcrest.CoreMatchers.nullValue; | ||
| import static org.hamcrest.CoreMatchers.sameInstance; | ||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.hasItem; | ||
| import static org.hamcrest.Matchers.hasItems; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.eq; | ||
| import static org.mockito.Mockito.doAnswer; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.reset; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import java.lang.reflect.Field; | ||
| import java.util.EnumSet; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import org.apache.karaf.features.Feature; | ||
| import org.apache.karaf.features.FeaturesService; | ||
| import org.apache.karaf.kar.KarService; | ||
| import org.eclipse.jdt.annotation.NonNullByDefault; | ||
| import org.eclipse.jdt.annotation.Nullable; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.openhab.core.events.EventPublisher; | ||
| import org.osgi.service.cm.ConfigurationAdmin; | ||
|
|
||
| /** | ||
| * Tests for the add-on installation resilience of {@link FeatureInstaller}: a single add-on that cannot be | ||
| * downloaded/resolved must not block the other add-ons, and a persistently failing add-on must not be retried | ||
| * forever. | ||
| * | ||
| * @author openHAB contributors - Initial contribution | ||
| */ | ||
| @NonNullByDefault | ||
| public class FeatureInstallerTest { | ||
|
|
||
| private static final String ADDON_A = "openhab-binding-a"; | ||
| private static final String ADDON_B = "openhab-binding-b"; | ||
| private static final String ADDON_BAD = "openhab-binding-bad"; | ||
|
|
||
| private @NonNullByDefault({}) FeaturesService featuresService; | ||
| private @NonNullByDefault({}) EventPublisher eventPublisher; | ||
| private @NonNullByDefault({}) FeatureInstaller featureInstaller; | ||
|
|
||
| /** The names of the features the mocked {@link FeaturesService} considers currently installed. */ | ||
| private final Set<String> installedFeatures = new HashSet<>(); | ||
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| ConfigurationAdmin configurationAdmin = mock(ConfigurationAdmin.class); | ||
| featuresService = mock(FeaturesService.class); | ||
| KarService karService = mock(KarService.class); | ||
| eventPublisher = mock(EventPublisher.class); | ||
|
|
||
| featureInstaller = new FeatureInstaller(configurationAdmin, featuresService, karService, eventPublisher, | ||
| Map.of()); | ||
| // Stop the background scheduler and discard the interactions triggered by the constructor so that the | ||
| // tests can drive installFeatures(...) deterministically. | ||
| featureInstaller.deactivate(); | ||
| reset(featuresService, eventPublisher); | ||
| } | ||
|
|
||
| /** | ||
| * A failing group installation (as thrown by the transactional Karaf {@code installFeatures} when one add-on | ||
| * cannot be downloaded) must fall back to installing the add-ons individually, so that the healthy add-ons are | ||
| * still installed. | ||
| */ | ||
| @Test | ||
| public void groupFailureFallsBackToIndividualInstalls() throws Exception { | ||
| when(featuresService.listInstalledFeatures()).thenAnswer(invocation -> featuresFor(installedFeatures)); | ||
| installEverythingButBrokenAddon(); | ||
|
|
||
| featureInstaller.installFeatures(Set.of(ADDON_A, ADDON_B, ADDON_BAD)); | ||
|
|
||
| // The two healthy add-ons are installed despite the broken one, which is not. | ||
| assertThat(installedFeatures, hasItems(ADDON_A, ADDON_B)); | ||
| assertThat(installedFeatures, not(hasItem(ADDON_BAD))); | ||
|
|
||
| // The healthy add-ons were retried individually after the group install failed. | ||
| verify(featuresService).installFeatures(eq(Set.of(ADDON_A)), any()); | ||
| verify(featuresService).installFeatures(eq(Set.of(ADDON_B)), any()); | ||
| } | ||
|
|
||
| /** | ||
| * A persistently failing add-on must be retried only a bounded number of times. Up to the limit the | ||
| * configuration cache is cleared so that the periodic sync job retries; once the limit is reached the cache is | ||
| * left intact so that the retry storm stops. | ||
| */ | ||
| @Test | ||
| public void persistentFailureStopsRetryingAfterMaxAttempts() throws Exception { | ||
|
Member
There was a problem hiding this comment. This test directly replaces The mocked installation behavior also models only an exception that occurs before any feature becomes installed, while Karaf may persist the installed-feature state and throw during a later deployment phase. Could the tests cover the observable lifecycle instead?
It would also be preferable to make the retry policy directly testable rather than starting a real scheduler, shutting it down and mutating private component state through reflection. |
||
| when(featuresService.listInstalledFeatures()).thenAnswer(invocation -> featuresFor(installedFeatures)); | ||
| installEverythingButBrokenAddon(); | ||
|
|
||
| Map<String, Object> sentinelConfig = new HashMap<>(); | ||
| sentinelConfig.put("key", "value"); | ||
|
|
||
| // Attempts before the limit clear the config cache so that the sync job retries the installation. | ||
| for (int attempt = 1; attempt < 5; attempt++) { | ||
| setConfigMapCache(sentinelConfig); | ||
| featureInstaller.installFeatures(Set.of(ADDON_BAD)); | ||
| assertThat("attempt " + attempt + " should schedule a retry", getConfigMapCache(), is(nullValue())); | ||
| } | ||
|
|
||
| // The final attempt gives up and keeps the cache so that the periodic sync job stops retrying. | ||
| setConfigMapCache(sentinelConfig); | ||
| featureInstaller.installFeatures(Set.of(ADDON_BAD)); | ||
| assertThat("the retry storm should stop after the maximum number of attempts", getConfigMapCache(), | ||
| is(sameInstance(sentinelConfig))); | ||
| } | ||
|
|
||
| /** | ||
| * Makes the mocked {@link FeaturesService} fail any installation request that contains the broken add-on and | ||
| * succeed for all other requests, tracking the successfully installed features. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| private void installEverythingButBrokenAddon() throws Exception { | ||
| doAnswer(invocation -> { | ||
| Set<String> requested = invocation.getArgument(0, Set.class); | ||
| if (requested.contains(ADDON_BAD)) { | ||
| throw new Exception("simulated download failure of " + ADDON_BAD); | ||
| } | ||
| installedFeatures.addAll(requested); | ||
| return null; | ||
| }).when(featuresService).installFeatures(any(Set.class), any(EnumSet.class)); | ||
| } | ||
|
|
||
| private Feature[] featuresFor(Set<String> names) { | ||
| return names.stream().map(name -> { | ||
| Feature feature = mock(Feature.class); | ||
| when(feature.getName()).thenReturn(name); | ||
| return feature; | ||
| }).toArray(Feature[]::new); | ||
| } | ||
|
|
||
| private void setConfigMapCache(@Nullable Map<String, Object> value) throws Exception { | ||
| Field field = FeatureInstaller.class.getDeclaredField("configMapCache"); | ||
| field.setAccessible(true); | ||
| field.set(featureInstaller, value); | ||
| } | ||
|
|
||
| private @Nullable Object getConfigMapCache() throws Exception { | ||
| Field field = FeatureInstaller.class.getDeclaredField("configMapCache"); | ||
| field.setAccessible(true); | ||
| return field.get(featureInstaller); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change overlaps directly with #5708, which addresses the same bundle-activation problem as part of a dedicated mDNS thread-safety change.
Submitting the existing
start()method to an executor is not sufficient to preserve the current lifecycle semantics. For example,addServiceListener()only adds a listener to theJmDNSinstances that already exist. A consumer that registers after activation returns but beforestart()creates the instances will silently lose that registration, because listeners are not retained and replayed.Startup can also race with
onChanged()and deactivation, potentially creating duplicate instances or publishing a creation result afterclose()has already run.#5708 already handles these concerns more comprehensively by retaining listeners and services, coordinating address and service operations, and rejecting or closing results that complete after deactivation.
Could the mDNS commit be removed from this PR and #5708 be referenced for this part of the problem?