Rework default memory limit setting (#8422)

Extract default machine memory limit setting from InternalEnvironmentFactory to recipe specific environment factories. Make memory limit attribute optional by respecting it by resource API subsystem. Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
eclipse-che · Jan 24, 2018 · 9d75f3e · 9d75f3e
1 parent 21c8aaf
commit 9d75f3e
Show file tree

Hide file tree

Showing 15 changed files with 377 additions and 65 deletions.
diff --git a/...pi-model/src/main/java/org/eclipse/che/api/core/model/workspace/config/MachineConfig.java b/...pi-model/src/main/java/org/eclipse/che/api/core/model/workspace/config/MachineConfig.java
@@ -21,9 +21,9 @@
 public interface MachineConfig {
 
   /**
-   * Name of the attribute from {@link #getAttributes()} which if present sets memory limit of the
-   * machine in bytes. If memory limit is set in environment specific recipe this attribute should
-   * override value from recipe.
+   * Name of the attribute from {@link #getAttributes()} which if present defines memory limit of
+   * the machine in bytes. If memory limit is set in environment specific recipe this attribute used
+   * in {@code MachineConfig} should override value from recipe.
    */
   String MEMORY_LIMIT_ATTRIBUTE = "memoryLimitBytes";
 

diff --git a/infrastructures/docker/environment/pom.xml b/infrastructures/docker/environment/pom.xml
@@ -74,6 +74,11 @@
             <groupId>org.eclipse.che.core</groupId>
             <artifactId>che-core-api-workspace-shared</artifactId>
         </dependency>
+        <dependency>
+            <groupId>ch.qos.logback</groupId>
+            <artifactId>logback-classic</artifactId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.mockito</groupId>
             <artifactId>mockito-core</artifactId>

diff --git a/...se/che/workspace/infrastructure/docker/environment/compose/ComposeEnvironmentFactory.java b/...se/che/workspace/infrastructure/docker/environment/compose/ComposeEnvironmentFactory.java
@@ -50,6 +50,7 @@ public class ComposeEnvironmentFactory extends InternalEnvironmentFactory<Compos
 
   private final ComposeServicesStartStrategy startStrategy;
   private final ComposeEnvironmentValidator composeValidator;
+  private final String defaultMachineMemorySizeAttribute;
 
   @Inject
   public ComposeEnvironmentFactory(
@@ -59,9 +60,11 @@ public ComposeEnvironmentFactory(
       ComposeEnvironmentValidator composeValidator,
       ComposeServicesStartStrategy startStrategy,
       @Named("che.workspace.default_memory_mb") long defaultMachineMemorySizeMB) {
-    super(installerRegistry, recipeRetriever, machinesValidator, defaultMachineMemorySizeMB);
+    super(installerRegistry, recipeRetriever, machinesValidator);
     this.startStrategy = startStrategy;
     this.composeValidator = composeValidator;
+    this.defaultMachineMemorySizeAttribute =
+        String.valueOf(defaultMachineMemorySizeMB * 1024 * 1024);
   }
 
   @Override
@@ -117,6 +120,8 @@ void addRamLimitAttribute(
         final Long ramLimit = entry.getValue().getMemLimit();
         if (ramLimit != null && ramLimit > 0) {
           attributes.put(MEMORY_LIMIT_ATTRIBUTE, String.valueOf(ramLimit));
+        } else {
+          attributes.put(MEMORY_LIMIT_ATTRIBUTE, defaultMachineMemorySizeAttribute);
         }
       }
     }

diff --git a/.../workspace/infrastructure/docker/environment/dockerfile/DockerfileEnvironmentFactory.java b/.../workspace/infrastructure/docker/environment/dockerfile/DockerfileEnvironmentFactory.java
@@ -11,7 +11,9 @@
 package org.eclipse.che.workspace.infrastructure.docker.environment.dockerfile;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Strings.isNullOrEmpty;
 import static java.lang.String.format;
+import static org.eclipse.che.api.core.model.workspace.config.MachineConfig.MEMORY_LIMIT_ATTRIBUTE;
 
 import java.util.List;
 import java.util.Map;
@@ -33,13 +35,17 @@
 public class DockerfileEnvironmentFactory
     extends InternalEnvironmentFactory<DockerfileEnvironment> {
 
+  private final String defaultMachineMemorySizeAttribute;
+
   @Inject
   public DockerfileEnvironmentFactory(
       InstallerRegistry installerRegistry,
       RecipeRetriever recipeRetriever,
       MachineConfigsValidator machinesValidator,
       @Named("che.workspace.default_memory_mb") long defaultMachineMemorySizeMB) {
-    super(installerRegistry, recipeRetriever, machinesValidator, defaultMachineMemorySizeMB);
+    super(installerRegistry, recipeRetriever, machinesValidator);
+    this.defaultMachineMemorySizeAttribute =
+        String.valueOf(defaultMachineMemorySizeMB * 1024 * 1024);
   }
 
   @Override
@@ -55,6 +61,19 @@ protected DockerfileEnvironment doCreate(
 
     checkArgument(dockerfile != null, "Dockerfile content should not be null.");
 
+    addRamLimitAttribute(machines);
+
     return new DockerfileEnvironment(dockerfile, recipe, machines, warnings);
   }
+
+  void addRamLimitAttribute(Map<String, InternalMachineConfig> machines) {
+    // sets default ram limit attribute if not present
+    for (InternalMachineConfig machineConfig : machines.values()) {
+      if (isNullOrEmpty(machineConfig.getAttributes().get(MEMORY_LIMIT_ATTRIBUTE))) {
+        machineConfig
+            .getAttributes()
+            .put(MEMORY_LIMIT_ATTRIBUTE, defaultMachineMemorySizeAttribute);
+      }
+    }
+  }
 }
diff --git a/...orkspace/infrastructure/docker/environment/dockerimage/DockerImageEnvironmentFactory.java b/...orkspace/infrastructure/docker/environment/dockerimage/DockerImageEnvironmentFactory.java
@@ -11,7 +11,9 @@
 package org.eclipse.che.workspace.infrastructure.docker.environment.dockerimage;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Strings.isNullOrEmpty;
 import static java.lang.String.format;
+import static org.eclipse.che.api.core.model.workspace.config.MachineConfig.MEMORY_LIMIT_ATTRIBUTE;
 
 import java.util.List;
 import java.util.Map;
@@ -35,13 +37,17 @@
 public class DockerImageEnvironmentFactory
     extends InternalEnvironmentFactory<DockerImageEnvironment> {
 
+  private final String defaultMachineMemorySizeAttribute;
+
   @Inject
   public DockerImageEnvironmentFactory(
       InstallerRegistry installerRegistry,
       RecipeRetriever recipeRetriever,
       MachineConfigsValidator machinesValidator,
       @Named("che.workspace.default_memory_mb") long defaultMachineMemorySizeMB) {
-    super(installerRegistry, recipeRetriever, machinesValidator, defaultMachineMemorySizeMB);
+    super(installerRegistry, recipeRetriever, machinesValidator);
+    this.defaultMachineMemorySizeAttribute =
+        String.valueOf(defaultMachineMemorySizeMB * 1024 * 1024);
   }
 
   @Override
@@ -72,6 +78,19 @@ protected DockerImageEnvironment doCreate(
 
     checkArgument(dockerImage != null, "Docker image should not be null.");
 
+    addRamLimitAttribute(machines);
+
     return new DockerImageEnvironment(dockerImage, recipe, machines, warnings);
   }
+
+  private void addRamLimitAttribute(Map<String, InternalMachineConfig> machines) {
+    // sets default ram limit attribute if not present
+    for (InternalMachineConfig machineConfig : machines.values()) {
+      if (isNullOrEmpty(machineConfig.getAttributes().get(MEMORY_LIMIT_ATTRIBUTE))) {
+        machineConfig
+            .getAttributes()
+            .put(MEMORY_LIMIT_ATTRIBUTE, defaultMachineMemorySizeAttribute);
+      }
+    }
+  }
 }
diff --git a/...he/workspace/infrastructure/docker/environment/compose/ComposeEnvironmentFactoryTest.java b/...he/workspace/infrastructure/docker/environment/compose/ComposeEnvironmentFactoryTest.java
@@ -40,6 +40,8 @@
 @Listeners(MockitoTestNGListener.class)
 public class ComposeEnvironmentFactoryTest {
 
+  private static final long DEFAULT_RAM_LIMIT_MB = 2048;
+  private static final long BYTES_IN_MB = 1024 * 1024;
   private static final String MACHINE_NAME_1 = "machine1";
   private static final String MACHINE_NAME_2 = "machine2";
 
@@ -60,13 +62,13 @@ public void setup() {
             machinesValidator,
             composeValidator,
             startStrategy,
-            2048);
+            DEFAULT_RAM_LIMIT_MB);
   }
 
   @Test
   public void testSetsRamLimitAttributeFromComposeService() throws Exception {
-    final long firstMachineLimit = 3072;
-    final long secondMachineLimit = 1028;
+    final long firstMachineLimit = 3072 * BYTES_IN_MB;
+    final long secondMachineLimit = 1028 * BYTES_IN_MB;
     final Map<String, InternalMachineConfig> machines =
         ImmutableMap.of(
             MACHINE_NAME_1,
@@ -89,7 +91,7 @@ public void testSetsRamLimitAttributeFromComposeService() throws Exception {
 
   @Test
   public void testDoNotOverrideRamLimitAttributeWhenItAlreadyPresent() throws Exception {
-    final long customRamLimit = 3072;
+    final long customRamLimit = 3072 * BYTES_IN_MB;
     final Map<String, String> attributes =
         ImmutableMap.of(MEMORY_LIMIT_ATTRIBUTE, String.valueOf(customRamLimit));
     final Map<String, InternalMachineConfig> machines =
@@ -108,7 +110,7 @@ public void testDoNotOverrideRamLimitAttributeWhenItAlreadyPresent() throws Exce
   @Test
   public void testAddsMachineConfIntoEnvAndSetsRamLimAttributeWhenMachinePresentOnlyInRecipe()
       throws Exception {
-    final long customRamLimit = 2048;
+    final long customRamLimit = 3072 * BYTES_IN_MB;
     final Map<String, InternalMachineConfig> machines = new HashMap<>();
     final Map<String, ComposeService> services =
         ImmutableMap.of(MACHINE_NAME_1, mockComposeService(customRamLimit));
@@ -121,6 +123,21 @@ public void testAddsMachineConfIntoEnvAndSetsRamLimAttributeWhenMachinePresentOn
     assertTrue(Arrays.equals(actual, expected));
   }
 
+  @Test
+  public void testSetRamLimitAttributeWhenRamLimitIsMissingInRecipeAndConfig() throws Exception {
+    final Map<String, InternalMachineConfig> machines =
+        ImmutableMap.of(MACHINE_NAME_1, mockInternalMachineConfig(new HashMap<>()));
+    final Map<String, ComposeService> services =
+        ImmutableMap.of(MACHINE_NAME_1, mockComposeService(0));
+
+    composeEnvironmentFactory.addRamLimitAttribute(machines, services);
+
+    final long[] actual = machinesRam(machines.values());
+    final long[] expected = new long[actual.length];
+    fill(expected, DEFAULT_RAM_LIMIT_MB * BYTES_IN_MB);
+    assertTrue(Arrays.equals(actual, expected));
+  }
+
   private static InternalMachineConfig mockInternalMachineConfig(Map<String, String> attributes) {
     final InternalMachineConfig machineConfigMock = mock(InternalMachineConfig.class);
     when(machineConfigMock.getAttributes()).thenReturn(attributes);

diff --git a/...kspace/infrastructure/docker/environment/dockerfile/DockerfileEnvironmentFactoryTest.java b/...kspace/infrastructure/docker/environment/dockerfile/DockerfileEnvironmentFactoryTest.java
@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2012-2018 Red Hat, Inc.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *   Red Hat, Inc. - initial API and implementation
+ */
+package org.eclipse.che.workspace.infrastructure.docker.environment.dockerfile;
+
+import static java.util.Arrays.fill;
+import static org.eclipse.che.api.core.model.workspace.config.MachineConfig.MEMORY_LIMIT_ATTRIBUTE;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertTrue;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import org.eclipse.che.api.installer.server.InstallerRegistry;
+import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig;
+import org.eclipse.che.api.workspace.server.spi.environment.InternalRecipe;
+import org.eclipse.che.api.workspace.server.spi.environment.MachineConfigsValidator;
+import org.eclipse.che.api.workspace.server.spi.environment.RecipeRetriever;
+import org.mockito.Mock;
+import org.mockito.testng.MockitoTestNGListener;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+/** @author Alexander Garagatyi */
+@Listeners(MockitoTestNGListener.class)
+public class DockerfileEnvironmentFactoryTest {
+
+  private static final long DEFAULT_RAM_LIMIT_MB = 2048;
+  private static final long BYTES_IN_MB = 1024 * 1024;
+  private static final String MACHINE_NAME = "machine";
+
+  @Mock private InternalRecipe recipe;
+  @Mock private InstallerRegistry installerRegistry;
+  @Mock private RecipeRetriever recipeRetriever;
+  @Mock private MachineConfigsValidator machinesValidator;
+
+  private DockerfileEnvironmentFactory factory;
+
+  @BeforeMethod
+  public void setUp() throws Exception {
+    factory =
+        new DockerfileEnvironmentFactory(
+            installerRegistry, recipeRetriever, machinesValidator, DEFAULT_RAM_LIMIT_MB);
+
+    when(recipe.getType()).thenReturn(DockerfileEnvironment.TYPE);
+    when(recipe.getContent()).thenReturn("");
+  }
+
+  @Test
+  public void testSetRamLimitAttributeWhenRamLimitIsMissingInConfig() throws Exception {
+    final Map<String, InternalMachineConfig> machines =
+        ImmutableMap.of(MACHINE_NAME, mockInternalMachineConfig(new HashMap<>()));
+
+    factory.doCreate(recipe, machines, Collections.emptyList());
+
+    final long[] actual = machinesRam(machines.values());
+    final long[] expected = new long[actual.length];
+    fill(expected, DEFAULT_RAM_LIMIT_MB * BYTES_IN_MB);
+    assertTrue(Arrays.equals(actual, expected));
+  }
+
+  private static InternalMachineConfig mockInternalMachineConfig(Map<String, String> attributes) {
+    final InternalMachineConfig machineConfigMock = mock(InternalMachineConfig.class);
+    when(machineConfigMock.getAttributes()).thenReturn(attributes);
+    return machineConfigMock;
+  }
+
+  private static long[] machinesRam(Collection<InternalMachineConfig> configs) {
+    return configs
+        .stream()
+        .mapToLong(m -> Long.parseLong(m.getAttributes().get(MEMORY_LIMIT_ATTRIBUTE)))
+        .toArray();
+  }
+}
diff --git a/...pace/infrastructure/docker/environment/dockerimage/DockerImageEnvironmentFactoryTest.java b/...pace/infrastructure/docker/environment/dockerimage/DockerImageEnvironmentFactoryTest.java
@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2012-2018 Red Hat, Inc.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *   Red Hat, Inc. - initial API and implementation
+ */
+package org.eclipse.che.workspace.infrastructure.docker.environment.dockerimage;
+
+import static java.util.Arrays.fill;
+import static org.eclipse.che.api.core.model.workspace.config.MachineConfig.MEMORY_LIMIT_ATTRIBUTE;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertTrue;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import org.eclipse.che.api.installer.server.InstallerRegistry;
+import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig;
+import org.eclipse.che.api.workspace.server.spi.environment.InternalRecipe;
+import org.eclipse.che.api.workspace.server.spi.environment.MachineConfigsValidator;
+import org.eclipse.che.api.workspace.server.spi.environment.RecipeRetriever;
+import org.mockito.Mock;
+import org.mockito.testng.MockitoTestNGListener;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+/** @author Alexander Garagatyi */
+@Listeners(MockitoTestNGListener.class)
+public class DockerImageEnvironmentFactoryTest {
+  private static final long DEFAULT_RAM_LIMIT_MB = 2048;
+  private static final long BYTES_IN_MB = 1024 * 1024;
+  private static final String MACHINE_NAME = "machine";
+
+  @Mock private InternalRecipe recipe;
+  @Mock private InstallerRegistry installerRegistry;
+  @Mock private RecipeRetriever recipeRetriever;
+  @Mock private MachineConfigsValidator machinesValidator;
+
+  private DockerImageEnvironmentFactory factory;
+
+  @BeforeMethod
+  public void setUp() throws Exception {
+    factory =
+        new DockerImageEnvironmentFactory(
+            installerRegistry, recipeRetriever, machinesValidator, DEFAULT_RAM_LIMIT_MB);
+
+    when(recipe.getType()).thenReturn(DockerImageEnvironment.TYPE);
+    when(recipe.getContent()).thenReturn("");
+  }
+
+  @Test
+  public void testSetRamLimitAttributeWhenRamLimitIsMissingInConfig() throws Exception {
+    final Map<String, InternalMachineConfig> machines =
+        ImmutableMap.of(MACHINE_NAME, mockInternalMachineConfig(new HashMap<>()));
+
+    factory.doCreate(recipe, machines, Collections.emptyList());
+
+    final long[] actual = machinesRam(machines.values());
+    final long[] expected = new long[actual.length];
+    fill(expected, DEFAULT_RAM_LIMIT_MB * BYTES_IN_MB);
+    assertTrue(Arrays.equals(actual, expected));
+  }
+
+  private static InternalMachineConfig mockInternalMachineConfig(Map<String, String> attributes) {
+    final InternalMachineConfig machineConfigMock = mock(InternalMachineConfig.class);
+    when(machineConfigMock.getAttributes()).thenReturn(attributes);
+    return machineConfigMock;
+  }
+
+  private static long[] machinesRam(Collection<InternalMachineConfig> configs) {
+    return configs
+        .stream()
+        .mapToLong(m -> Long.parseLong(m.getAttributes().get(MEMORY_LIMIT_ATTRIBUTE)))
+        .toArray();
+  }
+}