fix: incorrect started plugin records obtained from plugin manager (#4454)

#### 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
```
pull/4462/head^2
guqing 2023-08-23 11:28:13 +08:00 committed by GitHub
parent 6326ec1d86
commit 3e5e50fea5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 54 additions and 29 deletions

View File

@ -111,6 +111,9 @@ public class PluginReconciler implements Reconciler<Request> {
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<Request> {
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<Request> {
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<Request> {
}
void updateStatus(String name, UnaryOperator<Plugin.PluginStatus> 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<Plugin.PluginStatus> 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<Request> {
"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<Request> {
}
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<Request> {
}
/**
* 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<Request> {
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<Request> {
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());
}

View File

@ -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<Plugin> 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();