Repository: kinvolk/lokomotive (go)

Issue:
Rook: Ceph OSDs consume up to 12 GB memory
tl;dr: Provide a way for the user where they can limit the memory and CPU of the ceph sub-components like OSD, MGR, MON, etc.

---

Right now there is no way for a user to specify memory limits on the OSD or any other sub-components of Rook. Since there are no resource limits specified the pod uses all the memory availble on the host.

As you can see that the following OSD deployment has no Kubernetes resources (memory or cpu and limits or request) set. But still the env vars are trying to reference them. So here empty values are being referenced:

```yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
  name: rook-ceph-osd-28
  namespace: rook
...
      containers:
      - args:
        - --foreground
        - --id
        - "28"
        - --fsid
        - 77d48d93-351e-4a00-a554-d99e625914d7
        - --setuser
        - ceph
        - --setgroup
        - ceph
        - --crush-location=root=default host=lokomotive-production-storage-worker-2
          region=ewr1
        - --log-to-stderr=true
        - --err-to-stderr=true
        - --mon-cluster-log-to-stderr=true
        - '--log-stderr-prefix=debug '
        - --default-log-to-file=false
        - --default-mon-cluster-log-to-file=false
        - --ms-learn-addr-from-peer=false
        command:
        - ceph-osd
        env:
        - name: POD_MEMORY_LIMIT
          valueFrom:
            resourceFieldRef:
              divisor: "0"
              resource: limits.memory
        - name: POD_MEMORY_REQUEST
          valueFrom:
            resourceFieldRef:
              divisor: "0"
              resource: requests.memory
        - name: POD_CPU_LIMIT
          valueFrom:
            resourceFieldRef:
              divisor: "1"
              resource: limits.cpu
        - name: POD_CPU_REQUEST
          valueFrom:
            resourceFieldRef:
              divisor: "0"
              resource: requests.cpu
        image: ceph/ceph:v15.2.5-20200916
        name: osd
        resources: {}
```

One would expect that empty values are populated inside the pod, but if you see the env vars all the limit values are capped at the host limits:

```bash
POD_MEMORY_LIMIT=135039045632
POD_MEMORY_REQUEST=0
POD_CPU_LIMIT=32
POD_CPU_REQUEST=0
```

This host has 125 GB memory and 32 cores of CPU.

Above automatically reflects in the OSD config:

```
[root@rook-ceph-tools-6f7fccd4d6-fvgbk /]# ceph tell osd.28 config show | grep memory
    "osd_memory_base": "805306368",
    "osd_memory_cache_min": "134217728",
    "osd_memory_cache_resize_interval": "1.000000",
    "osd_memory_expected_fragmentation": "0.150000",
    "osd_memory_target": "108031236505",
    "osd_memory_target_cgroup_limit_ratio": "0.800000",
```

`osd_memory_target` is the amount of memory that OSD is allowed to expand upto. And it is appropriately set proportional to the host memory limit.

```
osd_memory_target = floor( POD_MEMORY_LIMIT * osd_memory_target_cgroup_limit_ratio )
```


Pull request:
rook-ceph: Allow user to provide sub-component resources
This PR adds a new parameter `resources` for sub-components' cpu and
memory requests and limits.

Fixes #1476

Patch:
diff --git a/docs/configuration-reference/components/rook-ceph.md b/docs/configuration-reference/components/rook-ceph.md
index 53f50148..943a2d3f 100644
--- a/docs/configuration-reference/components/rook-ceph.md
+++ b/docs/configuration-reference/components/rook-ceph.md
@@ -50,8 +50,28 @@ component "rook-ceph" {
     default        = true
     reclaim_policy = "Delete"
   }
-}
 
+  resources {
+    osd {
+      requests {
+        cpu    = "5"
+        memory = "5Gi"
+      }
+
+      limits {
+        cpu    = "5"
+        memory = "5Gi"
+      }
+    }
+
+    mon {}
+    mgr {}
+    mds {}
+    prepareosd {}
+    crashcollector {}
+    mgr_sidecar {}
+  }
+}
 ```
 
 The Ceph cluster needs to be deployed in the same namespace as the Rook operator at the moment.
@@ -65,14 +85,21 @@ Table of all the arguments accepted by the component.
 | Argument                       | Description                                                                                                                                                                                                                                                                                                                 | Default  | Type                                                                                                           | Required |
 |--------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------:|:---------------------------------------------------------------------------------------------------------------|:--------:|
 | `namespace`                    | Namespace to deploy the Ceph cluster into. Must be the same as the rook operator.                                                                                                                                                                                                                                           |  "rook"  | string                                                                                                         |  false   |
-| `monitor_count`                | Number of Ceph monitors to deploy. An odd number like 3 or 5 is recommended which should also be sufficient for most cases.                                                                                                                                                                                                 |     1    | number                                                                                                         |  false   |
+| `monitor_count`                | Number of Ceph monitors to deploy. An odd number like 3 or 5 is recommended which should also be sufficient for most cases.                                                                                                                                                                                                 |    1     | number                                                                                                         |  false   |
 | `enable_toolbox`               | Deploy the [toolbox pod](https://rook.io/docs/rook/master/ceph-toolbox.html) to debug and manage the Ceph cluster.                                                                                                                                                                                                          |  false   | bool                                                                                                           |  false   |
-| `node_affinity`                | Node affinity for deploying the Ceph cluster pods.                                                                                                                                                                                                                                                                          |     -    | list(object({key = string, operator = string, values = list(string)}))                                         |  false   |
-| `toleration`                   | Tolerations that the Ceph cluster pods will tolerate.                                                                                                                                                                                                                                                                       |     -    | list(object({key = string, effect = string, operator = string, value = string, toleration_seconds = string })) |  false   |
-| `metadata_device`              | Name of the device to store the metadata on each storage machine. **Note**: Provide just the name of the device and skip prefixing with `/dev/`.                                                                                                                                                                            |     -    | string                                                                                                         |  false   |
-| `storage_class.enable`         | Install Storage Class config.                                                                                                                                                                                                                                                                                               |   false  | bool                                                                                                           |  false   |
-| `storage_class.default`        | Make this Storage Class as a default one.                                                                                                                                                                                                                                                                                   |   false  | bool                                                                                                           |  false   |
+| `node_affinity`                | Node affinity for deploying the Ceph cluster pods.                                                                                                                                                                                                                                                                          |    -     | list(object({key = string, operator = string, values = list(string)}))                                         |  false   |
+| `toleration`                   | Tolerations that the Ceph cluster pods will tolerate.                                                                                                                                                                                                                                                                       |    -     | list(object({key = string, effect = string, operator = string, value = string, toleration_seconds = string })) |  false   |
+| `metadata_device`              | Name of the device to store the metadata on each storage machine. **Note**: Provide just the name of the device and skip prefixing with `/dev/`.                                                                                                                                                                            |    -     | string                                                                                                         |  false   |
+| `storage_class.enable`         | Install Storage Class config.                                                                                                                                                                                                                                                                                               |  false   | bool                                                                                                           |  false   |
+| `storage_class.default`        | Make this Storage Class as a default one.                                                                                                                                                                                                                                                                                   |  false   | bool                                                                                                           |  false   |
 | `storage_class.reclaim_policy` | Persistent volumes created with this storage class will have this reclaim policy. This field decides what happens to the volume after a user deletes a PVC. Valid values: `Retain`, `Recycle` and `Delete`. Read more in the [Kubernetes docs](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#reclaiming). | `Retain` | string                                                                                                         |  false   |
+| `resources.osd`                | Resource request and/or limits for OSDs. Read [this doc](https://rook.io/docs/rook/v1.6/ceph-cluster-crd.html#cluster-wide-resources-configuration-settings), to find out the minimum prescribed resource requirements.                                                                                                     |    -     | object(request {cpu = string, memory = string}, limits {cpu = string, memory = string})                        |  false   |
+| `resources.mon`                | Resource request and/or limits for MONs. Read [this doc](https://rook.io/docs/rook/v1.6/ceph-cluster-crd.html#cluster-wide-resources-configuration-settings), to find out the minimum prescribed resource requirements.                                                                                                     |    -     | object(request {cpu = string, memory = string}, limits {cpu = string, memory = string})                        |  false   |
+| `resources.mgr`                | Resource request and/or limits for MGRs. Read [this doc](https://rook.io/docs/rook/v1.6/ceph-cluster-crd.html#cluster-wide-resources-configuration-settings), to find out the minimum prescribed resource requirements.                                                                                                     |    -     | object(request {cpu = string, memory = string}, limits {cpu = string, memory = string})                        |  false   |
+| `resources.mds`                | Resource request and/or limits for MDS. Read [this doc](https://rook.io/docs/rook/v1.6/ceph-cluster-crd.html#cluster-wide-resources-configuration-settings), to find out the minimum prescribed resource requirements.                                                                                                      |    -     | object(request {cpu = string, memory = string}, limits {cpu = string, memory = string})                        |  false   |
+| `resources.prepareosd`         | Resource request and/or limits for OSD prepare job. Read [this doc](https://rook.io/docs/rook/v1.6/ceph-cluster-crd.html#cluster-wide-resources-configuration-settings), to find out the minimum prescribed resource requirements.                                                                                          |    -     | object(request {cpu = string, memory = string}, limits {cpu = string, memory = string})                        |  false   |
+| `resources.crashcollector`     | Resource request and/or limits for Crashcollector. Read [this doc](https://rook.io/docs/rook/v1.6/ceph-cluster-crd.html#cluster-wide-resources-configuration-settings), to find out the minimum prescribed resource requirements.                                                                                           |    -     | object(request {cpu = string, memory = string}, limits {cpu = string, memory = string})                        |  false   |
+| `resources.mgr_sidecar`        | Resource request and/or limits for MGR sidecar. Read [this doc](https://rook.io/docs/rook/v1.6/ceph-cluster-crd.html#cluster-wide-resources-configuration-settings), to find out the minimum prescribed resource requirements.                                                                                              |    -     | object(request {cpu = string, memory = string}, limits {cpu = string, memory = string})                        |  false   |
 
 ## Applying
 
diff --git a/pkg/components/rook-ceph/component.go b/pkg/components/rook-ceph/component.go
index e345e5a4..0beb7aff 100644
--- a/pkg/components/rook-ceph/component.go
+++ b/pkg/components/rook-ceph/component.go
@@ -41,6 +41,27 @@ type component struct {
 	TolerationsRaw string
 	StorageClass   *StorageClass `hcl:"storage_class,block"`
 	EnableToolbox  bool          `hcl:"enable_toolbox,optional"`
+
+	Resources *Resources `hcl:"resources,block"`
+}
+
+// Resources struct allows user to specify resource request and limits on the rook-ceph
+// sub-components.
+type Resources struct {
+	MON               *util.ResourceRequirements `hcl:"mon,block"`
+	MONRaw            string
+	MGR               *util.ResourceRequirements `hcl:"mgr,block"`
+	MGRRaw            string
+	OSD               *util.ResourceRequirements `hcl:"osd,block"`
+	OSDRaw            string
+	MDS               *util.ResourceRequirements `hcl:"mds,block"`
+	MDSRaw            string
+	PrepareOSD        *util.ResourceRequirements `hcl:"prepareosd,block"`
+	PrepareOSDRaw     string
+	CrashCollector    *util.ResourceRequirements `hcl:"crashcollector,block"`
+	CrashCollectorRaw string
+	MGRSidecar        *util.ResourceRequirements `hcl:"mgr_sidecar,block"`
+	MGRSidecarRaw     string
 }
 
 // StorageClass provides struct to enable it or make it default.
@@ -69,6 +90,44 @@ func (c *component) LoadConfig(configBody *hcl.Body, evalContext *hcl.EvalContex
 	return gohcl.DecodeBody(*configBody, evalContext, c)
 }
 
+func (c *component) addResourceRequirements() error {
+	if c.Resources == nil {
+		return nil
+	}
+
+	var err error
+
+	if c.Resources.MONRaw, err = util.RenderResourceRequirements(c.Resources.MON); err != nil {
+		return fmt.Errorf("rendering resources.mon: %w", err)
+	}
+
+	if c.Resources.MGRRaw, err = util.RenderResourceRequirements(c.Resources.MGR); err != nil {
+		return fmt.Errorf("rendering resources.mgr: %w", err)
+	}
+
+	if c.Resources.OSDRaw, err = util.RenderResourceRequirements(c.Resources.OSD); err != nil {
+		return fmt.Errorf("rendering resources.osd: %w", err)
+	}
+
+	if c.Resources.MDSRaw, err = util.RenderResourceRequirements(c.Resources.MDS); err != nil {
+		return fmt.Errorf("rendering resources.mds: %w", err)
+	}
+
+	if c.Resources.PrepareOSDRaw, err = util.RenderResourceRequirements(c.Resources.PrepareOSD); err != nil {
+		return fmt.Errorf("rendering resources.prepareosd: %w", err)
+	}
+
+	if c.Resources.CrashCollectorRaw, err = util.RenderResourceRequirements(c.Resources.CrashCollector); err != nil {
+		return fmt.Errorf("rendering resources.crashcollector: %w", err)
+	}
+
+	if c.Resources.MGRSidecarRaw, err = util.RenderResourceRequirements(c.Resources.MGRSidecar); err != nil {
+		return fmt.Errorf("rendering resources.mgr_sidecar: %w", err)
+	}
+
+	return nil
+}
+
 // TODO: Convert to Helm chart.
 func (c *component) RenderManifests() (map[string]string, error) {
 	// Generate YAML for Ceph cluster.
@@ -78,6 +137,10 @@ func (c *component) RenderManifests() (map[string]string, error) {
 		return nil, fmt.Errorf("rendering tolerations: %w", err)
 	}
 
+	if err := c.addResourceRequirements(); err != nil {
+		return nil, fmt.Errorf("rendering resources field: %w", err)
+	}
+
 	ret := make(map[string]string)
 
 	// Parse template with values
diff --git a/pkg/components/rook-ceph/manifests.go b/pkg/components/rook-ceph/manifests.go
index 58170abf..db055db7 100644
--- a/pkg/components/rook-ceph/manifests.go
+++ b/pkg/components/rook-ceph/manifests.go
@@ -75,7 +75,16 @@ spec:
       tolerations: {{ .TolerationsRaw }}
       {{- end }}
   annotations:
+  {{- if .Resources }}
   resources:
+    mon: {{ .Resources.MONRaw }}
+    mgr: {{ .Resources.MGRRaw }}
+    osd: {{ .Resources.OSDRaw }}
+    mds: {{ .Resources.MDSRaw }}
+    prepareosd: {{ .Resources.PrepareOSDRaw }}
+    crashcollector: {{ .Resources.CrashCollectorRaw }}
+    mgr-sidecar: {{ .Resources.MGRSidecarRaw }}
+  {{- end }}
   removeOSDsIfOutAndSafeToRemove: false
   storage: # cluster level storage configuration and selection
     useAllNodes: true
diff --git a/pkg/components/util/types.go b/pkg/components/util/types.go
index 0ea5a78f..31a55cdf 100644
--- a/pkg/components/util/types.go
+++ b/pkg/components/util/types.go
@@ -96,3 +96,30 @@ func (n *NodeSelector) Render() (string, error) {
 
 	return string(b), nil
 }
+
+// ResourceRequirements allows user to specify resource requests and/or limits.
+type ResourceRequirements struct {
+	Requests *ResourceList `hcl:"requests,block" json:"requests,omitempty"`
+	Limits   *ResourceList `hcl:"limits,block" json:"limits,omitempty"`
+}
+
+// ResourceList allows user to specify CPU and memory.
+type ResourceList struct {
+	CPU    string `hcl:"cpu,optional" json:"cpu,omitempty"`
+	Memory string `hcl:"memory,optional" json:"memory,omitempty"`
+}
+
+// RenderResourceRequirements takes a list of ResourceRequirements.
+// It returns a json string and an error if any.
+func RenderResourceRequirements(r *ResourceRequirements) (string, error) {
+	if r == nil {
+		return "", nil
+	}
+
+	b, err := json.Marshal(r)
+	if err != nil {
+		return "", err
+	}
+
+	return string(b), nil
+}


Test patch:
diff --git a/pkg/components/internal/testutil/jsonpath.go b/pkg/components/internal/testutil/jsonpath.go
index 21ee130c..04112e28 100644
--- a/pkg/components/internal/testutil/jsonpath.go
+++ b/pkg/components/internal/testutil/jsonpath.go
@@ -15,6 +15,7 @@
 package testutil
 
 import (
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"strings"
@@ -103,6 +104,29 @@ func MatchJSONPathInt64Value(t *testing.T, yamlConfig string, jsonPath string, e
 	}
 }
 
+// MatchJSONPathJSONValue is a helper function for component unit tests. It compares the JSON values
+// at a JSON path in a YAML config to the expected JSON string given by the user.
+// e.g.
+//
+// spec:
+//   resources:
+//     osd: {"requests":{"cpu":"5","memory":"5Gi"},"limits":{"cpu":"5","memory":"5Gi"}}
+func MatchJSONPathJSONValue(t *testing.T, yamlConfig string, jsonPath string, expected string) {
+	obj, err := jsonPathValue(yamlConfig, jsonPath)
+	if err != nil {
+		t.Fatalf("Extracting JSON path value: %v", err)
+	}
+
+	got, err := json.Marshal(obj)
+	if err != nil {
+		t.Fatalf("Marshalling JSON object: %v", err)
+	}
+
+	if string(got) != expected {
+		t.Fatalf("Expected: %s, Got: %s", expected, got)
+	}
+}
+
 // JSONPathExists checks if the given YAML config has an object at the given JSON path, also provide
 // what error to expect.
 func JSONPathExists(t *testing.T, yamlConfig string, jsonPath string, errExp string) {
diff --git a/pkg/components/rook-ceph/component_test.go b/pkg/components/rook-ceph/component_test.go
index 929aac2e..27958f10 100644
--- a/pkg/components/rook-ceph/component_test.go
+++ b/pkg/components/rook-ceph/component_test.go
@@ -92,6 +92,7 @@ func TestConversion(t *testing.T) {
 		expectedManifestName k8sutil.ObjectMetadata
 		expected             string
 		jsonPath             string
+		fn                   func(*testing.T, string, string, string)
 	}{
 		{
 			name: "default_reclaim_policy",
@@ -105,6 +106,7 @@ func TestConversion(t *testing.T) {
 			},
 			jsonPath: "{.reclaimPolicy}",
 			expected: "Retain",
+			fn:       testutil.MatchJSONPathStringValue,
 		},
 		{
 			name: "overridden_reclaim_policy",
@@ -119,6 +121,140 @@ func TestConversion(t *testing.T) {
 			},
 			jsonPath: "{.reclaimPolicy}",
 			expected: "Delete",
+			fn:       testutil.MatchJSONPathStringValue,
+		},
+		{
+			name: "resources_osd_requests_cpu",
+			inputConfig: `component "rook-ceph" {
+				resources {
+					osd {
+						requests {
+							cpu = "5"
+						}
+					}
+				}
+			}`,
+			expectedManifestName: k8sutil.ObjectMetadata{
+				Version: "ceph.rook.io/v1", Kind: "CephCluster", Name: "rook-ceph",
+			},
+			jsonPath: "{.spec.resources.osd}",
+			expected: "{\"requests\":{\"cpu\":\"5\"}}",
+			fn:       testutil.MatchJSONPathJSONValue,
+		},
+		{
+			name: "resources_mgr_requests_memory",
+			inputConfig: `component "rook-ceph" {
+				resources {
+					mgr {
+						requests {
+							memory = "10Gi"
+						}
+					}
+				}
+			}`,
+			expectedManifestName: k8sutil.ObjectMetadata{
+				Version: "ceph.rook.io/v1", Kind: "CephCluster", Name: "rook-ceph",
+			},
+			jsonPath: "{.spec.resources.mgr}",
+			expected: "{\"requests\":{\"memory\":\"10Gi\"}}",
+			fn:       testutil.MatchJSONPathJSONValue,
+		},
+		{
+			name: "resources_mon_limits_cpu",
+			inputConfig: `component "rook-ceph" {
+				resources {
+					mon {
+						limits {
+							cpu = "3"
+						}
+					}
+				}
+			}`,
+			expectedManifestName: k8sutil.ObjectMetadata{
+				Version: "ceph.rook.io/v1", Kind: "CephCluster", Name: "rook-ceph",
+			},
+			jsonPath: "{.spec.resources.mon}",
+			expected: "{\"limits\":{\"cpu\":\"3\"}}",
+			fn:       testutil.MatchJSONPathJSONValue,
+		},
+		{
+			name: "resources_mds_limits_memory",
+			inputConfig: `component "rook-ceph" {
+				resources {
+					mds {
+						limits {
+							memory = "2Gi"
+						}
+					}
+				}
+			}`,
+			expectedManifestName: k8sutil.ObjectMetadata{
+				Version: "ceph.rook.io/v1", Kind: "CephCluster", Name: "rook-ceph",
+			},
+			jsonPath: "{.spec.resources.mds}",
+			expected: "{\"limits\":{\"memory\":\"2Gi\"}}",
+			fn:       testutil.MatchJSONPathJSONValue,
+		},
+		{
+			name: "resources_prepareosd_requests",
+			inputConfig: `component "rook-ceph" {
+				resources {
+					prepareosd {
+						requests {
+							memory = "9Gi"
+							cpu    = "9"
+						}
+					}
+				}
+			}`,
+			expectedManifestName: k8sutil.ObjectMetadata{
+				Version: "ceph.rook.io/v1", Kind: "CephCluster", Name: "rook-ceph",
+			},
+			jsonPath: "{.spec.resources.prepareosd}",
+			expected: "{\"requests\":{\"cpu\":\"9\",\"memory\":\"9Gi\"}}",
+			fn:       testutil.MatchJSONPathJSONValue,
+		},
+		{
+			name: "resources_crashcollector_limits",
+			inputConfig: `component "rook-ceph" {
+				resources {
+					crashcollector {
+						limits {
+							cpu    = "7"
+							memory = "7Gi"
+						}
+					}
+				}
+			}`,
+			expectedManifestName: k8sutil.ObjectMetadata{
+				Version: "ceph.rook.io/v1", Kind: "CephCluster", Name: "rook-ceph",
+			},
+			jsonPath: "{.spec.resources.crashcollector}",
+			expected: "{\"limits\":{\"cpu\":\"7\",\"memory\":\"7Gi\"}}",
+			fn:       testutil.MatchJSONPathJSONValue,
+		},
+		{
+			name: "resources_mgr_sidecar_all",
+			inputConfig: `component "rook-ceph" {
+				resources {
+					mgr_sidecar {
+						limits {
+							cpu = "6"
+						}
+
+						requests {
+							memory = "1Gi"
+							cpu    = "1"
+						}
+					}
+				}
+			}`,
+			expectedManifestName: k8sutil.ObjectMetadata{
+				Version: "ceph.rook.io/v1", Kind: "CephCluster", Name: "rook-ceph",
+			},
+			jsonPath: "{.spec.resources.mgr-sidecar}",
+			expected: "{\"limits\":{\"cpu\":\"6\"},\"requests\":{\"cpu\":\"1\",\"memory\":\"1Gi\"}}",
+			fn:       testutil.MatchJSONPathJSONValue,
 		},
 	}
 
@@ -131,7 +267,7 @@ func TestConversion(t *testing.T) {
 			m := testutil.RenderManifests(t, component, Name, tc.inputConfig)
 			gotConfig := testutil.ConfigFromMap(t, m, tc.expectedManifestName)
 
-			testutil.MatchJSONPathStringValue(t, gotConfig, tc.jsonPath, tc.expected)
+			tc.fn(t, gotConfig, tc.jsonPath, tc.expected)
 		})
 	}
 }