Issue 130: Ability to specify container resources #129

adrianmo · 2019-03-04T16:56:09Z

Change log description

This PR will bring the following changes:

Set adequate default resource requests and limits for Pravega components
Expose those resource setting to allow custom configurations
Replace static Xmx JVM option with the dynamic, Docker-aware flags -XX:+UnlockExperimentalVMOptions -XX:+UseCGroupMemoryLimitForHeap which will set the heap memory size to the given Pod memory limit. (Ref: https://blogs.oracle.com/java-platform-group/java-se-support-for-docker-cpu-and-memory-limits)

At this moment, the PR is being used for testing with the goal of finding the suitable values for resource requests and limits.

Purpose of the change

Fix #130

How to test the change

When defining the Pravega cluster specification in the YAML file, include the following options to set custom resource requests and limits for each Pravega component.

apiVersion: "pravega.pravega.io/v1alpha1"
kind: "PravegaCluster"
metadata:
  name: "example"
spec:
  ...
  bookkeeper:
    ...
    resources:
      requests:
        memory: "3Gi"
        cpu: "1000m"
      limits:
        memory: "5Gi"
        cpu: "2000m"

  pravega:
    ...
    controllerResources:
      requests:
        memory: "1Gi"
        cpu: "1000m"
      limits:
        memory: "3Gi"
        cpu: "2000m"

    segmentStoreResources:
      requests:
        memory: "3Gi"
        cpu: "1000m"
      limits:
        memory: "5Gi"
        cpu: "2000m"

You can verify that those values have been correctly set by looking at the Pods description.

Signed-off-by: Adrián Moreno adrian@morenomartinez.com

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

* master: Run tests on GKE (#140)

* master: Issue 134,137: Fix BookKeeper errors (#135)

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

RaulGracia

Looks good, some comments.

RaulGracia · 2019-03-20T15:38:07Z

pkg/apis/pravega/v1alpha1/bookkeeper.go

+		changed = true
+		s.Resources = &v1.ResourceRequirements{
+			Requests: v1.ResourceList{
+				v1.ResourceCPU:    resource.MustParse("500m"),


Why these values are lower than the ones in cr-detailed.yaml? Shall we stick to a consistent defaults?

The default values are meant to represent the bare minimum requirements for Pravega to run. These values are intended to be use in a first contact with Pravega, functional testing, or development work. Therefore, we want to keep them low so that Pravega can be deployed to small Kubernetes clusters. Whereas the values in cr-detailed.yaml are just an example of how the default resources would be overridden to tune Pravega resources to a higher performance cluster.

RaulGracia · 2019-03-20T15:38:38Z

pkg/apis/pravega/v1alpha1/pravega.go

+		changed = true
+		s.ControllerResources = &v1.ResourceRequirements{
+			Requests: v1.ResourceList{
+				v1.ResourceCPU:    resource.MustParse("250m"),


Same comment here with the defaults compared to cr-detailed.yaml.

Responded in #129 (comment)

RaulGracia · 2019-03-20T15:50:59Z

pkg/controller/pravega/pravega_segmentstore.go

 	javaOpts := []string{
-		"-Xms1g -Xmx4g -XX:MaxDirectMemorySize=1g -Dpravegaservice.clusterName=" + pravegaCluster.Name,
+		"-Xms1g",
+		"-XX:+UnlockExperimentalVMOptions",


Are these options configurable as part of another PR, right?

No, JVM options are not externally configurable.

Ok, this may not be great, because we will rely on the default -Xmx value set by the JVM implementation. The default may be set as follows:

Often its default value is 1/4th of your physical memory or 1GB (whichever is smaller).

So, we may end up providing 4GBs of memory to a pod, but 1GB at most will be devoted to the JVM (despite the main purpose of that pod is to run the JVM for the Pravega process at hand). What is your opinion about this? Have you used -XX:+PrintFlagsFinal to see what is the actual -Xmx value that is being used? Would it make sense to set the -Xmx value in function to the provisioned memory resources for the pod?

RaulGracia · 2019-03-20T15:52:38Z

example/cr-detailed.yaml

@@ -12,6 +12,13 @@ spec:
      pullPolicy: IfNotPresent

    replicas: 3
+    resources:


Is there any test that validates that these values are properly passed to the process?

No, we will work on adding a test to validate that resources are passed down to pod templates correctly.

Test added. Could you please review it again? Thanks!

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

adrianmo

@Tristan1900 awesome work adding the test cases. Thanks!

RaulGracia

I have one comment that may be relevant for JVM provisioning.

RaulGracia · 2019-03-21T11:49:00Z

pkg/controller/pravega/pravega_segmentstore.go

 	javaOpts := []string{
-		"-Xms1g -Xmx4g -XX:MaxDirectMemorySize=1g -Dpravegaservice.clusterName=" + pravegaCluster.Name,
+		"-Xms1g",
+		"-XX:+UnlockExperimentalVMOptions",


Ok, this may not be great, because we will rely on the default -Xmx value set by the JVM implementation. The default may be set as follows:

Often its default value is 1/4th of your physical memory or 1GB (whichever is smaller).

So, we may end up providing 4GBs of memory to a pod, but 1GB at most will be devoted to the JVM (despite the main purpose of that pod is to run the JVM for the Pravega process at hand). What is your opinion about this? Have you used -XX:+PrintFlagsFinal to see what is the actual -Xmx value that is being used? Would it make sense to set the -Xmx value in function to the provisioned memory resources for the pod?

adrianmo · 2019-03-21T12:00:21Z

Would it make sense to set the -Xmx value in function to the provisioned memory resources for the pod?

@RaulGracia that's exactly what -XX:+UnlockExperimentalVMOptions -XX:+UseCGroupMemoryLimitForHeap flags do. These flags were introduced in Java 8u131 and Java 9 and, when they are set and -Xmx is not set, JVM heap size will be set according to the cgroup configuration, which is what we specify in the Pod resource limit. Source: https://blogs.oracle.com/java-platform-group/java-se-support-for-docker-cpu-and-memory-limits

RaulGracia

LGTM.

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

* master: Issue 130: Ability to specify container resources (#129) Issue 134,137: Fix BookKeeper errors (#135) Run tests on GKE (#140) Document update: README.md (#133) Add service account role definitions [skip ci] (#136)

Specify container resources for testing

4abafe4

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

adrianmo assigned RaulGracia and adrianmo Mar 4, 2019

adrianmo changed the title ~~[WiP] Ability to specify container resources~~ [WiP] Issue 130: Ability to specify container resources Mar 4, 2019

adrianmo and others added 9 commits March 13, 2019 12:51

Set Xmx and Xms Java opts to SS and C

33ec9d1

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

Store BK index directory in a PVC

44b7697

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

enable custom resource configuration

a50b533

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Merge master

50b8442

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Generate deepcopy and go fmt

2818fd6

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Merge branch 'master' into set-pod-resources

8be2cfe

* master: Run tests on GKE (#140)

Merge branch 'master' into set-pod-resources

213d449

* master: Issue 134,137: Fix BookKeeper errors (#135)

Fix golang errors

faac0f1

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

Adjust resource values

2ea7277

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

adrianmo changed the title ~~[WiP] Issue 130: Ability to specify container resources~~ Issue 130: Ability to specify container resources Mar 20, 2019

adrianmo requested a review from Tristan1900 March 20, 2019 13:49

adrianmo added 3 commits March 20, 2019 15:58

Set Xmx value based on memory limit

dd3413a

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

Reduce default resource consumption

4308f52

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

Further reduce resources

ad2c198

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

adrianmo requested a review from RaulGracia March 20, 2019 15:35

RaulGracia reviewed Mar 20, 2019

View reviewed changes

wenqi added 2 commits March 20, 2019 17:04

Add unit test to check resource is working

9878fac

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Refractor test

64b8d06

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Tristan1900 approved these changes Mar 21, 2019

View reviewed changes

Use literals instead of constants on unit test assertions

6acf293

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

adrianmo added the status/ready The issue is ready to be worked on; or the PR is ready to review label Mar 21, 2019

adrianmo commented Mar 21, 2019

View reviewed changes

RaulGracia requested changes Mar 21, 2019

View reviewed changes

RaulGracia approved these changes Mar 21, 2019

View reviewed changes

Wait after cluster destoryed

5a7e937

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

wenqi and others added 2 commits March 21, 2019 21:53

Go fmt

464f206

Signed-off-by: wenqi <Wenqi.Mou@emc.com>

Wait for PVCs to be deleted when destroying a cluster

e466ec5

Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>

adrianmo merged commit 78d9944 into master Mar 22, 2019

adrianmo deleted the set-pod-resources branch March 22, 2019 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 130: Ability to specify container resources #129

Issue 130: Ability to specify container resources #129

adrianmo commented Mar 4, 2019 •

edited

Loading

RaulGracia left a comment

RaulGracia Mar 20, 2019

adrianmo Mar 20, 2019

RaulGracia Mar 20, 2019

adrianmo Mar 20, 2019

RaulGracia Mar 20, 2019

adrianmo Mar 20, 2019

RaulGracia Mar 21, 2019 •

edited

Loading

RaulGracia Mar 20, 2019

adrianmo Mar 20, 2019

Tristan1900 Mar 21, 2019

adrianmo left a comment

RaulGracia left a comment

RaulGracia Mar 21, 2019 •

edited

Loading

adrianmo commented Mar 21, 2019 •

edited

Loading

RaulGracia left a comment

Issue 130: Ability to specify container resources #129

Issue 130: Ability to specify container resources #129

Conversation

adrianmo commented Mar 4, 2019 • edited Loading

Change log description

Purpose of the change

How to test the change

RaulGracia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaulGracia Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianmo left a comment

Choose a reason for hiding this comment

RaulGracia left a comment

Choose a reason for hiding this comment

RaulGracia Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

adrianmo commented Mar 21, 2019 • edited Loading

RaulGracia left a comment

Choose a reason for hiding this comment

adrianmo commented Mar 4, 2019 •

edited

Loading

RaulGracia Mar 21, 2019 •

edited

Loading

RaulGracia Mar 21, 2019 •

edited

Loading

adrianmo commented Mar 21, 2019 •

edited

Loading