From cf7e31410b86c10211414d5edd023063daf99166 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Thu, 3 Sep 2020 15:33:58 -0400 Subject: [PATCH] Add devfile property to allow enabling/disabling plugin merging Add attribute 'mergePlugins' to devfile to manually enable/disable plugin merging in the plugin broker. Default value when no property is present is controlled by property che.workspace.plugin_broker.default_merge_plugins Add a small note to the plugin broker images config lines in che.properties to make it clear that the Che Operator overrides the properties by default. Signed-off-by: Angel Misevski --- .../webapp/WEB-INF/classes/che/che.properties | 9 ++ .../KubernetesArtifactsBrokerApplier.java | 8 +- .../wsplugins/PluginBrokerManager.java | 4 +- .../wsplugins/SidecarToolingProvisioner.java | 21 ++++- .../BrokerEnvironmentFactory.java | 63 ++++++++----- .../KubernetesArtifactsBrokerApplierTest.java | 11 +-- .../SidecarToolingProvisionerTest.java | 89 +++++++++++++++---- .../BrokerEnvironmentFactoryTest.java | 77 +++++++++++++--- .../OpenshiftBrokerEnvironmentFactory.java | 10 ++- .../che/api/workspace/shared/Constants.java | 20 ++++- 10 files changed, 240 insertions(+), 72 deletions(-) diff --git a/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties b/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties index e1f1e537c25..071aa25ceb8 100644 --- a/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties +++ b/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties @@ -543,9 +543,18 @@ che.singleport.wildcard_domain.ipless=false # Docker image of Che plugin broker app that resolves workspace tooling configuration and copies # plugins dependencies to a workspace +# +# Note these images are overridden by the Che Operator by default; changing the images here will not +# have an effect if Che is installed via Operator. che.workspace.plugin_broker.metadata.image=quay.io/eclipse/che-plugin-metadata-broker:v3.4.0 che.workspace.plugin_broker.artifacts.image=quay.io/eclipse/che-plugin-artifacts-broker:v3.4.0 +# Configures the default behavior of the plugin brokers when provisioning plugins into a workspace. +# If set to true, the plugin brokers will attempt to merge plugins when possible (i.e. they run in +# the same sidecar image and do not have conflicting settings). This value is the default setting +# used when the devfile does not specify otherwise, via the "mergePlugins" attribute. +che.workspace.plugin_broker.default_merge_plugins=false + # Docker image of Che plugin broker app that resolves workspace tooling configuration and copies # plugins dependencies to a workspace che.workspace.plugin_broker.pull_policy=Always diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesArtifactsBrokerApplier.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesArtifactsBrokerApplier.java index 7f106d7d1e9..adaea421226 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesArtifactsBrokerApplier.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesArtifactsBrokerApplier.java @@ -50,10 +50,14 @@ public KubernetesArtifactsBrokerApplier(BrokerEnvironmentFactory brokerEnviro * broker's configmap, machines, and volumes added in addition to the init container */ public void apply( - E workspaceEnvironment, RuntimeIdentity runtimeID, Collection pluginFQNs) + E workspaceEnvironment, + RuntimeIdentity runtimeID, + Collection pluginFQNs, + boolean mergePlugins) throws InfrastructureException { - E brokerEnvironment = brokerEnvironmentFactory.createForArtifactsBroker(pluginFQNs, runtimeID); + E brokerEnvironment = + brokerEnvironmentFactory.createForArtifactsBroker(pluginFQNs, runtimeID, mergePlugins); Map workspacePods = workspaceEnvironment.getPodsData(); if (workspacePods.size() != 1) { diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/PluginBrokerManager.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/PluginBrokerManager.java index 33a8705bbb9..df69e027e86 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/PluginBrokerManager.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/PluginBrokerManager.java @@ -102,6 +102,7 @@ public List getTooling( StartSynchronizer startSynchronizer, Collection pluginFQNs, boolean isEphemeral, + boolean mergePlugins, Map startOptions) throws InfrastructureException { @@ -109,7 +110,8 @@ public List getTooling( KubernetesNamespace kubernetesNamespace = factory.getOrCreate(identity); BrokersResult brokersResult = new BrokersResult(); - E brokerEnvironment = brokerEnvironmentFactory.createForMetadataBroker(pluginFQNs, identity); + E brokerEnvironment = + brokerEnvironmentFactory.createForMetadataBroker(pluginFQNs, identity, mergePlugins); if (isEphemeral) { EphemeralWorkspaceUtility.makeEphemeral(brokerEnvironment.getAttributes()); } diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java index ac8bd2bc88a..e62f85e46ce 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java @@ -11,12 +11,15 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.wsplugins; +import static org.eclipse.che.api.workspace.shared.Constants.MERGE_PLUGINS_ATTRIBUTE; + import com.google.common.annotations.Beta; import com.google.common.collect.ImmutableMap; import java.util.Collection; import java.util.List; import java.util.Map; import javax.inject.Inject; +import javax.inject.Named; import org.eclipse.che.api.core.model.workspace.runtime.RuntimeIdentity; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.api.workspace.server.wsplugins.ChePluginsApplier; @@ -44,17 +47,20 @@ public class SidecarToolingProvisioner { private final KubernetesArtifactsBrokerApplier artifactsBrokerApplier; private final PluginFQNParser pluginFQNParser; private final PluginBrokerManager pluginBrokerManager; + private final boolean defaultMergePlugins; @Inject public SidecarToolingProvisioner( Map workspaceNextAppliers, KubernetesArtifactsBrokerApplier artifactsBrokerApplier, PluginFQNParser pluginFQNParser, - PluginBrokerManager pluginBrokerManager) { + PluginBrokerManager pluginBrokerManager, + @Named("che.workspace.plugin_broker.default_merge_plugins") String defaultMergePlugins) { this.workspaceNextAppliers = ImmutableMap.copyOf(workspaceNextAppliers); this.artifactsBrokerApplier = artifactsBrokerApplier; this.pluginFQNParser = pluginFQNParser; this.pluginBrokerManager = pluginBrokerManager; + this.defaultMergePlugins = Boolean.parseBoolean(defaultMergePlugins); } @Traced @@ -79,12 +85,21 @@ public void provision( } boolean isEphemeral = EphemeralWorkspaceUtility.isEphemeral(environment.getAttributes()); + boolean mergePlugins = shouldMergePlugins(environment.getAttributes()); List chePlugins = pluginBrokerManager.getTooling( - identity, startSynchronizer, pluginFQNs, isEphemeral, startOptions); + identity, startSynchronizer, pluginFQNs, isEphemeral, mergePlugins, startOptions); pluginsApplier.apply(identity, environment, chePlugins); - artifactsBrokerApplier.apply(environment, identity, pluginFQNs); + artifactsBrokerApplier.apply(environment, identity, pluginFQNs, mergePlugins); LOG.debug("Finished sidecar tooling provisioning workspace '{}'", identity.getWorkspaceId()); } + + private boolean shouldMergePlugins(Map attributes) { + String devfileMergePlugins = attributes.get(MERGE_PLUGINS_ATTRIBUTE); + if (devfileMergePlugins != null) { + return Boolean.parseBoolean(devfileMergePlugins); + } + return defaultMergePlugins; + } } diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java index a80cf603fb3..9a04664d7eb 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java @@ -105,9 +105,11 @@ public BrokerEnvironmentFactory( * @param runtimeID ID of the runtime the broker would be started * @return kubernetes environment (or its extension) with the Plugin broker objects */ - public E createForMetadataBroker(Collection pluginFQNs, RuntimeIdentity runtimeID) + public E createForMetadataBroker( + Collection pluginFQNs, RuntimeIdentity runtimeID, boolean mergePlugins) throws InfrastructureException { - BrokersConfigs brokersConfigs = getBrokersConfigs(pluginFQNs, runtimeID, metadataBrokerImage); + BrokersConfigs brokersConfigs = + getBrokersConfigs(pluginFQNs, runtimeID, metadataBrokerImage, mergePlugins); return doCreate(brokersConfigs); } @@ -116,11 +118,14 @@ public E createForMetadataBroker(Collection pluginFQNs, RuntimeIdenti * * @param pluginFQNs fully qualified names of plugins that needs to be resolved by the broker * @param runtimeID ID of the runtime the broker would be started + * @param mergePlugins whether the broker should be configured to merge plugins where possible * @return kubernetes environment (or its extension) with the Plugin broker objects */ - public E createForArtifactsBroker(Collection pluginFQNs, RuntimeIdentity runtimeID) + public E createForArtifactsBroker( + Collection pluginFQNs, RuntimeIdentity runtimeID, boolean mergePlugins) throws InfrastructureException { - BrokersConfigs brokersConfigs = getBrokersConfigs(pluginFQNs, runtimeID, artifactsBrokerImage); + BrokersConfigs brokersConfigs = + getBrokersConfigs(pluginFQNs, runtimeID, artifactsBrokerImage, mergePlugins); brokersConfigs .machines .values() @@ -130,7 +135,10 @@ public E createForArtifactsBroker(Collection pluginFQNs, RuntimeIdent } protected BrokersConfigs getBrokersConfigs( - Collection pluginFQNs, RuntimeIdentity runtimeID, String brokerImage) + Collection pluginFQNs, + RuntimeIdentity runtimeID, + String brokerImage, + boolean mergePlugins) throws InfrastructureException { String configMapName = generateUniqueName(CONFIG_MAP_NAME_SUFFIX); @@ -143,7 +151,8 @@ protected BrokersConfigs getBrokersConfigs( authEnableEnvVarProvider.get(runtimeID), machineTokenEnvVarProvider.get(runtimeID)) .map(this::asEnvVar) .collect(Collectors.toList()); - Container container = newContainer(runtimeID, envVars, brokerImage, configMapVolume); + Container container = + newContainer(runtimeID, envVars, brokerImage, configMapVolume, mergePlugins); pod.getSpec().getContainers().add(container); pod.getSpec().getVolumes().add(newConfigMapVolume(configMapName, configMapVolume)); @@ -165,9 +174,10 @@ private Container newContainer( RuntimeIdentity runtimeId, List envVars, String image, - @Nullable String brokerVolumeName) { + @Nullable String brokerVolumeName, + boolean mergePlugins) { String containerName = generateContainerNameFromImageRef(image); - String[] cmdArgs = getCommandLineArgs(runtimeId).toArray(new String[0]); + String[] cmdArgs = getCommandLineArgs(runtimeId, mergePlugins).toArray(new String[0]); final ContainerBuilder cb = new ContainerBuilder() .withName(containerName) @@ -178,7 +188,7 @@ private Container newContainer( if (brokerVolumeName != null) { cb.withVolumeMounts( new VolumeMount(CONF_FOLDER + "/", null, brokerVolumeName, true, null, null)); - cb.addToArgs("-metas", CONF_FOLDER + "/" + CONFIG_FILE); + cb.addToArgs("--metas", CONF_FOLDER + "/" + CONFIG_FILE); } Container container = cb.build(); Containers.addRamLimit(container, "250Mi"); @@ -186,21 +196,26 @@ private Container newContainer( return container; } - protected List getCommandLineArgs(RuntimeIdentity runtimeId) { - return new ArrayList( - Arrays.asList( - "--push-endpoint", - cheWebsocketEndpoint, - "--runtime-id", - String.format( - "%s:%s:%s", - runtimeId.getWorkspaceId(), - MoreObjects.firstNonNull(runtimeId.getEnvName(), ""), - runtimeId.getOwnerId()), - "--cacert", - certProvisioner.isConfigured() ? certProvisioner.getCertPath() : "", - "--registry-address", - Strings.nullToEmpty(pluginRegistryUrl))); + protected List getCommandLineArgs(RuntimeIdentity runtimeId, boolean mergePlugins) { + ArrayList args = + new ArrayList<>( + Arrays.asList( + "--push-endpoint", + cheWebsocketEndpoint, + "--runtime-id", + String.format( + "%s:%s:%s", + runtimeId.getWorkspaceId(), + MoreObjects.firstNonNull(runtimeId.getEnvName(), ""), + runtimeId.getOwnerId()), + "--cacert", + certProvisioner.isConfigured() ? certProvisioner.getCertPath() : "", + "--registry-address", + Strings.nullToEmpty(pluginRegistryUrl))); + if (mergePlugins) { + args.add("--merge-plugins"); + } + return args; } private Pod newPod() { diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesArtifactsBrokerApplierTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesArtifactsBrokerApplierTest.java index 7860d7b97bf..9056a27ff80 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesArtifactsBrokerApplierTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesArtifactsBrokerApplierTest.java @@ -13,6 +13,7 @@ import static org.eclipse.che.workspace.infrastructure.kubernetes.Names.createMachineNameAnnotations; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.Mockito.doReturn; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; @@ -123,14 +124,14 @@ public void setUp() throws Exception { .build(); doReturn(brokerEnvironment) .when(brokerEnvironmentFactory) - .createForArtifactsBroker(any(), any()); + .createForArtifactsBroker(any(), any(), anyBoolean()); applier = new KubernetesArtifactsBrokerApplier<>(brokerEnvironmentFactory); } @Test public void shouldAddBrokerMachineToWorkspaceEnvironment() throws Exception { - applier.apply(workspaceEnvironment, runtimeID, pluginFQNs); + applier.apply(workspaceEnvironment, runtimeID, pluginFQNs, false); assertNotNull(workspaceEnvironment.getMachines()); assertTrue(workspaceEnvironment.getMachines().values().contains(brokerMachine)); @@ -138,7 +139,7 @@ public void shouldAddBrokerMachineToWorkspaceEnvironment() throws Exception { @Test public void shouldAddBrokerConfigMapsToWorkspaceEnvironment() throws Exception { - applier.apply(workspaceEnvironment, runtimeID, pluginFQNs); + applier.apply(workspaceEnvironment, runtimeID, pluginFQNs, false); ConfigMap workspaceConfigMap = workspaceEnvironment.getConfigMaps().get(BROKER_CONFIGMAP_NAME); assertNotNull(workspaceConfigMap); @@ -153,7 +154,7 @@ public void shouldAddBrokerConfigMapsToWorkspaceEnvironment() throws Exception { @Test public void shouldAddBrokerAsInitContainerOnWorkspacePod() throws Exception { - applier.apply(workspaceEnvironment, runtimeID, pluginFQNs); + applier.apply(workspaceEnvironment, runtimeID, pluginFQNs, false); List initContainers = workspacePod.getSpec().getInitContainers(); assertEquals(initContainers.size(), 1); @@ -162,7 +163,7 @@ public void shouldAddBrokerAsInitContainerOnWorkspacePod() throws Exception { @Test public void shouldAddBrokerVolumesToWorkspacePod() throws Exception { - applier.apply(workspaceEnvironment, runtimeID, pluginFQNs); + applier.apply(workspaceEnvironment, runtimeID, pluginFQNs, false); List workspaceVolumes = workspacePod.getSpec().getVolumes(); assertEquals(workspaceVolumes.size(), 1); diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/SidecarToolingProvisionerTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/SidecarToolingProvisionerTest.java index 2c87b679b9a..f5aa552640b 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/SidecarToolingProvisionerTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/SidecarToolingProvisionerTest.java @@ -12,7 +12,10 @@ package org.eclipse.che.workspace.infrastructure.kubernetes; import static java.util.Collections.emptyMap; +import static org.eclipse.che.api.workspace.shared.Constants.MERGE_PLUGINS_ATTRIBUTE; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; @@ -55,6 +58,8 @@ public class SidecarToolingProvisionerTest { @Mock private PluginBrokerManager brokerManager; @Mock private KubernetesEnvironment nonEphemeralEnvironment; @Mock private KubernetesEnvironment ephemeralEnvironment; + @Mock private KubernetesEnvironment mergePluginsEnvironment; + @Mock private KubernetesEnvironment noMergePluginsEnvironment; @Mock private RuntimeIdentity runtimeId; @Mock private ChePluginsApplier chePluginsApplier; @@ -66,43 +71,93 @@ public class SidecarToolingProvisionerTest { private Collection pluginFQNs = ImmutableList.of(new PluginFQN(null, PLUGIN_FQN_ID)); private SidecarToolingProvisioner provisioner; + private SidecarToolingProvisioner mergePluginsProvisioner; @BeforeMethod public void setUp() throws Exception { - Map workspaceNextAppliers = - ImmutableMap.of(RECIPE_TYPE, chePluginsApplier); - Map ephemeralEnvironmentAttributes = new HashMap<>(environmentAttributesBase); - EphemeralWorkspaceUtility.makeEphemeral(ephemeralEnvironmentAttributes); - Map nonEphemeralEnvironmentAttributes = - new HashMap<>(environmentAttributesBase); + Map ephemeralEnvAttributes = new HashMap<>(environmentAttributesBase); + EphemeralWorkspaceUtility.makeEphemeral(ephemeralEnvAttributes); + Map nonEphemeralEnvAttributes = new HashMap<>(environmentAttributesBase); + + Map mergePluginsEnvAttributes = new HashMap<>(environmentAttributesBase); + mergePluginsEnvAttributes.put(MERGE_PLUGINS_ATTRIBUTE, "true"); + Map noMergePluginsEnvAttributes = new HashMap<>(environmentAttributesBase); + noMergePluginsEnvAttributes.put(MERGE_PLUGINS_ATTRIBUTE, "false"); lenient().doReturn(RECIPE_TYPE).when(nonEphemeralEnvironment).getType(); lenient().doReturn(RECIPE_TYPE).when(ephemeralEnvironment).getType(); - lenient() - .doReturn(nonEphemeralEnvironmentAttributes) - .when(nonEphemeralEnvironment) - .getAttributes(); - lenient().doReturn(ephemeralEnvironmentAttributes).when(ephemeralEnvironment).getAttributes(); + lenient().doReturn(RECIPE_TYPE).when(mergePluginsEnvironment).getType(); + lenient().doReturn(RECIPE_TYPE).when(noMergePluginsEnvironment).getType(); + lenient().doReturn(nonEphemeralEnvAttributes).when(nonEphemeralEnvironment).getAttributes(); + lenient().doReturn(ephemeralEnvAttributes).when(ephemeralEnvironment).getAttributes(); + lenient().doReturn(mergePluginsEnvAttributes).when(mergePluginsEnvironment).getAttributes(); + lenient().doReturn(noMergePluginsEnvAttributes).when(noMergePluginsEnvironment).getAttributes(); doReturn(pluginFQNs).when(pluginFQNParser).parsePlugins(any()); - - provisioner = - new SidecarToolingProvisioner<>( - workspaceNextAppliers, artifactsBrokerApplier, pluginFQNParser, brokerManager); } @Test public void shouldIncludexArtifactsBrokerWhenWorkspaceIsNotEphemeral() throws Exception { + provisioner = getSidecarToolingProvisioner("false"); provisioner.provision(runtimeId, startSynchronizer, nonEphemeralEnvironment, emptyMap()); verify(chePluginsApplier, times(1)).apply(any(), any(), any()); - verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any()); + verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any(), anyBoolean()); } @Test public void shouldIncludeArtifactsBrokerWhenWorkspaceIsEphemeral() throws Exception { + provisioner = getSidecarToolingProvisioner("false"); provisioner.provision(runtimeId, startSynchronizer, ephemeralEnvironment, emptyMap()); verify(chePluginsApplier, times(1)).apply(any(), any(), any()); - verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any()); + verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any(), anyBoolean()); + } + + @Test + public void shouldMergePluginsWhenDefaultPropertyIsTrue() throws Exception { + provisioner = getSidecarToolingProvisioner("true"); + provisioner.provision(runtimeId, startSynchronizer, nonEphemeralEnvironment, emptyMap()); + + verify(brokerManager, times(1)).getTooling(any(), any(), any(), anyBoolean(), eq(true), any()); + verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any(), eq(true)); + } + + @Test + public void shouldNotMergePluginsWhenDefaultPropertyIsFalse() throws Exception { + provisioner = getSidecarToolingProvisioner("false"); + provisioner.provision(runtimeId, startSynchronizer, nonEphemeralEnvironment, emptyMap()); + + verify(brokerManager, times(1)).getTooling(any(), any(), any(), anyBoolean(), eq(false), any()); + verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any(), eq(false)); + } + + @Test + public void shouldMergePluginsWhenEnvironmentOverridesToTrue() throws Exception { + provisioner = getSidecarToolingProvisioner("false"); + provisioner.provision(runtimeId, startSynchronizer, mergePluginsEnvironment, emptyMap()); + + verify(brokerManager, times(1)).getTooling(any(), any(), any(), anyBoolean(), eq(true), any()); + verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any(), eq(true)); + } + + @Test + public void shouldNotMergePluginsWhenEnvironmentOverridesToFalse() throws Exception { + provisioner = getSidecarToolingProvisioner("true"); + provisioner.provision(runtimeId, startSynchronizer, noMergePluginsEnvironment, emptyMap()); + + verify(brokerManager, times(1)).getTooling(any(), any(), any(), anyBoolean(), eq(false), any()); + verify(artifactsBrokerApplier, times(1)).apply(any(), any(), any(), eq(false)); + } + + private SidecarToolingProvisioner getSidecarToolingProvisioner( + String mergePlugins) { + Map workspaceNextAppliers = + ImmutableMap.of(RECIPE_TYPE, chePluginsApplier); + return new SidecarToolingProvisioner<>( + workspaceNextAppliers, + artifactsBrokerApplier, + pluginFQNParser, + brokerManager, + mergePlugins); } } diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactoryTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactoryTest.java index d671d3b26ec..8d13ec1325a 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactoryTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactoryTest.java @@ -100,7 +100,7 @@ public void testMetadataBrokerSelfSignedCertificate() throws Exception { ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForMetadataBroker(pluginFQNs, runtimeId); + factory.createForMetadataBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); @@ -128,11 +128,12 @@ public void testMetadataBrokerSelfSignedCertificate() throws Exception { "/tmp/che/cacert", "--registry-address", DEFAULT_REGISTRY, - "-metas", + "--metas", "/broker-config/config.json", }); } + @Test public void testArtifactsBrokerSelfSignedCertificate() throws Exception { when(certProvisioner.isConfigured()).thenReturn(true); when(certProvisioner.getCertPath()).thenReturn("/tmp/che/cacert"); @@ -141,7 +142,7 @@ public void testArtifactsBrokerSelfSignedCertificate() throws Exception { ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForArtifactsBroker(pluginFQNs, runtimeId); + factory.createForArtifactsBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); @@ -159,21 +160,71 @@ public void testArtifactsBrokerSelfSignedCertificate() throws Exception { assertEquals( container.getArgs().toArray(), new String[] { - "-push-endpoint", + "--push-endpoint", PUSH_ENDPOINT, - "-runtime-id", + "--runtime-id", String.format( "%s:%s:%s", runtimeId.getWorkspaceId(), runtimeId.getEnvName(), runtimeId.getOwnerId()), - "-cacert", + "--cacert", "/tmp/che/cacert", "--registry-address", DEFAULT_REGISTRY, - "-metas", + "--metas", "/broker-config/config.json", }); } + @Test + public void testAddsMergeArgToArtifactsBroker() throws Exception { + // given + Collection pluginFQNs = singletonList(new PluginFQN(null, "id")); + ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); + + // when + factory.createForArtifactsBroker(pluginFQNs, runtimeId, true); + + // then + verify(factory).doCreate(captor.capture()); + BrokersConfigs brokersConfigs = captor.getValue(); + + List containers = + brokersConfigs + .pods + .values() + .stream() + .flatMap(p -> p.getSpec().getContainers().stream()) + .collect(Collectors.toList()); + assertEquals(containers.size(), 1); + Container container = containers.get(0); + assertTrue(container.getArgs().stream().anyMatch(e -> "--merge-plugins".equals(e))); + } + + @Test + public void testAddsMergeArgToMetadataBroker() throws Exception { + // given + Collection pluginFQNs = singletonList(new PluginFQN(null, "id")); + ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); + + // when + factory.createForMetadataBroker(pluginFQNs, runtimeId, true); + + // then + verify(factory).doCreate(captor.capture()); + BrokersConfigs brokersConfigs = captor.getValue(); + + List containers = + brokersConfigs + .pods + .values() + .stream() + .flatMap(p -> p.getSpec().getContainers().stream()) + .collect(Collectors.toList()); + assertEquals(containers.size(), 1); + Container container = containers.get(0); + assertTrue(container.getArgs().stream().anyMatch(e -> "--merge-plugins".equals(e))); + } + @Test public void shouldNameContainersAfterMetadataPluginBrokerImage() throws Exception { // given @@ -181,7 +232,7 @@ public void shouldNameContainersAfterMetadataPluginBrokerImage() throws Exceptio ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForMetadataBroker(pluginFQNs, runtimeId); + factory.createForMetadataBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); @@ -200,7 +251,7 @@ public void shouldNameContainersAfterArtifactsPluginBrokerImage() throws Excepti ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForArtifactsBroker(pluginFQNs, runtimeId); + factory.createForArtifactsBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); @@ -222,7 +273,7 @@ public void shouldCreateConfigMapWithPluginFQNsWithMetadataBroker() throws Excep ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForMetadataBroker(pluginFQNs, runtimeId); + factory.createForMetadataBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); @@ -254,7 +305,7 @@ public void shouldCreateConfigMapWithPluginFQNsWithArtifactsBroker() throws Exce ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForArtifactsBroker(pluginFQNs, runtimeId); + factory.createForArtifactsBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); @@ -283,7 +334,7 @@ public void shouldIncludePluginsVolumeInArtifactsBroker() throws Exception { ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForArtifactsBroker(pluginFQNs, runtimeId); + factory.createForArtifactsBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); @@ -300,7 +351,7 @@ public void shouldNotIncludePluginsVolumeInMetadataBroker() throws Exception { ArgumentCaptor captor = ArgumentCaptor.forClass(BrokersConfigs.class); // when - factory.createForMetadataBroker(pluginFQNs, runtimeId); + factory.createForMetadataBroker(pluginFQNs, runtimeId, false); // then verify(factory).doCreate(captor.capture()); diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/wsplugins/brokerphases/OpenshiftBrokerEnvironmentFactory.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/wsplugins/brokerphases/OpenshiftBrokerEnvironmentFactory.java index e71dfc2b7f1..6e8a2cd6f07 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/wsplugins/brokerphases/OpenshiftBrokerEnvironmentFactory.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/wsplugins/brokerphases/OpenshiftBrokerEnvironmentFactory.java @@ -81,8 +81,10 @@ protected OpenShiftEnvironment doCreate(BrokersConfigs brokersConfigs) { @Override public OpenShiftEnvironment createForMetadataBroker( - Collection pluginFQNs, RuntimeIdentity runtimeID) throws InfrastructureException { - BrokersConfigs brokersConfigs = getBrokersConfigs(pluginFQNs, runtimeID, metadataBrokerImage); + Collection pluginFQNs, RuntimeIdentity runtimeID, boolean mergePlugins) + throws InfrastructureException { + BrokersConfigs brokersConfigs = + getBrokersConfigs(pluginFQNs, runtimeID, metadataBrokerImage, mergePlugins); OpenShiftEnvironment openShiftEnvironment = doCreate(brokersConfigs); OpenShiftProject openshiftProject = factory.getOrCreate(runtimeID); trustedCAProvisioner.provision(openShiftEnvironment, openshiftProject); @@ -90,8 +92,8 @@ public OpenShiftEnvironment createForMetadataBroker( } @Override - protected List getCommandLineArgs(RuntimeIdentity runtimeId) { - List cmdArgs = super.getCommandLineArgs(runtimeId); + protected List getCommandLineArgs(RuntimeIdentity runtimeId, boolean mergePlugins) { + List cmdArgs = super.getCommandLineArgs(runtimeId, mergePlugins); if (trustedCAProvisioner.isTrustedStoreInitialized()) { cmdArgs.add("--cadir"); diff --git a/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java b/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java index 3db754d546d..320f795782d 100644 --- a/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java +++ b/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java @@ -13,6 +13,7 @@ import org.eclipse.che.api.core.model.workspace.Workspace; import org.eclipse.che.api.core.model.workspace.WorkspaceConfig; +import org.eclipse.che.api.core.model.workspace.devfile.Devfile; import org.eclipse.che.api.core.model.workspace.runtime.Machine; import org.eclipse.che.api.core.model.workspace.runtime.Server; @@ -105,7 +106,7 @@ public final class Constants { /** * The attribute allows to configure workspace to be ephemeral with no PVC attached on K8S / - * OpenShift infrastructure. Should be set/read from {@link WorkspaceConfig#getAttributes}. + * OpenShift infrastructure. Should be set/read from {@link Devfile#getAttributes}. * *

Value is expected to be boolean, and if set to 'false' regardless of the PVC strategy, * workspace volumes would be created as `emptyDir`. When a workspace Pod is removed for any @@ -118,12 +119,25 @@ public final class Constants { */ public static final String PERSIST_VOLUMES_ATTRIBUTE = "persistVolumes"; + /** + * This attribute enables or disables merging plugins while provisioning a workspace. When + * enabled, the plugin broker will be configured to attempt to merge plugins where possible (when + * two or more plugins share the same image and do not have conflicting settings). Should be + * set/read from {@link Devfile#getAttributes}. + * + *

Value is expected to be boolean; if it set to true, then the broker will attempt to merge + * plugins in the current devfile, otherwise it will not. The behavior when this property is not + * present in a devfile is governed by the configuration property {@code + * che.workspace.plugin_broker.default_merge_plugins}. + */ + public static final String MERGE_PLUGINS_ATTRIBUTE = "mergePlugins"; + /** * The attribute allows to configure workspace with async storage support this configuration. Make * sense only in case org.eclipse.che.api.workspace.shared.Constants#PERSIST_VOLUMES_ATTRIBUTE set * to 'false'. * - *

Should be set/read from {@link WorkspaceConfig#getAttributes}. + *

Should be set/read from {@link Devfile#getAttributes}. * *

Value is expected to be boolean, and if set to 'true' special plugin will be added to * workspace. It will provide ability to backup/restore project source to the async storage. @@ -134,7 +148,7 @@ public final class Constants { /** * Contains a list of workspace tooling plugins that should be used in a workspace. Should be - * set/read from {@link WorkspaceConfig#getAttributes}. + * set/read from {@link Devfile#getAttributes}. * *

Value is comma separated list of plugins in a format: '< plugin1ID >,'
* Spaces around commas are trimmed.