From 3e5e50fea5fa37e81de59def77ec589c4d99bdcb Mon Sep 17 00:00:00 2001 From: guqing <38999863+guqing@users.noreply.github.com> Date: Wed, 23 Aug 2023 11:28:13 +0800 Subject: [PATCH] fix: incorrect started plugin records obtained from plugin manager (#4454) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### What type of PR is this? /kind bug /area core /milestone 2.9.x #### What this PR does / why we need it: 修复从插件管理器中获取已启动插件的记录不正确的问题 由于 PR #4403 优化了 plugin extension 的 status 与内存状态的同步方式，优先级改为以数据库为准但这样状态维护变得复杂，所以此 PR 还是以内存为准但不同的是： 1. 当状态不一致时在 reconciler 中先将数据库的和内存状态都统一为停止状态即调用 haloPluginManager.stopPlugin 然后将停止状态更新到 status 的 phase 上，在继续后续的逻辑 2. 如果在更新 status 失败时加上重试避免因乐观锁而容易导致插件启动或停止成功但 status 更新失败导致的不一致几率问题。 经过上述两点的双重保障，多次测试后暂没有发现状态不一致的场景 how to test it? 1. 多安装几个插件十个以上最好，测试启动后通过 HaloPluginManager 获取已启动插件名称是否与 Console 已启动插件列表一致 ```java haloPluginManager.getStartedPlugins() ``` 2. 对于提供了 console 功能的插件不会出现启动成功但 status 的 entry 为空的情况 #### Does this PR introduce a user-facing change? ```release-note None ``` --- .../reconciler/PluginReconciler.java | 79 +++++++++++++------ .../reconciler/PluginReconcilerTest.java | 4 +- 2 files changed, 54 insertions(+), 29 deletions(-) diff --git a/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java b/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java index 58e315165..1d079137a 100644 --- a/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java +++ b/application/src/main/java/run/halo/app/core/extension/reconciler/PluginReconciler.java @@ -111,6 +111,9 @@ public class PluginReconciler implements Reconciler { log.error("Failed to reconcile plugin: [{}]", request.name(), e); persistenceFailureStatus(request.name(), e); return Result.doNotRetry(); + } catch (Exception e) { + persistenceFailureStatus(request.name(), e); + throw e; } } @@ -144,12 +147,16 @@ public class PluginReconciler implements Reconciler { String logoUrl = generateAccessibleLogoUrl(plugin); status.setLogo(logoUrl); - // Synchronize to plugin state in manager based on the phase of database - // to avoid the plugin state in manager is inconsistent with the database - // It is possible that the in-memory plugin has successfully started, - // but the status update of the database has failed. - // The status in the database will prevail - getPluginWrapper(name).setPluginState(status.getPhase()); + // If phase in status is not equal to plugin state, then reset plugin to + // stopped state and keep the state in memory consistent with the database + PluginState pluginState = getPluginWrapper(name).getPluginState(); + status.setPhase(pluginState); + if (!Objects.equals(status.getPhase(), pluginState)) { + // stop and set phase + status.setPhase(haloPluginManager.stopPlugin(name)); + status.setEntry(StringUtils.EMPTY); + status.setStylesheet(StringUtils.EMPTY); + } return status; }); return false; @@ -323,6 +330,7 @@ public class PluginReconciler implements Reconciler { updateStatus(pluginName, status -> { PluginWrapper pluginWrapper = haloPluginManager.getPlugin(pluginName); if (pluginWrapper != null) { + haloPluginManager.stopPlugin(pluginName); pluginWrapper.setPluginState(PluginState.FAILED); pluginWrapper.setFailedException(e); } @@ -372,6 +380,25 @@ public class PluginReconciler implements Reconciler { } void updateStatus(String name, UnaryOperator operator) { + try { + retryTemplate.execute(callback -> { + try { + doUpdateStatus(name, operator); + } catch (Exception e) { + // trigger retry + throw new IllegalStateException(e); + } + return null; + }); + } catch (Exception e) { + haloPluginManager.stopPlugin(name); + PluginWrapper pluginWrapper = haloPluginManager.getPlugin(name); + pluginWrapper.setPluginState(PluginState.FAILED); + throw e; + } + } + + void doUpdateStatus(String name, UnaryOperator operator) { client.fetch(Plugin.class, name).ifPresent(plugin -> { Plugin.PluginStatus oldStatus = JsonUtils.deepCopy(plugin.statusNonNull()); Plugin.PluginStatus newStatus = @@ -412,17 +439,17 @@ public class PluginReconciler implements Reconciler { "The plugin is disabled for some reason and cannot be started."); } + PluginState currentState = haloPluginManager.startPlugin(name); + if (!PluginState.STARTED.equals(currentState)) { + PluginStartingError staringErrorInfo = getStaringErrorInfo(name); + log.debug("Failed to start plugin: " + staringErrorInfo.getDevMessage(), + pluginWrapper.getFailedException()); + throw new IllegalStateException(staringErrorInfo.getMessage(), + pluginWrapper.getFailedException()); + } + String pluginVersion = pluginWrapper.getDescriptor().getVersion(); updateStatus(name, status -> { - PluginState currentState = haloPluginManager.startPlugin(name); - if (!PluginState.STARTED.equals(currentState)) { - PluginStartingError staringErrorInfo = getStaringErrorInfo(name); - log.debug("Failed to start plugin: " + staringErrorInfo.getDevMessage(), - pluginWrapper.getFailedException()); - throw new IllegalStateException(staringErrorInfo.getMessage(), - pluginWrapper.getFailedException()); - } - status.setLastStartTime(Instant.now()); String jsBundlePath = @@ -468,11 +495,11 @@ public class PluginReconciler implements Reconciler { } void doStop(String name) { + PluginState currentState = haloPluginManager.stopPlugin(name); + if (!PluginState.STOPPED.equals(currentState)) { + throw new IllegalStateException("Failed to stop plugin: " + name); + } updateStatus(name, status -> { - PluginState currentState = haloPluginManager.stopPlugin(name); - if (!PluginState.STOPPED.equals(currentState)) { - throw new IllegalStateException("Failed to stop plugin: " + name); - } status.setPhase(currentState); // reset js bundle path status.setStylesheet(StringUtils.EMPTY); @@ -569,17 +596,17 @@ public class PluginReconciler implements Reconciler { } /** - * Returns absolute plugin path. - * if plugin path is absolute, use it directly in development mode. - * otherwise, combine plugin path with plugin root path. - * Note: plugin location without scheme + * Returns an absolute plugin path. + * if a plugin path is absolute, use it directly in development mode. + * otherwise, combine a plugin path with a plugin root path. + * Note: plugin location without a scheme */ String buildPluginLocation(String name, String pluginPathString) { Assert.notNull(name, "Plugin name must not be null"); Assert.notNull(pluginPathString, "Plugin path must not be null"); Path pluginsRoot = toPath(haloPluginManager.getPluginsRoot().toString()); Path pluginPath = toPath(pluginPathString); - // if plugin path is absolute, use it directly in development mode + // if a plugin path is absolute, use it directly in development mode if (pluginPath.isAbsolute()) { if (!isDevelopmentMode(name) && !pluginPath.startsWith(pluginsRoot)) { throw new DoNotRetryException( @@ -638,7 +665,7 @@ public class PluginReconciler implements Reconciler { if (BooleanUtils.isNotTrue(plugin.getSpec().getEnabled())) { return false; } - // phase is not started or plugin state is not started should start + // phase is not started, or plugin state is not started should start return !PluginState.STARTED.equals(plugin.statusNonNull().getPhase()) || !PluginState.STARTED.equals(pluginWrapper.getPluginState()); } @@ -648,7 +675,7 @@ public class PluginReconciler implements Reconciler { if (BooleanUtils.isNotFalse(plugin.getSpec().getEnabled())) { return false; } - // phase is not stopped or plugin state is not stopped should stop + // phase is not stopped, or plugin state is not stopped should stop return !PluginState.STOPPED.equals(plugin.statusNonNull().getPhase()) || !PluginState.STOPPED.equals(pluginWrapper.getPluginState()); } diff --git a/application/src/test/java/run/halo/app/core/extension/reconciler/PluginReconcilerTest.java b/application/src/test/java/run/halo/app/core/extension/reconciler/PluginReconcilerTest.java index c497b81f8..00228014f 100644 --- a/application/src/test/java/run/halo/app/core/extension/reconciler/PluginReconcilerTest.java +++ b/application/src/test/java/run/halo/app/core/extension/reconciler/PluginReconcilerTest.java @@ -125,8 +125,6 @@ class PluginReconcilerTest { // mock plugin real state is started when(pluginWrapper.getPluginState()).thenReturn(PluginState.STOPPED); var pluginDescriptor = mock(PluginDescriptor.class); - when(pluginWrapper.getDescriptor()).thenReturn(pluginDescriptor); - when(pluginDescriptor.getVersion()).thenReturn("1.0.0"); PluginStartingError pluginStartingError = PluginStartingError.of("apples", "error message", "dev message"); @@ -202,7 +200,7 @@ class PluginReconcilerTest { when(pluginWrapper.getPluginState()).thenReturn(PluginState.STARTED); ArgumentCaptor pluginCaptor = doReconcileWithoutRequeue(); - verify(extensionClient, times(3)).update(any(Plugin.class)); + verify(extensionClient, times(4)).update(any(Plugin.class)); Plugin updateArgs = pluginCaptor.getValue(); assertThat(updateArgs).isNotNull();