jbonofre commented on code in PR #3010:
URL: https://github.com/apache/karaf/pull/3010#discussion_r4308537914
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/Deployer.java:
##########
@@ -1045,9 +1048,19 @@ && isUpdateable(resource) &&
!deployment.bundleChecksums.containsKey(bundle.getB
toStart.remove(bundle);
}
}
- if (!exceptions.isEmpty()) {
- throw new MultiException("Error restarting bundles",
exceptions);
- }
+ }
+ // All bundles have been started (failures included, resuming would
not help with those), unless the framework
Review Comment:
The comment says failed deployments shouldn't be resumed, but that only
holds for `BundleException`s collected in the start loop. Anything thrown
between the first save and here leaves `deploymentPending=true` on disk:
`installConfigs`/`installLibraries`, `refreshPackages`, `resolveBundles`, or a
`RuntimeException` from `startBundle`. The same failing deployment then runs
again on every restart, and every time the features Activator is reconfigured.
With the default `featuresBootAsynchronous=false`, that blocks the Activator
thread each time.
I'd clear the flag in a `finally` around everything after the first save,
keeping the "framework is stopping" check, so only a real interruption leaves
it set.
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/Deployer.java:
##########
@@ -949,6 +949,9 @@ && isUpdateable(resource) &&
!deployment.bundleChecksums.containsKey(bundle.getB
newState.installedFeatures.putAll(installedFeatures);
newState.stateFeatures.putAll(stateFeatures);
newState.managedBundles.putAll(managedBundles);
+ // the bundles are installed, but not yet refreshed/resolved/started:
if the framework stops before this
+ // deployment is complete, they would stay unstarted, so remember to
resume it on the next start
+ newState.deploymentPending.set(true);
Review Comment:
By the time the flag is saved, bundles have already been stopped,
uninstalled, updated and installed. For a large feature or KAR, that is the
longest part of the deployment. If Karaf stops during that phase, `state.json`
has neither the new requirements nor the flag, so nothing is resumed and the
new bundles stay installed but unmanaged and not started. That is the same bug
this PR fixes, just in a different window. Bundles that were stopped
persistently because their start level changes (`stopBundle(bundle, 0)` at line
783) also stay stopped.
Can we save the new requirements and the pending marker before the first
bundle operation (right after `DEPLOYMENT_STARTED`)? The resume re-resolves
from the requirements, and the Deployer already turns installed bundles that a
feature needs into managed bundles (around line 471). So the resume shouldn't
need the intermediate state, but a test for that case would confirm it.
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/Deployer.java:
##########
@@ -1045,9 +1048,19 @@ && isUpdateable(resource) &&
!deployment.bundleChecksums.containsKey(bundle.getB
toStart.remove(bundle);
}
}
- if (!exceptions.isEmpty()) {
- throw new MultiException("Error restarting bundles",
exceptions);
- }
+ }
+ // All bundles have been started (failures included, resuming would
not help with those), unless the framework
+ // has started to shut down meanwhile: then the starts do not persist
and the deployment has to be resumed.
+ // While the framework is still raising its start level (e.g. boot
features), the system bundle is STARTING.
+ Bundle systemBundle = dstate.bundles.get(0L);
+ if (systemBundle == null || (systemBundle.getState() &
(Bundle.STARTING | Bundle.ACTIVE)) != 0) {
+ newState.deploymentPending.set(false);
+ callback.saveState(newState);
Review Comment:
If this deployment refreshed the features bundle (the
`toRefresh.containsKey(serviceBundle)` branch above), this save runs on the old
instance, whose bundle context is no longer valid. `StateStorage` calls
`bundleContext.getDataFile("state.json")`, which throws
`IllegalStateException`, and `saveState()` only catches `IOException`.
So `deploy()` fails before the `MultiException`, the feature events and
`DEPLOYMENT_FINISHED`, and `feature:install` reports an error even though
everything is installed. The flag also stays `true` on disk. Before this
change, nothing touched the bundle context after the refresh.
I'd skip this save when `serviceBundle` was refreshed and let the new
instance own the state from there. With a per-launch check like the one
suggested in BootFeaturesInstaller, the worst case is one unneeded re-provision
on the next restart.
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/FeaturesServiceImpl.java:
##########
@@ -258,6 +258,20 @@ public void bootDone() {
}
}
+ @Override
+ public boolean isDeploymentPending() {
+ synchronized (lock) {
+ return state.deploymentPending.get();
+ }
+ }
+
+ @Override
+ public void resumeDeployment() throws Exception {
+ State state = copyState();
+ doProvisionInThread(state.requirements, emptyMap(), state,
getFeaturesById(),
+ EnumSet.noneOf(Option.class));
Review Comment:
The options of the interrupted deployment aren't saved, so this replays it
with default options. If a `feature:install --no-auto-refresh` (or a deployment
with `NoAutoManageBundles`, `NoAutoRefreshUnmanagedBundles`, etc.) is
interrupted, it is resumed with auto refresh and bundle management turned on,
so it changes bundles the user asked us to leave alone.
The self-update path already handles this: `persistResolveRequest()` writes
the requirements and options to the `resolve` file, and `checkResolve()`
replays them with those options. Could we save the options the same way next to
the pending marker?
Also, `feature:uninstall -c` interrupted before `deleteConfigs` won't delete
the configurations on resume, even with the options. By then the features are
already gone from `installedFeatures`, so the resume sees nothing to uninstall.
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/BootFeaturesInstaller.java:
##########
@@ -91,7 +91,21 @@ public BootFeaturesInstaller(BundleContext bundleContext,
*/
public void start() {
if (bootManaged.isBootDone()) {
- publishBootFinished();
+ if (bootManaged.isDeploymentPending()) {
Review Comment:
`deploymentPending` is also `true` on disk while a deployment is still
running, so this check can't tell "interrupted" from "in progress".
When a deployment refreshes the features bundle itself, the Activator is
restarted. For example, `feature:install eventadmin` does this because
features-core has an optional import of `org.osgi.service.event`, which is why
`ensureAllClassesLoaded()` exists. The new `FeaturesServiceImpl` loads
`state.json` with `deploymentPending=true`, and this calls `resumeDeployment()`
while the old deployment thread is still resolving and starting bundles.
`executor.shutdown()` doesn't interrupt that thread. A feature `<config>` for
`org.apache.karaf.features` has the same effect, since it re-runs
`doStop()`/`doStart()`. Either way, two deployers end up stopping, refreshing
and starting the same bundles at the same time.
Could we resume only after a real framework restart? One way: store the
framework UUID of the launch that started the deployment
(`bundleContext.getProperty(Constants.FRAMEWORK_UUID)`) instead of a boolean,
and resume only when it differs from the current one. The UUID is regenerated
each time the framework is initialized but stays the same across a refresh of
the features bundle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
