Skip to content

Commit 9b10349

Browse files
patelsmit32123patelsmit32123
authored
Fix setting jvm heap size params (#1107)
### Issues Resolved Fixes #1106 ### Check List - [ ] Commits are signed per the DCO using --signoff - [ ] Unittest added for the new/changed functionality and all unit tests are successful - [ ] Customer-visible features documented - [ ] No linter warnings (`make lint`) If CRDs are changed: - [ ] CRD YAMLs updated (`make manifests`) and also copied into the helm chart - [ ] Changes to CRDs documented Please refer to the [PR guidelines](https://github.com/opensearch-project/opensearch-k8s-operator/blob/main/docs/developing.md#submitting-a-pr) before submitting this pull request. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). Signed-off-by: patelsmit32123 <[email protected]>
1 parent 76e65c7 commit 9b10349

File tree

4 files changed

+111
-34
lines changed

4 files changed

+111
-34
lines changed

‎opensearch-operator/pkg/builders/cluster.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,8 @@ func NewSTSForNodePool(
165165
// vendor ="elasticsearch"
166166
}
167167

168-
jvm := helpers.CalculateJvmHeapSize(&node)
168+
jvmHeapSizeSettings := helpers.CalculateJvmHeapSizeSettings(node.Resources.Requests.Memory())
169+
jvm := helpers.AppendJvmHeapSizeSettings(node.Jvm, jvmHeapSizeSettings)
169170

170171
// If node role `search` defined add required experimental flag if version less than 2.7
171172
if helpers.ContainsString(selectedRoles, "search") && helpers.CompareVersions(cr.Spec.General.Version, "2.7.0") {
@@ -790,12 +791,8 @@ func NewBootstrapPod(
790791
}
791792
resources := cr.Spec.Bootstrap.Resources
792793

793-
var jvm string
794-
if cr.Spec.Bootstrap.Jvm == "" {
795-
jvm = "-Xmx512M -Xms512M"
796-
} else {
797-
jvm = cr.Spec.Bootstrap.Jvm
798-
}
794+
jvmHeapSizeSettings := helpers.CalculateJvmHeapSizeSettings(cr.Spec.Bootstrap.Resources.Requests.Memory())
795+
jvm := helpers.AppendJvmHeapSizeSettings(cr.Spec.Bootstrap.Jvm, jvmHeapSizeSettings)
799796

800797
image := helpers.ResolveImage(cr, nil)
801798
initHelperImage := helpers.ResolveInitHelperImage(cr)

‎opensearch-operator/pkg/builders/cluster_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ var _ = Describe("Builders", func() {
279279

280280
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
281281
Name: "OPENSEARCH_JAVA_OPTS",
282-
Value: "-Xmx512M -Xms512M -Dopensearch.experimental.feature.searchable_snapshot.enabled=true -Dopensearch.transport.cname_in_publish_address=true",
282+
Value: "-Xms512M -Xmx512M -Dopensearch.experimental.feature.searchable_snapshot.enabled=true -Dopensearch.transport.cname_in_publish_address=true",
283283
}))
284284
})
285285

@@ -297,7 +297,7 @@ var _ = Describe("Builders", func() {
297297

298298
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
299299
Name: "OPENSEARCH_JAVA_OPTS",
300-
Value: "-Xmx512M -Xms512M -Dopensearch.transport.cname_in_publish_address=true",
300+
Value: "-Xms512M -Xmx512M -Dopensearch.transport.cname_in_publish_address=true",
301301
}))
302302
})
303303

@@ -409,7 +409,7 @@ var _ = Describe("Builders", func() {
409409
result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
410410
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
411411
Name: "OPENSEARCH_JAVA_OPTS",
412-
Value: "-Xmx1024M -Xms1024M -Dopensearch.transport.cname_in_publish_address=true",
412+
Value: "-Xms1024M -Xmx1024M -Dopensearch.transport.cname_in_publish_address=true",
413413
}))
414414
})
415415
It("should set jvm to half of memory request when memory request is fraction and jvm are not provided", func() {
@@ -424,7 +424,7 @@ var _ = Describe("Builders", func() {
424424
result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
425425
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
426426
Name: "OPENSEARCH_JAVA_OPTS",
427-
Value: "-Xmx768M -Xms768M -Dopensearch.transport.cname_in_publish_address=true",
427+
Value: "-Xms768M -Xmx768M -Dopensearch.transport.cname_in_publish_address=true",
428428
}))
429429
})
430430

@@ -440,7 +440,7 @@ var _ = Describe("Builders", func() {
440440
result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
441441
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
442442
Name: "OPENSEARCH_JAVA_OPTS",
443-
Value: "-Xmx953M -Xms953M -Dopensearch.transport.cname_in_publish_address=true",
443+
Value: "-Xms953M -Xmx953M -Dopensearch.transport.cname_in_publish_address=true",
444444
}))
445445
})
446446
It("should set jvm to default when memory request and jvm are not provided", func() {
@@ -449,24 +449,24 @@ var _ = Describe("Builders", func() {
449449
result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
450450
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
451451
Name: "OPENSEARCH_JAVA_OPTS",
452-
Value: "-Xmx512M -Xms512M -Dopensearch.transport.cname_in_publish_address=true",
452+
Value: "-Xms512M -Xmx512M -Dopensearch.transport.cname_in_publish_address=true",
453453
}))
454454
})
455455
It("should set NodePool.Jvm as jvm when it jvm is provided", func() {
456456
clusterObject := ClusterDescWithVersion("2.2.1")
457457
nodePool := opsterv1.NodePool{
458-
Jvm: "-Xmx1024M -Xms1024M",
458+
Jvm: "-Xms1024M -Xmx1024M",
459459
}
460460
result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
461461
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
462462
Name: "OPENSEARCH_JAVA_OPTS",
463-
Value: "-Xmx1024M -Xms1024M -Dopensearch.transport.cname_in_publish_address=true",
463+
Value: "-Xms1024M -Xmx1024M -Dopensearch.transport.cname_in_publish_address=true",
464464
}))
465465
})
466466
It("should set NodePool.jvm as jvm when jvm and memory request are provided", func() {
467467
clusterObject := ClusterDescWithVersion("2.2.1")
468468
nodePool := opsterv1.NodePool{
469-
Jvm: "-Xmx1024M -Xms1024M",
469+
Jvm: "-Xms1024M -Xmx1024M",
470470
Resources: corev1.ResourceRequirements{
471471
Requests: corev1.ResourceList{
472472
corev1.ResourceMemory: resource.MustParse("4Gi"),
@@ -476,7 +476,7 @@ var _ = Describe("Builders", func() {
476476
result := NewSTSForNodePool("foobar", &clusterObject, nodePool, "foobar", nil, nil, nil)
477477
Expect(result.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
478478
Name: "OPENSEARCH_JAVA_OPTS",
479-
Value: "-Xmx1024M -Xms1024M -Dopensearch.transport.cname_in_publish_address=true",
479+
Value: "-Xms1024M -Xmx1024M -Dopensearch.transport.cname_in_publish_address=true",
480480
}))
481481
})
482482

‎opensearch-operator/pkg/helpers/helpers.go‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,11 @@ import (
55
"errors"
66
"fmt"
77
"io"
8+
"k8s.io/apimachinery/pkg/api/resource"
89
"log"
910
"reflect"
1011
"sort"
12+
"strings"
1113
"time"
1214

1315
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -444,25 +446,23 @@ func ComposePDB(cr *opsterv1.OpenSearchCluster, nodepool *opsterv1.NodePool) pol
444446
return newpdb
445447
}
446448

447-
func CalculateJvmHeapSize(nodePool *opsterv1.NodePool) string {
448-
jvmHeapSizeTemplate := "-Xmx%s -Xms%s"
449-
450-
if nodePool.Jvm == "" {
451-
memoryLimit := nodePool.Resources.Requests.Memory()
452-
453-
// Memory request is not present
454-
if memoryLimit.IsZero() {
455-
return fmt.Sprintf(jvmHeapSizeTemplate, "512M", "512M")
456-
}
457-
458-
// Set Java Heap size to half of the node pool memory size
459-
megabytes := float64((memoryLimit.Value() / 2) / 1024.0 / 1024.0)
460-
461-
heapSize := fmt.Sprintf("%vM", megabytes)
462-
return fmt.Sprintf(jvmHeapSizeTemplate, heapSize, heapSize)
449+
func AppendJvmHeapSizeSettings(jvm string, heapSizeSettings string) string {
450+
if strings.Contains(jvm, "Xms") || strings.Contains(jvm, "Xmx") {
451+
return jvm
452+
}
453+
if jvm == "" {
454+
return heapSizeSettings
463455
}
456+
return fmt.Sprintf("%s %s", jvm, heapSizeSettings)
457+
}
464458

465-
return nodePool.Jvm
459+
func CalculateJvmHeapSizeSettings(memoryRequest *resource.Quantity) string {
460+
var memoryRequestMb int64 = 512
461+
if memoryRequest != nil && !memoryRequest.IsZero() {
462+
memoryRequestMb = ((memoryRequest.Value() / 2.0) / 1024.0) / 1024.0
463+
}
464+
// Set Java Heap size to half of the node pool memory request for both Xms and Xmx
465+
return fmt.Sprintf("-Xms%dM -Xmx%dM", memoryRequestMb, memoryRequestMb)
466466
}
467467

468468
func IsUpgradeInProgress(status opsterv1.ClusterStatus) bool {

‎opensearch-operator/pkg/helpers/helpers_test.go‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
. "github.com/onsi/ginkgo/v2"
66
. "github.com/onsi/gomega"
77
corev1 "k8s.io/api/core/v1"
8+
"k8s.io/apimachinery/pkg/api/resource"
89
"k8s.io/utils/ptr"
910
)
1011

@@ -162,3 +163,82 @@ var _ = Describe("Helper Functions", func() {
162163
})
163164

164165
})
166+
167+
var _ = Describe("JVM Heap Size Functions", func() {
168+
Describe("AppendJvmHeapSizeSettings", func() {
169+
Context("when JVM string already contains Xmx", func() {
170+
It("should return the original JVM string unchanged", func() {
171+
jvm := "-XX:+UseG1GC -Xmx2g -XX:MaxDirectMemorySize=1g"
172+
heapSizeSettings := "-Xms1g -Xmx2g"
173+
174+
result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
175+
176+
Expect(result).To(Equal(jvm))
177+
})
178+
})
179+
180+
Context("when JVM string already contains Xms", func() {
181+
It("should return the original JVM string unchanged", func() {
182+
jvm := "-XX:+UseG1GC -Xms1g -XX:MaxDirectMemorySize=1g"
183+
heapSizeSettings := "-Xms1g -Xmx2g"
184+
185+
result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
186+
187+
Expect(result).To(Equal(jvm))
188+
})
189+
})
190+
191+
Context("when JVM string is empty", func() {
192+
It("should return only the heap size settings", func() {
193+
jvm := ""
194+
heapSizeSettings := "-Xmx1g -Xms1g"
195+
196+
result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
197+
198+
Expect(result).To(Equal(heapSizeSettings))
199+
})
200+
})
201+
202+
Context("when JVM string does not contain Xmx or Xms", func() {
203+
It("should append the heap size settings", func() {
204+
jvm := "-XX:+UseG1GC -XX:MaxDirectMemorySize=1g"
205+
heapSizeSettings := "-Xmx1g -Xms1g"
206+
expected := "-XX:+UseG1GC -XX:MaxDirectMemorySize=1g -Xmx1g -Xms1g"
207+
208+
result := AppendJvmHeapSizeSettings(jvm, heapSizeSettings)
209+
210+
Expect(result).To(Equal(expected))
211+
})
212+
})
213+
})
214+
215+
Describe("CalculateJvmHeapSizeSettings", func() {
216+
Context("when memory request is nil", func() {
217+
It("should return default 512M for both Xms and Xmx", func() {
218+
result := CalculateJvmHeapSizeSettings(nil)
219+
220+
Expect(result).To(Equal("-Xms512M -Xmx512M"))
221+
})
222+
})
223+
224+
Context("when memory request is zero", func() {
225+
It("should return default 512M for both Xms and Xmx", func() {
226+
memoryRequest := resource.MustParse("0")
227+
228+
result := CalculateJvmHeapSizeSettings(&memoryRequest)
229+
230+
Expect(result).To(Equal("-Xms512M -Xmx512M"))
231+
})
232+
})
233+
234+
Context("when memory request is provided", func() {
235+
It("should calculate both Xms and Xmx from request", func() {
236+
memoryRequest := resource.MustParse("2Gi")
237+
238+
result := CalculateJvmHeapSizeSettings(&memoryRequest)
239+
240+
Expect(result).To(Equal("-Xms1024M -Xmx1024M"))
241+
})
242+
})
243+
})
244+
})

0 commit comments

Comments
 (0)