[WIP] Add set_memory_request_to_limit option (zalando#406)

sdudoladov · web-flow · commit 45c89b3da4f7 · 2018-11-15T14:00:08.000+01:00
* Add set_memory_request_to_limit option
diff --git a/README.md b/README.md
@@ -90,6 +90,8 @@ cd postgres-operator
 ./run_operator_locally.sh
 ```
 
+Note we provide the `/manifests` directory as an example only; you should consider adjusting the manifests to your particular setting.
+
 ## Running and testing the operator
 
 The best way to test the operator is to run it locally in [minikube](https://kubernetes.io/docs/getting-started-guides/minikube/). See developer docs(`docs/developer.yaml`) for details.
diff --git a/delivery.yaml b/delivery.yaml
@@ -22,7 +22,7 @@ pipeline:
             go version
         - desc: 'Install Docker'
           cmd: |
-            curl -sSL https://get.docker.com/ | sh
+            curl -fLOsS https://delivery.cloud.zalando.com/utils/ensure-docker && sh ensure-docker && rm ensure-docker
         - desc: 'Symlink sources into the GOPATH'
           cmd: |
             mkdir -p $OPERATOR_TOP_DIR
diff --git a/docs/administrator.md b/docs/administrator.md
@@ -41,12 +41,12 @@ manifests:
 
 ```bash
     $ kubectl create namespace test
-    $ kubectl config set-context --namespace=test
+    $ kubectl config set-context $(kubectl config current-context) --namespace=test
 ```
 
 All subsequent `kubectl` commands will work with the `test` namespace. The
-operator  will run in this namespace and look up needed resources - such as its
-config map - there.
+operator will run in this namespace and look up needed resources - such as its
+config map - there. Please note that the namespace for service accounts and cluster role bindings in [operator RBAC rules](manifests/operator-service-account-rbac.yaml) needs to be adjusted to the non-default value.
 
 ## Specify the namespace to watch
 
diff --git a/docs/reference/operator_parameters.md b/docs/reference/operator_parameters.md
@@ -221,6 +221,9 @@ CRD-based configuration.
   memory limits for the postgres containers, unless overridden by cluster-specific
   settings. The default is `1Gi`.
 
+* **set_memory_request_to_limit**
+  Set `memory_request` to `memory_limit` for all Postgres clusters (the default value is also increased). This prevents certain cases of memory overcommitment at the cost of overprovisioning memory and potential scheduling problems for containers with high memory limits due to the lack of memory on Kubernetes cluster nodes. This affects all containers (Postgres, Scalyr sidecar, and other sidecars). The default is `false`.
+
 ## Operator timeouts
 
 This set of parameters define various timeouts related to some operator
diff --git a/manifests/complete-postgres-manifest.yaml b/manifests/complete-postgres-manifest.yaml
@@ -6,7 +6,7 @@ metadata:
 spec:
   teamId: "ACID"
   volume:
-    size: 5Gi
+    size: 1Gi
   numberOfInstances: 2
   users: #Application/Robot users
     zalando:
@@ -31,7 +31,7 @@ spec:
       memory: 100Mi
     limits:
       cpu: 300m
-      memory: 3000Mi
+      memory: 300Mi
   patroni:
     initdb:
       encoding: "UTF8"
diff --git a/manifests/configmap.yaml b/manifests/configmap.yaml
@@ -10,11 +10,12 @@ data:
 
   debug_logging: "true"
   workers: "4"
-  docker_image: registry.opensource.zalan.do/acid/spilo-cdp-10:1.4-p29
+  docker_image: registry.opensource.zalan.do/acid/spilo-cdp-10:1.5-p35
   pod_service_account_name: "zalando-postgres-operator"
   secret_name_template: '{username}.{cluster}.credentials'
   super_username: postgres
   enable_teams_api: "false"
+  # set_memory_request_to_limit: "true"
   # postgres_superuser_teams: "postgres_superusers"
   # enable_team_superuser: "false"
   # team_admin_role: "admin"
diff --git a/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go b/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go
@@ -131,6 +131,7 @@ type OperatorConfigurationData struct {
 	PostgresUsersConfiguration PostgresUsersConfiguration   `json:"users"`
 	Kubernetes                 KubernetesMetaConfiguration  `json:"kubernetes"`
 	PostgresPodResources       PostgresPodResourcesDefaults `json:"postgres_pod_resources"`
+	SetMemoryRequestToLimit    bool                         `json:"set_memory_request_to_limit,omitempty"`
 	Timeouts                   OperatorTimeouts             `json:"timeouts"`
 	LoadBalancer               LoadBalancerConfiguration    `json:"load_balancer"`
 	AWSGCP                     AWSGCPConfiguration          `json:"aws_or_gcp"`
diff --git a/pkg/apis/acid.zalan.do/v1/postgresql_type.go b/pkg/apis/acid.zalan.do/v1/postgresql_type.go
@@ -90,8 +90,8 @@ type ResourceDescription struct {
 
 // Resources describes requests and limits for the cluster resouces.
 type Resources struct {
-	ResourceRequest ResourceDescription `json:"requests,omitempty"`
-	ResourceLimits  ResourceDescription `json:"limits,omitempty"`
+	ResourceRequests ResourceDescription `json:"requests,omitempty"`
+	ResourceLimits   ResourceDescription `json:"limits,omitempty"`
 }
 
 // Patroni contains Patroni-specific configuration
diff --git a/pkg/apis/acid.zalan.do/v1/util_test.go b/pkg/apis/acid.zalan.do/v1/util_test.go
@@ -240,8 +240,8 @@ var unmarshalCluster = []struct {
 					Slots:                map[string]map[string]string{"permanent_logical_1": {"type": "logical", "database": "foo", "plugin": "pgoutput"}},
 				},
 				Resources: Resources{
-					ResourceRequest: ResourceDescription{CPU: "10m", Memory: "50Mi"},
-					ResourceLimits:  ResourceDescription{CPU: "300m", Memory: "3000Mi"},
+					ResourceRequests: ResourceDescription{CPU: "10m", Memory: "50Mi"},
+					ResourceLimits:   ResourceDescription{CPU: "300m", Memory: "3000Mi"},
 				},
 
 				TeamID:              "ACID",
diff --git a/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go b/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go
diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go
@@ -92,18 +92,18 @@ func (c *Cluster) makeDefaultResources() acidv1.Resources {
 	defaultRequests := acidv1.ResourceDescription{CPU: config.DefaultCPURequest, Memory: config.DefaultMemoryRequest}
 	defaultLimits := acidv1.ResourceDescription{CPU: config.DefaultCPULimit, Memory: config.DefaultMemoryLimit}
 
-	return acidv1.Resources{ResourceRequest: defaultRequests, ResourceLimits: defaultLimits}
+	return acidv1.Resources{ResourceRequests: defaultRequests, ResourceLimits: defaultLimits}
 }
 
 func generateResourceRequirements(resources acidv1.Resources, defaultResources acidv1.Resources) (*v1.ResourceRequirements, error) {
 	var err error
 
-	specRequests := resources.ResourceRequest
+	specRequests := resources.ResourceRequests
 	specLimits := resources.ResourceLimits
 
 	result := v1.ResourceRequirements{}
 
-	result.Requests, err = fillResourceList(specRequests, defaultResources.ResourceRequest)
+	result.Requests, err = fillResourceList(specRequests, defaultResources.ResourceRequests)
 	if err != nil {
 		return nil, fmt.Errorf("could not fill resource requests: %v", err)
 	}
@@ -377,8 +377,8 @@ func generateSidecarContainers(sidecars []acidv1.Sidecar,
 
 			resources, err := generateResourceRequirements(
 				makeResources(
-					sidecar.Resources.ResourceRequest.CPU,
-					sidecar.Resources.ResourceRequest.Memory,
+					sidecar.Resources.ResourceRequests.CPU,
+					sidecar.Resources.ResourceRequests.Memory,
 					sidecar.Resources.ResourceLimits.CPU,
 					sidecar.Resources.ResourceLimits.Memory,
 				),
@@ -625,7 +625,7 @@ func getBucketScopeSuffix(uid string) string {
 
 func makeResources(cpuRequest, memoryRequest, cpuLimit, memoryLimit string) acidv1.Resources {
 	return acidv1.Resources{
-		ResourceRequest: acidv1.ResourceDescription{
+		ResourceRequests: acidv1.ResourceDescription{
 			CPU:    cpuRequest,
 			Memory: memoryRequest,
 		},
@@ -644,6 +644,60 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*v1beta1.State
 		podTemplate         *v1.PodTemplateSpec
 		volumeClaimTemplate *v1.PersistentVolumeClaim
 	)
+
+	if c.OpConfig.SetMemoryRequestToLimit {
+
+		// controller adjusts the default memory request at operator startup
+
+		request := spec.Resources.ResourceRequests.Memory
+		if request == "" {
+			request = c.OpConfig.DefaultMemoryRequest
+		}
+
+		limit := spec.Resources.ResourceLimits.Memory
+		if limit == "" {
+			limit = c.OpConfig.DefaultMemoryLimit
+		}
+
+		isSmaller, err := util.RequestIsSmallerThanLimit(request, limit)
+		if err != nil {
+			return nil, err
+		}
+		if isSmaller {
+			c.logger.Warningf("The memory request of %v for the Postgres container is increased to match the memory limit of %v.", request, limit)
+			spec.Resources.ResourceRequests.Memory = limit
+
+		}
+
+		// controller adjusts the Scalyr sidecar request at operator startup
+		// as this sidecar is managed separately
+
+		// adjust sidecar containers defined for that particular cluster
+		for _, sidecar := range spec.Sidecars {
+
+			// TODO #413
+			sidecarRequest := sidecar.Resources.ResourceRequests.Memory
+			if request == "" {
+				request = c.OpConfig.DefaultMemoryRequest
+			}
+
+			sidecarLimit := sidecar.Resources.ResourceLimits.Memory
+			if limit == "" {
+				limit = c.OpConfig.DefaultMemoryLimit
+			}
+
+			isSmaller, err := util.RequestIsSmallerThanLimit(sidecarRequest, sidecarLimit)
+			if err != nil {
+				return nil, err
+			}
+			if isSmaller {
+				c.logger.Warningf("The memory request of %v for the %v sidecar container is increased to match the memory limit of %v.", sidecar.Resources.ResourceRequests.Memory, sidecar.Name, sidecar.Resources.ResourceLimits.Memory)
+				sidecar.Resources.ResourceRequests.Memory = sidecar.Resources.ResourceLimits.Memory
+			}
+		}
+
+	}
+
 	defaultResources := c.makeDefaultResources()
 
 	resourceRequirements, err := generateResourceRequirements(spec.Resources, defaultResources)
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -110,6 +110,29 @@ func (c *Controller) initOperatorConfig() {
 	c.opConfig = config.NewFromMap(configMapData)
 	c.warnOnDeprecatedOperatorParameters()
 
+	if c.opConfig.SetMemoryRequestToLimit {
+
+		isSmaller, err := util.RequestIsSmallerThanLimit(c.opConfig.DefaultMemoryRequest, c.opConfig.DefaultMemoryLimit)
+		if err != nil {
+			panic(err)
+		}
+		if isSmaller {
+			c.logger.Warningf("The default memory request of %v for Postgres containers is increased to match the default memory limit of %v.", c.opConfig.DefaultMemoryRequest, c.opConfig.DefaultMemoryLimit)
+			c.opConfig.DefaultMemoryRequest = c.opConfig.DefaultMemoryLimit
+		}
+
+		isSmaller, err = util.RequestIsSmallerThanLimit(c.opConfig.ScalyrMemoryRequest, c.opConfig.ScalyrMemoryLimit)
+		if err != nil {
+			panic(err)
+		}
+		if isSmaller {
+			c.logger.Warningf("The memory request of %v for the Scalyr sidecar container is increased to match the memory limit of %v.", c.opConfig.ScalyrMemoryRequest, c.opConfig.ScalyrMemoryLimit)
+			c.opConfig.ScalyrMemoryRequest = c.opConfig.ScalyrMemoryLimit
+		}
+
+		// generateStatefulSet adjusts values for individual Postgres clusters
+	}
+
 }
 
 func (c *Controller) modifyConfigFromEnvironment() {
diff --git a/pkg/controller/operator_config.go b/pkg/controller/operator_config.go
@@ -55,6 +55,7 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur
 	result.DefaultMemoryRequest = fromCRD.PostgresPodResources.DefaultMemoryRequest
 	result.DefaultCPULimit = fromCRD.PostgresPodResources.DefaultCPULimit
 	result.DefaultMemoryLimit = fromCRD.PostgresPodResources.DefaultMemoryLimit
+	result.SetMemoryRequestToLimit = fromCRD.SetMemoryRequestToLimit
 
 	result.ResourceCheckInterval = time.Duration(fromCRD.Timeouts.ResourceCheckInterval)
 	result.ResourceCheckTimeout = time.Duration(fromCRD.Timeouts.ResourceCheckTimeout)
diff --git a/pkg/util/config/config.go b/pkg/util/config/config.go
@@ -104,6 +104,7 @@ type Config struct {
 	PodTerminateGracePeriod  time.Duration     `name:"pod_terminate_grace_period" default:"5m"`
 	ProtectedRoles           []string          `name:"protected_role_names" default:"admin"`
 	PostgresSuperuserTeams   []string          `name:"postgres_superuser_teams" default:""`
+	SetMemoryRequestToLimit  bool              `name:"set_memory_request_to_limit" defaults:"false"`
 }
 
 // MustMarshal marshals the config or panics
diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -3,12 +3,14 @@ package util
 import (
 	"crypto/md5" // #nosec we need it to for PostgreSQL md5 passwords
 	"encoding/hex"
+	"fmt"
 	"math/rand"
 	"regexp"
 	"strings"
 	"time"
 
 	"github.com/motomux/pretty"
+	resource "k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/zalando-incubator/postgres-operator/pkg/spec"
@@ -127,3 +129,19 @@ func Coalesce(val, defaultVal string) string {
 	}
 	return val
 }
+
+// RequestIsSmallerThanLimit
+func RequestIsSmallerThanLimit(requestStr, limitStr string) (bool, error) {
+
+	request, err := resource.ParseQuantity(requestStr)
+	if err != nil {
+		return false, fmt.Errorf("could not parse memory request %v : %v", requestStr, err)
+	}
+
+	limit, err2 := resource.ParseQuantity(limitStr)
+	if err2 != nil {
+		return false, fmt.Errorf("could not parse memory limit %v : %v", limitStr, err2)
+	}
+
+	return request.Cmp(limit) == -1, nil
+}
diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
@@ -69,6 +69,17 @@ var substringMatch = []struct {
 	{regexp.MustCompile(`aaaa (\d+) bbbb`), "aaaa 123 bbbb", nil},
 }
 
+var requestIsSmallerThanLimitTests = []struct {
+	request string
+	limit   string
+	out     bool
+}{
+	{"1G", "2G", true},
+	{"1G", "1Gi", true}, // G is 1000^3 bytes, Gi is 1024^3 bytes
+	{"1024Mi", "1G", false},
+	{"1e9", "1G", false}, // 1e9 bytes == 1G
+}
+
 func TestRandomPassword(t *testing.T) {
 	const pwdLength = 10
 	pwd := RandomPassword(pwdLength)
@@ -143,3 +154,15 @@ func TestMapContains(t *testing.T) {
 		}
 	}
 }
+
+func TestRequestIsSmallerThanLimit(t *testing.T) {
+	for _, tt := range requestIsSmallerThanLimitTests {
+		res, err := RequestIsSmallerThanLimit(tt.request, tt.limit)
+		if err != nil {
+			t.Errorf("RequestIsSmallerThanLimit returned unexpected error: %#v", err)
+		}
+		if res != tt.out {
+			t.Errorf("RequestIsSmallerThanLimit expected: %#v, got: %#v", tt.out, res)
+		}
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -90,8 +90,8 @@ type ResourceDescription struct {`
`90`	`90`
`91`	`91`	`// Resources describes requests and limits for the cluster resouces.`
`92`	`92`	`type Resources struct {`
`93`		- ResourceRequest ResourceDescription `json:"requests,omitempty"`
`94`		- ResourceLimits ResourceDescription `json:"limits,omitempty"`
	`93`	+ ResourceRequests ResourceDescription `json:"requests,omitempty"`
	`94`	+ ResourceLimits ResourceDescription `json:"limits,omitempty"`
`95`	`95`	`}`
`96`	`96`
`97`	`97`	`// Patroni contains Patroni-specific configuration`
Original file line number	Diff line number	Diff line change
`@@ -104,6 +104,7 @@ type Config struct {`
`104`	`104`	PodTerminateGracePeriod time.Duration `name:"pod_terminate_grace_period" default:"5m"`
`105`	`105`	ProtectedRoles []string `name:"protected_role_names" default:"admin"`
`106`	`106`	PostgresSuperuserTeams []string `name:"postgres_superuser_teams" default:""`
	`107`	+ SetMemoryRequestToLimit bool `name:"set_memory_request_to_limit" defaults:"false"`
`107`	`108`	`}`
`108`	`109`
`109`	`110`	`// MustMarshal marshals the config or panics`