Repository: EnMasseProject/enmasse (java)

Issue:
Allow liveness/readiness probe settings to be overridden from infraconfig
StandardInfraConfigSpecBroker/BrokeredInfraConfigSpecBroker have a podTemplate which is used to influence the statefulset put down for the broker.  Currently it allows things like tolerances/affinity/resources to be controlled.

It would be good the liveness/readiness settings were controllable this way to.   We should also review if there are other tuneable parameters that should be respected too.

Pull request:
Fix #2864: Allow liveness/readiness probe timeouts to be configured from the infraconfigs
Also applied resource requirements and env var to initcontainers too. Bolstered unit tests.

Patch:
diff --git a/api-model/src/main/java/io/enmasse/address/model/KubeUtil.java b/api-model/src/main/java/io/enmasse/address/model/KubeUtil.java
index cc7de06f2..18de372a3 100644
--- a/api-model/src/main/java/io/enmasse/address/model/KubeUtil.java
+++ b/api-model/src/main/java/io/enmasse/address/model/KubeUtil.java
@@ -143,31 +143,73 @@ public final class KubeUtil {
                 actualPodSpec.setTolerations(podSpec.getTolerations());
             }
 
+            for (Container desiredContainer : podSpec.getInitContainers()) {
+                for (Container actualContainer : actualPodSpec.getInitContainers()) {
+                    if (actualContainer.getName() != null && actualContainer.getName().equals(desiredContainer.getName())) {
+                        applyDesiredResources(desiredContainer, actualContainer);
+                        applyDesiredEnvironment(actualContainer, desiredContainer);
+                    }
+                }
+            }
+
             for (Container desiredContainer : podSpec.getContainers()) {
                 for (Container actualContainer : actualPodSpec.getContainers()) {
                     if (actualContainer.getName() != null && actualContainer.getName().equals(desiredContainer.getName())) {
-                        if (desiredContainer.getResources() != null) {
-                            actualContainer.setResources(desiredContainer.getResources());
-                        }
-
-                        List<EnvVar> desiredEnv = desiredContainer.getEnv();
-                        if (desiredEnv != null && !desiredEnv.isEmpty()) {
-                            if (actualContainer.getEnv() == null || actualContainer.getEnv().isEmpty()) {
-                                actualContainer.setEnv(desiredEnv);
-                            } else {
-                                Set<String> newNames = desiredEnv.stream().map(EnvVar::getName).collect(Collectors.toSet());
-                                List<EnvVar> current = new ArrayList<>(actualContainer.getEnv());
-                                current.removeIf(e -> e.getName() != null && newNames.contains(e.getName()));
-                                current.addAll(desiredEnv);
-                                actualContainer.setEnv(current);
-                            }
-                        }
+                        applyDesiredResources(desiredContainer, actualContainer);
+                        applyDesiredEnvironment(actualContainer, desiredContainer);
+                        applyDesiredProbeSettings(actualContainer.getLivenessProbe(), desiredContainer.getLivenessProbe());
+                        applyDesiredProbeSettings(actualContainer.getReadinessProbe(), desiredContainer.getReadinessProbe());
                     }
                 }
             }
         }
     }
 
+    private static void applyDesiredResources(Container desiredContainer, Container actualContainer) {
+        if (desiredContainer.getResources() != null) {
+            actualContainer.setResources(desiredContainer.getResources());
+        }
+    }
+
+    private static void applyDesiredProbeSettings(Probe actualProbe, Probe desiredProbe) {
+        if (actualProbe != null && desiredProbe != null) {
+            if (desiredProbe.getInitialDelaySeconds() != null) {
+                actualProbe.setInitialDelaySeconds(desiredProbe.getInitialDelaySeconds());
+            }
+
+            if (desiredProbe.getPeriodSeconds() != null) {
+                actualProbe.setPeriodSeconds(desiredProbe.getPeriodSeconds());
+            }
+
+            if (desiredProbe.getTimeoutSeconds() != null) {
+                actualProbe.setTimeoutSeconds(desiredProbe.getTimeoutSeconds());
+            }
+
+            if (desiredProbe.getSuccessThreshold() != null) {
+                actualProbe.setSuccessThreshold(desiredProbe.getSuccessThreshold());
+            }
+
+            if (desiredProbe.getFailureThreshold() != null) {
+                actualProbe.setFailureThreshold(desiredProbe.getFailureThreshold());
+            }
+        }
+    }
+
+    private static void applyDesiredEnvironment(Container actualContainer, Container desiredContainer) {
+        List<EnvVar> desiredEnv = desiredContainer.getEnv();
+        if (desiredEnv != null && !desiredEnv.isEmpty()) {
+            if (actualContainer.getEnv() == null || actualContainer.getEnv().isEmpty()) {
+                actualContainer.setEnv(desiredEnv);
+            } else {
+                Set<String> newNames = desiredEnv.stream().map(EnvVar::getName).collect(Collectors.toSet());
+                List<EnvVar> current = new ArrayList<>(actualContainer.getEnv());
+                current.removeIf(e -> e.getName() != null && newNames.contains(e.getName()));
+                current.addAll(desiredEnv);
+                actualContainer.setEnv(current);
+            }
+        }
+    }
+
 
     public static void validateName(String name) {
         if (name == null) {
diff --git a/broker-plugin/plugin/src/main/resources/brokered/broker.xml b/broker-plugin/plugin/src/main/resources/brokered/broker.xml
index a60595750..354f4ea8d 100644
--- a/broker-plugin/plugin/src/main/resources/brokered/broker.xml
+++ b/broker-plugin/plugin/src/main/resources/brokered/broker.xml
@@ -93,7 +93,7 @@ under the License.
       <address-settings>
          <!--default for catch all-->
          <address-setting match="#">
-            <dead-letter-address>DLQ</dead-letter-address>
+            <max-delivery-attempts>-1</max-delivery-attempts>
             <redelivery-delay>0</redelivery-delay>
             <!-- with -1 only the global-max-size is in use for limiting -->
             <max-size-bytes>-1</max-size-bytes>
diff --git a/broker-plugin/plugin/src/main/resources/standard/colocated/broker.xml b/broker-plugin/plugin/src/main/resources/standard/colocated/broker.xml
index dc54c820c..e9b4fef95 100644
--- a/broker-plugin/plugin/src/main/resources/standard/colocated/broker.xml
+++ b/broker-plugin/plugin/src/main/resources/standard/colocated/broker.xml
@@ -83,7 +83,7 @@ under the License.
       <address-settings>
          <!--default for catch all-->
          <address-setting match="#">
-            <dead-letter-address>DLQ</dead-letter-address>
+            <max-delivery-attempts>-1</max-delivery-attempts>
             <redelivery-delay>0</redelivery-delay>
             <!-- with -1 only the global-max-size is in use for limiting -->
             <max-size-bytes>-1</max-size-bytes>
diff --git a/broker-plugin/plugin/src/main/resources/standard/sharded-queue/broker.xml b/broker-plugin/plugin/src/main/resources/standard/sharded-queue/broker.xml
index 5e9c76f81..442b6fb86 100644
--- a/broker-plugin/plugin/src/main/resources/standard/sharded-queue/broker.xml
+++ b/broker-plugin/plugin/src/main/resources/standard/sharded-queue/broker.xml
@@ -85,7 +85,7 @@ under the License.
       <address-settings>
          <!--default for catch all-->
          <address-setting match="#">
-            <dead-letter-address>DLQ</dead-letter-address>
+            <max-delivery-attempts>-1</max-delivery-attempts>
             <redelivery-delay>0</redelivery-delay>
             <!-- with -1 only the global-max-size is in use for limiting -->
             <max-size-bytes>-1</max-size-bytes>
diff --git a/broker-plugin/plugin/src/main/resources/standard/sharded-topic/broker.xml b/broker-plugin/plugin/src/main/resources/standard/sharded-topic/broker.xml
index 018f816ad..28cbad42c 100644
--- a/broker-plugin/plugin/src/main/resources/standard/sharded-topic/broker.xml
+++ b/broker-plugin/plugin/src/main/resources/standard/sharded-topic/broker.xml
@@ -86,7 +86,7 @@ under the License.
       <address-settings>
          <!--default for catch all-->
          <address-setting match="#">
-            <dead-letter-address>DLQ</dead-letter-address>
+            <max-delivery-attempts>-1</max-delivery-attempts>
             <redelivery-delay>0</redelivery-delay>
             <!-- with -1 only the global-max-size is in use for limiting -->
             <max-size-bytes>-1</max-size-bytes>


Test patch:
diff --git a/api-model/src/test/java/io/enmasse/address/model/v1/address/KubeUtilTest.java b/api-model/src/test/java/io/enmasse/address/model/v1/address/KubeUtilTest.java
index 4519c3e4e..97fba7f54 100644
--- a/api-model/src/test/java/io/enmasse/address/model/v1/address/KubeUtilTest.java
+++ b/api-model/src/test/java/io/enmasse/address/model/v1/address/KubeUtilTest.java
@@ -6,12 +6,16 @@ package io.enmasse.address.model.v1.address;
 
 import io.enmasse.address.model.KubeUtil;
 
+import static org.hamcrest.Matchers.*;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.params.provider.Arguments.of;
 
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
 import java.util.stream.Stream;
 
-import org.hamcrest.Matchers;
+import io.fabric8.kubernetes.api.model.*;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
@@ -27,6 +31,11 @@ import static org.hamcrest.MatcherAssert.assertThat;
 
 public class KubeUtilTest {
 
+    private static final EnvVar FOO1_ENVVAR = new EnvVarBuilder().withName("FOO1").withValue("BAR").build();
+    private static final EnvVar FOO1_UPD_ENVVAR = new EnvVarBuilder().withName("FOO1").withValue("BAZ").build();
+    private static final EnvVar FOO2_ENVAR = new EnvVarBuilder().withName("FOO2").withValue("BAR").build();
+    private static final EnvVar FOO3_ENVVAR = new EnvVarBuilder().withName("FOO3").withValue("BAX").build();
+
     @Test
     public void testLeaveSpaceForPodIdentifier() {
         String address = "receiver-round-robincli_rhearatherlongaddresswhichcanbeverylongblablabla";
@@ -42,6 +51,169 @@ public class KubeUtilTest {
         assertNull(KubeUtil.sanitizeUserName(null));
     }
 
+    @Test
+    public void appliesContainerResourcesToPodTemplate() {
+        Container actualContainer = new ContainerBuilder()
+                .withName("foo").build();
+        Map<String, Quantity> widgets = Collections.singletonMap("widgets", new QuantityBuilder().withAmount("10").build());
+        ResourceRequirements resources = new ResourceRequirementsBuilder().withLimits(widgets).build();
+        Container desiredContainer = new ContainerBuilder()
+                .withName("foo")
+                .withResources(resources).build();
+
+        PodTemplateSpec actual = doApplyContainers(actualContainer, desiredContainer);
+
+        Container container = actual.getSpec().getContainers().get(0);
+        assertThat(container.getResources(), equalTo(resources));
+    }
+
+    @Test
+    public void appliesContainerOrderIgnored() {
+        Container actualFooContainer = new ContainerBuilder()
+                .withName("foo").build();
+        Container actualBarContainer = new ContainerBuilder()
+                .withName("bar").build();
+
+        Map<String, Quantity> widgets = Collections.singletonMap("widgets", new QuantityBuilder().withAmount("10").build());
+        ResourceRequirements resources = new ResourceRequirementsBuilder().withLimits(widgets).build();
+        Container desiredFooContainer = new ContainerBuilder()
+                .withName("foo")
+                .withResources(resources).build();
+
+        PodTemplateSpec actual1 = new PodTemplateSpecBuilder()
+                .withNewSpec()
+                .addToContainers(actualBarContainer, actualFooContainer)
+                .endSpec()
+                .build();
+
+        PodTemplateSpec desired = new PodTemplateSpecBuilder()
+                .withNewSpec()
+                .addToContainers(desiredFooContainer)
+                .endSpec()
+                .build();
+
+        KubeUtil.applyPodTemplate(actual1, desired);
+
+        PodTemplateSpec actual = actual1;
+
+        Container barContainer = actual.getSpec().getContainers().get(0);
+        assertThat(barContainer.getName(), equalTo("bar"));
+        assertThat(barContainer.getResources(), nullValue());
+
+        Container fooContainer = actual.getSpec().getContainers().get(1);
+        assertThat(fooContainer.getName(), equalTo("foo"));
+        assertThat(fooContainer.getResources(), equalTo(resources));
+    }
+
+    @Test
+    public void appliesContainerEnvVarToPodTemplate() {
+        Container actualContainer = new ContainerBuilder()
+                .withName("foo")
+                .withEnv(FOO1_ENVVAR, FOO2_ENVAR).build();
+        Container desiredContainer = new ContainerBuilder()
+                .withName("foo")
+                .withEnv(FOO1_UPD_ENVVAR, FOO3_ENVVAR).build();
+
+        PodTemplateSpec actual = doApplyContainers(actualContainer, desiredContainer);
+
+        Container container = actual.getSpec().getContainers().get(0);
+        List<EnvVar> probe = container.getEnv();
+        assertThat(probe.size(), equalTo(3));
+        assertThat(probe, containsInAnyOrder(FOO1_UPD_ENVVAR, FOO2_ENVAR, FOO3_ENVVAR));
+    }
+
+    @Test
+    public void appliesInitContainerEnvVarToPodTemplate() {
+        Container actualContainer = new ContainerBuilder()
+                .withName("foo")
+                .withEnv(FOO1_ENVVAR).build();
+        Container desiredContainer = new ContainerBuilder()
+                .withName("foo")
+                .withEnv(FOO1_UPD_ENVVAR).build();
+
+        PodTemplateSpec actual = doApplyInitContainers(actualContainer, desiredContainer);
+
+        Container container = actual.getSpec().getInitContainers().get(0);
+        List<EnvVar> probe = container.getEnv();
+        assertThat(probe.size(), equalTo(1));
+        assertThat(probe, containsInAnyOrder(FOO1_UPD_ENVVAR));
+    }
+
+    @Test
+    public void appliesContainerLivenessProbeSettingsToPodTemplate() {
+        Container actualContainer = new ContainerBuilder()
+                .withName("foo")
+                .withLivenessProbe(new ProbeBuilder()
+                        .withInitialDelaySeconds(1)
+                        .withNewPeriodSeconds(2)
+                        .withFailureThreshold(4).build()).build();
+        Container desiredContainer = new ContainerBuilder()
+                .withName("foo")
+                .withLivenessProbe(new ProbeBuilder()
+                        .withInitialDelaySeconds(10)
+                        .withSuccessThreshold(80).build()).build();
+        PodTemplateSpec actual = doApplyContainers(actualContainer, desiredContainer);
+
+        Container container = actual.getSpec().getContainers().get(0);
+        Probe probe = container.getLivenessProbe();
+        assertThat(probe.getInitialDelaySeconds(), equalTo(10));
+        assertThat(probe.getPeriodSeconds(), equalTo(2));
+        assertThat(probe.getFailureThreshold(), equalTo(4));
+        assertThat(probe.getSuccessThreshold(), equalTo(80));
+    }
+
+    @Test
+    public void appliesContainerReadinessProbeSettingsToPodTemplate() {
+        Container actualContainer = new ContainerBuilder()
+                .withName("foo")
+                .withReadinessProbe(new ProbeBuilder()
+                        .withInitialDelaySeconds(1).build()).build();
+        Container desiredContainer = new ContainerBuilder()
+                .withName("foo")
+                .withReadinessProbe(new ProbeBuilder()
+                        .withInitialDelaySeconds(10).build()).build();
+        PodTemplateSpec actual = doApplyContainers(actualContainer, desiredContainer);
+
+        Container container = actual.getSpec().getContainers().get(0);
+        assertThat(container.getReadinessProbe().getInitialDelaySeconds(), equalTo(10));
+    }
+
+    private PodTemplateSpec doApplyContainers(Container actualContainer, Container desiredContainer) {
+        PodTemplateSpec actual = new PodTemplateSpecBuilder()
+                .withNewSpec()
+                .addToContainers(actualContainer)
+                .endSpec()
+                .build();
+
+        PodTemplateSpec desired = new PodTemplateSpecBuilder()
+                .withNewSpec()
+                .addToContainers(desiredContainer)
+                .endSpec()
+                .build();
+
+        KubeUtil.applyPodTemplate(actual, desired);
+
+        return actual;
+    }
+
+    private PodTemplateSpec doApplyInitContainers(Container actualContainer, Container desiredContainer) {
+        PodTemplateSpec actual = new PodTemplateSpecBuilder()
+                .withNewSpec()
+                .addToInitContainers(actualContainer)
+                .endSpec()
+                .build();
+
+        PodTemplateSpec desired = new PodTemplateSpecBuilder()
+                .withNewSpec()
+                .addToInitContainers(desiredContainer)
+                .endSpec()
+                .build();
+
+        KubeUtil.applyPodTemplate(actual, desired);
+
+        return actual;
+    }
+
     protected static Stream<Arguments> commonNames() {
         return Stream.of(
                         of("foo", "foo"),
@@ -81,7 +253,7 @@ public class KubeUtilTest {
     public void testSanitizeAddressName(final String input, final String output) {
         final String result = KubeUtil.sanitizeName(input);
 
-        assertThat(result.length(), Matchers.lessThanOrEqualTo(60));
+        assertThat(result.length(), lessThanOrEqualTo(60));
         assertEquals(output, result);
 
     }