Fix setting jvm heap size params

Signed-off-by: patelsmit32123 <[email protected]>

Author: patelsmit32123

opensearch-operator/pkg/builders/cluster.go

@@ -169,7 +169,8 @@ func NewSTSForNodePool(
 		// vendor ="elasticsearch"
 	}
 
-	jvm := helpers.CalculateJvmHeapSize(&node)
+	jvmHeapSizeSettings := helpers.CalculateJvmHeapSizeSettings(node.Resources.Requests.Memory())
+	jvm := helpers.AppendJvmHeapSizeSettings(node.Jvm, jvmHeapSizeSettings)
 
 	// If node role `search` defined add required experimental flag if version less than 2.7
 	if helpers.ContainsString(selectedRoles, "search") && helpers.CompareVersions(cr.Spec.General.Version, "2.7.0") {
@@ -788,12 +789,8 @@ func NewBootstrapPod(
 	}
 	resources := cr.Spec.Bootstrap.Resources
 
-	var jvm string
-	if cr.Spec.Bootstrap.Jvm == "" {
-		jvm = "-Xmx512M -Xms512M"
-	} else {
-		jvm = cr.Spec.Bootstrap.Jvm
-	}
+	jvmHeapSizeSettings := helpers.CalculateJvmHeapSizeSettings(cr.Spec.Bootstrap.Resources.Requests.Memory())
+	jvm := helpers.AppendJvmHeapSizeSettings(cr.Spec.Bootstrap.Jvm, jvmHeapSizeSettings)
 
 	image := helpers.ResolveImage(cr, nil)
 	initHelperImage := helpers.ResolveInitHelperImage(cr)

opensearch-operator/pkg/builders/cluster_test.go

@@ -279,7 +279,7 @@ var _ = Describe("Builders", func() {
 
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx512M -Xms512M -Dopensearch.experimental.feature.searchable_snapshot.enabled=true -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms512M -Xmx512M -Dopensearch.experimental.feature.searchable_snapshot.enabled=true -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 
@@ -297,7 +297,7 @@ var _ = Describe("Builders", func() {
 
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx512M -Xms512M -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms512M -Xmx512M -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 
@@ -355,7 +355,7 @@ var _ = Describe("Builders", func() {
 			result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx1024M -Xms1024M -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms1024M -Xmx1024M -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 		It("should set jvm to half of memory request when memory request is fraction and jvm are not provided", func() {
@@ -370,7 +370,7 @@ var _ = Describe("Builders", func() {
 			result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx768M -Xms768M -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms768M -Xmx768M -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 
@@ -386,7 +386,7 @@ var _ = Describe("Builders", func() {
 			result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx953M -Xms953M -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms953M -Xmx953M -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 		It("should set jvm to default when memory request and jvm are not provided", func() {
@@ -395,24 +395,24 @@ var _ = Describe("Builders", func() {
 			result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx512M -Xms512M -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms512M -Xmx512M -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 		It("should set NodePool.Jvm as jvm when it jvm is provided", func() {
 			clusterObject := ClusterDescWithVersion("2.2.1")
 			nodePool := opsterv1.NodePool{
-				Jvm: "-Xmx1024M -Xms1024M",
+				Jvm: "-Xms1024M -Xmx1024M",
 			}
 			result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx1024M -Xms1024M -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms1024M -Xmx1024M -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 		It("should set NodePool.jvm as jvm when jvm and memory request are provided", func() {
 			clusterObject := ClusterDescWithVersion("2.2.1")
 			nodePool := opsterv1.NodePool{
-				Jvm: "-Xmx1024M -Xms1024M",
+				Jvm: "-Xms1024M -Xmx1024M",
 				Resources: corev1.ResourceRequirements{
 					Requests: corev1.ResourceList{
 						corev1.ResourceMemory: resource.MustParse("4Gi"),
@@ -422,7 +422,7 @@ var _ = Describe("Builders", func() {
 			result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
 			Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
 				Name:  "OPENSEARCH_JAVA_OPTS",
-				Value: "-Xmx1024M -Xms1024M -Dopensearch.transport.cname_in_publish_address=true",
+				Value: "-Xms1024M -Xmx1024M -Dopensearch.transport.cname_in_publish_address=true",
 			}))
 		})
 	})

opensearch-operator/pkg/helpers/helpers.go

@@ -5,9 +5,11 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"k8s.io/apimachinery/pkg/api/resource"
 	"log"
 	"reflect"
 	"sort"
+	"strings"
 	"time"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -439,25 +441,23 @@ func ComposePDB(cr *opsterv1.OpenSearchCluster, nodepool *opsterv1.NodePool) pol
 	return newpdb
 }
 
-func CalculateJvmHeapSize(nodePool *opsterv1.NodePool) string {
-	jvmHeapSizeTemplate := "-Xmx%s -Xms%s"
-
-	if nodePool.Jvm == "" {
-		memoryLimit := nodePool.Resources.Requests.Memory()
-
-		// Memory request is not present
-		if memoryLimit.IsZero() {
-			return fmt.Sprintf(jvmHeapSizeTemplate, "512M", "512M")
-		}
-
-		// Set Java Heap size to half of the node pool memory size
-		megabytes := float64((memoryLimit.Value() / 2) / 1024.0 / 1024.0)
-
-		heapSize := fmt.Sprintf("%vM", megabytes)
-		return fmt.Sprintf(jvmHeapSizeTemplate, heapSize, heapSize)
+func AppendJvmHeapSizeSettings(jvm string, heapSizeSettings string) string {
+	if strings.Contains(jvm, "Xms") || strings.Contains(jvm, "Xmx") {
+		return jvm
+	}
+	if jvm == "" {
+		return heapSizeSettings
 	}
+	return fmt.Sprintf("%s %s", jvm, heapSizeSettings)
+}
 
-	return nodePool.Jvm
+func CalculateJvmHeapSizeSettings(memoryRequest *resource.Quantity) string {
+	var memoryRequestMb int64 = 512
+	if memoryRequest != nil && !memoryRequest.IsZero() {
+		memoryRequestMb = ((memoryRequest.Value() / 2.0) / 1024.0) / 1024.0
+	}
+	// Set Java Heap size to half of the node pool memory request for both Xms and Xmx
+	return fmt.Sprintf("-Xms%dM -Xmx%dM", memoryRequestMb, memoryRequestMb)
 }
 
 func IsUpgradeInProgress(status opsterv1.ClusterStatus) bool {

opensearch-operator/pkg/helpers/helpers_test.go

@@ -0,0 +1,86 @@
+package helpers
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+var _ = Describe("JVM Heap Size Functions", func() {
+	Describe("AppendJvmHeapSizeSettings", func() {
+		Context("when JVM string already contains Xmx", func() {
+			It("should return the original JVM string unchanged", func() {
+				jvm := "-XX:+UseG1GC -Xmx2g -XX:MaxDirectMemorySize=1g"
+				heapSizeSettings := "-Xms1g -Xmx2g"
+
+				result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
+
+				Expect(result).To(Equal(jvm))
+			})
+		})
+
+		Context("when JVM string already contains Xms", func() {
+			It("should return the original JVM string unchanged", func() {
+				jvm := "-XX:+UseG1GC -Xms1g -XX:MaxDirectMemorySize=1g"
+				heapSizeSettings := "-Xms1g -Xmx2g"
+
+				result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
+
+				Expect(result).To(Equal(jvm))
+			})
+		})
+
+		Context("when JVM string is empty", func() {
+			It("should return only the heap size settings", func() {
+				jvm := ""
+				heapSizeSettings := "-Xmx1g -Xms1g"
+
+				result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
+
+				Expect(result).To(Equal(heapSizeSettings))
+			})
+		})
+
+		Context("when JVM string does not contain Xmx or Xms", func() {
+			It("should append the heap size settings", func() {
+				jvm := "-XX:+UseG1GC -XX:MaxDirectMemorySize=1g"
+				heapSizeSettings := "-Xmx1g -Xms1g"
+				expected := "-XX:+UseG1GC -XX:MaxDirectMemorySize=1g -Xmx1g -Xms1g"
+
+				result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
+
+				Expect(result).To(Equal(expected))
+			})
+		})
+	})
+
+	Describe("CalculateJvmHeapSizeSettings", func() {
+		Context("when memory request is nil", func() {
+			It("should return default 512M for both Xms and Xmx", func() {
+				result := CalculateJvmHeapSizeSettings(nil)
+
+				Expect(result).To(Equal("-Xms512M -Xmx512M"))
+			})
+		})
+
+		Context("when memory request is zero", func() {
+			It("should return default 512M for both Xms and Xmx", func() {
+				memoryRequest := resource.MustParse("0")
+
+				result := CalculateJvmHeapSizeSettings(&memoryRequest)
+
+				Expect(result).To(Equal("-Xms512M -Xmx512M"))
+			})
+		})
+
+		Context("when memory request is provided", func() {
+			It("should calculate both Xms and Xmx from request", func() {
+				memoryRequest := resource.MustParse("2Gi")
+
+				result := CalculateJvmHeapSizeSettings(&memoryRequest)
+
+				Expect(result).To(Equal("-Xms1024M -Xmx1024M"))
+			})
+		})
+	})
+})