From b437756157e739448ec64a1f7ddf375f59207fb9 Mon Sep 17 00:00:00 2001 From: guqing <38999863+guqing@users.noreply.github.com> Date: Mon, 21 Aug 2023 14:46:12 +0800 Subject: [PATCH] refactor: optimize plugin status updates in plugin reconciler (#4403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### What type of PR is this? /kind improvement /area core /milestone 2.9.x #### What this PR does / why we need it: 优化插件 Reconciler 中对 status 的更新 how to test it? 测试插件启动和停止没有问题即可，着重看一下 status 中是否会存在 stylesheet 和 entry 期望有值但却没有值的情况是否会发生 #### Does this PR introduce a user-facing change? ```release-note None ``` --- .../reconciler/PluginReconciler.java | 157 +++++++++--------- .../reconciler/PluginReconcilerTest.java | 81 +++++++-- 2 files changed, 147 insertions(+), 91 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 cb42c5f06..58e315165 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 @@ -22,6 +22,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; +import java.util.function.UnaryOperator; import lombok.AllArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.BooleanUtils; @@ -139,34 +140,36 @@ public class PluginReconciler implements Reconciler { } createInitialReverseProxyIfNotPresent(plugin); - // filled logo path - generateAccessibleLogoUrl(plugin); + updateStatus(name, status -> { + String logoUrl = generateAccessibleLogoUrl(plugin); + status.setLogo(logoUrl); - // update phase - Plugin.PluginStatus status = plugin.statusNonNull(); - PluginWrapper pluginWrapper = getPluginWrapper(name); - status.setPhase(pluginWrapper.getPluginState()); - updateStatus(plugin.getMetadata().getName(), status); + // 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()); + return status; + }); return false; }) .orElse(false); } - void generateAccessibleLogoUrl(Plugin plugin) { + String generateAccessibleLogoUrl(Plugin plugin) { String logo = plugin.getSpec().getLogo(); if (StringUtils.isBlank(logo)) { - return; + return null; } - Plugin.PluginStatus status = plugin.statusNonNull(); - if (PathUtils.isAbsoluteUri(logo)) { - status.setLogo(logo); - } else { + if (!PathUtils.isAbsoluteUri(logo)) { String assetsPrefix = PluginConst.assertsRoutePrefix(plugin.getMetadata().getName()); String versionedLogo = applyVersioningToStaticResource(logo, plugin.getSpec().getVersion()); - status.setLogo(PathUtils.combinePath(assetsPrefix, versionedLogo)); + return PathUtils.combinePath(assetsPrefix, versionedLogo); } + return logo; } Optional lookupPluginSetting(String name, String settingName) { @@ -231,18 +234,19 @@ public class PluginReconciler implements Reconciler { // Fix gh-3224 // Maybe Setting is being created and cannot be queried. so try again. if (settingOption.isEmpty()) { - Plugin.PluginStatus status = plugin.statusNonNull(); - status.setPhase(PluginState.FAILED); - var condition = Condition.builder() - .type("BackOff") - .reason("BackOff") - .message("Wait for setting [" + settingName + "] creation") - .status(ConditionStatus.FALSE) - .lastTransitionTime(Instant.now()) - .build(); - Plugin.PluginStatus.nullSafeConditions(status) - .addAndEvictFIFO(condition); - updateStatus(plugin.getMetadata().getName(), status); + updateStatus(plugin.getMetadata().getName(), status -> { + status.setPhase(PluginState.FAILED); + var condition = Condition.builder() + .type("BackOff") + .reason("BackOff") + .message("Wait for setting [" + settingName + "] creation") + .status(ConditionStatus.FALSE) + .lastTransitionTime(Instant.now()) + .build(); + Plugin.PluginStatus.nullSafeConditions(status) + .addAndEvictFIFO(condition); + return status; + }); // need requeue return true; } @@ -316,9 +320,7 @@ public class PluginReconciler implements Reconciler { } void persistenceFailureStatus(String pluginName, Throwable e) { - client.fetch(Plugin.class, pluginName).ifPresent(plugin -> { - Plugin.PluginStatus status = plugin.statusNonNull(); - + updateStatus(pluginName, status -> { PluginWrapper pluginWrapper = haloPluginManager.getPlugin(pluginName); if (pluginWrapper != null) { pluginWrapper.setPluginState(PluginState.FAILED); @@ -327,7 +329,6 @@ public class PluginReconciler implements Reconciler { status.setPhase(PluginState.FAILED); - Plugin.PluginStatus oldStatus = JsonUtils.deepCopy(status); Condition condition = Condition.builder() .type(PluginState.FAILED.toString()) .reason("UnexpectedState") @@ -337,9 +338,7 @@ public class PluginReconciler implements Reconciler { .build(); Plugin.PluginStatus.nullSafeConditions(status) .addAndEvictFIFO(condition); - if (!Objects.equals(oldStatus, status)) { - client.update(plugin); - } + return status; }); } @@ -352,33 +351,35 @@ public class PluginReconciler implements Reconciler { } if (pluginWrapper == null) { - Plugin.PluginStatus status = new Plugin.PluginStatus(); - status.setPhase(PluginState.FAILED); - String errorMsg = "Plugin " + name + " not found in plugin manager."; - Condition condition = Condition.builder() - .type(PluginState.FAILED.toString()) - .reason("PluginNotFound") - .message(errorMsg) - .status(ConditionStatus.FALSE) - .lastTransitionTime(Instant.now()) - .build(); - Plugin.PluginStatus.nullSafeConditions(status) - .addAndEvictFIFO(condition); - updateStatus(name, status); + updateStatus(name, status -> { + status.setPhase(PluginState.FAILED); + + Condition condition = Condition.builder() + .type(PluginState.FAILED.toString()) + .reason("PluginNotFound") + .message(errorMsg) + .status(ConditionStatus.FALSE) + .lastTransitionTime(Instant.now()) + .build(); + Plugin.PluginStatus.nullSafeConditions(status) + .addAndEvictFIFO(condition); + return status; + }); throw new DoNotRetryException(errorMsg); } return pluginWrapper; } - void updateStatus(String name, Plugin.PluginStatus status) { - if (status == null) { - return; - } + void updateStatus(String name, UnaryOperator operator) { client.fetch(Plugin.class, name).ifPresent(plugin -> { Plugin.PluginStatus oldStatus = JsonUtils.deepCopy(plugin.statusNonNull()); - plugin.setStatus(status); - URI loadLocation = status.getLoadLocation(); + Plugin.PluginStatus newStatus = + Optional.ofNullable(operator.apply(plugin.statusNonNull())) + .orElse(new Plugin.PluginStatus()); + plugin.setStatus(newStatus); + + URI loadLocation = newStatus.getLoadLocation(); if (loadLocation == null) { String pluginPath = nullSafeAnnotations(plugin).get(PLUGIN_PATH); if (StringUtils.isNotBlank(pluginPath)) { @@ -387,9 +388,9 @@ public class PluginReconciler implements Reconciler { } else { loadLocation = getPluginWrapper(name).getPluginPath().toUri(); } - status.setLoadLocation(loadLocation); + newStatus.setLoadLocation(loadLocation); } - if (!Objects.equals(oldStatus, status)) { + if (!Objects.equals(oldStatus, newStatus)) { client.update(plugin); } }); @@ -411,20 +412,19 @@ public class PluginReconciler implements Reconciler { "The plugin is disabled for some reason and cannot be started."); } - client.fetch(Plugin.class, name).ifPresent(plugin -> { - final Plugin.PluginStatus status = plugin.statusNonNull(); - final Plugin.PluginStatus oldStatus = JsonUtils.deepCopy(status); - + 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()); - throw new IllegalStateException(staringErrorInfo.getMessage()); + log.debug("Failed to start plugin: " + staringErrorInfo.getDevMessage(), + pluginWrapper.getFailedException()); + throw new IllegalStateException(staringErrorInfo.getMessage(), + pluginWrapper.getFailedException()); } - plugin.statusNonNull().setLastStartTime(Instant.now()); + status.setLastStartTime(Instant.now()); - final String pluginVersion = plugin.getSpec().getVersion(); String jsBundlePath = BundleResourceUtils.getJsBundlePath(haloPluginManager, name); jsBundlePath = applyVersioningToStaticResource(jsBundlePath, pluginVersion); @@ -445,9 +445,7 @@ public class PluginReconciler implements Reconciler { .build(); Plugin.PluginStatus.nullSafeConditions(status) .addAndEvictFIFO(condition); - if (!Objects.equals(oldStatus, status)) { - client.update(plugin); - } + return status; }); } @@ -470,10 +468,7 @@ public class PluginReconciler implements Reconciler { } void doStop(String name) { - client.fetch(Plugin.class, name).ifPresent(plugin -> { - final Plugin.PluginStatus status = plugin.statusNonNull(); - final Plugin.PluginStatus oldStatus = JsonUtils.deepCopy(status); - + updateStatus(name, status -> { PluginState currentState = haloPluginManager.stopPlugin(name); if (!PluginState.STOPPED.equals(currentState)) { throw new IllegalStateException("Failed to stop plugin: " + name); @@ -492,9 +487,7 @@ public class PluginReconciler implements Reconciler { .build(); Plugin.PluginStatus.nullSafeConditions(status) .addAndEvictFIFO(condition); - if (!Objects.equals(oldStatus, status)) { - client.update(plugin); - } + return status; }); } @@ -640,16 +633,24 @@ public class PluginReconciler implements Reconciler { return Paths.get(pathString).toUri(); } - private boolean shouldReconcileStartState(Plugin plugin) { + boolean shouldReconcileStartState(Plugin plugin) { PluginWrapper pluginWrapper = getPluginWrapper(plugin.getMetadata().getName()); - return BooleanUtils.isTrue(plugin.getSpec().getEnabled()) - && !PluginState.STARTED.equals(pluginWrapper.getPluginState()); + if (BooleanUtils.isNotTrue(plugin.getSpec().getEnabled())) { + return false; + } + // phase is not started or plugin state is not started should start + return !PluginState.STARTED.equals(plugin.statusNonNull().getPhase()) + || !PluginState.STARTED.equals(pluginWrapper.getPluginState()); } - private boolean shouldReconcileStopState(Plugin plugin) { + boolean shouldReconcileStopState(Plugin plugin) { PluginWrapper pluginWrapper = getPluginWrapper(plugin.getMetadata().getName()); - return BooleanUtils.isFalse(plugin.getSpec().getEnabled()) - && PluginState.STARTED.equals(pluginWrapper.getPluginState()); + if (BooleanUtils.isNotFalse(plugin.getSpec().getEnabled())) { + return false; + } + // phase is not stopped or plugin state is not stopped should stop + return !PluginState.STOPPED.equals(plugin.statusNonNull().getPhase()) + || !PluginState.STOPPED.equals(pluginWrapper.getPluginState()); } private void addFinalizerIfNecessary(Plugin oldPlugin) { 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 714cf9b60..c497b81f8 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 @@ -37,6 +37,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.stubbing.Answer; +import org.pf4j.PluginDescriptor; import org.pf4j.PluginState; import org.pf4j.PluginWrapper; import org.pf4j.RuntimeMode; @@ -91,6 +92,10 @@ class PluginReconcilerTest { void reconcileOkWhenPluginManagerStartSuccessfully() { Plugin plugin = need2ReconcileForStartupState(); when(pluginWrapper.getPluginState()).thenReturn(PluginState.STOPPED); + var pluginDescriptor = mock(PluginDescriptor.class); + when(pluginWrapper.getDescriptor()).thenReturn(pluginDescriptor); + when(pluginDescriptor.getVersion()).thenReturn("1.0.0"); + when(extensionClient.fetch(eq(Plugin.class), eq("apples"))).thenReturn(Optional.of(plugin)); when(haloPluginManager.startPlugin(any())).thenAnswer((Answer) invocation -> { // mock plugin real state is started @@ -119,6 +124,9 @@ 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"); @@ -309,9 +317,8 @@ class PluginReconcilerTest { plugin.setSpec(new Plugin.PluginSpec()); plugin.getSpec().setLogo("https://example.com/logo.png"); plugin.getSpec().setVersion("1.0.0"); - pluginReconciler.generateAccessibleLogoUrl(plugin); - assertThat(plugin.statusNonNull().getLogo()) - .isEqualTo("https://example.com/logo.png"); + String logo = pluginReconciler.generateAccessibleLogoUrl(plugin); + assertThat(logo).isEqualTo("https://example.com/logo.png"); } @Test @@ -320,8 +327,7 @@ class PluginReconcilerTest { plugin.setSpec(new Plugin.PluginSpec()); plugin.getSpec().setLogo("https://example.com/logo.png?hello=world"); plugin.getSpec().setVersion("1.0.0"); - pluginReconciler.generateAccessibleLogoUrl(plugin); - assertThat(plugin.statusNonNull().getLogo()) + assertThat(pluginReconciler.generateAccessibleLogoUrl(plugin)) .isEqualTo("https://example.com/logo.png?hello=world"); } @@ -331,8 +337,7 @@ class PluginReconcilerTest { plugin.setSpec(new Plugin.PluginSpec()); plugin.getSpec().setLogo(null); plugin.getSpec().setVersion("1.0.0"); - pluginReconciler.generateAccessibleLogoUrl(plugin); - assertThat(plugin.statusNonNull().getLogo()).isNull(); + assertThat(pluginReconciler.generateAccessibleLogoUrl(plugin)).isNull(); } @Test @@ -341,8 +346,7 @@ class PluginReconcilerTest { plugin.setSpec(new Plugin.PluginSpec()); plugin.getSpec().setLogo(""); plugin.getSpec().setVersion("1.0.0"); - pluginReconciler.generateAccessibleLogoUrl(plugin); - assertThat(plugin.statusNonNull().getLogo()).isNull(); + assertThat(pluginReconciler.generateAccessibleLogoUrl(plugin)).isNull(); } @Test @@ -353,8 +357,7 @@ class PluginReconcilerTest { plugin.getMetadata().setName("fake-plugin"); plugin.getSpec().setLogo("/static/logo.jpg"); plugin.getSpec().setVersion("1.0.0"); - pluginReconciler.generateAccessibleLogoUrl(plugin); - assertThat(plugin.statusNonNull().getLogo()) + assertThat(pluginReconciler.generateAccessibleLogoUrl(plugin)) .isEqualTo("/plugins/fake-plugin/assets/static/logo.jpg?version=1.0.0"); } @@ -366,8 +369,7 @@ class PluginReconcilerTest { plugin.getMetadata().setName("fake-plugin"); plugin.getSpec().setLogo("data:image/gif;base64,R0lGODfake"); plugin.getSpec().setVersion("2.0.0"); - pluginReconciler.generateAccessibleLogoUrl(plugin); - assertThat(plugin.statusNonNull().getLogo()) + assertThat(pluginReconciler.generateAccessibleLogoUrl(plugin)) .isEqualTo("data:image/gif;base64,R0lGODfake"); } } @@ -498,10 +500,13 @@ class PluginReconcilerTest { String name = "fake-plugin"; Plugin plugin = new Plugin(); Plugin.PluginStatus status = new Plugin.PluginStatus(); + plugin.setMetadata(new Metadata()); + plugin.getMetadata().setName(name); plugin.setStatus(status); when(extensionClient.fetch(eq(Plugin.class), eq(name))) .thenReturn(Optional.of(plugin)); PluginWrapper pluginWrapper = mock(PluginWrapper.class); + when(pluginWrapper.getPluginPath()).thenReturn(Paths.get("/path/to/plugin.jar")); when(haloPluginManager.getPlugin(eq(name))) .thenReturn(pluginWrapper); Throwable error = mock(Throwable.class); @@ -516,6 +521,56 @@ class PluginReconcilerTest { verify(pluginWrapper).setFailedException(eq(error)); } + @Test + void shouldReconcileStartState() { + Plugin plugin = new Plugin(); + plugin.setMetadata(new Metadata()); + plugin.getMetadata().setName("fake-plugin"); + plugin.setSpec(new Plugin.PluginSpec()); + + PluginWrapper pluginWrapper = mock(PluginWrapper.class); + when(haloPluginManager.getPlugin(eq("fake-plugin"))).thenReturn(pluginWrapper); + + plugin.getSpec().setEnabled(false); + assertThat(pluginReconciler.shouldReconcileStartState(plugin)).isFalse(); + + plugin.getSpec().setEnabled(true); + plugin.statusNonNull().setPhase(PluginState.RESOLVED); + assertThat(pluginReconciler.shouldReconcileStartState(plugin)).isTrue(); + + when(pluginWrapper.getPluginState()).thenReturn(PluginState.STOPPED); + assertThat(pluginReconciler.shouldReconcileStartState(plugin)).isTrue(); + + plugin.statusNonNull().setPhase(PluginState.STARTED); + when(pluginWrapper.getPluginState()).thenReturn(PluginState.STARTED); + assertThat(pluginReconciler.shouldReconcileStartState(plugin)).isFalse(); + } + + @Test + void shouldReconcileStopState() { + Plugin plugin = new Plugin(); + plugin.setMetadata(new Metadata()); + plugin.getMetadata().setName("fake-plugin"); + plugin.setSpec(new Plugin.PluginSpec()); + + PluginWrapper pluginWrapper = mock(PluginWrapper.class); + when(haloPluginManager.getPlugin(eq("fake-plugin"))).thenReturn(pluginWrapper); + + plugin.getSpec().setEnabled(true); + assertThat(pluginReconciler.shouldReconcileStopState(plugin)).isFalse(); + + plugin.getSpec().setEnabled(false); + plugin.statusNonNull().setPhase(PluginState.RESOLVED); + assertThat(pluginReconciler.shouldReconcileStopState(plugin)).isTrue(); + + when(pluginWrapper.getPluginState()).thenReturn(PluginState.STOPPED); + assertThat(pluginReconciler.shouldReconcileStopState(plugin)).isTrue(); + + plugin.statusNonNull().setPhase(PluginState.STOPPED); + when(pluginWrapper.getPluginState()).thenReturn(PluginState.STOPPED); + assertThat(pluginReconciler.shouldReconcileStopState(plugin)).isFalse(); + } + private ArgumentCaptor doReconcileNeedRequeue() { ArgumentCaptor pluginCaptor = ArgumentCaptor.forClass(Plugin.class); doNothing().when(extensionClient).update(pluginCaptor.capture());