PR comments fixes

Author: David Kowalski

pkg/controller/common/defaults/pod_template.go

@@ -321,6 +321,10 @@ func (b *PodTemplateBuilder) WithPreStopHook(handler corev1.Handler) *PodTemplat
 		b.Container.Lifecycle = &corev1.Lifecycle{}
 	}
 
-	b.Container.Lifecycle.PreStop = &handler
+	if b.Container.Lifecycle.PreStop == nil {
+		// no user-provided hook, we can use our own
+		b.Container.Lifecycle.PreStop = &handler
+	}
+
 	return b
 }

pkg/controller/elasticsearch/nodespec/defaults.go

@@ -58,7 +58,7 @@ func DefaultEnvVars(httpCfg commonv1.HTTPConfig, serviceName string) []corev1.En
 			{Name: settings.EnvProbePasswordPath, Value: path.Join(esvolume.ProbeUserSecretMountPath, user.InternalProbeUserName)},
 			{Name: settings.EnvProbeUsername, Value: user.InternalProbeUserName},
 			{Name: settings.EnvReadinessProbeProtocol, Value: httpCfg.Protocol()},
-			{Name: settings.ServiceName, Value: serviceName},
+			{Name: settings.HeadlessServiceName, Value: serviceName},
 
 			// Disable curl/libnss use of sqlite caching to avoid triggering an issue in linux/kubernetes
 			// where the kernel's dentry cache grows by 5mb every time curl is invoked. This cache usage

pkg/controller/elasticsearch/nodespec/lifecycle_hook.go

@@ -22,18 +22,33 @@ const PreStopHookScriptConfigKey = "pre-stop-hook-script.sh"
 const PreStopHookScript = `#!/usr/bin/env bash
 
 # This script will wait for up to $MAX_WAIT_SECONDS for $POD_IP to disappear from DNS record,
-# then it will wait additional $ADDN_WAIT_SECONDS and exit. This slows down the process shutdown
+# then it will wait additional $ADDITIONAL_WAIT_SECONDS and exit. This slows down the process shutdown
 # and allows to make changes to the pool gracefully, without blackholing traffic when DNS
-# contains IP that is already inactive. Assumes $SERVICE_NAME and $POD_IP env variables are defined.
+# contains IP that is already inactive. Assumes $HEADLESS_SERVICE_NAME and $POD_IP env variables are defined.
 
-MAX_WAIT_SECONDS=20 # max time to wait for pods IP to disappear from DNS
-ADDN_WAIT_SECONDS=1 # additional wait, allows queries to successfully use IP from DNS from before pod termination
+# max time to wait for pods IP to disappear from DNS. As this runs in parallel to grace period
+# (defaulting to 30s) after which process is SIGKILLed, it should be set to allow enough time
+# for the process to gracefully terminate.
+MAX_WAIT_SECONDS=20
+
+# additional wait, allows queries to successfully use IP from DNS from before pod termination
+# this gives a little bit more time for clients that resolved DNS just before DNS record
+# was updated.
+ADDITIONAL_WAIT_SECONDS=1
+
+START_TIME=$(date +%s)
+while true; do
+   ELAPSED_TIME=$(($(date +%s) - $START_TIME))
+
+   if [ $ELAPSED_TIME -gt $MAX_WAIT_SECONDS ]; then
+      exit 1
+   fi
+
+   if ! getent hosts $HEADLESS_SERVICE_NAME | grep $POD_IP; then
+      sleep $ADDITIONAL_WAIT_SECONDS
+      exit 0
+   fi
 
-for i in {1..$MAX_WAIT_SECONDS}
-do
-   getent hosts $SERVICE_NAME | grep $POD_IP || sleep $ADDN_WAIT_SECONDS && exit 0
    sleep 1
 done
-
-exit 1
 `

pkg/controller/elasticsearch/settings/environment.go

@@ -11,7 +11,7 @@ const (
 	EnvProbePasswordPath      = "PROBE_PASSWORD_PATH"
 	EnvProbeUsername          = "PROBE_USERNAME"
 	EnvReadinessProbeProtocol = "READINESS_PROBE_PROTOCOL"
-	ServiceName               = "SERVICE_NAME"
+	HeadlessServiceName       = "HEADLESS_SERVICE_NAME"
 
 	// EnvPodName and EnvPodIP are injected as env var into the ES pod at runtime,
 	// to be referenced in ES configuration file

test/e2e/es/mutation_test.go

@@ -5,6 +5,7 @@
 package es
 
 import (
+	"encoding/json"
 	"fmt"
 	"testing"
 	"time"
@@ -250,6 +251,7 @@ func TestMutationWhileLoadTesting(t *testing.T) {
 	w := test.NewOnceWatcher(
 		"load test",
 		func(k *test.K8sClient, t *testing.T) {
+			// wait for the LoadBalancer IP to be available
 			var ip string
 			for {
 				svc, err := k.GetService(b.Elasticsearch.Namespace, esv1.HTTPService(b.Elasticsearch.Name))
@@ -274,9 +276,11 @@ func TestMutationWhileLoadTesting(t *testing.T) {
 		func(k *test.K8sClient, t *testing.T) {
 			attacker.Stop()
 			metrics.Close()
-			assert.Equal(t, 1, len(metrics.StatusCodes))
+			bytes, _ := json.Marshal(metrics)
+			msgAndArgs := []interface{}{"metrics: %v", string(bytes)}
+			assert.Equal(t, 1, len(metrics.StatusCodes), msgAndArgs)
 			if _, ok := metrics.StatusCodes["401"]; !ok {
-				assert.Fail(t, "all status codes should be 401")
+				assert.Fail(t, "all status codes should be 401", msgAndArgs)
 			}
 		})